Re: [Qemu-devel] [PATCH] qemu-img convert: Fix copy length for the iteration

[Kevin Wolf]

Am 11.02.2015 um 15:39 hat Max Reitz geschrieben:
> On 2015-02-11 at 06:08, Kevin Wolf wrote:
> >qemu-img converts already checks the block status of the next sectors to
> >copy and skips them if it knows that they don't need to be copied (e.g.
> >because they are zero and the destination is zero-initialised).
> >
> >It doesn't, however, limit the number of sectors read to the number of
> >allocated sectors when copying data yet. This means that when using -B,
> >some data is needlessly copied from the backing file into the overlay.
> >For images without backing files, it means that more data is read than
> >is necessary, even though the zero buffer detection will still keep
> >those sectors sparse.
> >
> >This patch changes qemu-img to respect the number of allocated sectors
> >and limit its requests accordingly when processing data, which fixes -B
> >to expose the expected behaviour and is a performance optimisation for
> >other cases.
> >
> >Reported-by: Max Reitz <address@hidden>
> >Signed-off-by: Kevin Wolf <address@hidden>
> >---
> >  qemu-img.c                 |  1 +
> >  tests/qemu-iotests/122     | 56 
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/122.out | 11 +++++++++
> >  tests/qemu-iotests/group   |  1 +
> >  4 files changed, 69 insertions(+)
> >  create mode 100755 tests/qemu-iotests/122
> >  create mode 100644 tests/qemu-iotests/122.out
> >
> >diff --git a/qemu-img.c b/qemu-img.c
> >index e148af8..b94afe2 100644
> >--- a/qemu-img.c
> >+++ b/qemu-img.c
> >@@ -1809,6 +1809,7 @@ restart:
> >              }
> >              n = MIN(nb_sectors, bufsectors);
> >+            n = MIN(n, sector_num_next_status - sector_num);
> 
> This needs to be done only if sector_num_next_status > sector_num,
> otherwise we'll run into an infinite loop.
> 
> Apparently I accidentally broke -n for qemu-img convert, but once I
> get it working again, you can see that this command:
> 
> $ qemu-img convert -n -O raw test.qcow2 nbd://localhost:10809
> 
> does not terminate (because out_baseimg is NULL and has_zero_init is
> false due to nbd not supporting that, which means that
> sector_num_next_status is never set to anything but 0).
> 
> Other than that, this looks good to me.

Except that it means that the whole function is crap.

We should unconditionally be doing the bdrv_co_get_block_status() and
save at least the reads if we can. This would also enable us to use
bdrv_co_write_zeroes() for the cases where an explicit zero write is
needed.

I guess it's time to rewrite the whole thing...

Kevin