block: don't allow the same type rq_qos add more than once

[ Upstream commit 4e9bbf6653c17823bae3a4b7ce596ba3542ec745 ]

In our test of iocost, we encountered some list add/del corruptions of
inner_walk list in ioc_timer_fn.

The reason can be described as follows:

cpu 0					cpu 1
ioc_qos_write				ioc_qos_write

ioc = q_to_ioc(queue);
if (!ioc) {
        ioc = kzalloc();
					ioc = q_to_ioc(queue);
					if (!ioc) {
						ioc = kzalloc();
						...
						rq_qos_add(q, rqos);
					}
        ...
        rq_qos_add(q, rqos);
        ...
}

When the io.cost.qos file is written by two cpus concurrently, rq_qos may
be added to one disk twice. In that case, there will be two iocs enabled
and running on one disk. They own different iocgs on their active list. In
the ioc_timer_fn function, because of the iocgs from two iocs have the
same root iocg, the root iocg's walk_list may be overwritten by each other
and this leads to list add/del corruptions in building or destroying the
inner_walk list.

And so far, the blk-rq-qos framework works in case that one instance for
one type rq_qos per queue by default. This patch make this explicit and
also fix the crash above.

Signed-off-by: Jinke Han <hanjinke.666@bytedance.com>
Reviewed-by: Muchun Song <songmuchun@bytedance.com>
Acked-by: Tejun Heo <tj@kernel.org>
Cc: <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/20220720093616.70584-1-hanjinke.666@bytedance.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Sasha Levin <sashal@kernel.org>

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 10851493940c39ebb2314458f145d40e8955c063..069193dee95b0bfd6709b87bb0a16aa7ddfa2b1c 100644 (file)
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2893,15 +2893,21 @@ static int blk_iocost_init(struct request_queue *q)
         * called before policy activation completion, can't assume that the
         * target bio has an iocg associated and need to test for NULL iocg.
         */
-       rq_qos_add(q, rqos);
+       ret = rq_qos_add(q, rqos);
+       if (ret)
+               goto err_free_ioc;
+
        ret = blkcg_activate_policy(q, &blkcg_policy_iocost);
-       if (ret) {
-               rq_qos_del(q, rqos);
-               free_percpu(ioc->pcpu_stat);
-               kfree(ioc);
-               return ret;
-       }
+       if (ret)
+               goto err_del_qos;
        return 0;
+
+err_del_qos:
+       rq_qos_del(q, rqos);
+err_free_ioc:
+       free_percpu(ioc->pcpu_stat);
+       kfree(ioc);
+       return ret;
 }
 
 static struct blkcg_policy_data *ioc_cpd_alloc(gfp_t gfp)

diff --git a/block/blk-iolatency.c b/block/blk-iolatency.c
index d85f30a85ee754c5ac40ea0473e6eda624f248e6..bdef8395af6e72fa38f3890ccda48fb58e7511ed 100644 (file)
--- a/block/blk-iolatency.c
+++ b/block/blk-iolatency.c
@@ -772,19 +772,23 @@ int blk_iolatency_init(struct request_queue *q)
        rqos->ops = &blkcg_iolatency_ops;
        rqos->q = q;
 
-       rq_qos_add(q, rqos);
-
+       ret = rq_qos_add(q, rqos);
+       if (ret)
+               goto err_free;
        ret = blkcg_activate_policy(q, &blkcg_policy_iolatency);
-       if (ret) {
-               rq_qos_del(q, rqos);
-               kfree(blkiolat);
-               return ret;
-       }
+       if (ret)
+               goto err_qos_del;
 
        timer_setup(&blkiolat->timer, blkiolatency_timer_fn, 0);
        INIT_WORK(&blkiolat->enable_work, blkiolatency_enable_work_fn);
 
        return 0;
+
+err_qos_del:
+       rq_qos_del(q, rqos);
+err_free:
+       kfree(blkiolat);
+       return ret;
 }
 
 static void iolatency_set_min_lat_nsec(struct blkcg_gq *blkg, u64 val)

diff --git a/block/blk-rq-qos.h b/block/blk-rq-qos.h
index 68267007da1c673efd721a559a7232b5014c69ff..1655f76b6a1b67ccc42e02aa6fbfe1778b6170bd 100644 (file)
--- a/block/blk-rq-qos.h
+++ b/block/blk-rq-qos.h
@@ -86,7 +86,7 @@ static inline void rq_wait_init(struct rq_wait *rq_wait)
        init_waitqueue_head(&rq_wait->wait);
 }
 
-static inline void rq_qos_add(struct request_queue *q, struct rq_qos *rqos)
+static inline int rq_qos_add(struct request_queue *q, struct rq_qos *rqos)
 {
        /*
         * No IO can be in-flight when adding rqos, so freeze queue, which
@@ -98,6 +98,8 @@ static inline void rq_qos_add(struct request_queue *q, struct rq_qos *rqos)
        blk_mq_freeze_queue(q);
 
        spin_lock_irq(&q->queue_lock);
+       if (rq_qos_id(q, rqos->id))
+               goto ebusy;
        rqos->next = q->rq_qos;
        q->rq_qos = rqos;
        spin_unlock_irq(&q->queue_lock);
@@ -106,6 +108,13 @@ static inline void rq_qos_add(struct request_queue *q, struct rq_qos *rqos)
 
        if (rqos->ops->debugfs_attrs)
                blk_mq_debugfs_register_rqos(rqos);
+
+       return 0;
+ebusy:
+       spin_unlock_irq(&q->queue_lock);
+       blk_mq_unfreeze_queue(q);
+       return -EBUSY;
+
 }
 
 static inline void rq_qos_del(struct request_queue *q, struct rq_qos *rqos)

diff --git a/block/blk-wbt.c b/block/blk-wbt.c
index 0c119be0e813316c28049e9d7a0c6cdc1eddf4f5..ae6ea0b5457991b193b4d5de6d45cdcc7a2cc90d 100644 (file)
--- a/block/blk-wbt.c
+++ b/block/blk-wbt.c
@@ -820,6 +820,7 @@ int wbt_init(struct request_queue *q)
 {
        struct rq_wb *rwb;
        int i;
+       int ret;
 
        rwb = kzalloc(sizeof(*rwb), GFP_KERNEL);
        if (!rwb)
@@ -846,7 +847,10 @@ int wbt_init(struct request_queue *q)
        /*
         * Assign rwb and add the stats callback.
         */
-       rq_qos_add(q, &rwb->rqos);
+       ret = rq_qos_add(q, &rwb->rqos);
+       if (ret)
+               goto err_free;
+
        blk_stat_add_callback(q, rwb->cb);
 
        rwb->min_lat_nsec = wbt_default_latency_nsec(q);
@@ -855,4 +859,10 @@ int wbt_init(struct request_queue *q)
        wbt_set_write_cache(q, test_bit(QUEUE_FLAG_WC, &q->queue_flags));
 
        return 0;
+
+err_free:
+       blk_stat_free_callback(rwb->cb);
+       kfree(rwb);
+       return ret;
+
 }