[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] copy-before-write: allow specifying minimum cluster size
From: |
Fiona Ebner |
Subject: |
Re: [PATCH 1/2] copy-before-write: allow specifying minimum cluster size |
Date: |
Mon, 13 May 2024 15:24:28 +0200 |
User-agent: |
Mozilla Thunderbird |
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
- Re: [PATCH 1/2] copy-before-write: allow specifying minimum cluster size,
Fiona Ebner <=