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 <mre...@redhat.com> > >Signed-off-by: Kevin Wolf <kw...@redhat.com> > >--- > > 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
