From: Jinlong Chen Date: Wed, 19 Oct 2022 13:20:53 +0000 (+0800) Subject: block: fix up elevator_type refcounting X-Git-Url: https://www.infradead.org/git/?a=commitdiff_plain;h=refs%2Fheads%2Felevator-refs;p=users%2Fhch%2Fblock.git block: fix up elevator_type refcounting The current reference management logic of io scheduler modules contains refcnt problems. For example, blk_mq_init_sched may fail before or after the calling of e->ops.init_sched. If it fails before the calling, it does nothing to the reference to the io scheduler module. But if it fails after the calling, it releases the reference by calling kobject_put(&eq->kobj). As the callers of blk_mq_init_sched can't know exactly where the failure happens, they can't handle the reference to the io scheduler module properly: releasing the reference on failure results in double-release if blk_mq_init_sched has released it, and not releasing the reference results in ghost reference if blk_mq_init_sched did not release it either. The same problem also exists in io schedulers' init_sched implementations. We can address the problem by adding releasing statements to the error handling procedures of blk_mq_init_sched and init_sched implementations. But that is counterintuitive and requires modifications to existing io schedulers. Instead, We make elevator_alloc get the io scheduler module references that will be released by elevator_release. And then, we match each elevator_get with an elevator_put. Therefore, each reference to an io scheduler module explicitly has its own getter and releaser, and we no longer need to worry about the refcnt problems. The bugs and the patch can be validated with tools here: https://github.com/nickyc975/linux_elv_refcnt_bug.git Signed-off-by: Jinlong Chen [hch: split out a few bits into separate patches, use a non-try module_get in elevator_alloc] Signed-off-by: Christoph Hellwig --- diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c index 1f49a0a9f441..23d1a90fec42 100644 --- a/block/blk-mq-sched.c +++ b/block/blk-mq-sched.c @@ -555,6 +555,7 @@ static int blk_mq_init_sched_shared_tags(struct request_queue *queue) return 0; } +/* caller must have a reference to @e, will grab another one if successful */ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e) { unsigned int flags = q->tag_set->flags; diff --git a/block/blk-mq.c b/block/blk-mq.c index 536ece823358..b641629147d9 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -4591,6 +4591,8 @@ static void blk_mq_elv_switch_back(struct list_head *head, mutex_lock(&q->sysfs_lock); elevator_switch(q, t); + /* drop the reference acquired in blk_mq_elv_switch_none */ + elevator_put(t); mutex_unlock(&q->sysfs_lock); } diff --git a/block/elevator.c b/block/elevator.c index ad43c69c630b..74ce0c00827f 100644 --- a/block/elevator.c +++ b/block/elevator.c @@ -140,6 +140,7 @@ struct elevator_queue *elevator_alloc(struct request_queue *q, if (unlikely(!eq)) return NULL; + __elevator_get(e); eq->type = e; kobject_init(&eq->kobj, &elv_ktype); mutex_init(&eq->sysfs_lock); @@ -644,8 +645,9 @@ void elevator_init_mq(struct request_queue *q) if (err) { pr_warn("\"%s\" elevator initialization failed, " "falling back to \"none\"\n", e->elevator_name); - elevator_put(e); } + + elevator_put(e); } /* @@ -710,6 +712,7 @@ static int __elevator_change(struct request_queue *q, const char *name) { char __name[ELV_NAME_MAX]; struct elevator_type *e; + int ret; strlcpy(__name, name, sizeof(__name)); name = strstrip(__name); @@ -737,8 +740,9 @@ static int __elevator_change(struct request_queue *q, const char *name) if (!e) return -EINVAL; } - - return elevator_switch(q, e); + ret = elevator_switch(q, e); + elevator_put(e); + return ret; } ssize_t elv_iosched_store(struct request_queue *q, const char *name,