[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH] qcow2: Discard/zero clusters by byte count
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH] qcow2: Discard/zero clusters by byte count |
Date: |
Tue, 6 Dec 2016 15:26:00 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 |
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