Re: [PATCH 2/2] blk: Fix lock inversion between ioc lock and bfqd lock

On Fri 21-05-21 21:36:05, Ming Lei wrote:

On Fri, May 21, 2021 at 02:05:51PM +0200, Jan Kara wrote:

quoted

On Fri 21-05-21 14:54:09, Ming Lei wrote:

quoted

On Thu, May 20, 2021 at 08:29:49PM -0700, Khazhy Kumykov wrote:

quoted

On Thu, May 20, 2021 at 5:57 PM Ming Lei [off-list ref] wrote:

quoted

On Fri, May 21, 2021 at 12:33:53AM +0200, Jan Kara wrote:

quoted

Lockdep complains about lock inversion between ioc->lock and bfqd->lock:

bfqd -> ioc:
 put_io_context+0x33/0x90 -> ioc->lock grabbed
 blk_mq_free_request+0x51/0x140
 blk_put_request+0xe/0x10
 blk_attempt_req_merge+0x1d/0x30
 elv_attempt_insert_merge+0x56/0xa0
 blk_mq_sched_try_insert_merge+0x4b/0x60
 bfq_insert_requests+0x9e/0x18c0 -> bfqd->lock grabbed

We could move blk_put_request() into scheduler code, then the lock
inversion is avoided. So far only mq-deadline and bfq calls into
blk_mq_sched_try_insert_merge(), and this change should be small.

We'd potentially be putting multiple requests if we keep the recursive merge.

Oh, we still can pass a list to hold all requests to be freed, then free
them all outside in scheduler code.

If we cannot really get rid of the recursive merge (not yet convinced),
this is also an option I've considered. I was afraid what can we use in
struct request to attach request to a list but it seems .merged_requests
handlers remove the request from the queuelist already so we should be fine
using that.

The request has been removed from scheduler queue, and safe to free,
so it is safe to be held in one temporary list.

Not quite, there's still ->finish_request hook that will be called from
blk_mq_free_request() on the request and e.g. BFQ performs quite a lot of
cleanup there. But yes, at least queuelist seems to be available for reuse
here.

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR