Re: [Qemu-block] [PATCH] qcow2: Discard/zero clusters by byte count

[Eric Blake]

On 12/06/2016 03:01 PM, Max Reitz wrote:
> On 03.12.2016 19:19, Eric Blake wrote:
>> Passing a byte offset, but sector count, when we ultimately
>> want to operate on cluster granularity, is madness.  Clean up
>> the interfaces to take byte offset and count.  Rename
>> qcow2_discard_clusters() and qcow2_zero_clusters() to the
>> shorter qcow2_discard() and qcow2_zero() to make sure backports
>> don't get confused by changed units.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> ---
>>
>> 2.9 material, depends on 'Don't strand clusters near 2G intervals
>> during commit'
>>

>> @@ -1595,20 +1593,24 @@ static int zero_single_l2(BlockDriverState *bs, 
>> uint64_t offset,
>>      return nb_clusters;
>>  }
>>
>> -int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int 
>> nb_sectors,
>> -                        int flags)
>> +int qcow2_zero(BlockDriverState *bs, uint64_t offset, uint64_t count,
>> +               int flags)
> 
> Hmm, I personally liked qcow2_zero_clusters() better. qcow2_zero()
> doesn't really express that it means the verb "to zero".
> 
> Also, while you are making a good point why the function should be
> renamed, qcow2_zero_clusters() was much more accurate because offset and
> count are expected to be cluster-aligned.
> 
> The only alternative I can come up with would be "qcow2_write_zeroes";
> that at least solves the first issue I have with this, but not the
> second one...

Maybe qcow2_cluster_zeroize() and qcow2_cluster_discard()? That gets the
benefit of the rename (to force all callers to use the right semantics),
while still being legible as an object-verb naming: the action
('discard' or 'zeroize') is performed on 'qcow2 cluster' objects.

> 
>>  {
>>      BDRVQcow2State *s = bs->opaque;
>>      uint64_t nb_clusters;
>>      int ret;
>>
>> +    /* Block layer guarantees cluster alignment */
> 
> Hm, it's rather qcow2_co_pwrite_zeroes(), isn't it? The block layer will
> split unaligned requests into head, body and tail and it will still
> submit head and tail (though separately).

Hmm, good point.  I'll have to come up with some way to reword that.

> 
> As far as I can see, it's qcow2_co_pwrite_zeroes() which then guarantees
> that only the aligned part gets through to qcow2_zero().
> 
> 
> The patch looks good apart from these nit picks, though.
> 
> Max
> 
>> +    assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
>> +    assert(QEMU_IS_ALIGNED(count, s->cluster_size));

And since I'm adding assertions that the zero operation is never
attempted on unaligned parts, maybe I should also add asserts that
discards are never unaligned, perhaps as a prereq patch.

I'll wait a bit and see if anyone else has better naming ideas for the
functions, before I try to send a v2.

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

signature.asc
Description: OpenPGP digital signature