Re: [PATCH 00/17] Improve qcow2 all-zero detection

[Max Reitz <mreitz@redhat.com>]

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

On 04.02.20 19:53, Eric Blake wrote:
> On 2/4/20 11:32 AM, Max Reitz wrote:
>> On 31.01.20 18:44, Eric Blake wrote:
>>> Based-on: <20200124103458.1525982-2-david.edmondson@oracle.com>
>>> ([PATCH v2 1/2] qemu-img: Add --target-is-zero to convert)
>>>
>>> I'm working on adding an NBD extension that reports whether an image
>>> is already all zero when the client first connects.  I initially
>>> thought I could write the NBD code to just call bdrv_has_zero_init(),
>>> but that turned out to be a bad assumption that instead resulted in
>>> this patch series.  The NBD patch will come later (and cross-posted to
>>> the NBD protocol, libnbd, nbdkit, and qemu, as it will affect all four
>>> repositories).
>>
>> We had a discussion about this on IRC, and as far as I remember I wasn’t
>> quite sold on the “why”.  So, again, I wonder why this is needed.
>>
>> I mean, it does make intuitive sense to want to know whether an image is
>> fully zero, but if I continue thinking about it I don’t know any case
>> where we would need to figure it out and where we could accept “We don’t
>> know” as an answer.  So I’m looking for use cases, but this cover letter
>> doesn’t mention any.  (And from a quick glance I don’t see this series
>> using the flag, actually.)
> 
> Patch 10/17 has:
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index e60217e6c382..c8519a74f738 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1985,10 +1985,12 @@ static int convert_do_copy(ImgConvertState *s)
>      int64_t sector_num = 0;
> 
>      /* Check whether we have zero initialisation or can get it
> efficiently */
> -    if (!s->has_zero_init && s->target_is_new && s->min_sparse &&
> -        !s->target_has_backing) {
> -        s->has_zero_init = !!(bdrv_known_zeroes(blk_bs(s->target)) &
> -                              BDRV_ZERO_CREATE);
> +    if (!s->has_zero_init && s->min_sparse && !s->target_has_backing) {
> +        ret = bdrv_known_zeroes(blk_bs(s->target));
> +        if (ret & BDRV_ZERO_OPEN ||
> +            (s->target_is_new && ret & BDRV_ZERO_CREATE)) {
> +            s->has_zero_init = true;
> +        }
>      }

OK, I expected users to come in a separate patch.

> That's the use case: when copying into a destination file, it's useful
> to know if the destination already reads as all zeroes, before
> attempting a fallback to bdrv_make_zero(BDRV_REQ_NO_FALLBACK) or calls
> to block status checking for holes.

But that was my point on IRC.  Is it really more useful if
bdrv_make_zero() is just as quick?  (And the fact that NBD doesn’t have
an implementation looks more like a problem with NBD to me.)

(Considering that at least the code we discussed on IRC didn’t work for
preallocated images, which was the one point where we actually have a
problem in practice.)

>> (We have a use case with convert -n to freshly created image files, but
>> my position on this on IRC was that we want the --target-is-zero flag
>> for that anyway: Auto-detection may always break, our preferred default
>> behavior may always change, so if you want convert -n not to touch the
>> target image except to write non-zero data from the source, we need a
>> --target-is-zero flag and users need to use it.  Well, management
>> layers, because I don’t think users would use convert -n anyway.
>>
>> And with --target-is-zero and users effectively having to use it, I
>> don’t think that’s a good example of a use case.)
> 
> Yes, there will still be cases where you have to use --target-is-zero
> because the image itself couldn't report that it already reads as
> zeroes, but there are also enough cases where the destination is already
> known to read zeroes and it's a shame to tell the user that 'you have to
> add --target-is-zero to get faster copying even though we could have
> inferred it on your behalf'.

How is it a shame?  I think only management tools would use convert -n.
 Management tools want reliable behavior.  If you want reliable
behavior, you have to use --target-is-zero anyway.  So I don’t see the
actual benefit for qemu-img convert.

>> I suppose there is the point of blockdev-create + blockdev-mirror: This
>> has exactly the same problem as convert -n.  But again, if you really
>> want blockdev-mirror not just to force-zero the image, you probably need
>> to tell it so explicitly (i.e., with a --target-is-zero flag for
>> blockdev-mirror).
>>
>> (Well, I suppose we could save us a target-is-zero for mirror if we took
>> this series and had a filter driver that force-reports BDRV_ZERO_OPEN.
>> But, well, please no.)
>>
>> But maybe I’m just an idiot and there is no reason not to take this
>> series and make blockdev-create + blockdev-mirror do the sensible thing
>> by default in most cases. *shrug*
> 
> My argument for taking the series _is_ that the common case can be made
> more efficient without user effort.

The thing is, I don’t see the user effort.  I don’t think users use
convert -n or backup manually.  And for management tools, it isn’t
really effort to add another switch.

> Yes, we still need the knob for
> when the common case isn't already smart enough,

But the user can’t know when qemu isn’t smart enough.  So users who care
have to always give the flag.

> but the difference in
> avoiding a pre-zeroing pass is noticeable when copying images around

I’m sure it is, but the question I ask is whether in practice we
wouldn’t get --target-is-zero in all of these cases anyway.


So I’m not sold on “it works most of the time”, because if it’s just
most of the time, then we’ll likely see --target-is-zero all of the time.

OTOH, I suppose that with the new qcow2 extension, it would always work
for the following case:
(1) Create a qcow2 file,
(2) Immediately (with the next qemu-img/QMP invocation) use it as a
target of convert -n or mirror or anything similar.

If so, that means it works reliably all of the time for a common case.
I guess that’d be enough for me.

Max

> (and more than just for qcow2 - my followup series to improve NBD is
> similarly useful given how much work has already been invested in
> mapping NBD into storage access over https in the upper layers like ovirt).
> 



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