Re: [Qemu-devel] [PATCH v7 3/3] qcow2: Discard unaligned tail when wiping image

[Eric Blake]

On 03/31/2017 07:51 AM, Max Reitz wrote:
> On 31.03.2017 00:36, Eric Blake wrote:
>> The previous commit pointed out a subtle difference between the
>> fast and slow path of qcow2_make_empty(), where we failed to discard
>> the final (partial) cluster of an unaligned image.
>>

>> +    /* The caller must cluster-align start; round end down except at EOF */
>> +    assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
>> +    if (end_offset != bs->total_sectors * BDRV_SECTOR_SIZE) {
>> +        end_offset = start_of_cluster(s, end_offset);
>>      }
> 
> This change looks good and works for qcow2_make_empty(), but
> qcow2_co_pdiscard() will still drop these requests:

Yes, but qcow2_co_pdiscard() is advisory, and leaving the last partial
cluster undiscarded (consistent for what we do for all other partial
cluster requests) is different than our documented notion that
qcow2_make_empty() gets rid of all clusters no matter what.

> We don't necessarily have to fix it for 2.9, so:

Agreed that enhancing pdiscard to special-case a partial cluster at EOF
is not a bug fix, and therefore 2.10 material if we even want it.

> 
> Reviewed-by: Max Reitz <address@hidden>
> 
>>
>>      nb_clusters = size_to_clusters(s, end_offset - offset);
>> diff --git a/tests/qemu-iotests/176.out b/tests/qemu-iotests/176.out
>> index 990a41c..6271fa7 100644
>> --- a/tests/qemu-iotests/176.out
>> +++ b/tests/qemu-iotests/176.out
>> @@ -35,7 +35,7 @@ Offset          Length          File
>>  Offset          Length          File
>>  0x7ffd0000      0x10000         TEST_DIR/t.IMGFMT.base
>>  0x7ffe0000      0x20000         TEST_DIR/t.IMGFMT.itmd
>> -0x83400000      0x200           TEST_DIR/t.IMGFMT
>> +0x83400000      0x200           TEST_DIR/t.IMGFMT.itmd

As to your comment about swapping patches 2 and 3, if I did that, then I
would not have this change to 176.out during the bug fix, and would
instead squash this line into the single patch touching the testsuite to
add the enhancement.  How important is the swapped order? Do I need to
respin for that, or is it something a maintainer could tweak, or is the
series fine as-is?  For what it's worth, the policy of single test after
the patch is somewhat opposite of Markus' approach of test first showing
the buggy behavior, then patch that includes the testsuite fix to match
the patch.  But I can live with either order, so I won't respin without
an explicit request to do so.

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

signature.asc
Description: OpenPGP digital signature