qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH for-2.8 v2] qcow2: Don't strand clusters near 2G


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH for-2.8 v2] qcow2: Don't strand clusters near 2G intervals during commit
Date: Tue, 6 Dec 2016 08:42:18 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0

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 <address@hidden>
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]