Libguestfs - Feb 2020 - [RFC] lib: allow to specify physical/logical block size for disks

From: Nikolay Ivanets <stenavin@gmail.com>

I faced with situation where libguestfs cannot recognize partitions on a
disk image which was partitioned on a system with "4K native" sector
size support.

In order to fix the issue we need to allow users to specify desired
physical and/or logical block size per drive basis.

It is definitely not a complete patch but rather a way to request for a
comments.  Nevertheless it is already working patch.  I've added an
optional parameters to add_drive API method which allow specifying
physical and logical block size for a drive separetely.

There are no documentation and tests yet. Input parameters are not
validated for correctness.

Here are my questions:
- Am I move in the right direction adding support for physical/logical
block size?
- I'm not sure I've made a good choise for parameter names:
'pblocksize'
and 'lblocksize' respectively.  I've tried to avoid long names like
'physicalblocksize' while keeping readability and semantic.
- Do we want to add the same optional parameters to 'add_drive_scratch'
API method?  I think it would be nice but it is up to you.
- What about 'add_dom', 'add_libvirt_dom' API methods?  Should
they also
handle 'blokio' tag in domain XML and act respectively?
- Anything else I didn't spot yet?
- Do we want guestfish to accept physical/logical block size per drive
from command line?
- What about other virt tools like virt-df, virt-cat and so on?

Sorry for a long list of questions.
---
 generator/actions_core.ml |  2 +-
 lib/drives.c              | 14 ++++++++++++++
 lib/guestfs-internal.h    |  2 ++
 lib/launch-direct.c       | 24 ++++++++++++++++++++++++
 lib/launch-libvirt.c      | 21 +++++++++++++++++++++
 lib/launch-uml.c          | 10 ++++++++++
 6 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/generator/actions_core.ml b/generator/actions_core.ml
index cb7e8dcd0..9de3a6484 100644
--- a/generator/actions_core.ml
+++ b/generator/actions_core.ml
@@ -210,7 +210,7 @@ this function fails and the C<errno> is set to
C<EINVAL>." };
 
   { defaults with
     name = "add_drive"; added = (0, 0, 3);
-    style = RErr, [String (PlainString, "filename")], [OBool
"readonly"; OString "format"; OString "iface";
OString "name"; OString "label"; OString
"protocol"; OStringList "server"; OString
"username"; OString "secret"; OString "cachemode";
OString "discard"; OBool "copyonread"];
+    style = RErr, [String (PlainString, "filename")], [OBool
"readonly"; OString "format"; OString "iface";
OString "name"; OString "label"; OString
"protocol"; OStringList "server"; OString
"username"; OString "secret"; OString "cachemode";
OString "discard"; OBool "copyonread"; OInt
"pblocksize"; OInt "lblocksize"];
     once_had_no_optargs = true;
     blocking = false;
     fish_alias = ["add"];
diff --git a/lib/drives.c b/lib/drives.c
index 5a8d29ab4..bb160cc34 100644
--- a/lib/drives.c
+++ b/lib/drives.c
@@ -58,6 +58,8 @@ struct drive_create_data {
   const char *cachemode;
   enum discard discard;
   bool copyonread;
+  int pblocksize;
+  int lblocksize;
 };
 
 COMPILE_REGEXP (re_hostname_port, "(.*):(\\d+)$", 0)
@@ -114,6 +116,8 @@ create_drive_file (guestfs_h *g,
   drv->cachemode = data->cachemode ? safe_strdup (g, data->cachemode)
: NULL;
   drv->discard = data->discard;
   drv->copyonread = data->copyonread;
+  drv->pblocksize = data->pblocksize;
+  drv->lblocksize = data->lblocksize;
 
   if (data->readonly) {
     if (create_overlay (g, drv) == -1) {
@@ -150,6 +154,8 @@ create_drive_non_file (guestfs_h *g,
   drv->cachemode = data->cachemode ? safe_strdup (g, data->cachemode)
: NULL;
   drv->discard = data->discard;
   drv->copyonread = data->copyonread;
+  drv->pblocksize = data->pblocksize;
+  drv->lblocksize = data->lblocksize;
 
   if (data->readonly) {
     if (create_overlay (g, drv) == -1) {
@@ -767,6 +773,14 @@ guestfs_impl_add_drive_opts (guestfs_h *g, const char
*filename,
     optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_COPYONREAD_BITMASK
     ? optargs->copyonread : false;
 
+  data.pblocksize +    optargs->bitmask &
GUESTFS_ADD_DRIVE_OPTS_PBLOCKSIZE_BITMASK
+    ? optargs->pblocksize : 0;
+
+  data.lblocksize +    optargs->bitmask &
GUESTFS_ADD_DRIVE_OPTS_LBLOCKSIZE_BITMASK
+    ? optargs->lblocksize : 0;
+
   if (data.readonly && data.discard == discard_enable) {
     error (g, _("discard support cannot be enabled on read-only
drives"));
     free_drive_servers (data.servers, data.nr_servers);
diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index 6799c265d..20f22a674 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -261,6 +261,8 @@ struct drive {
   char *cachemode;
   enum discard discard;
   bool copyonread;
+  int pblocksize;
+  int lblocksize;
 };
 
 /* Extra hv parameters (from guestfs_config). */
diff --git a/lib/launch-direct.c b/lib/launch-direct.c
index ae6ca093b..518bd24fc 100644
--- a/lib/launch-direct.c
+++ b/lib/launch-direct.c
@@ -273,6 +273,26 @@ add_drive_standard_params (guestfs_h *g, struct
backend_direct_data *data,
   return -1;
 }
 
+/**
+ * Add the blockio elements of the C<-device> parameter.
+ */
+static int
+add_device_blockio_params (guestfs_h *g, struct qemuopts *qopts,
+                           struct drive *drv)
+{
+  if (drv->pblocksize)
+    append_list_format ("physical_block_size=%d",
drv->pblocksize);
+  if (drv->lblocksize)
+    append_list_format ("logical_block_size=%d", drv->lblocksize);
+
+  return 0;
+
+  /* This label is called implicitly from the qemuopts macros on error. */
+ qemuopts_error:
+  perrorf (g, "qemuopts");
+  return -1;  
+}
+
 static int
 add_drive (guestfs_h *g, struct backend_direct_data *data,
            struct qemuopts *qopts, size_t i, struct drive *drv)
@@ -291,6 +311,8 @@ add_drive (guestfs_h *g, struct backend_direct_data *data,
       append_list_format ("drive=hd%zu", i);
       if (drv->disk_label)
         append_list_format ("serial=%s", drv->disk_label);
+      if (add_device_blockio_params (g, qopts, drv) == -1)
+        return -1;
     } end_list ();
   }
 #if defined(__arm__) || defined(__aarch64__) || defined(__powerpc__)
@@ -317,6 +339,8 @@ add_drive (guestfs_h *g, struct backend_direct_data *data,
       append_list_format ("drive=hd%zu", i);
       if (drv->disk_label)
         append_list_format ("serial=%s", drv->disk_label);
+      if (add_device_blockio_params (g, qopts, drv) == -1)
+        return -1;
     } end_list ();
   }
 
diff --git a/lib/launch-libvirt.c b/lib/launch-libvirt.c
index f2cad9300..4ce2fa683 100644
--- a/lib/launch-libvirt.c
+++ b/lib/launch-libvirt.c
@@ -1043,6 +1043,7 @@ static int construct_libvirt_xml_disk (guestfs_h *g, const
struct backend_libvir
 static int construct_libvirt_xml_disk_target (guestfs_h *g, xmlTextWriterPtr
xo, size_t drv_index);
 static int construct_libvirt_xml_disk_driver_qemu (guestfs_h *g, const struct
backend_libvirt_data *data, struct drive *drv, xmlTextWriterPtr xo, const char
*format, const char *cachemode, enum discard discard, bool copyonread);
 static int construct_libvirt_xml_disk_address (guestfs_h *g, xmlTextWriterPtr
xo, size_t drv_index);
+static int construct_libvirt_xml_disk_blockio (guestfs_h *g, xmlTextWriterPtr
xo, int pblocksize, int lblocksize);
 static int construct_libvirt_xml_disk_source_hosts (guestfs_h *g,
xmlTextWriterPtr xo, const struct drive_source *src);
 static int construct_libvirt_xml_disk_source_seclabel (guestfs_h *g, const
struct backend_libvirt_data *data, xmlTextWriterPtr xo);
 static int construct_libvirt_xml_appliance (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
@@ -1578,6 +1579,10 @@ construct_libvirt_xml_disk (guestfs_h *g,
     if (construct_libvirt_xml_disk_address (g, xo, drv_index) == -1)
       return -1;
 
+    if (construct_libvirt_xml_disk_blockio (g, xo, drv->pblocksize,
+                                            drv->lblocksize) == -1)
+      return -1;
+
   } end_element (); /* </disk> */
 
   return 0;
@@ -1685,6 +1690,22 @@ construct_libvirt_xml_disk_address (guestfs_h *g,
xmlTextWriterPtr xo,
   return 0;
 }
 
+static int construct_libvirt_xml_disk_blockio (guestfs_h *g,
+                                               xmlTextWriterPtr xo,
+                                               int pblocksize, int lblocksize)
+{
+  if (pblocksize || lblocksize) {
+    start_element ("blockio") {
+      if (pblocksize)
+        attribute_format ("physical_block_size", "%d",
pblocksize);
+      if (lblocksize)
+        attribute_format ("logical_block_size", "%d",
lblocksize);
+    } end_element ();
+  }
+
+  return 0;
+}
+
 static int
 construct_libvirt_xml_disk_source_hosts (guestfs_h *g,
                                          xmlTextWriterPtr xo,
diff --git a/lib/launch-uml.c b/lib/launch-uml.c
index da20c17d9..274287b58 100644
--- a/lib/launch-uml.c
+++ b/lib/launch-uml.c
@@ -124,6 +124,16 @@ uml_supported (guestfs_h *g)
              _("uml backend does not support drives with ‘discard’
parameter set to ‘enable’"));
       return false;
     }
+    if (drv->pblocksize) {
+      error (g,
+             _("uml backend does not support drives with
‘physical_block_size’ parameter"));
+      return false;
+    }
+    if (drv->lblocksize) {
+      error (g,
+             _("uml backend does not support drives with
‘logical_block_size’ parameter"));
+      return false;
+    }
   }
 
   return true;
-- 
2.17.2

On Sat, Feb 08, 2020 at 01:25:28AM +0200, Mykola Ivanets
wrote:
> From: Nikolay Ivanets <stenavin@gmail.com>
> 
> I faced with situation where libguestfs cannot recognize partitions on a
> disk image which was partitioned on a system with "4K native"
sector
> size support.
> 
> In order to fix the issue we need to allow users to specify desired
> physical and/or logical block size per drive basis.
> 
> It is definitely not a complete patch but rather a way to request for a
> comments.  Nevertheless it is already working patch.  I've added an
> optional parameters to add_drive API method which allow specifying
> physical and logical block size for a drive separetely.
> 
> There are no documentation and tests yet. Input parameters are not
> validated for correctness.
The patch is generally fine, but ...
> Here are my questions:
> - Am I move in the right direction adding support for physical/logical
> block size?
I'm fairly sure we only need one of these.  I'm just not
sure which one we need.  I think we need to ask an expert
or look at the qemu / kernel code to find out exactly what
each setting really does.
> - I'm not sure I've made a good choise for parameter names:
'pblocksize'
> and 'lblocksize' respectively.  I've tried to avoid long names
like
> 'physicalblocksize' while keeping readability and semantic.
If we only have one, we can use "blocksize".  But it does
require us to answer the previous one.
> - Do we want to add the same optional parameters to
'add_drive_scratch'
> API method?  I think it would be nice but it is up to you.
It should also be added to add_domain and add_libvirt_dom (note all
the APIs which have 'discard' and 'copyonread' already).

It could be added to add_drive_scratch, I guess.  However it doesn't
seem very useful for scratch drives (why create a scratch drive with
4K sectors which will be thrown away in the end?)
> - What about 'add_dom', 'add_libvirt_dom' API methods? 
Should they also
> handle 'blokio' tag in domain XML and act respectively?
Yes.
> - Anything else I didn't spot yet?
> - Do we want guestfish to accept physical/logical block size per drive
> from command line?
If you can be bothered, but best to put it in a second patch.
> - What about other virt tools like virt-df, virt-cat and so on?
If you change the command line handling, then it should apply to other
tools mostly automatically.  Have a look at how the --format option
works.

Thanks,

Rich.
> Sorry for a long list of questions.
> ---
>  generator/actions_core.ml |  2 +-
>  lib/drives.c              | 14 ++++++++++++++
>  lib/guestfs-internal.h    |  2 ++
>  lib/launch-direct.c       | 24 ++++++++++++++++++++++++
>  lib/launch-libvirt.c      | 21 +++++++++++++++++++++
>  lib/launch-uml.c          | 10 ++++++++++
>  6 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/generator/actions_core.ml b/generator/actions_core.ml
> index cb7e8dcd0..9de3a6484 100644
> --- a/generator/actions_core.ml
> +++ b/generator/actions_core.ml
> @@ -210,7 +210,7 @@ this function fails and the C<errno> is set to
C<EINVAL>." };
>  
>    { defaults with
>      name = "add_drive"; added = (0, 0, 3);
> -    style = RErr, [String (PlainString, "filename")], [OBool
"readonly"; OString "format"; OString "iface";
OString "name"; OString "label"; OString
"protocol"; OStringList "server"; OString
"username"; OString "secret"; OString "cachemode";
OString "discard"; OBool "copyonread"];
> +    style = RErr, [String (PlainString, "filename")], [OBool
"readonly"; OString "format"; OString "iface";
OString "name"; OString "label"; OString
"protocol"; OStringList "server"; OString
"username"; OString "secret"; OString "cachemode";
OString "discard"; OBool "copyonread"; OInt
"pblocksize"; OInt "lblocksize"];
>      once_had_no_optargs = true;
>      blocking = false;
>      fish_alias = ["add"];
> diff --git a/lib/drives.c b/lib/drives.c
> index 5a8d29ab4..bb160cc34 100644
> --- a/lib/drives.c
> +++ b/lib/drives.c
> @@ -58,6 +58,8 @@ struct drive_create_data {
>    const char *cachemode;
>    enum discard discard;
>    bool copyonread;
> +  int pblocksize;
> +  int lblocksize;
>  };
>  
>  COMPILE_REGEXP (re_hostname_port, "(.*):(\\d+)$", 0)
> @@ -114,6 +116,8 @@ create_drive_file (guestfs_h *g,
>    drv->cachemode = data->cachemode ? safe_strdup (g,
data->cachemode) : NULL;
>    drv->discard = data->discard;
>    drv->copyonread = data->copyonread;
> +  drv->pblocksize = data->pblocksize;
> +  drv->lblocksize = data->lblocksize;
>  
>    if (data->readonly) {
>      if (create_overlay (g, drv) == -1) {
> @@ -150,6 +154,8 @@ create_drive_non_file (guestfs_h *g,
>    drv->cachemode = data->cachemode ? safe_strdup (g,
data->cachemode) : NULL;
>    drv->discard = data->discard;
>    drv->copyonread = data->copyonread;
> +  drv->pblocksize = data->pblocksize;
> +  drv->lblocksize = data->lblocksize;
>  
>    if (data->readonly) {
>      if (create_overlay (g, drv) == -1) {
> @@ -767,6 +773,14 @@ guestfs_impl_add_drive_opts (guestfs_h *g, const char
*filename,
>      optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_COPYONREAD_BITMASK
>      ? optargs->copyonread : false;
>  
> +  data.pblocksize > +    optargs->bitmask &
GUESTFS_ADD_DRIVE_OPTS_PBLOCKSIZE_BITMASK
> +    ? optargs->pblocksize : 0;
> +
> +  data.lblocksize > +    optargs->bitmask &
GUESTFS_ADD_DRIVE_OPTS_LBLOCKSIZE_BITMASK
> +    ? optargs->lblocksize : 0;
> +
>    if (data.readonly && data.discard == discard_enable) {
>      error (g, _("discard support cannot be enabled on read-only
drives"));
>      free_drive_servers (data.servers, data.nr_servers);
> diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
> index 6799c265d..20f22a674 100644
> --- a/lib/guestfs-internal.h
> +++ b/lib/guestfs-internal.h
> @@ -261,6 +261,8 @@ struct drive {
>    char *cachemode;
>    enum discard discard;
>    bool copyonread;
> +  int pblocksize;
> +  int lblocksize;
>  };
>  
>  /* Extra hv parameters (from guestfs_config). */
> diff --git a/lib/launch-direct.c b/lib/launch-direct.c
> index ae6ca093b..518bd24fc 100644
> --- a/lib/launch-direct.c
> +++ b/lib/launch-direct.c
> @@ -273,6 +273,26 @@ add_drive_standard_params (guestfs_h *g, struct
backend_direct_data *data,
>    return -1;
>  }
>  
> +/**
> + * Add the blockio elements of the C<-device> parameter.
> + */
> +static int
> +add_device_blockio_params (guestfs_h *g, struct qemuopts *qopts,
> +                           struct drive *drv)
> +{
> +  if (drv->pblocksize)
> +    append_list_format ("physical_block_size=%d",
drv->pblocksize);
> +  if (drv->lblocksize)
> +    append_list_format ("logical_block_size=%d",
drv->lblocksize);
> +
> +  return 0;
> +
> +  /* This label is called implicitly from the qemuopts macros on error. */
> + qemuopts_error:
> +  perrorf (g, "qemuopts");
> +  return -1;  
> +}
> +
>  static int
>  add_drive (guestfs_h *g, struct backend_direct_data *data,
>             struct qemuopts *qopts, size_t i, struct drive *drv)
> @@ -291,6 +311,8 @@ add_drive (guestfs_h *g, struct backend_direct_data
*data,
>        append_list_format ("drive=hd%zu", i);
>        if (drv->disk_label)
>          append_list_format ("serial=%s", drv->disk_label);
> +      if (add_device_blockio_params (g, qopts, drv) == -1)
> +        return -1;
>      } end_list ();
>    }
>  #if defined(__arm__) || defined(__aarch64__) || defined(__powerpc__)
> @@ -317,6 +339,8 @@ add_drive (guestfs_h *g, struct backend_direct_data
*data,
>        append_list_format ("drive=hd%zu", i);
>        if (drv->disk_label)
>          append_list_format ("serial=%s", drv->disk_label);
> +      if (add_device_blockio_params (g, qopts, drv) == -1)
> +        return -1;
>      } end_list ();
>    }
>  
> diff --git a/lib/launch-libvirt.c b/lib/launch-libvirt.c
> index f2cad9300..4ce2fa683 100644
> --- a/lib/launch-libvirt.c
> +++ b/lib/launch-libvirt.c
> @@ -1043,6 +1043,7 @@ static int construct_libvirt_xml_disk (guestfs_h *g,
const struct backend_libvir
>  static int construct_libvirt_xml_disk_target (guestfs_h *g,
xmlTextWriterPtr xo, size_t drv_index);
>  static int construct_libvirt_xml_disk_driver_qemu (guestfs_h *g, const
struct backend_libvirt_data *data, struct drive *drv, xmlTextWriterPtr xo, const
char *format, const char *cachemode, enum discard discard, bool copyonread);
>  static int construct_libvirt_xml_disk_address (guestfs_h *g,
xmlTextWriterPtr xo, size_t drv_index);
> +static int construct_libvirt_xml_disk_blockio (guestfs_h *g,
xmlTextWriterPtr xo, int pblocksize, int lblocksize);
>  static int construct_libvirt_xml_disk_source_hosts (guestfs_h *g,
xmlTextWriterPtr xo, const struct drive_source *src);
>  static int construct_libvirt_xml_disk_source_seclabel (guestfs_h *g, const
struct backend_libvirt_data *data, xmlTextWriterPtr xo);
>  static int construct_libvirt_xml_appliance (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
> @@ -1578,6 +1579,10 @@ construct_libvirt_xml_disk (guestfs_h *g,
>      if (construct_libvirt_xml_disk_address (g, xo, drv_index) == -1)
>        return -1;
>  
> +    if (construct_libvirt_xml_disk_blockio (g, xo, drv->pblocksize,
> +                                            drv->lblocksize) == -1)
> +      return -1;
> +
>    } end_element (); /* </disk> */
>  
>    return 0;
> @@ -1685,6 +1690,22 @@ construct_libvirt_xml_disk_address (guestfs_h *g,
xmlTextWriterPtr xo,
>    return 0;
>  }
>  
> +static int construct_libvirt_xml_disk_blockio (guestfs_h *g,
> +                                               xmlTextWriterPtr xo,
> +                                               int pblocksize, int
lblocksize)
> +{
> +  if (pblocksize || lblocksize) {
> +    start_element ("blockio") {
> +      if (pblocksize)
> +        attribute_format ("physical_block_size", "%d",
pblocksize);
> +      if (lblocksize)
> +        attribute_format ("logical_block_size", "%d",
lblocksize);
> +    } end_element ();
> +  }
> +
> +  return 0;
> +}
> +
>  static int
>  construct_libvirt_xml_disk_source_hosts (guestfs_h *g,
>                                           xmlTextWriterPtr xo,
> diff --git a/lib/launch-uml.c b/lib/launch-uml.c
> index da20c17d9..274287b58 100644
> --- a/lib/launch-uml.c
> +++ b/lib/launch-uml.c
> @@ -124,6 +124,16 @@ uml_supported (guestfs_h *g)
>               _("uml backend does not support drives with ‘discard’
parameter set to ‘enable’"));
>        return false;
>      }
> +    if (drv->pblocksize) {
> +      error (g,
> +             _("uml backend does not support drives with
‘physical_block_size’ parameter"));
> +      return false;
> +    }
> +    if (drv->lblocksize) {
> +      error (g,
> +             _("uml backend does not support drives with
‘logical_block_size’ parameter"));
> +      return false;
> +    }
>    }
>  
>    return true;
> -- 
> 2.17.2
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Sat, Feb 08, 2020 at 01:25:28AM +0200, Mykola Ivanets
wrote:
> From: Nikolay Ivanets <stenavin@gmail.com>
> 
> I faced with situation where libguestfs cannot recognize partitions on a
> disk image which was partitioned on a system with "4K native"
sector
> size support.
Do you have a small test case for this?
> In order to fix the issue we need to allow users to specify desired
> physical and/or logical block size per drive basis.
It seems like physical_block_size / logical_block_size in qemu are
completely undocumented.  However I did some experiments with patching
libguestfs and examining the qemu and parted code.  Here are my
observations:

(1) Setting only physical_block_size = 4096 seems to do nothing.

(2) Setting only logical_block_size = 4096 is explicitly rejected by
virtio-scsi:

https://git.qemu.org/?p=qemu.git;a=blob;f=hw/scsi/scsi-disk.c;h=10d0794d60f196f177563aae00bed2181f5c1bb1;hb=HEAD#l2352

(A similar test exists for virtio-blk)

(3) Setting both physical_block_size = logical_block_size = 4096
changes how parted partitions GPT disks.  The partition table is
clearly using 4K sectors as you can see by examining the disk
afterwards with hexdump.

(4) Neither setting changes MBR partitioning by parted, although my
interpretation of Wikipedia indicates that it should be possible to
create a MBR disk with 4K sector size.  Maybe I'm doing something
wrong, or parted just doesn't support this case.

So it appears that we should just have one blocksize control (maybe
called "sectorsize"?) which sets both physical_block_size and
logical_block_size to the same value.  It may also be worth enforcing
that blocksize/sectorsize must be set to 512 or 4096 (which we can
relax later if necessary).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

пн, 10 лют. 2020 о 10:53 Richard W.M. Jones <rjones@redhat.com>
пише:
>
> On Sat, Feb 08, 2020 at 01:25:28AM +0200, Mykola Ivanets wrote:
> > From: Nikolay Ivanets <stenavin@gmail.com>
> >
> > I faced with situation where libguestfs cannot recognize partitions on
a
> > disk image which was partitioned on a system with "4K
native" sector
> > size support.
> >
> > In order to fix the issue we need to allow users to specify desired
> > physical and/or logical block size per drive basis.
> >
> > It is definitely not a complete patch but rather a way to request for
a
> > comments.  Nevertheless it is already working patch.  I've added
an
> > optional parameters to add_drive API method which allow specifying
> > physical and logical block size for a drive separetely.
> >
> > There are no documentation and tests yet. Input parameters are not
> > validated for correctness.
>
> The patch is generally fine, but ...
>
> > Here are my questions:
> > - Am I move in the right direction adding support for physical/logical
> > block size?
>
> I'm fairly sure we only need one of these.  I'm just not
> sure which one we need.  I think we need to ask an expert
> or look at the qemu / kernel code to find out exactly what
> each setting really does.
Here are what they do stand for:
physical_block_size: The physical block size the disk will report to
the guest OS. For Linux this would be the value returned by the
BLKPBSZGET ioctl and describes the disk's hardware sector size which
can be relevant for the alignment of disk data. We don't have an API
to get this one.

logical_block_size: The logical block size the disk will report to the
guest OS. For Linux this would be the value returned by the BLKSSZGET
ioctl and describes the smallest units for disk I/O. We have
blockdev-getsz API to get this value.

How do they use. If your HDD has physical block size = 4096 you might
want make I/O request equals to (or multiple) this value. Otherwise
you might hit performance penalty. I think, the same is valid for
virtual disk image which is located on physical storage with 4K
physical sector size.
> > - I'm not sure I've made a good choise for parameter names:
'pblocksize'
> > and 'lblocksize' respectively.  I've tried to avoid long
names like
> > 'physicalblocksize' while keeping readability and semantic.
>
> If we only have one, we can use "blocksize".  But it does
> require us to answer the previous one.
Here are possible combinations of [pl]blocksize in real world:

physical | logical
-----------------------
4096      | 4096
4096      | 512
512        | 512

Having both values equals works except for case #2 which will not
impact on functionality, only performance might be hit.
If we want to simplify our API - we might expose the only parameter
called 'blocksize' and set physical_block_size and logical_block_size
to the same value.

On the other hand, reading this values from libvirt XML we will take
logical_block_size and attach disk to libguestfs with
physical_block_size == logical_block_size.
> > - Do we want to add the same optional parameters to
'add_drive_scratch'
> > API method?  I think it would be nice but it is up to you.
>
> It should also be added to add_domain and add_libvirt_dom (note all
> the APIs which have 'discard' and 'copyonread' already).
Yeah, already did that.
> It could be added to add_drive_scratch, I guess.  However it doesn't
> seem very useful for scratch drives (why create a scratch drive with
> 4K sectors which will be thrown away in the end?)
For testing purpose and API consistency, or again if you have 4K
backing storage.
> > - What about 'add_dom', 'add_libvirt_dom' API methods?
Should they also
> > handle 'blokio' tag in domain XML and act respectively?
>
> Yes.
Yeah, already did that.
> > - Anything else I didn't spot yet?
> > - Do we want guestfish to accept physical/logical block size per drive
> > from command line?
>
> If you can be bothered, but best to put it in a second patch.
OK.
> > - What about other virt tools like virt-df, virt-cat and so on?
>
> If you change the command line handling, then it should apply to other
> tools mostly automatically.  Have a look at how the --format option
> works.
Yeah, I spot that already.

--
  Mykola Ivanets

пн, 10 лют. 2020 о 13:43 Richard W.M. Jones <rjones@redhat.com>
пише:
>
> On Sat, Feb 08, 2020 at 01:25:28AM +0200, Mykola Ivanets wrote:
> > From: Nikolay Ivanets <stenavin@gmail.com>
> >
> > I faced with situation where libguestfs cannot recognize partitions on
a
> > disk image which was partitioned on a system with "4K
native" sector
> > size support.
>
> Do you have a small test case for this?
We can easily create one with patched libguestfs and attach disk to
unpatched libguestfs.
> > In order to fix the issue we need to allow users to specify desired
> > physical and/or logical block size per drive basis.
>
> It seems like physical_block_size / logical_block_size in qemu are
> completely undocumented.  However I did some experiments with patching
> libguestfs and examining the qemu and parted code.  Here are my
> observations:
>
> (1) Setting only physical_block_size = 4096 seems to do nothing.
See my thoughts on this in previous email.
> (2) Setting only logical_block_size = 4096 is explicitly rejected by
> virtio-scsi:
>
>
https://git.qemu.org/?p=qemu.git;a=blob;f=hw/scsi/scsi-disk.c;h=10d0794d60f196f177563aae00bed2181f5c1bb1;hb=HEAD#l2352
>
> (A similar test exists for virtio-blk)
>
> (3) Setting both physical_block_size = logical_block_size = 4096
> changes how parted partitions GPT disks.  The partition table is
> clearly using 4K sectors as you can see by examining the disk
> afterwards with hexdump.
>
> (4) Neither setting changes MBR partitioning by parted, although my
> interpretation of Wikipedia indicates that it should be possible to
> create a MBR disk with 4K sector size.  Maybe I'm doing something
> wrong, or parted just doesn't support this case.
>
> So it appears that we should just have one blocksize control (maybe
> called "sectorsize"?) which sets both physical_block_size and
> logical_block_size to the same value.  It may also be worth enforcing
> that blocksize/sectorsize must be set to 512 or 4096 (which we can
> relax later if necessary).
If we stick with the only parameter, I think blocksize might be better name,
especially if we want to split this parameter somewhere latter.

Here are more precise restrictions:

Both values must be a power of 2 between 512 and 32768.
logical_block_size must be
less or equals to physical_block_size.

--
  Mykola Ivanets

On Sat, Feb 08, 2020 at 01:25:28AM +0200, Mykola Ivanets
wrote:
> From: Nikolay Ivanets <stenavin@gmail.com>
> 
> I faced with situation where libguestfs cannot recognize partitions on a
> disk image which was partitioned on a system with "4K native"
sector
> size support.
What type of partition table is this problem seen with ?  MBR or GPT or
both ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

пн, 10 лют. 2020 о 14:34 Daniel P. Berrangé <berrange@redhat.com>
пише:
>
> On Sat, Feb 08, 2020 at 01:25:28AM +0200, Mykola Ivanets wrote:
> > From: Nikolay Ivanets <stenavin@gmail.com>
> >
> > I faced with situation where libguestfs cannot recognize partitions on
a
> > disk image which was partitioned on a system with "4K
native" sector
> > size support.
>
> What type of partition table is this problem seen with ?  MBR or GPT or
> both ?
I have virtual disk with Win 2019 installed on GPT where LBA1 (GPT
Header) at offset 4096.
I've attached the file where you can read how Windows sees its volumes.
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange
:|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com
:|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange
:|
>

Am 10.02.2020 um 12:43 hat Richard W.M. Jones
geschrieben:
> On Sat, Feb 08, 2020 at 01:25:28AM +0200, Mykola Ivanets wrote:
> > From: Nikolay Ivanets <stenavin@gmail.com>
> > 
> > I faced with situation where libguestfs cannot recognize partitions on
a
> > disk image which was partitioned on a system with "4K
native" sector
> > size support.
> 
> Do you have a small test case for this?
> 
> > In order to fix the issue we need to allow users to specify desired
> > physical and/or logical block size per drive basis.
> 
> It seems like physical_block_size / logical_block_size in qemu are
> completely undocumented.  However I did some experiments with patching
> libguestfs and examining the qemu and parted code.  Here are my
> observations:
> 
> (1) Setting only physical_block_size = 4096 seems to do nothing.
The guest sees the physical_block_size and can try to keep its requests
aligned as an optimisation. But it doesn't actually make a semantic
difference as to how the content of the disk is accessed.
> (2) Setting only logical_block_size = 4096 is explicitly rejected by
> virtio-scsi:
> 
>
https://git.qemu.org/?p=qemu.git;a=blob;f=hw/scsi/scsi-disk.c;h=10d0794d60f196f177563aae00bed2181f5c1bb1;hb=HEAD#l2352
> 
> (A similar test exists for virtio-blk)
> 
> (3) Setting both physical_block_size = logical_block_size = 4096
> changes how parted partitions GPT disks.  The partition table is
> clearly using 4K sectors as you can see by examining the disk
> afterwards with hexdump.
This is what you want for emulating a 4k native disk.
> (4) Neither setting changes MBR partitioning by parted, although my
> interpretation of Wikipedia indicates that it should be possible to
> create a MBR disk with 4K sector size.  Maybe I'm doing something
> wrong, or parted just doesn't support this case.
I seem to remember that 4k native disks require GPT, but if you say you
read otherwise, I'm not 100% sure about this any more.
> So it appears that we should just have one blocksize control (maybe
> called "sectorsize"?) which sets both physical_block_size and
> logical_block_size to the same value.  It may also be worth enforcing
> that blocksize/sectorsize must be set to 512 or 4096 (which we can
> relax later if necessary).
A single option (to control logical_block_size) makes sense for
libguestfs. physical_block_size is only relevant for the appliance and
not for the resulting image, so it can be treated as an implementation
detail.

Kevin

On 10/02/20 12:43, Richard W.M. Jones wrote:
> So it appears that we should just have one blocksize control (maybe
> called "sectorsize"?) which sets both physical_block_size and
> logical_block_size to the same value.  It may also be worth enforcing
> that blocksize/sectorsize must be set to 512 or 4096 (which we can
> relax later if necessary).
Yes, physical_sector_size is basically a hint to the OS and is of rather
limited usefulness.

Paolo