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.
