Re: [PATC] block: update queue limits atomically

[Jens Axboe <axboe@kernel.dk>]

On 3/19/25 8:22 PM, Ming Lei wrote:
> On Wed, Mar 19, 2025 at 10:18:39PM +0100, Mikulas Patocka wrote:
>>
>>
>> On Tue, 18 Mar 2025, Jens Axboe wrote:
>>
>>>> Yeah, it looks fine, but I feel it is still fragile, and not sure it is one
>>>> accepted solution.
>>>
>>> Agree - it'd be much better to have the bio drivers provide the same
>>> guarantees that we get on the request side, rather than play games with
>>> this and pretend that concurrent update and usage is fine.
>>>
>>> -- 
>>> Jens Axboe
>>
>> And what mechanism should they use to read the queue limits?
>> * locking? (would degrade performance)
>> * percpu-rwsem? (no overhead for readers, writers wait for the RCU 
>>   synchronization)
>> * RCU?
>> * anything else?
> 
> 1) queue usage counter is for covering fast IO code path
> 
> - in __submit_bio(), queue usage counter is grabbed when calling
>   ->submit_bio()
> 
> - the only trouble should be from dm-crypt or thin-provision which offloads
> bio submission to other context, so you can grab the usage counter by
> percpu_ref_get(&q->q_usage_counter) until this bio submission or queue
> limit consumption is done

Indeed - this is an entirely solved problem already, it's just that the
bio bypassing gunk thinks it can get away with just bypassing all of
that. The mechanisms very much exist and are used by the request path,
which is why this problem doesn't exist there.

> 2) slow path: dm_set_device_limits
> 
> which is done before DM disk is on, so it should be fine by holding limit lock.
> 
> 3) changing queue limits from bio->end_io() or request completion handler
> 
> - this usage need fix

All looks reasonable.

-- 
Jens Axboe