[Libguestfs] [RFC] lib: allow to specify physical/logical block size for disks
Richard W.M. Jones
rjones at redhat.com
Mon Feb 10 08:53:35 UTC 2020
On Sat, Feb 08, 2020 at 01:25:28AM +0200, Mykola Ivanets wrote:
> From: Nikolay Ivanets <stenavin at 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
More information about the Libguestfs
mailing list