Re: [PATCH v2 2/5] block: Support atomic writes limits for stacked devices

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

On 30/10/2024 13:50, Christoph Hellwig wrote:
>>   
>> +static void blk_stack_atomic_writes_limits(struct queue_limits *t, struct queue_limits *b)
> Avoid the overly long line here.

sure

> 
>> +	if (t->atomic_write_hw_max) {
> Maybe split this branch and the code for when it is not set into
> separate helpers to keep the function to a size where it can be
> easily understood?

I was trying to reduce indentation, but it does read a bit messy now, so 
I can try break into a smaller function.

> 
>> +	/* Check first bottom device limits */
>> +	if (!b->atomic_write_hw_boundary)
>> +		goto check_unit;
>> +	/*
>> +	 * Ensure atomic write boundary is aligned with chunk sectors. Stacked
>> +	 * devices store chunk sectors in t->io_min.
>> +	 */
>> +	if (b->atomic_write_hw_boundary > t->io_min &&
>> +	    b->atomic_write_hw_boundary % t->io_min)
>> +		goto unsupported;
>> +	else if (t->io_min > b->atomic_write_hw_boundary &&
> No need for the else here.
> 
>> +		 t->io_min % b->atomic_write_hw_boundary)
>> +		goto unsupported;
>> +
>> +	t->atomic_write_hw_boundary = b->atomic_write_hw_boundary;
>> +
>> +check_unit:
> Maybe instead of the check_unit goto just move the checks between the
> goto above and this into a branch?

I'm not sure, but I can try to avoid using the "goto check_unit" just to 
skip code.

> 
> Otherwise this looks conceptually fine to me.

ok, thanks!