Re: [PATCH] block: Fix potential deadlock warning in blk_mq_mark_tag_wait

[Yu Kuai]

Hi,

在 2024/08/15 4:22, Jens Axboe 写道:
> On 8/14/24 1:52 PM, Bart Van Assche wrote:
> > On 8/14/24 4:35 AM, Li Lingfeng wrote:
> > > When interrupt is turned on while a lock holding by spin_lock_irq it
> > > throws a warning because of potential deadlock.
> >
> > Which tool reported the warning? Please mention this in the patch
> > description.
> > > blk_mq_mark_tag_wait
> > >    spin_lock_irq(&wq->lock)
> > >         --> turn off interrupt and get lockA
> > >    blk_mq_get_driver_tag
> > >     __blk_mq_tag_busy
> > >      spin_lock_irq(&tags->lock)
> > >      spin_unlock_irq(&tags->lock)
> > >         --> release lockB and turn on interrupt accidentally

This looks correct, however, many details are hidden:

t1: IO dispatch
blk_mq_prep_dispatch_rq
 blk_mq_get_driver_tag
  __blk_mq_get_driver_tag
   __blk_mq_alloc_driver_tag
    blk_mq_tag_busy -> tag is already busy
    // failed to get driver tag

 blk_mq_mark_tag_wait
  spin_lock_irq(&wq->lock) -> lock A
  __add_wait_queue(wq, wait) -> wait queue active
  blk_mq_get_driver_tag
  __blk_mq_tag_busy
-> 1) tag must be idle, which means there can't be inflight IO
   spin_lock_irq(&tags->lock) -> lock B
   spin_unlock_irq(&tags->lock) -> unlock B
-> 2) context must be preempt by IO interrupt to trigger deadlock.

So, the deadlock is not possible in theory, there can't be inflight IO
if __blk_mq_tag_busy what to hold the second lock, while deadlock
require IO to be done.

Any way, the change looks good to me.

Thanks,
Kuai

> > The above call chain does not match the code in Linus' master tree.
> > Please fix this.
> >
> > > Fix it by using spin_lock_irqsave to get lockB instead of spin_lock_irq.
> > > Fixes: 4f1731df60f9 ("blk-mq: fix potential io hang by wrong 'wake_batch'")
> > > Signed-off-by: Li Lingfeng <lilingfeng3@xxxxxxxxxx>
> >
> > Please leave a blank line between the patch description and the section
> > with tags.
>
> Please just include the actual lockdep trace rather than a doctored up
> one, it's a lot more descriptive. And use the real lock names rather
> than turn it into hypotheticals.