| Message ID | 20240814113542.911023-1-lilingfeng@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix potential deadlock warning in blk_mq_mark_tag_wait | expand |
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 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@huawei.com> Please leave a blank line between the patch description and the section with tags. > - spin_lock_irq(&tags->lock); > + spin_lock_irqsave(&tags->lock, flags); > users = tags->active_queues + 1; > WRITE_ONCE(tags->active_queues, users); > blk_mq_update_wake_batch(tags, users); > - spin_unlock_irq(&tags->lock); > + spin_unlock_irqrestore(&tags->lock, flags); > } The code changes however look good to me. Thanks, Bart.
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 > > 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@huawei.com> > > 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.
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@huawei.com> >> >> 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. >
diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c index cc57e2dd9a0b..2cafcf11ee8b 100644 --- a/block/blk-mq-tag.c +++ b/block/blk-mq-tag.c @@ -38,6 +38,7 @@ static void blk_mq_update_wake_batch(struct blk_mq_tags *tags, void __blk_mq_tag_busy(struct blk_mq_hw_ctx *hctx) { unsigned int users; + unsigned long flags; struct blk_mq_tags *tags = hctx->tags; /* @@ -56,11 +57,11 @@ void __blk_mq_tag_busy(struct blk_mq_hw_ctx *hctx) return; } - spin_lock_irq(&tags->lock); + spin_lock_irqsave(&tags->lock, flags); users = tags->active_queues + 1; WRITE_ONCE(tags->active_queues, users); blk_mq_update_wake_batch(tags, users); - spin_unlock_irq(&tags->lock); + spin_unlock_irqrestore(&tags->lock, flags); } /*