On Fri, May 21, 2021 at 02:05:51PM +0200, Jan Kara wrote: > On Fri 21-05-21 14:54:09, Ming Lei wrote: > > On Thu, May 20, 2021 at 08:29:49PM -0700, Khazhy Kumykov wrote: > > > On Thu, May 20, 2021 at 5:57 PM Ming Lei <ming.lei@xxxxxxxxxx> wrote: > > > > > > > > On Fri, May 21, 2021 at 12:33:53AM +0200, Jan Kara wrote: > > > > > 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. Thanks, Ming
