QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Max Reitz <mreitz@redhat.com>
To: Eric Blake <eblake@redhat.com>, qemu-devel@nongnu.org
Cc: kwolf@redhat.com,
	Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	Alberto Garcia <berto@igalia.com>,
	qemu-block@nongnu.org
Subject: Re: [PATCH v2 3/4] qcow2: Avoid feature name extension on small cluster size
Date: Wed, 25 Mar 2020 14:52:00 +0100
Message-ID: <61b5c2d0-fc76-cc70-ab25-bd75cd785d69@redhat.com> (raw)
In-Reply-To: <f900616b-8e98-d0b1-efd7-f53b8c241c8f@redhat.com>

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

On 25.03.20 14:18, Eric Blake wrote:
> On 3/25/20 7:42 AM, Max Reitz wrote:
>> 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).
>>
> 
>>> -    /* 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) {
> 
> Yes.  But when you consider that sizeof(features) is a compile-time
> constant, it isn't really dynamic for a given qemu release, but rather a
> different way to spell things; about the only thing it would buy us is
> that our cutoff window for what cluster size no longer gets the header
> may automatically shift from 2k to 4k to 8k as future qemu versions add
> more qcow2 features.

Yes.

> If we want to write it like that, which size limit
> do you propose?  Or asked differently, how much space should we reserve
> for other extension headers + backing file name?

Well, that was the “2k/3k/...” list. :)

The backing file name is limited to 1k, so I suppose that plus 1k for a
potential external data filename, plus 1k for the rest, so 3k in total?

Now that I look into the spec, I see that it actually doesn’t say that
the backing filename has to be part of the header cluster.  But, well.

It also only says that the image header must be part of the first
cluster, which in my opinion doesn’t necessarily include its extensions.
 So header extensions (and the backing filename) could actually be in
consecutive clusters.  But that of course would be much more difficult
to implement.

Max


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

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-24 17:42 [PATCH for-5.0 v2 0/4] bug fixes extracted from larger qcow2 zero bit work Eric Blake
2020-03-24 17:42 ` [PATCH v2 1/4] qcow2: Comment typo fixes Eric Blake
2020-03-24 17:42 ` [PATCH v2 2/4] qcow2: List autoclear bit names in header Eric Blake
2020-03-24 17:42 ` [PATCH v2 3/4] qcow2: Avoid feature name extension on small cluster size Eric Blake
2020-03-25 12:42   ` Max Reitz
2020-03-25 13:18     ` Eric Blake
2020-03-25 13:52       ` Max Reitz [this message]
2020-03-25 14:01         ` Eric Blake
2020-03-24 17:42 ` [PATCH v2 4/4] sheepdog: Consistently set bdrv_has_zero_init_truncate Eric Blake
2020-03-24 19:33   ` John Snow
2020-03-25 14:45   ` [PATCH-for-5.0 " Philippe Mathieu-Daudé
2020-03-26 13:32 ` [PATCH for-5.0 v2 0/4] bug fixes extracted from larger qcow2 zero bit work Max Reitz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=61b5c2d0-fc76-cc70-ab25-bd75cd785d69@redhat.com \
    --to=mreitz@redhat.com \
    --cc=berto@igalia.com \
    --cc=eblake@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=vsementsov@virtuozzo.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git