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

[Eric Blake]

On 2/4/20 11:32 AM, Max Reitz wrote:
> On 31.01.20 18:44, Eric Blake wrote:
> > Based-on: <address@hidden>
> > ([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;
+        }
     }

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.

> (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'.

> 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.  Yes, we still need the knob for 
when the common case isn't already smart enough, but the difference in 
avoiding a pre-zeroing pass is noticeable when copying images around 
(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).

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org