| Message ID | 20250410133029.2487054-16-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | block: unify elevator changing and fix lockdep warning | expand |
On 4/10/25 7:00 PM, Ming Lei wrote: > scheduler's ->exit() is called with queue frozen and elevator lock is held, and > wbt_enable_default() can't be called with queue frozen, otherwise the > following lockdep warning is triggered: > > #6 (&q->rq_qos_mutex){+.+.}-{4:4}: > #5 (&eq->sysfs_lock){+.+.}-{4:4}: > #4 (&q->elevator_lock){+.+.}-{4:4}: > #3 (&q->q_usage_counter(io)#3){++++}-{0:0}: > #2 (fs_reclaim){+.+.}-{0:0}: > #1 (&sb->s_type->i_mutex_key#3){+.+.}-{4:4}: > #0 (&q->debugfs_mutex){+.+.}-{4:4}: > > Fix the issue by moving wbt_enable_default() out of bfq's exit(), and > call it from elevator_change_done(). [...] > diff --git a/block/elevator.c b/block/elevator.c > index 1cc640a9db3e..9cf78db4d6a4 100644 > --- a/block/elevator.c > +++ b/block/elevator.c > @@ -676,8 +676,13 @@ int elevator_change_done(struct request_queue *q, struct elev_change_ctx *ctx) > int ret = 0; > > if (ctx->old) { > + bool enable_wbt = test_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT, > + &ctx->old->flags); > + > elv_unregister_queue(q, ctx->old); > kobject_put(&ctx->old->kobj); > + if (enable_wbt) > + wbt_enable_default(q->disk); > } wbt_enable_default is also called from ioc_qos_write and blk_register_queue with ->elevator_lock protection. So avoiding protection here doesn't seem correct. Thanks, --Nilay
On Fri, Apr 11, 2025 at 12:50:42AM +0530, Nilay Shroff wrote: > > > On 4/10/25 7:00 PM, Ming Lei wrote: > > scheduler's ->exit() is called with queue frozen and elevator lock is held, and > > wbt_enable_default() can't be called with queue frozen, otherwise the > > following lockdep warning is triggered: > > > > #6 (&q->rq_qos_mutex){+.+.}-{4:4}: > > #5 (&eq->sysfs_lock){+.+.}-{4:4}: > > #4 (&q->elevator_lock){+.+.}-{4:4}: > > #3 (&q->q_usage_counter(io)#3){++++}-{0:0}: > > #2 (fs_reclaim){+.+.}-{0:0}: > > #1 (&sb->s_type->i_mutex_key#3){+.+.}-{4:4}: > > #0 (&q->debugfs_mutex){+.+.}-{4:4}: > > > > Fix the issue by moving wbt_enable_default() out of bfq's exit(), and > > call it from elevator_change_done(). > > [...] > > > diff --git a/block/elevator.c b/block/elevator.c > > index 1cc640a9db3e..9cf78db4d6a4 100644 > > --- a/block/elevator.c > > +++ b/block/elevator.c > > @@ -676,8 +676,13 @@ int elevator_change_done(struct request_queue *q, struct elev_change_ctx *ctx) > > int ret = 0; > > > > if (ctx->old) { > > + bool enable_wbt = test_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT, > > + &ctx->old->flags); > > + > > elv_unregister_queue(q, ctx->old); > > kobject_put(&ctx->old->kobj); > > + if (enable_wbt) > > + wbt_enable_default(q->disk); > > } > wbt_enable_default is also called from ioc_qos_write and blk_register_queue > with ->elevator_lock protection. So avoiding protection here doesn't seem > correct. The only code which needs the protection should be: ``` wbt_enable_default(): ... if (q->elevator && test_bit(ELEVATOR_FLAG_DISABLE_WBT, &q->elevator->flags)) enable = false; ``` The flag can be moved to request queue from elevator queue, will do it in V2. Thanks, Ming
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index abd80dc13562..18018c8cf84d 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -7211,7 +7211,7 @@ static void bfq_exit_queue(struct elevator_queue *e) blk_stat_disable_accounting(bfqd->queue); clear_bit(ELEVATOR_FLAG_DISABLE_WBT, &e->flags); - wbt_enable_default(bfqd->queue->disk); + set_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT, &e->flags); kfree(bfqd); } diff --git a/block/elevator.c b/block/elevator.c index 1cc640a9db3e..9cf78db4d6a4 100644 --- a/block/elevator.c +++ b/block/elevator.c @@ -676,8 +676,13 @@ int elevator_change_done(struct request_queue *q, struct elev_change_ctx *ctx) int ret = 0; if (ctx->old) { + bool enable_wbt = test_bit(ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT, + &ctx->old->flags); + elv_unregister_queue(q, ctx->old); kobject_put(&ctx->old->kobj); + if (enable_wbt) + wbt_enable_default(q->disk); } if (ctx->new) { ret = elv_register_queue(q, ctx->new, ctx->uevent); diff --git a/block/elevator.h b/block/elevator.h index 87848fdc8a52..7b66be5b8295 100644 --- a/block/elevator.h +++ b/block/elevator.h @@ -122,6 +122,7 @@ struct elevator_queue #define ELEVATOR_FLAG_REGISTERED 0 #define ELEVATOR_FLAG_DISABLE_WBT 1 +#define ELEVATOR_FLAG_ENABLE_WBT_ON_EXIT 2 /* Holding data for changing elevator */ struct elev_change_ctx {
scheduler's ->exit() is called with queue frozen and elevator lock is held, and wbt_enable_default() can't be called with queue frozen, otherwise the following lockdep warning is triggered: #6 (&q->rq_qos_mutex){+.+.}-{4:4}: #5 (&eq->sysfs_lock){+.+.}-{4:4}: #4 (&q->elevator_lock){+.+.}-{4:4}: #3 (&q->q_usage_counter(io)#3){++++}-{0:0}: #2 (fs_reclaim){+.+.}-{0:0}: #1 (&sb->s_type->i_mutex_key#3){+.+.}-{4:4}: #0 (&q->debugfs_mutex){+.+.}-{4:4}: Fix the issue by moving wbt_enable_default() out of bfq's exit(), and call it from elevator_change_done(). Signed-off-by: Ming Lei <ming.lei@redhat.com> --- block/bfq-iosched.c | 2 +- block/elevator.c | 5 +++++ block/elevator.h | 1 + 3 files changed, 7 insertions(+), 1 deletion(-)