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

[Max Reitz]

On 31.03.2017 15:56, Eric Blake wrote:
> 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.

Why would we not 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.

Right.

>                       How important is the swapped order?

As you can probably guess, technically not important. But I having
reference outputs that are not actually a reference kind of defeats the
purpose in my opinion.

>                                                            Do I need to
> respin for that, or is it something a maintainer could tweak, or is the
> series fine as-is?

Of course I can fix the code, but changing the commit messages is a bit
more involved...

>                     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.

Well, then consider this an explicit request. :-)

Max

signature.asc
Description: OpenPGP digital signature