Re: [Qemu-block] [PATCH] qemu-img: Enable BDRV_REQ_MAY_UNMAP in convert

[Kevin Wolf]

Am 25.03.2019 um 09:47 hat Kevin Wolf geschrieben:
> Am 24.03.2019 um 01:20 hat Nir Soffer geschrieben:
> > With Kevin's "block: Fix slow pre-zeroing in qemu-img convert"[1]
> > we skip the pre zero step called like this:
> > 
> >     blk_make_zero(s->target, BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK)
> > 
> > And we write zeroes later using:
> > 
> >     blk_co_pwrite_zeroes(s->target,
> >                          sector_num << BDRV_SECTOR_BITS,
> >                          n << BDRV_SECTOR_BITS, 0);
> > 
> > Since we use flags=0, this is translated to NBD_CMD_WRITE_ZEROES with
> > NBD_CMD_FLAG_NO_HOLE flag, which cause the NBD server to allocated space
> > instead of punching a hole.
> > 
> > Here is an example failure:
> > 
> > $ dd if=/dev/urandom of=src.img bs=1M count=5
> > $ truncate -s 50m src.img
> > $ truncate -s 50m dst.img
> > $ nbdkit -f -v -e '' -U nbd.sock file file=dst.img
> > 
> > $ ./qemu-img convert -n src.img nbd:unix:nbd.sock
> > 
> > We can see in nbdkit log that it received the NBD_CMD_FLAG_NO_HOLE
> > (may_trim=0):
> > 
> > nbdkit: file[1]: debug: newstyle negotiation: flags: export 0x4d
> > nbdkit: file[1]: debug: pwrite count=2097152 offset=0
> > nbdkit: file[1]: debug: pwrite count=2097152 offset=2097152
> > nbdkit: file[1]: debug: pwrite count=1048576 offset=4194304
> > nbdkit: file[1]: debug: zero count=33554432 offset=5242880 may_trim=0
> > nbdkit: file[1]: debug: zero count=13631488 offset=38797312 may_trim=0
> > nbdkit: file[1]: debug: flush
> > 
> > And the image became fully allocated:
> > 
> > $ qemu-img info dst.img
> > virtual size: 50M (52428800 bytes)
> > disk size: 50M
> > 
> > With this change we see that nbdkit did not receive the
> > NBD_CMD_FLAG_NO_HOLE (may_trim=1):
> > 
> > nbdkit: file[1]: debug: newstyle negotiation: flags: export 0x4d
> > nbdkit: file[1]: debug: pwrite count=2097152 offset=0
> > nbdkit: file[1]: debug: pwrite count=2097152 offset=2097152
> > nbdkit: file[1]: debug: pwrite count=1048576 offset=4194304
> > nbdkit: file[1]: debug: zero count=33554432 offset=5242880 may_trim=1
> > nbdkit: file[1]: debug: zero count=13631488 offset=38797312 may_trim=1
> > nbdkit: file[1]: debug: flush
> > 
> > And the file is sparse as expected:
> > 
> > $ qemu-img info dst.img
> > virtual size: 50M (52428800 bytes)
> > disk size: 5.0M
> > 
> > Tested on top of Kevin patches:
> > http://lists.nongnu.org/archive/html/qemu-block/2019-03/msg00755.html
> > 
> > I'm not sure this change is correct for all cases, posting for
> > discussion.
> > 
> > [1] http://lists.nongnu.org/archive/html/qemu-block/2019-03/msg00761.html
> > 
> > Signed-off-by: Nir Soffer <address@hidden>
> 
> Good catch!
> 
> >  qemu-img.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 8ee63daeae..ca9deb3758 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -1752,11 +1752,12 @@ static int coroutine_fn 
> > convert_co_write(ImgConvertState *s, int64_t sector_num,
> >                  assert(!s->target_has_backing);
> >                  break;
> >              }
> >              ret = blk_co_pwrite_zeroes(s->target,
> >                                         sector_num << BDRV_SECTOR_BITS,
> > -                                       n << BDRV_SECTOR_BITS, 0);
> > +                                       n << BDRV_SECTOR_BITS,
> > +                                       BDRV_REQ_MAY_UNMAP);
> >              if (ret < 0) {
> >                  return ret;
> >              }
> >              break;
> >          }
> 
> I think the fix is at least almost correct. There's one case that I'm
> unsure about and that is -S 0, which is documented to request a fully
> allocated target image. So at first I thought that for !s->min_sparse
> we shouldn't set the flag.
> 
> But then, we also have the case right above this where we just do
> nothing for s->has_zero_init. This is implementing the same case (known
> zeros on the source), and nobody has ever complained about it. So maybe
> your patch is fine after all.

Nobody seems to disagree, so applied to the block branch.

Kevin