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 <mre...@redhat.com>
>
>>
>> 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
