Re: [PATCH v2 3/4] qcow2: Avoid feature name extension on small cluster size

[Max Reitz]

On 24.03.20 18:42, Eric Blake wrote:
> As the feature name table can be quite large (over 9k if all 64 bits
> of all three feature fields have names; a mere 8 features leaves only
> 8 bytes for a backing file name in a 512-byte cluster), it is unwise
> to emit this optional header in images with small cluster sizes.
> 
> Update iotest 036 to skip running on small cluster sizes; meanwhile,
> note that iotest 061 never passed on alternative cluster sizes
> (however, I limited this patch to tests with output affected by adding
> feature names, rather than auditing for other tests that are not
> robust to alternative cluster sizes).

That’s a bit more brave than necessary, considering I don’t think anyone
has ever run the iotests with the cluster_size option.  (I certainly
don’t, and I don’t plan to, because I don’t think it’s that important to
test that.)  There are certainly many other iotests that fail with a
non-default cluster size.

Not that it’s wrong care about it.  On the opposite, I’m happy you do. :)

> Signed-off-by: Eric Blake <address@hidden>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Reviewed-by: Alberto Garcia <address@hidden>
> ---
>  block/qcow2.c          | 11 +++++++++--
>  tests/qemu-iotests/036 |  6 ++++--
>  tests/qemu-iotests/061 |  6 ++++--
>  3 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 67b0c214fba4..9475ace57163 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2823,8 +2823,15 @@ int qcow2_update_header(BlockDriverState *bs)
>          buflen -= ret;
>      }
> 
> -    /* Feature table */
> -    if (s->qcow_version >= 3) {
> +    /*
> +     * Feature table.  A mere 8 feature names occupies 392 bytes, and
> +     * when coupled with the v3 minimum header of 104 bytes plus the
> +     * 8-byte end-of-extension marker, that would leave only 8 bytes
> +     * for a backing file name in an image with 512-byte clusters.
> +     * Thus, we choose to omit this header for cluster sizes 4k and
> +     * smaller.

Can’t we do this decision more dynamically?  Like, always omit it when
cluster_size - sizeof(features) < 2k/3k/...?

Max

> +     */
> +    if (s->qcow_version >= 3 && s->cluster_size > 4096) {
>          static const Qcow2Feature features[] = {
>              {
>                  .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,

signature.asc
Description: OpenPGP digital signature