Re: [PATCH 1/2] copy-before-write: allow specifying minimum cluster size

[Fiona Ebner <f.ebner@proxmox.com>]

Am 26.03.24 um 10:06 schrieb Markus Armbruster:
>> @@ -365,7 +368,13 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>>  
>>      GLOBAL_STATE_CODE();
>>  
>> -    cluster_size = block_copy_calculate_cluster_size(target->bs, errp);
>> +    if (min_cluster_size && !is_power_of_2(min_cluster_size)) {
> 
> min_cluster_size is int64_t, is_power_of_2() takes uint64_t.  Bad if
> min_cluster_size is negative.  Could this happen?
> 

No, because it comes in as a uint32_t via the QAPI (the internal caller
added by patch 2/2 from the backup code also gets the value via QAPI and
there uint32_t is used too).

---snip---

>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 0a72c590a8..85c8f88f6e 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -4625,12 +4625,18 @@
>>  #     @on-cbw-error parameter will decide how this failure is handled.
>>  #     Default 0. (Since 7.1)
>>  #
>> +# @min-cluster-size: Minimum size of blocks used by copy-before-write
>> +#     operations.  Has to be a power of 2.  No effect if smaller than
>> +#     the maximum of the target's cluster size and 64 KiB.  Default 0.
>> +#     (Since 9.0)
>> +#
>>  # Since: 6.2
>>  ##
>>  { 'struct': 'BlockdevOptionsCbw',
>>    'base': 'BlockdevOptionsGenericFormat',
>>    'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
>> -            '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32' } }
>> +            '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32',
>> +            '*min-cluster-size': 'uint32' } }
> 
> Elsewhere in the schema, we use either 'int' or 'size' for cluster-size.
> Why the difference?
> 

The motivation was to disallow negative values up front and have it work
with block_copy_calculate_cluster_size(), whose result is an int64_t. If
I go with 'int', I'll have to add a check to disallow negative values.
If I go with 'size', I'll have to add a check for to disallow too large
values.

Which approach should I go with?

Best Regards,
Fiona