Re: [PATCH 03/14] block: add an API to atomically update queue limits

[Christoph Hellwig <hch@lst.de>]

On Tue, Jan 30, 2024 at 11:46:24AM +0000, John Garry wrote:
>> +{
>> +	if (!lim->zoned) {
>> +		if (WARN_ON_ONCE(lim->max_open_zones) ||
>> +		    WARN_ON_ONCE(lim->max_active_zones) ||
>> +		    WARN_ON_ONCE(lim->zone_write_granularity) ||
>> +		    WARN_ON_ONCE(lim->max_zone_append_sectors))
>
> nit: some - like me - prefer {} for multi-line if statements, but that's 
> personal taste
>
>> +			return -EINVAL;

That would be really weird and contrary to the normal Linux style.

>> +	if (!lim->logical_block_size)
>> +		lim->logical_block_size = SECTOR_SIZE;
>> +	if (lim->physical_block_size < lim->logical_block_size)
>> +		lim->physical_block_size = lim->physical_block_size;
>
> I guess that should really be:
> lim->physical_block_size = lim->logical_block_size;

Thanks, that does need fixing.

>> +	lim->max_hw_sectors = round_down(lim->max_hw_sectors,
>> +			lim->logical_block_size >> SECTOR_SHIFT);
>> +
>> +	/*
>> +	 * The actual max_sectors value is a complex beast and also takes the
>> +	 * max_dev_sectors value (set by SCSI ULPs) and a user configurable
>> +	 * value into account.  The ->max_sectors value is always calculated
>> +	 * from these, so directly setting it won't have any effect.
>> +	 */
>> +	max_hw_sectors = min_not_zero(lim->max_hw_sectors,
>> +				lim->max_dev_sectors);
>
> nit: maybe we should use a different variable for this for sake of clarity

What variable name would work better for you?

>> +	/*
>> +	 * We require drivers to at least do logical block aligned I/O, but
>> +	 * historically could not check for that due to the separate calls
>> +	 * to set the limits.  Once the transition is finished the check
>> +	 * below should be narrowed down to check the logical block size.
>> +	 */
>> +	if (!lim->dma_alignment)
>> +		lim->dma_alignment = SECTOR_SIZE - 1;
>
> It would be also nice to update blk_set_default_limits to use this (and not 
> '511') or also any other instances of hard-coding SECTOR_SIZE for 512

That would be nice, but defintively not in scope for this patch.