| Message ID | 20230527091904.3001833-1-linan666@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-iocost: use spin_lock_irqsave in adjust_inuse_and_calc_cost | expand |
Hi, 在 2023/05/27 17:19, linan666@huaweicloud.com 写道: > From: Li Nan <linan122@huawei.com> > > adjust_inuse_and_calc_cost() use spin_lock_irq() and IRQ will be enabled > when unlock. DEADLOCK might happen if we have held other locks and disabled > IRQ before invoking it. > > Fix it by using spin_lock_irqsave() instead, which can keep IRQ state > consistent with before when unlock. > > ================================ > WARNING: inconsistent lock state > 5.10.0-02758-g8e5f91fd772f #26 Not tainted > -------------------------------- > inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage. > kworker/2:3/388 [HC0[0]:SC0[0]:HE0:SE1] takes: > ffff888118c00c28 (&bfqd->lock){?.-.}-{2:2}, at: spin_lock_irq > ffff888118c00c28 (&bfqd->lock){?.-.}-{2:2}, at: bfq_bio_merge+0x141/0x390 > {IN-HARDIRQ-W} state was registered at: > __lock_acquire+0x3d7/0x1070 > lock_acquire+0x197/0x4a0 > __raw_spin_lock_irqsave > _raw_spin_lock_irqsave+0x3b/0x60 > bfq_idle_slice_timer_body > bfq_idle_slice_timer+0x53/0x1d0 > __run_hrtimer+0x477/0xa70 > __hrtimer_run_queues+0x1c6/0x2d0 > hrtimer_interrupt+0x302/0x9e0 > local_apic_timer_interrupt > __sysvec_apic_timer_interrupt+0xfd/0x420 > run_sysvec_on_irqstack_cond > sysvec_apic_timer_interrupt+0x46/0xa0 > asm_sysvec_apic_timer_interrupt+0x12/0x20 > irq event stamp: 837522 > hardirqs last enabled at (837521): [<ffffffff84b9419d>] __raw_spin_unlock_irqrestore > hardirqs last enabled at (837521): [<ffffffff84b9419d>] _raw_spin_unlock_irqrestore+0x3d/0x40 > hardirqs last disabled at (837522): [<ffffffff84b93fa3>] __raw_spin_lock_irq > hardirqs last disabled at (837522): [<ffffffff84b93fa3>] _raw_spin_lock_irq+0x43/0x50 > softirqs last enabled at (835852): [<ffffffff84e00558>] __do_softirq+0x558/0x8ec > softirqs last disabled at (835845): [<ffffffff84c010ff>] asm_call_irq_on_stack+0xf/0x20 > > other info that might help us debug this: > Possible unsafe locking scenario: > > CPU0 > ---- > lock(&bfqd->lock); > <Interrupt> > lock(&bfqd->lock); > > *** DEADLOCK *** > > 3 locks held by kworker/2:3/388: > #0: ffff888107af0f38 ((wq_completion)kthrotld){+.+.}-{0:0}, at: process_one_work+0x742/0x13f0 > #1: ffff8881176bfdd8 ((work_completion)(&td->dispatch_work)){+.+.}-{0:0}, at: process_one_work+0x777/0x13f0 > #2: ffff888118c00c28 (&bfqd->lock){?.-.}-{2:2}, at: spin_lock_irq > #2: ffff888118c00c28 (&bfqd->lock){?.-.}-{2:2}, at: bfq_bio_merge+0x141/0x390 > > stack backtrace: > CPU: 2 PID: 388 Comm: kworker/2:3 Not tainted 5.10.0-02758-g8e5f91fd772f #26 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014 > Workqueue: kthrotld blk_throtl_dispatch_work_fn > Call Trace: > __dump_stack lib/dump_stack.c:77 [inline] > dump_stack+0x107/0x167 > print_usage_bug > valid_state > mark_lock_irq.cold+0x32/0x3a > mark_lock+0x693/0xbc0 > mark_held_locks+0x9e/0xe0 > __trace_hardirqs_on_caller > lockdep_hardirqs_on_prepare.part.0+0x151/0x360 > trace_hardirqs_on+0x5b/0x180 > __raw_spin_unlock_irq > _raw_spin_unlock_irq+0x24/0x40 > spin_unlock_irq > adjust_inuse_and_calc_cost+0x4fb/0x970 > ioc_rqos_merge+0x277/0x740 > __rq_qos_merge+0x62/0xb0 > rq_qos_merge > bio_attempt_back_merge+0x12c/0x4a0 > blk_mq_sched_try_merge+0x1b6/0x4d0 > bfq_bio_merge+0x24a/0x390 > __blk_mq_sched_bio_merge+0xa6/0x460 > blk_mq_sched_bio_merge > blk_mq_submit_bio+0x2e7/0x1ee0 > __submit_bio_noacct_mq+0x175/0x3b0 > submit_bio_noacct+0x1fb/0x270 > blk_throtl_dispatch_work_fn+0x1ef/0x2b0 > process_one_work+0x83e/0x13f0 > process_scheduled_works > worker_thread+0x7e3/0xd80 > kthread+0x353/0x470 > ret_from_fork+0x1f/0x30 So this happens when iocost is used together with bfq, performance will be quite bad in this case, I don't think there will be any real use case. However, the changes looks reasonable, feel free to add: Reviewed-by: Yu Kuai <yukuai3@huawei.com> > > Fixes: b0853ab4a238 ("blk-iocost: revamp in-period donation snapbacks") > Signed-off-by: Li Nan <linan122@huawei.com> > --- > block/blk-iocost.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/block/blk-iocost.c b/block/blk-iocost.c > index 82e634d552d9..7581893e0d82 100644 > --- a/block/blk-iocost.c > +++ b/block/blk-iocost.c > @@ -2438,6 +2438,7 @@ static u64 adjust_inuse_and_calc_cost(struct ioc_gq *iocg, u64 vtime, > u32 hwi, adj_step; > s64 margin; > u64 cost, new_inuse; > + unsigned long flags; > > current_hweight(iocg, NULL, &hwi); > old_hwi = hwi; > @@ -2456,11 +2457,11 @@ static u64 adjust_inuse_and_calc_cost(struct ioc_gq *iocg, u64 vtime, > iocg->inuse == iocg->active) > return cost; > > - spin_lock_irq(&ioc->lock); > + spin_lock_irqsave(&ioc->lock, flags); > > /* we own inuse only when @iocg is in the normal active state */ > if (iocg->abs_vdebt || list_empty(&iocg->active_list)) { > - spin_unlock_irq(&ioc->lock); > + spin_unlock_irqrestore(&ioc->lock, flags); > return cost; > } > > @@ -2481,7 +2482,7 @@ static u64 adjust_inuse_and_calc_cost(struct ioc_gq *iocg, u64 vtime, > } while (time_after64(vtime + cost, now->vnow) && > iocg->inuse != iocg->active); > > - spin_unlock_irq(&ioc->lock); > + spin_unlock_irqrestore(&ioc->lock, flags); > > TRACE_IOCG_PATH(inuse_adjust, iocg, now, > old_inuse, iocg->inuse, old_hwi, hwi); >
On Sat, May 27, 2023 at 05:19:04PM +0800, linan666@huaweicloud.com wrote: > From: Li Nan <linan122@huawei.com> > > adjust_inuse_and_calc_cost() use spin_lock_irq() and IRQ will be enabled > when unlock. DEADLOCK might happen if we have held other locks and disabled > IRQ before invoking it. > > Fix it by using spin_lock_irqsave() instead, which can keep IRQ state > consistent with before when unlock. ... > Fixes: b0853ab4a238 ("blk-iocost: revamp in-period donation snapbacks") > Signed-off-by: Li Nan <linan122@huawei.com> Acked-by: Tejun Heo <tj@kernel.org> Thanks.
On Sat, 27 May 2023 17:19:04 +0800, linan666@huaweicloud.com wrote: > adjust_inuse_and_calc_cost() use spin_lock_irq() and IRQ will be enabled > when unlock. DEADLOCK might happen if we have held other locks and disabled > IRQ before invoking it. > > Fix it by using spin_lock_irqsave() instead, which can keep IRQ state > consistent with before when unlock. > > [...] Applied, thanks! [1/1] blk-iocost: use spin_lock_irqsave in adjust_inuse_and_calc_cost commit: 8d211554679d0b23702bd32ba04aeac0c1c4f660 Best regards,
diff --git a/block/blk-iocost.c b/block/blk-iocost.c index 82e634d552d9..7581893e0d82 100644 --- a/block/blk-iocost.c +++ b/block/blk-iocost.c @@ -2438,6 +2438,7 @@ static u64 adjust_inuse_and_calc_cost(struct ioc_gq *iocg, u64 vtime, u32 hwi, adj_step; s64 margin; u64 cost, new_inuse; + unsigned long flags; current_hweight(iocg, NULL, &hwi); old_hwi = hwi; @@ -2456,11 +2457,11 @@ static u64 adjust_inuse_and_calc_cost(struct ioc_gq *iocg, u64 vtime, iocg->inuse == iocg->active) return cost; - spin_lock_irq(&ioc->lock); + spin_lock_irqsave(&ioc->lock, flags); /* we own inuse only when @iocg is in the normal active state */ if (iocg->abs_vdebt || list_empty(&iocg->active_list)) { - spin_unlock_irq(&ioc->lock); + spin_unlock_irqrestore(&ioc->lock, flags); return cost; } @@ -2481,7 +2482,7 @@ static u64 adjust_inuse_and_calc_cost(struct ioc_gq *iocg, u64 vtime, } while (time_after64(vtime + cost, now->vnow) && iocg->inuse != iocg->active); - spin_unlock_irq(&ioc->lock); + spin_unlock_irqrestore(&ioc->lock, flags); TRACE_IOCG_PATH(inuse_adjust, iocg, now, old_inuse, iocg->inuse, old_hwi, hwi);