diff mbox series

[09/15] blk-rq-qos: make rq_qos_add and rq_qos_del more useful

Message ID 20230117081257.3089859-10-hch@lst.de (mailing list archive)
State New, archived
Headers show
Series [01/15] blk-cgroup: don't defer blkg_free to a workqueue | expand

Commit Message

Christoph Hellwig Jan. 17, 2023, 8:12 a.m. UTC 
Switch to passing a gendisk, and make rq_qos_add initialize all required
fields and drop the not required q argument from rq_qos_del.  Also move
the code out of line given how large it is.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-iocost.c    | 13 ++-------
 block/blk-iolatency.c | 14 +++------
 block/blk-rq-qos.c    | 67 +++++++++++++++++++++++++++++++++++++++++++
 block/blk-rq-qos.h    | 62 ++-------------------------------------
 block/blk-wbt.c       |  5 +---
 5 files changed, 78 insertions(+), 83 deletions(-)

Comments

Andreas Herrmann Jan. 20, 2023, 9:57 a.m. UTC | #1 
On Tue, Jan 17, 2023 at 09:12:51AM +0100, Christoph Hellwig wrote:
> Switch to passing a gendisk, and make rq_qos_add initialize all required
> fields and drop the not required q argument from rq_qos_del.  Also move
> the code out of line given how large it is.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-iocost.c    | 13 ++-------
>  block/blk-iolatency.c | 14 +++------
>  block/blk-rq-qos.c    | 67 +++++++++++++++++++++++++++++++++++++++++++
>  block/blk-rq-qos.h    | 62 ++-------------------------------------
>  block/blk-wbt.c       |  5 +---
>  5 files changed, 78 insertions(+), 83 deletions(-)
> 
> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> index 6f39ca99e9d76f..9b5c0d23c9ce8b 100644
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c
> @@ -2836,9 +2836,7 @@ static struct rq_qos_ops ioc_rqos_ops = {
>  
>  static int blk_iocost_init(struct gendisk *disk)
>  {
> -	struct request_queue *q = disk->queue;
>  	struct ioc *ioc;
> -	struct rq_qos *rqos;
>  	int i, cpu, ret;
>  
>  	ioc = kzalloc(sizeof(*ioc), GFP_KERNEL);
> @@ -2861,11 +2859,6 @@ static int blk_iocost_init(struct gendisk *disk)
>  		local64_set(&ccs->rq_wait_ns, 0);
>  	}
>  
> -	rqos = &ioc->rqos;
> -	rqos->id = RQ_QOS_COST;
> -	rqos->ops = &ioc_rqos_ops;
> -	rqos->q = q;
> -
>  	spin_lock_init(&ioc->lock);
>  	timer_setup(&ioc->timer, ioc_timer_fn, 0);
>  	INIT_LIST_HEAD(&ioc->active_iocgs);
> @@ -2889,17 +2882,17 @@ static int blk_iocost_init(struct gendisk *disk)
>  	 * called before policy activation completion, can't assume that the
>  	 * target bio has an iocg associated and need to test for NULL iocg.
>  	 */
> -	ret = rq_qos_add(q, rqos);
> +	ret = rq_qos_add(&ioc->rqos, disk, RQ_QOS_COST, &ioc_rqos_ops);
>  	if (ret)
>  		goto err_free_ioc;
>  
> -	ret = blkcg_activate_policy(q, &blkcg_policy_iocost);
> +	ret = blkcg_activate_policy(disk->queue, &blkcg_policy_iocost);
>  	if (ret)
>  		goto err_del_qos;
>  	return 0;
>  
>  err_del_qos:
> -	rq_qos_del(q, rqos);
> +	rq_qos_del(&ioc->rqos);
>  err_free_ioc:
>  	free_percpu(ioc->pcpu_stat);
>  	kfree(ioc);
> diff --git a/block/blk-iolatency.c b/block/blk-iolatency.c
> index b55eac2cf91944..1c394bd77aa0b4 100644
> --- a/block/blk-iolatency.c
> +++ b/block/blk-iolatency.c
> @@ -757,24 +757,18 @@ static void blkiolatency_enable_work_fn(struct work_struct *work)
>  
>  int blk_iolatency_init(struct gendisk *disk)
>  {
> -	struct request_queue *q = disk->queue;
>  	struct blk_iolatency *blkiolat;
> -	struct rq_qos *rqos;
>  	int ret;
>  
>  	blkiolat = kzalloc(sizeof(*blkiolat), GFP_KERNEL);
>  	if (!blkiolat)
>  		return -ENOMEM;
>  
> -	rqos = &blkiolat->rqos;
> -	rqos->id = RQ_QOS_LATENCY;
> -	rqos->ops = &blkcg_iolatency_ops;
> -	rqos->q = q;
> -
> -	ret = rq_qos_add(q, rqos);
> +	ret = rq_qos_add(&blkiolat->rqos, disk, RQ_QOS_LATENCY,
> +			 &blkcg_iolatency_ops);
>  	if (ret)
>  		goto err_free;
> -	ret = blkcg_activate_policy(q, &blkcg_policy_iolatency);
> +	ret = blkcg_activate_policy(disk->queue, &blkcg_policy_iolatency);
>  	if (ret)
>  		goto err_qos_del;
>  
> @@ -784,7 +778,7 @@ int blk_iolatency_init(struct gendisk *disk)
>  	return 0;
>  
>  err_qos_del:
> -	rq_qos_del(q, rqos);
> +	rq_qos_del(&blkiolat->rqos);
>  err_free:
>  	kfree(blkiolat);
>  	return ret;
> diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c
> index 88f0fe7dcf5451..14bee1bd761362 100644
> --- a/block/blk-rq-qos.c
> +++ b/block/blk-rq-qos.c
> @@ -294,3 +294,70 @@ void rq_qos_exit(struct request_queue *q)
>  		rqos->ops->exit(rqos);
>  	}
>  }
> +
> +int rq_qos_add(struct rq_qos *rqos, struct gendisk *disk, enum rq_qos_id id,
> +		struct rq_qos_ops *ops)
> +{
> +	struct request_queue *q = disk->queue;
> +
> +	rqos->q = q;
> +	rqos->id = id;
> +	rqos->ops = ops;
> +
> +	/*
> +	 * No IO can be in-flight when adding rqos, so freeze queue, which
> +	 * is fine since we only support rq_qos for blk-mq queue.
> +	 *
> +	 * Reuse ->queue_lock for protecting against other concurrent
> +	 * rq_qos adding/deleting
> +	 */
> +	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);
> +
> +	blk_mq_unfreeze_queue(q);
> +
> +	if (rqos->ops->debugfs_attrs) {
> +		mutex_lock(&q->debugfs_mutex);
> +		blk_mq_debugfs_register_rqos(rqos);
> +		mutex_unlock(&q->debugfs_mutex);
> +	}
> +
> +	return 0;
> +ebusy:
> +	spin_unlock_irq(&q->queue_lock);
> +	blk_mq_unfreeze_queue(q);
> +	return -EBUSY;
> +}
> +
> +void rq_qos_del(struct rq_qos *rqos)
> +{
> +	struct request_queue *q = rqos->q;
> +	struct rq_qos **cur;
> +
> +	/*
> +	 * See comment in rq_qos_add() about freezing queue & using
> +	 * ->queue_lock.
> +	 */
> +	blk_mq_freeze_queue(q);
> +
> +	spin_lock_irq(&q->queue_lock);
> +	for (cur = &q->rq_qos; *cur; cur = &(*cur)->next) {
> +		if (*cur == rqos) {
> +			*cur = rqos->next;
> +			break;
> +		}
> +	}
> +	spin_unlock_irq(&q->queue_lock);
> +
> +	blk_mq_unfreeze_queue(q);
> +
> +	mutex_lock(&q->debugfs_mutex);
> +	blk_mq_debugfs_unregister_rqos(rqos);
> +	mutex_unlock(&q->debugfs_mutex);
> +}
> diff --git a/block/blk-rq-qos.h b/block/blk-rq-qos.h
> index 1ef1f7d4bc3cbc..22552785aa31ed 100644
> --- a/block/blk-rq-qos.h
> +++ b/block/blk-rq-qos.h
> @@ -85,65 +85,9 @@ static inline void rq_wait_init(struct rq_wait *rq_wait)
>  	init_waitqueue_head(&rq_wait->wait);
>  }
>  
> -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
> -	 * is fine since we only support rq_qos for blk-mq queue.
> -	 *
> -	 * Reuse ->queue_lock for protecting against other concurrent
> -	 * rq_qos adding/deleting
> -	 */
> -	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);
> -
> -	blk_mq_unfreeze_queue(q);
> -
> -	if (rqos->ops->debugfs_attrs) {
> -		mutex_lock(&q->debugfs_mutex);
> -		blk_mq_debugfs_register_rqos(rqos);
> -		mutex_unlock(&q->debugfs_mutex);
> -	}
> -
> -	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)
> -{
> -	struct rq_qos **cur;
> -
> -	/*
> -	 * See comment in rq_qos_add() about freezing queue & using
> -	 * ->queue_lock.
> -	 */
> -	blk_mq_freeze_queue(q);
> -
> -	spin_lock_irq(&q->queue_lock);
> -	for (cur = &q->rq_qos; *cur; cur = &(*cur)->next) {
> -		if (*cur == rqos) {
> -			*cur = rqos->next;
> -			break;
> -		}
> -	}
> -	spin_unlock_irq(&q->queue_lock);
> -
> -	blk_mq_unfreeze_queue(q);
> -
> -	mutex_lock(&q->debugfs_mutex);
> -	blk_mq_debugfs_unregister_rqos(rqos);
> -	mutex_unlock(&q->debugfs_mutex);
> -}
> +int rq_qos_add(struct rq_qos *rqos, struct gendisk *disk, enum rq_qos_id id,
> +		struct rq_qos_ops *ops);
> +void rq_qos_del(struct rq_qos *rqos);
>  
>  typedef bool (acquire_inflight_cb_t)(struct rq_wait *rqw, void *private_data);
>  typedef void (cleanup_cb_t)(struct rq_wait *rqw, void *private_data);
> diff --git a/block/blk-wbt.c b/block/blk-wbt.c
> index 473ae72befaf1a..97149a4f10e600 100644
> --- a/block/blk-wbt.c
> +++ b/block/blk-wbt.c
> @@ -855,9 +855,6 @@ int wbt_init(struct gendisk *disk)
>  	for (i = 0; i < WBT_NUM_RWQ; i++)
>  		rq_wait_init(&rwb->rq_wait[i]);
>  
> -	rwb->rqos.id = RQ_QOS_WBT;
> -	rwb->rqos.ops = &wbt_rqos_ops;
> -	rwb->rqos.q = q;
>  	rwb->last_comp = rwb->last_issue = jiffies;
>  	rwb->win_nsec = RWB_WINDOW_NSEC;
>  	rwb->enable_state = WBT_STATE_ON_DEFAULT;
> @@ -870,7 +867,7 @@ int wbt_init(struct gendisk *disk)
>  	/*
>  	 * Assign rwb and add the stats callback.
>  	 */
> -	ret = rq_qos_add(q, &rwb->rqos);
> +	ret = rq_qos_add(&rwb->rqos, q->disk, RQ_QOS_WBT, &wbt_rqos_ops);
                                     ^^^^^^^
				     disk (no?)
>  	if (ret)
>  		goto err_free;
>  
> -- 
> 2.39.0
> 

Otherwise looks good to me. Feel free to add
Reviewed-by: Andreas Herrmann <aherrmann@suse.de>
Hannes Reinecke Jan. 27, 2023, 7:08 a.m. UTC | #2 
On 1/17/23 09:12, Christoph Hellwig wrote:
> Switch to passing a gendisk, and make rq_qos_add initialize all required
> fields and drop the not required q argument from rq_qos_del.  Also move
> the code out of line given how large it is.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   block/blk-iocost.c    | 13 ++-------
>   block/blk-iolatency.c | 14 +++------
>   block/blk-rq-qos.c    | 67 +++++++++++++++++++++++++++++++++++++++++++
>   block/blk-rq-qos.h    | 62 ++-------------------------------------
>   block/blk-wbt.c       |  5 +---
>   5 files changed, 78 insertions(+), 83 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
diff mbox series

Patch

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 6f39ca99e9d76f..9b5c0d23c9ce8b 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2836,9 +2836,7 @@  static struct rq_qos_ops ioc_rqos_ops = {
 
 static int blk_iocost_init(struct gendisk *disk)
 {
-	struct request_queue *q = disk->queue;
 	struct ioc *ioc;
-	struct rq_qos *rqos;
 	int i, cpu, ret;
 
 	ioc = kzalloc(sizeof(*ioc), GFP_KERNEL);
@@ -2861,11 +2859,6 @@  static int blk_iocost_init(struct gendisk *disk)
 		local64_set(&ccs->rq_wait_ns, 0);
 	}
 
-	rqos = &ioc->rqos;
-	rqos->id = RQ_QOS_COST;
-	rqos->ops = &ioc_rqos_ops;
-	rqos->q = q;
-
 	spin_lock_init(&ioc->lock);
 	timer_setup(&ioc->timer, ioc_timer_fn, 0);
 	INIT_LIST_HEAD(&ioc->active_iocgs);
@@ -2889,17 +2882,17 @@  static int blk_iocost_init(struct gendisk *disk)
 	 * called before policy activation completion, can't assume that the
 	 * target bio has an iocg associated and need to test for NULL iocg.
 	 */
-	ret = rq_qos_add(q, rqos);
+	ret = rq_qos_add(&ioc->rqos, disk, RQ_QOS_COST, &ioc_rqos_ops);
 	if (ret)
 		goto err_free_ioc;
 
-	ret = blkcg_activate_policy(q, &blkcg_policy_iocost);
+	ret = blkcg_activate_policy(disk->queue, &blkcg_policy_iocost);
 	if (ret)
 		goto err_del_qos;
 	return 0;
 
 err_del_qos:
-	rq_qos_del(q, rqos);
+	rq_qos_del(&ioc->rqos);
 err_free_ioc:
 	free_percpu(ioc->pcpu_stat);
 	kfree(ioc);
diff --git a/block/blk-iolatency.c b/block/blk-iolatency.c
index b55eac2cf91944..1c394bd77aa0b4 100644
--- a/block/blk-iolatency.c
+++ b/block/blk-iolatency.c
@@ -757,24 +757,18 @@  static void blkiolatency_enable_work_fn(struct work_struct *work)
 
 int blk_iolatency_init(struct gendisk *disk)
 {
-	struct request_queue *q = disk->queue;
 	struct blk_iolatency *blkiolat;
-	struct rq_qos *rqos;
 	int ret;
 
 	blkiolat = kzalloc(sizeof(*blkiolat), GFP_KERNEL);
 	if (!blkiolat)
 		return -ENOMEM;
 
-	rqos = &blkiolat->rqos;
-	rqos->id = RQ_QOS_LATENCY;
-	rqos->ops = &blkcg_iolatency_ops;
-	rqos->q = q;
-
-	ret = rq_qos_add(q, rqos);
+	ret = rq_qos_add(&blkiolat->rqos, disk, RQ_QOS_LATENCY,
+			 &blkcg_iolatency_ops);
 	if (ret)
 		goto err_free;
-	ret = blkcg_activate_policy(q, &blkcg_policy_iolatency);
+	ret = blkcg_activate_policy(disk->queue, &blkcg_policy_iolatency);
 	if (ret)
 		goto err_qos_del;
 
@@ -784,7 +778,7 @@  int blk_iolatency_init(struct gendisk *disk)
 	return 0;
 
 err_qos_del:
-	rq_qos_del(q, rqos);
+	rq_qos_del(&blkiolat->rqos);
 err_free:
 	kfree(blkiolat);
 	return ret;
diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c
index 88f0fe7dcf5451..14bee1bd761362 100644
--- a/block/blk-rq-qos.c
+++ b/block/blk-rq-qos.c
@@ -294,3 +294,70 @@  void rq_qos_exit(struct request_queue *q)
 		rqos->ops->exit(rqos);
 	}
 }
+
+int rq_qos_add(struct rq_qos *rqos, struct gendisk *disk, enum rq_qos_id id,
+		struct rq_qos_ops *ops)
+{
+	struct request_queue *q = disk->queue;
+
+	rqos->q = q;
+	rqos->id = id;
+	rqos->ops = ops;
+
+	/*
+	 * No IO can be in-flight when adding rqos, so freeze queue, which
+	 * is fine since we only support rq_qos for blk-mq queue.
+	 *
+	 * Reuse ->queue_lock for protecting against other concurrent
+	 * rq_qos adding/deleting
+	 */
+	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);
+
+	blk_mq_unfreeze_queue(q);
+
+	if (rqos->ops->debugfs_attrs) {
+		mutex_lock(&q->debugfs_mutex);
+		blk_mq_debugfs_register_rqos(rqos);
+		mutex_unlock(&q->debugfs_mutex);
+	}
+
+	return 0;
+ebusy:
+	spin_unlock_irq(&q->queue_lock);
+	blk_mq_unfreeze_queue(q);
+	return -EBUSY;
+}
+
+void rq_qos_del(struct rq_qos *rqos)
+{
+	struct request_queue *q = rqos->q;
+	struct rq_qos **cur;
+
+	/*
+	 * See comment in rq_qos_add() about freezing queue & using
+	 * ->queue_lock.
+	 */
+	blk_mq_freeze_queue(q);
+
+	spin_lock_irq(&q->queue_lock);
+	for (cur = &q->rq_qos; *cur; cur = &(*cur)->next) {
+		if (*cur == rqos) {
+			*cur = rqos->next;
+			break;
+		}
+	}
+	spin_unlock_irq(&q->queue_lock);
+
+	blk_mq_unfreeze_queue(q);
+
+	mutex_lock(&q->debugfs_mutex);
+	blk_mq_debugfs_unregister_rqos(rqos);
+	mutex_unlock(&q->debugfs_mutex);
+}
diff --git a/block/blk-rq-qos.h b/block/blk-rq-qos.h
index 1ef1f7d4bc3cbc..22552785aa31ed 100644
--- a/block/blk-rq-qos.h
+++ b/block/blk-rq-qos.h
@@ -85,65 +85,9 @@  static inline void rq_wait_init(struct rq_wait *rq_wait)
 	init_waitqueue_head(&rq_wait->wait);
 }
 
-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
-	 * is fine since we only support rq_qos for blk-mq queue.
-	 *
-	 * Reuse ->queue_lock for protecting against other concurrent
-	 * rq_qos adding/deleting
-	 */
-	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);
-
-	blk_mq_unfreeze_queue(q);
-
-	if (rqos->ops->debugfs_attrs) {
-		mutex_lock(&q->debugfs_mutex);
-		blk_mq_debugfs_register_rqos(rqos);
-		mutex_unlock(&q->debugfs_mutex);
-	}
-
-	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)
-{
-	struct rq_qos **cur;
-
-	/*
-	 * See comment in rq_qos_add() about freezing queue & using
-	 * ->queue_lock.
-	 */
-	blk_mq_freeze_queue(q);
-
-	spin_lock_irq(&q->queue_lock);
-	for (cur = &q->rq_qos; *cur; cur = &(*cur)->next) {
-		if (*cur == rqos) {
-			*cur = rqos->next;
-			break;
-		}
-	}
-	spin_unlock_irq(&q->queue_lock);
-
-	blk_mq_unfreeze_queue(q);
-
-	mutex_lock(&q->debugfs_mutex);
-	blk_mq_debugfs_unregister_rqos(rqos);
-	mutex_unlock(&q->debugfs_mutex);
-}
+int rq_qos_add(struct rq_qos *rqos, struct gendisk *disk, enum rq_qos_id id,
+		struct rq_qos_ops *ops);
+void rq_qos_del(struct rq_qos *rqos);
 
 typedef bool (acquire_inflight_cb_t)(struct rq_wait *rqw, void *private_data);
 typedef void (cleanup_cb_t)(struct rq_wait *rqw, void *private_data);
diff --git a/block/blk-wbt.c b/block/blk-wbt.c
index 473ae72befaf1a..97149a4f10e600 100644
--- a/block/blk-wbt.c
+++ b/block/blk-wbt.c
@@ -855,9 +855,6 @@  int wbt_init(struct gendisk *disk)
 	for (i = 0; i < WBT_NUM_RWQ; i++)
 		rq_wait_init(&rwb->rq_wait[i]);
 
-	rwb->rqos.id = RQ_QOS_WBT;
-	rwb->rqos.ops = &wbt_rqos_ops;
-	rwb->rqos.q = q;
 	rwb->last_comp = rwb->last_issue = jiffies;
 	rwb->win_nsec = RWB_WINDOW_NSEC;
 	rwb->enable_state = WBT_STATE_ON_DEFAULT;
@@ -870,7 +867,7 @@  int wbt_init(struct gendisk *disk)
 	/*
 	 * Assign rwb and add the stats callback.
 	 */
-	ret = rq_qos_add(q, &rwb->rqos);
+	ret = rq_qos_add(&rwb->rqos, q->disk, RQ_QOS_WBT, &wbt_rqos_ops);
 	if (ret)
 		goto err_free;