Re: [PATCH v3 5/5] block: use chunk_sectors when evaluating stacked atomic write limits

[John Garry <john.g.garry@oracle.com>]

On 03/07/2025 14:31, Mikulas Patocka wrote:
> 
> 
> On Thu, 3 Jul 2025, John Garry wrote:
> 
>> The atomic write unit max value is limited by any stacked device stripe
>> size.
>>
>> It is required that the atomic write unit is a power-of-2 factor of the
>> stripe size.
>>
>> Currently we use io_min limit to hold the stripe size, and check for a
>> io_min <= SECTOR_SIZE when deciding if we have a striped stacked device.
>>
>> Nilay reports that this causes a problem when the physical block size is
>> greater than SECTOR_SIZE [0].
>>
>> Furthermore, io_min may be mutated when stacking devices, and this makes
>> it a poor candidate to hold the stripe size. Such an example (of when
>> io_min may change) would be when the io_min is less than the physical
>> block size.
>>
>> Use chunk_sectors to hold the stripe size, which is more appropriate.
>>
>> [0] https://urldefense.com/v3/__https://lore.kernel.org/linux-block/888f3b1d-7817-4007-b3b3-1a2ea04df771@linux.ibm.com/T/*mecca17129f72811137d3c2f1e477634e77f06781__;Iw!!ACWV5N9M2RV99hQ!OoKnbVR6yxyDj7-7bpZceNOD59hud0wfw_-fZLPgcGi9XdFQyfpfFFmbYzR_HdvM8epaJqe_dCGnIEgDPMze$
>>
>> Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>> ---
>>   block/blk-settings.c | 51 +++++++++++++++++++++++++-------------------
>>   1 file changed, 29 insertions(+), 22 deletions(-)
>>
>> diff --git a/block/blk-settings.c b/block/blk-settings.c
>> index 7ca21fb32598..20d3563f5d3f 100644
>> --- a/block/blk-settings.c
>> +++ b/block/blk-settings.c
>> @@ -596,41 +596,47 @@ static bool blk_stack_atomic_writes_boundary_head(struct queue_limits *t,
>>   	return true;
>>   }
>>   
>> +static inline unsigned int max_pow_of_two_factor(const unsigned int nr)
>> +{
>> +	return 1 << (ffs(nr) - 1);
> 
> This could be changed to "nr & -nr".

Sure, but I doubt if that is a more natural form.

> 
>> +}
>>   
>> -/* Check stacking of first bottom device */
>> -static bool blk_stack_atomic_writes_head(struct queue_limits *t,
>> -				struct queue_limits *b)
>> +static void blk_stack_atomic_writes_chunk_sectors(struct queue_limits *t)
>>   {
>> -	if (b->atomic_write_hw_boundary &&
>> -	    !blk_stack_atomic_writes_boundary_head(t, b))
>> -		return false;
>> +	unsigned int chunk_bytes = t->chunk_sectors << SECTOR_SHIFT;
> 
> What about integer overflow?

I suppose theoretically it could happen, and I'm happy to change.

However there seems to be precedent in assuming it won't:

- in stripe_op_hints(), we hold chunk_size in an unsigned int
- in raid0_set_limits(), we hold mddev->chunk_sectors << 9 in 
lim.io_min, which is an unsigned int type.

Please let me know your thoughts on also changing these sort of 
instances. Is it realistic to expect chunk_bytes > UINT_MAX?

Thanks,
John