On 12/05/2016 01:52 PM, John Snow wrote: > > > On 12/05/2016 10:49 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.
>> +++ b/tests/qemu-iotests/097 >> @@ -46,7 +46,7 @@ _supported_proto file >> _supported_os Linux >> >> >> -# Four passes: >> +# Four main passes: >> # 0: Two-layer backing chain, commit to upper backing file (implicitly) >> # (in this case, the top image will be emptied) >> # 1: Two-layer backing chain, commit to upper backing file (explicitly) >> @@ -56,22 +56,30 @@ _supported_os Linux >> # 3: Two-layer backing chain, commit to lower backing file >> # (in this case, the top image will implicitly stay unchanged) >> # >> +# Each pass is run twice, since qcow2 has different code paths for cleaning >> +# an image depending on whether it has a snapshot. >> +# >> # 020 already tests committing, so this only tests whether image chains are >> # working properly and that all images above the base are emptied; >> therefore, >> -# no complicated patterns are necessary >> +# no complicated patterns are necessary. Check near the 2G mark, as qcow2 >> +# has been buggy at that boundary in the past. >> for i in 0 1 2 3; do >> +for j in 0 1; do >> >> echo >> -echo "=== Test pass $i ===" >> +echo "=== Test pass $i.$j ===" > > I skimmed the test changes, but it looks good. By the way, it was test 0.0 that fails without the change to qcow2.c (the changes added the x.0 passes; the pre-existing x.1 passes all succeed because they use the fast path, and passes 1-3.y succeed because they aren't clearing all clusters). Maybe I could have mentioned that in the commit message, and/or split this into two patches to make the test changes have an easier diff (one to shift the portion of the test image being written from 0-0x30000 out to 0x7ffd0000-0x80000000, the other to double the number of passes). But at this point, it's probably not worth a respin if we're going to get it in 2.8; but I'm okay if the maintainer wants to add the above paragraph into the commit message. > > Reviewed-by: John Snow <js...@redhat.com> > -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
