Re: [RFC PATCH v2 24/26] qcow2: Add the 'extended_l2' option and the QCOW2_INCOMPAT_EXTL2 bit

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6478 bytes --]

On 26.10.19 23:25, Alberto Garcia wrote:
> Now that the implementation of subclusters is complete we can finally
> add the necessary options to create and read images with this feature,
> which we call "extended L2 entries".
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2.c                    |  46 ++++++++++++++
>  block/qcow2.h                    |   8 ++-
>  include/block/block_int.h        |   1 +
>  qapi/block-core.json             |   6 ++
>  tests/qemu-iotests/031.out       |   8 +--
>  tests/qemu-iotests/036.out       |   4 +-
>  tests/qemu-iotests/049.out       | 102 +++++++++++++++----------------
>  tests/qemu-iotests/060.out       |   1 +
>  tests/qemu-iotests/061.out       |  20 +++---
>  tests/qemu-iotests/065           |  18 ++++--
>  tests/qemu-iotests/082.out       |  48 ++++++++++++---
>  tests/qemu-iotests/085.out       |  38 ++++++------
>  tests/qemu-iotests/144.out       |   4 +-
>  tests/qemu-iotests/182.out       |   2 +-
>  tests/qemu-iotests/185.out       |   8 +--
>  tests/qemu-iotests/198.out       |   2 +
>  tests/qemu-iotests/206.out       |   4 ++
>  tests/qemu-iotests/242.out       |   5 ++
>  tests/qemu-iotests/255.out       |   8 +--
>  tests/qemu-iotests/common.filter |   1 +
>  20 files changed, 224 insertions(+), 110 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 537569ce88..b1fa7ab5da 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1347,6 +1347,12 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>      s->subcluster_size = s->cluster_size / s->subclusters_per_cluster;
>      s->subcluster_bits = ctz32(s->subcluster_size);
>  
> +    if (s->subcluster_size < (1 << MIN_CLUSTER_BITS)) {
> +        error_setg(errp, "Unsupported subcluster size: %d", s->subcluster_size);
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
>      /* Check support for various header values */
>      if (header.refcount_order > 6) {
>          error_setg(errp, "Reference count entry width too large; may not "

It feels like this hunk should be part of the patch that added the
s->subcluster_size assignment.

> @@ -2806,6 +2812,11 @@ int qcow2_update_header(BlockDriverState *bs)
>                  .bit  = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR,
>                  .name = "lazy refcounts",
>              },
> +            {
> +                .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
> +                .bit  = QCOW2_INCOMPAT_EXTL2_BITNR,
> +                .name = "extended L2 entries",
> +            },
>          };
>  
>          ret = header_ext_add(buf, QCOW2_EXT_MAGIC_FEATURE_TABLE,
> @@ -3271,6 +3282,27 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
>          goto out;
>      }
>  
> +    if (!qcow2_opts->has_extended_l2) {
> +        qcow2_opts->extended_l2 = false;
> +    }
> +    if (qcow2_opts->extended_l2) {
> +        unsigned min_cluster_size =
> +            (1 << MIN_CLUSTER_BITS) * QCOW_MAX_SUBCLUSTERS_PER_CLUSTER;
> +        if (version < 3) {
> +            error_setg(errp, "Extended L2 entries are only supported with "
> +                       "compatibility level 1.1 and above (use version=v3 or "

Pre-existing, but old-style creation doesn’t allow version=v3.  I
suppose someone(tm) should fix that...

> +                       "greater)");
> +            ret = -EINVAL;
> +            goto out;
> +        }
> +        if (cluster_size < min_cluster_size) {
> +            error_setg(errp, "Extended L2 entries are only supported with "
> +                       "cluster sizes of at least %u bytes", min_cluster_size);
> +            ret = -EINVAL;
> +            goto out;
> +        }
> +    }
> +
>      if (!qcow2_opts->has_preallocation) {
>          qcow2_opts->preallocation = PREALLOC_MODE_OFF;
>      }

[...]

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index aa97ee2641..6e4c81ae1c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -66,6 +66,8 @@
>  #                 standalone (read-only) raw image without looking at qcow2
>  #                 metadata (since: 4.0)
>  #
> +# @extended-l2: true if the image has extended L2 entries (since 4.2)
> +#

I think there should be the same note here as for lazy-refcounts.  So
this field is not present for compat=0.10, and present otherwise.
(Without that note, it looks as if this field maybe is only present if
it’s true.)

(Personally, I’d also prefer it to be an “iff”, but who cares.)

>  # @lazy-refcounts: on or off; only valid for compat >= 1.1
>  #
>  # @corrupt: true if the image has been marked corrupt; only valid for
> @@ -85,6 +87,7 @@
>        'compat': 'str',
>        '*data-file': 'str',
>        '*data-file-raw': 'bool',
> +      '*extended-l2': 'bool',
>        '*lazy-refcounts': 'bool',
>        '*corrupt': 'bool',
>        'refcount-bits': 'int',
> @@ -4372,6 +4375,8 @@
>  # @data-file-raw    True if the external data file must stay valid as a
>  #                   standalone (read-only) raw image without looking at qcow2
>  #                   metadata (default: false; since: 4.0)
> +# @extended-l2      True if the image has extended L2 entries

s/if the image has/to make the image have/ (or something like it)

> +#                   (default: false; since 4.2)
>  # @size             Size of the virtual disk in bytes
>  # @version          Compatibility level (default: v3)
>  # @backing-file     File name of the backing file if a backing file

[...]

> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
> index 9f418b4881..45c0fe746b 100644
> --- a/tests/qemu-iotests/common.filter
> +++ b/tests/qemu-iotests/common.filter
> @@ -137,6 +137,7 @@ _filter_img_create()
>          -e "s# adapter_type=[^ ]*##g" \
>          -e "s# hwversion=[^ ]*##g" \
>          -e "s# lazy_refcounts=\\(on\\|off\\)##g" \
> +        -e "s# extended_l2=\\(on\\|off\\)##g" \
>          -e "s# block_size=[0-9]\\+##g" \
>          -e "s# block_state_zero=\\(on\\|off\\)##g" \
>          -e "s# log_size=[0-9]\\+##g" \

Filtering it seems like a good idea so one could run the iotests with -o
extended_l2=on.  There are still a ton of tests that don’t really seem
filtered, though.  Maybe my “Allow ./check -o data_file” series improves
that? :?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]