Re: [PATCH v3 1/5] blk-mq-sched: introduce high level elevator lock

[Damien Le Moal <dlemoal@kernel.org>]

On 8/11/25 10:01, Yu Kuai wrote:
>>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
>>> index 55a0fd105147..1a2da5edbe13 100644
>>> --- a/block/blk-mq-sched.c
>>> +++ b/block/blk-mq-sched.c
>>> @@ -113,7 +113,14 @@ static int __blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
>>>   		if (budget_token < 0)
>>>   			break;
>>>   
>>> -		rq = e->type->ops.dispatch_request(hctx);
>>> +		if (blk_queue_sq_sched(q)) {
>>> +			elevator_lock(e);
>>> +			rq = e->type->ops.dispatch_request(hctx);
>>> +			elevator_unlock(e);
>>
>> I do not think this is safe for bfq since bfq uses the irqsave/irqrestore spin
>> lock variant. If it is safe, this needs a big comment block explaining why
>> and/or the rules regarding the scheduler use of this lock.
> 
> It's correct, however, this patch doesn't change bfq yet, and it's like:
> 
> elevator_lock
> spin_lock_irq(&bfqd->lock)
> spin_unlock_irq(&bfqd->lock)
> elevator_unlock
> 
> Patch 3 remove bfqd->lock and convert this to:
> 
> elevator_lock_irq
> elevator_unlock_irq.

I do not understand. Since q->elevator->lock is already taken here, without IRQ
disabled, how can bfq_dispatch_request method again take this same lock with IRQ
disabled ? That cannot possibly work.

The other side of this is that if you use elevator_lock(e) to call
e->type->ops.dispatch_request(hctx), it means that the scheduler *can NOT* use
that same lock in its completion path, since that can be called from IRQ
context. This may not be needed/a problem right now, but I think this needs a
comment in this patch to mention this.

-- 
Damien Le Moal
Western Digital Research