Re: [PATCH -next v2] blk-mq: fix panic during blk_mq_run_work_fn()

"yukuai (C)" <yukuai3@xxxxxxxxxx> · Sat, 21 May 2022 11:33:11 +0800

在 2022/05/20 21:56, Ming Lei 写道:
On Fri, May 20, 2022 at 08:01:31PM +0800, yukuai (C) wrote:
在 2022/05/20 19:39, Ming Lei 写道:

In short:

1) run queue can be in-progress during cleanup queue, or returns from
cleanup queue; we drain it in both blk_cleanup_queue() and
disk_release_mq(), see commit 2a19b28f7929 ("blk-mq: cancel blk-mq dispatch
work in both blk_cleanup_queue and disk_release()")
I understand that, however, there is no garantee new 'hctx->run_work'
won't be queued after 'drain it', for this crash, I think this is how

No, run queue activity will be shutdown after both disk_release_mq()
and blk_cleanup_queue() are done.

disk_release_mq() is called after all FS IOs are done, so there isn't
any run queue from FS IO code path, either sync or async.

In blk_cleanup_queue(), we only focus on passthrough request, and
passthrough request is always explicitly allocated & freed by
its caller, so once queue is frozen, all sync dispatch activity
for passthrough request has been done, then it is enough to just cancel
dispatch work for avoiding any dispatch activity.

Hi, Ming

Thanks for you explanation, it really help me understand the code
better.

In our test kernel, elevator_exit() is not called from
disk_release_mq(), that is the reason I thought differently about
root cause...

That is why both request queue and hctx can be released safely
after the two are done.

it triggered:

assum that there is no io, while some bfq_queue is still busy:

blk_cleanup_queue
  blk_freeze_queue
  blk_mq_cancel_work_sync
  cancel_delayed_work_sync(hctx1)
				blk_mq_run_work_fn -> hctx2
				 __blk_mq_run_hw_queue
				  blk_mq_sched_dispatch_requests
				   __blk_mq_do_dispatch_sched
				    blk_mq_delay_run_hw_queues
				     blk_mq_delay_run_hw_queue
				      -> add hctx1->run_work again
  cancel_delayed_work_sync(hctx2)

Yes, even blk_mq_delay_run_hw_queues() can be called after all
hctx->run_work are canceled since __blk_mq_run_hw_queue() could be
running in sync io code path, not via ->run_work.

And my patch will fix the issue, won't it?

Yes, like I said before, your patch do make sense. It seems like
commit 28ce942fa2d5 ("block: move blk_exit_queue into disk_release")
is the real fix for the crash in out test.

Thanks,
Kuai

Thanks,
Ming

.