[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v4 2/7] qcow2: Discard/zero clusters by byte cou
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v4 2/7] qcow2: Discard/zero clusters by byte count |
Date: |
Wed, 1 Feb 2017 02:54:26 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 |
On 30.01.2017 17:52, Eric Blake wrote:
> On 01/28/2017 02:21 PM, Max Reitz wrote:
>> On 20.12.2016 20:15, 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 both offset and count as bytes, while
>>> still keeping the assertion added previously that the caller
>>> must align the values to a cluster. Then rename things to
>>> make sure backports don't get confused by changed units:
>>> instead of qcow2_discard_clusters() and qcow2_zero_clusters(),
>>> we now have qcow2_cluster_discard and qcow2_cluster_zeroize().
>>>
>>> Signed-off-by: Eric Blake <address@hidden>
>>>
>
>>> +int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset,
>>> + uint64_t count, int flags);
>>
>> I know byte count parameters are often called "count", but I find it a
>> bit problematic if the function name contains a word that can be a unit.
>> In these cases, it's not entirely clear whether "count" refers to bytes
>> or clusters. Maybe "length" or "size" would be better?
>
> Now that you mention it, 'cluster' and 'count' is indeed confusing. I'm
> leaning towards 'size'. Do I need to respin, or is that something that
> the maintainer is comfortable tweaking?
There's probably not too much that could go wrong, but I wouldn't be
exactly comfortable with it.
>> Purely subjective and thus optional, of course.
>>
>> Another thing: I'm not quite positive whether qcow2_cluster_zeroize()
>> can actually handle a byte count greater than INT_MAX. If you could pass
>> such a number to it, the multiplication "ret * s->cluster_size" might
>> overflow. It's only caller qcow2_co_pwrite_zeroes() will never exceed
>> that limit, but maybe this should be made clear somewhere -- either by
>> making the parameter an int instead of a uint64_t or by asserting it in
>> the function.
>
> I'd lean towards an assertion, especially since it would be nice to
> someday unify all the internal block interfaces to get to a 64-bit
> interface wherever possible
Kevin once said "We should write code that makes sense today, not code
that will make sense some day in the future." Or something like that. :-)
> (limiting ourselves to 32-bit is necessary
> for some drivers, like NBD, but that limitation should be enforced via
> proper BlockLimits, rather than by inherent limits in our API). An
> assertion with 64-bit types is better than yet one more place to audit
> for missing assertions if we use a 32-bit type now and then widen to
> 64-bit later.
Depends on the viewpoint. You're right, it does prevent us from
accidentally implicitly narrowing a 64 bit size, but on the other hand,
it won't be clear just from looking at the function signature that this
function actually only supports a 32 bit parameter.
Not sure what to do here. Maybe switch to a language that would warn us
before implicitly narrowing values. */me ducks*
As long as the assertion is visible enough (i.e. immediately after the
variable declaration block), it should be fine.
Max
>>> -int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
>>> - int nb_sectors, enum qcow2_discard_type type, bool full_discard)
>>> +int qcow2_cluster_discard(BlockDriverState *bs, uint64_t offset,
>>> + uint64_t count, enum qcow2_discard_type type,
>>> + bool full_discard)
>>> {
>>> BDRVQcow2State *s = bs->opaque;
>>> - uint64_t end_offset;
>>> + uint64_t end_offset = offset + count;
>>> uint64_t nb_clusters;
>>> int ret;
>>>
>>> - end_offset = offset + (nb_sectors << BDRV_SECTOR_BITS);
>>> -
>>> /* Caller must pass aligned values */
>>> assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
>>> assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size));
>>
>> Directly below this we have "offset - end_offset" which could be
>> shortened to "count" (or "length" or "size" or...).
>
> Okay, there's now enough comments that it's worth me respinning a v5.
signature.asc
Description: OpenPGP digital signature