Re: [Qemu-devel] [PATCH v4 2/7] qcow2: Discard/zero clusters by byte count

[Max Reitz]

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