On 12/03/2016 11:34 AM, Eric Blake wrote: > The qcow2_make_empty() function is reached during 'qemu-img commit', > in order to clear out ALL clusters of an image. However, if the > image cannot use the fast code path (true if the image is format > 0.10, or if the image contains a snapshot), the cluster size is > larger than 512, and the image is larger than 2G in size, then our > choice of sector_step causes problems. Since it is not cluster > aligned, but qcow2_discard_clusters() silently ignores an unaligned > head or tail, we are leaving clusters allocated. > > Enhance the testsuite to expose the flaw, and patch the problem by > ensuring our step size is aligned. > > [qcow2_discard_clusters() is a GROSS interface: it takes a mix of > byte offset and sector count to perform cluster operations. But > fixing it to use a saner byte/byte rather than byte/sector interface, > and/or asserting that the counts are now aligned thanks to both > this patch and commit 3482b9b, is material for another day.] > > Signed-off-by: Eric Blake <ebl...@redhat.com> > --- > block/qcow2.c | 3 +- > tests/qemu-iotests/097 | 41 +++++--- > tests/qemu-iotests/097.out | 249 > +++++++++++++++++++++++++++++++++------------ > 3 files changed, 210 insertions(+), 83 deletions(-)
> +++ b/block/qcow2.c > @@ -2808,7 +2808,8 @@ static int qcow2_make_empty(BlockDriverState *bs) > { > BDRVQcow2State *s = bs->opaque; > uint64_t start_sector; > - int sector_step = INT_MAX / BDRV_SECTOR_SIZE; > + int sector_step = QEMU_ALIGN_DOWN(INT_MAX / BDRV_SECTOR_SIZE, > + s->cluster_size); Oh shoot. I got the units wrong, and made the slow path do more loop iterations than necessary (rounding sectors to cluster size in bytes is inappropriate - either the rounding has to occur before division, or the rounding needs to be by secotrs instead of bytes). I'll send a v2 that gets the math right. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature