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

[Christoph Hellwig]

On Thu, Jan 25, 2024 at 10:28:49AM +0000, John Garry wrote:
>
>> +
>> +int blk_validate_limits(struct queue_limits *lim)
>> +{
>> +	unsigned int max_hw_sectors;
>> +
>> +	if (!lim->logical_block_size)
>> +		lim->logical_block_size = SECTOR_SIZE;
>
> nit: This function is doing a bit more than validating, as the function 
> name suggests

Well, it validates and fixes up.  But that sucks as a name.  If you
have a good naming suggestion I can pick it up, but I couldn't come
up with a better name.

>> +	if (!lim->physical_block_size)
>> +		lim->physical_block_size = SECTOR_SIZE;
>> +	if (lim->physical_block_size < lim->logical_block_size)
>> +		lim->physical_block_size = lim->physical_block_size;
>> +
>> +	if (!lim->io_min)
>> +		lim->io_min = SECTOR_SIZE;
>> +	if (lim->io_min < lim->physical_block_size)
>> +		lim->io_min = lim->physical_block_size;
>> +
>> +	if (!lim->max_hw_sectors)
>> +		lim->max_hw_sectors = BLK_SAFE_MAX_SECTORS;
>> +	if (WARN_ON_ONCE(lim->max_hw_sectors < PAGE_SIZE / SECTOR_SIZE))
>> +		return -EINVAL;
>
> The WARN_ON_ONCE usage is odd, as it will only ever WARN once for a 
> specific q, while other queues associated with other drivers may have the 
> same limit issue. But I suppose if one issue is fixed, then the other will 
> make itself known...

Yeah.  The idea is to give a loud warning for the API misuse as this
is not an error a normal user action could trigger.

>> +	int error = blk_validate_limits(lim);
>> +
>> +	if (!error) {
>> +		q->limits = *lim;
>
> this is duplicating what blk_alloc_queue() does

I originally had another helper to do the limits assignment
and the blk_apply_bdi_limits.  But as only one caller needs
the blk_apply_bdi_limits it felt a little silly.

>> +static inline struct queue_limits
>> +queue_limits_start_update(struct request_queue *q)
>> +	__acquires(q->limits_lock)
>> +{
>> +	mutex_lock(&q->limits_lock);
>> +	return q->limits;
>> +}
>> +int queue_limits_commit_update(struct request_queue *q,
>> +		struct queue_limits *lim);
>
> It ain't so nice that the code for queue_limits_start_update() and 
> queue_limits_commit_update() pair are in separate files.

We do that for a lot of APIs where part is inline.  And I really do
want queue_limits_start_update inline as passing large structs by
value generates horrible code, and for this API to work it needs
to be returned by value.  In fact it probably should be __always_inline
to avoid gcc doing stupid thing with -Os.