On Sat, Apr 19, 2025 at 07:25:31PM +0530, Nilay Shroff wrote:
>
>
> On 4/18/25 10:06 PM, Ming Lei wrote:
> > Move elv_register[unregister]_queue out of ->elevator_lock & queue freezing,
> > so we can kill many lockdep warnings.
> >
> > elv_register[unregister]_queue() is serialized, and just dealing with sysfs/
> > debugfs things, no need to be done with queue frozen.
> >
> > With this change, elevator's ->exit() is called before calling
> > elv_unregister_queue, then user may call into ->show()/store() of elevator's
> > sysfs attributes, and we have covered this issue by adding `ELEVATOR_FLAG_DYNG`.
> >
> > For blk-mq debugfs, hctx->sched_tags is always checked with ->elevator_lock by
> > debugfs code, meantime hctx->sched_tags is updated with ->elevator_lock, so
> > there isn't such issue.
> >
> > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> > ---
> > block/blk-mq.c | 9 ++++----
> > block/blk.h | 1 +
> > block/elevator.c | 58 ++++++++++++++++++++++++++++++++++--------------
> > block/elevator.h | 5 +++++
> > 4 files changed, 52 insertions(+), 21 deletions(-)
> >
>
> [...]
>
> > +static void elevator_exit(struct request_queue *q)
> > +{
> > + __elevator_exit(q);
> > + kobject_put(&q->elevator->kobj);
> > }
>
> [...]
> > +int elevator_change_done(struct request_queue *q, struct elv_change_ctx *ctx)
> > +{
> > + int ret = 0;
> > +
> > + if (ctx->old) {
> > + elv_unregister_queue(q, ctx->old);
> > + kobject_put(&ctx->old->kobj);
> > + }
> > + if (ctx->new) {
> > + ret = elv_register_queue(q, ctx->new, ctx->uevent);
> > + if (ret) {
> > + unsigned memflags = blk_mq_freeze_queue(q);
> > +
> > + mutex_lock(&q->elevator_lock);
> > + elevator_exit(q);
> > + mutex_unlock(&q->elevator_lock);
> > + blk_mq_unfreeze_queue(q, memflags);
> > + }
> > + }
> > + return 0;
> > +}
> > +
> It seems that we're still leaking ->elevator_lock in sysfs/kernfs with
> the above elevator_exit call. I think we probably want to move out
> kobject_put(&q->elevator->kobj) from elevator_exit and invoke it
> after we release ->elevator_lock in elevator_change_done.
q->elevator_lock is owned by request queue instead of elevator queue, so it
shouldn't be one issue, or can you explain in details?
Thanks,
Ming
