more I/O context cleanup

more I/O context cleanup

[Christoph Hellwig]

Hi Jens,

this series dusts off a few more bits in the I/O context handling, and
makes a chunk of code only needed by the BFQ I/O scheduler optional.

Diffstat:
 block/Kconfig             |    3 
 block/Kconfig.iosched     |    1 
 block/blk-ioc.c           |  153 +++++++++++++++++++---------------------------
 block/blk.h               |    6 +
 include/linux/iocontext.h |    7 +-
 5 files changed, 80 insertions(+), 90 deletions(-)

[PATCH 1/7] block: remove the nr_task field from struct io_context

[Christoph Hellwig]

Nothing ever looks at ->nr_tasks, so remove it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-ioc.c           | 3 ---
 include/linux/iocontext.h | 1 -
 2 files changed, 4 deletions(-)

diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 536fb496ad763..96336c2134efa 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -207,7 +207,6 @@ void exit_io_context(struct task_struct *task)
 	task->io_context = NULL;
 	task_unlock(task);
 
-	atomic_dec(&ioc->nr_tasks);
 	put_io_context_active(ioc);
 }
 
@@ -259,7 +258,6 @@ static struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
 		return NULL;
 
 	atomic_long_set(&ioc->refcount, 1);
-	atomic_set(&ioc->nr_tasks, 1);
 	atomic_set(&ioc->active_ref, 1);
 	spin_lock_init(&ioc->lock);
 	INIT_RADIX_TREE(&ioc->icq_tree, GFP_ATOMIC);
@@ -339,7 +337,6 @@ int __copy_io(unsigned long clone_flags, struct task_struct *tsk)
 	if (clone_flags & CLONE_IO) {
 		atomic_long_inc(&ioc->refcount);
 		atomic_inc(&ioc->active_ref);
-		atomic_inc(&ioc->nr_tasks);
 		tsk->io_context = ioc;
 	} else if (ioprio_valid(ioc->ioprio)) {
 		tsk->io_context = alloc_io_context(GFP_KERNEL, NUMA_NO_NODE);
diff --git a/include/linux/iocontext.h b/include/linux/iocontext.h
index c1229fbd6691c..82c7f4f5f4f59 100644
--- a/include/linux/iocontext.h
+++ b/include/linux/iocontext.h
@@ -99,7 +99,6 @@ struct io_cq {
 struct io_context {
 	atomic_long_t refcount;
 	atomic_t active_ref;
-	atomic_t nr_tasks;
 
 	/* all the fields below are protected by this lock */
 	spinlock_t lock;
-- 
2.30.2

[PATCH 2/7] block: simplify struct io_context refcounting

[Christoph Hellwig]

Don't hold a reference to ->refcount for each active reference, but
just one for all active references.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-ioc.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 96336c2134efa..9cde3906be3c6 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -180,10 +180,8 @@ static void put_io_context_active(struct io_context *ioc)
 {
 	struct io_cq *icq;
 
-	if (!atomic_dec_and_test(&ioc->active_ref)) {
-		put_io_context(ioc);
+	if (!atomic_dec_and_test(&ioc->active_ref))
 		return;
-	}
 
 	spin_lock_irq(&ioc->lock);
 	hlist_for_each_entry(icq, &ioc->icq_list, ioc_node) {
@@ -335,7 +333,6 @@ int __copy_io(unsigned long clone_flags, struct task_struct *tsk)
 	 * Share io context with parent, if CLONE_IO is set
 	 */
 	if (clone_flags & CLONE_IO) {
-		atomic_long_inc(&ioc->refcount);
 		atomic_inc(&ioc->active_ref);
 		tsk->io_context = ioc;
 	} else if (ioprio_valid(ioc->ioprio)) {
-- 
2.30.2

[PATCH 3/7] block: refactor put_iocontext_active

[Christoph Hellwig]

Factor out a ioc_exit_icqs helper to tear down the icqs and the fold
the rest of put_iocontext_active into exit_io_context.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-ioc.c | 41 ++++++++++++++---------------------------
 1 file changed, 14 insertions(+), 27 deletions(-)

diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 9cde3906be3c6..0380e33930e31 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -54,6 +54,16 @@ static void ioc_exit_icq(struct io_cq *icq)
 	icq->flags |= ICQ_EXITED;
 }
 
+static void ioc_exit_icqs(struct io_context *ioc)
+{
+	struct io_cq *icq;
+
+	spin_lock_irq(&ioc->lock);
+	hlist_for_each_entry(icq, &ioc->icq_list, ioc_node)
+		ioc_exit_icq(icq);
+	spin_unlock_irq(&ioc->lock);
+}
+
 /*
  * Release an icq. Called with ioc locked for blk-mq, and with both ioc
  * and queue locked for legacy.
@@ -169,32 +179,6 @@ void put_io_context(struct io_context *ioc)
 }
 EXPORT_SYMBOL_GPL(put_io_context);
 
-/**
- * put_io_context_active - put active reference on ioc
- * @ioc: ioc of interest
- *
- * Put an active reference to an ioc.  If active reference reaches zero after
- * put, @ioc can never issue further IOs and ioscheds are notified.
- */
-static void put_io_context_active(struct io_context *ioc)
-{
-	struct io_cq *icq;
-
-	if (!atomic_dec_and_test(&ioc->active_ref))
-		return;
-
-	spin_lock_irq(&ioc->lock);
-	hlist_for_each_entry(icq, &ioc->icq_list, ioc_node) {
-		if (icq->flags & ICQ_EXITED)
-			continue;
-
-		ioc_exit_icq(icq);
-	}
-	spin_unlock_irq(&ioc->lock);
-
-	put_io_context(ioc);
-}
-
 /* Called by the exiting task */
 void exit_io_context(struct task_struct *task)
 {
@@ -205,7 +189,10 @@ void exit_io_context(struct task_struct *task)
 	task->io_context = NULL;
 	task_unlock(task);
 
-	put_io_context_active(ioc);
+	if (atomic_dec_and_test(&ioc->active_ref)) {
+		ioc_exit_icqs(ioc);
+		put_io_context(ioc);
+	}
 }
 
 static void __ioc_clear_queue(struct list_head *icq_list)
-- 
2.30.2

[PATCH 4/7] block: remove the NULL ioc check in put_io_context

[Christoph Hellwig]

No caller passes in a NULL pointer, so remove the check.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-ioc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 0380e33930e31..04f3d2b0ca7db 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -155,9 +155,6 @@ void put_io_context(struct io_context *ioc)
 	unsigned long flags;
 	bool free_ioc = false;
 
-	if (ioc == NULL)
-		return;
-
 	BUG_ON(atomic_long_read(&ioc->refcount) <= 0);
 
 	/*
-- 
2.30.2

[PATCH 5/7] block: refactor put_io_context

[Christoph Hellwig]

Move the code to delay freeing the icqs into a separate helper.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-ioc.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 04f3d2b0ca7db..902bca2b273ba 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -143,6 +143,24 @@ static void ioc_release_fn(struct work_struct *work)
 	kmem_cache_free(iocontext_cachep, ioc);
 }
 
+/*
+ * Releasing icqs requires reverse order double locking and we may already be
+ ( holding a queue_lock.  Do it asynchronously from a workqueue.
+ */
+static bool ioc_delay_free(struct io_context *ioc)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ioc->lock, flags);
+	if (!hlist_empty(&ioc->icq_list)) {
+		queue_work(system_power_efficient_wq, &ioc->release_work);
+		spin_unlock_irqrestore(&ioc->lock, flags);
+		return true;
+	}
+	spin_unlock_irqrestore(&ioc->lock, flags);
+	return false;
+}
+
 /**
  * put_io_context - put a reference of io_context
  * @ioc: io_context to put
@@ -152,26 +170,8 @@ static void ioc_release_fn(struct work_struct *work)
  */
 void put_io_context(struct io_context *ioc)
 {
-	unsigned long flags;
-	bool free_ioc = false;
-
 	BUG_ON(atomic_long_read(&ioc->refcount) <= 0);
-
-	/*
-	 * Releasing ioc requires reverse order double locking and we may
-	 * already be holding a queue_lock.  Do it asynchronously from wq.
-	 */
-	if (atomic_long_dec_and_test(&ioc->refcount)) {
-		spin_lock_irqsave(&ioc->lock, flags);
-		if (!hlist_empty(&ioc->icq_list))
-			queue_work(system_power_efficient_wq,
-					&ioc->release_work);
-		else
-			free_ioc = true;
-		spin_unlock_irqrestore(&ioc->lock, flags);
-	}
-
-	if (free_ioc)
+	if (atomic_long_dec_and_test(&ioc->refcount) && !ioc_delay_free(ioc))
 		kmem_cache_free(iocontext_cachep, ioc);
 }
 EXPORT_SYMBOL_GPL(put_io_context);
-- 
2.30.2

[PATCH 6/7] block: cleanup ioc_clear_queue

[Christoph Hellwig]

Fold __ioc_clear_queue into ioc_clear_queue, remove the spurious RCU
criticial section and unify the irq locking style.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-ioc.c | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 902bca2b273ba..32ae006e1b3e8 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -192,27 +192,6 @@ void exit_io_context(struct task_struct *task)
 	}
 }
 
-static void __ioc_clear_queue(struct list_head *icq_list)
-{
-	unsigned long flags;
-
-	rcu_read_lock();
-	while (!list_empty(icq_list)) {
-		struct io_cq *icq = list_entry(icq_list->next,
-						struct io_cq, q_node);
-		struct io_context *ioc = icq->ioc;
-
-		spin_lock_irqsave(&ioc->lock, flags);
-		if (icq->flags & ICQ_DESTROYED) {
-			spin_unlock_irqrestore(&ioc->lock, flags);
-			continue;
-		}
-		ioc_destroy_icq(icq);
-		spin_unlock_irqrestore(&ioc->lock, flags);
-	}
-	rcu_read_unlock();
-}
-
 /**
  * ioc_clear_queue - break any ioc association with the specified queue
  * @q: request_queue being cleared
@@ -227,7 +206,15 @@ void ioc_clear_queue(struct request_queue *q)
 	list_splice_init(&q->icq_list, &icq_list);
 	spin_unlock_irq(&q->queue_lock);
 
-	__ioc_clear_queue(&icq_list);
+	while (!list_empty(&icq_list)) {
+		struct io_cq *icq =
+			list_entry(icq_list.next, struct io_cq, q_node);
+
+		spin_lock_irq(&icq->ioc->lock);
+		if (!(icq->flags & ICQ_DESTROYED))
+			ioc_destroy_icq(icq);
+		spin_unlock_irq(&icq->ioc->lock);
+	}
 }
 
 static struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
-- 
2.30.2

[PATCH 7/7] block: only build the icq tracking code when needed

[Christoph Hellwig]

Only bfq needs to code to track icq, so make it conditional.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/Kconfig             |  3 ++
 block/Kconfig.iosched     |  1 +
 block/blk-ioc.c           | 64 ++++++++++++++++++++++++---------------
 block/blk.h               |  6 ++++
 include/linux/iocontext.h |  6 ++--
 5 files changed, 53 insertions(+), 27 deletions(-)

diff --git a/block/Kconfig b/block/Kconfig
index c6ce41a5e5b27..d5d4197b7ed2d 100644
--- a/block/Kconfig
+++ b/block/Kconfig
@@ -35,6 +35,9 @@ config BLK_CGROUP_RWSTAT
 config BLK_DEV_BSG_COMMON
 	tristate
 
+config BLK_ICQ
+	bool
+
 config BLK_DEV_BSGLIB
 	bool "Block layer SG support v4 helper lib"
 	select BLK_DEV_BSG_COMMON
diff --git a/block/Kconfig.iosched b/block/Kconfig.iosched
index 885fee86dfcae..6155161460862 100644
--- a/block/Kconfig.iosched
+++ b/block/Kconfig.iosched
@@ -18,6 +18,7 @@ config MQ_IOSCHED_KYBER
 
 config IOSCHED_BFQ
 	tristate "BFQ I/O scheduler"
+	select BLK_ICQ
 	help
 	BFQ I/O scheduler for BLK-MQ. BFQ distributes the bandwidth of
 	of the device among all processes according to their weights,
diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 32ae006e1b3e8..5f99b9c833328 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -30,6 +30,7 @@ static void get_io_context(struct io_context *ioc)
 	atomic_long_inc(&ioc->refcount);
 }
 
+#ifdef CONFIG_BLK_ICQ
 static void icq_free_icq_rcu(struct rcu_head *head)
 {
 	struct io_cq *icq = container_of(head, struct io_cq, __rcu_head);
@@ -161,6 +162,40 @@ static bool ioc_delay_free(struct io_context *ioc)
 	return false;
 }
 
+/**
+ * ioc_clear_queue - break any ioc association with the specified queue
+ * @q: request_queue being cleared
+ *
+ * Walk @q->icq_list and exit all io_cq's.
+ */
+void ioc_clear_queue(struct request_queue *q)
+{
+	LIST_HEAD(icq_list);
+
+	spin_lock_irq(&q->queue_lock);
+	list_splice_init(&q->icq_list, &icq_list);
+	spin_unlock_irq(&q->queue_lock);
+
+	while (!list_empty(&icq_list)) {
+		struct io_cq *icq =
+			list_entry(icq_list.next, struct io_cq, q_node);
+
+		spin_lock_irq(&icq->ioc->lock);
+		if (!(icq->flags & ICQ_DESTROYED))
+			ioc_destroy_icq(icq);
+		spin_unlock_irq(&icq->ioc->lock);
+	}
+}
+#else /* CONFIG_BLK_ICQ */
+static inline void ioc_exit_icqs(struct io_context *ioc)
+{
+}
+static inline bool ioc_delay_free(struct io_context *ioc)
+{
+	return false;
+}
+#endif /* CONFIG_BLK_ICQ */
+
 /**
  * put_io_context - put a reference of io_context
  * @ioc: io_context to put
@@ -192,31 +227,6 @@ void exit_io_context(struct task_struct *task)
 	}
 }
 
-/**
- * ioc_clear_queue - break any ioc association with the specified queue
- * @q: request_queue being cleared
- *
- * Walk @q->icq_list and exit all io_cq's.
- */
-void ioc_clear_queue(struct request_queue *q)
-{
-	LIST_HEAD(icq_list);
-
-	spin_lock_irq(&q->queue_lock);
-	list_splice_init(&q->icq_list, &icq_list);
-	spin_unlock_irq(&q->queue_lock);
-
-	while (!list_empty(&icq_list)) {
-		struct io_cq *icq =
-			list_entry(icq_list.next, struct io_cq, q_node);
-
-		spin_lock_irq(&icq->ioc->lock);
-		if (!(icq->flags & ICQ_DESTROYED))
-			ioc_destroy_icq(icq);
-		spin_unlock_irq(&icq->ioc->lock);
-	}
-}
-
 static struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
 {
 	struct io_context *ioc;
@@ -228,10 +238,12 @@ static struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
 
 	atomic_long_set(&ioc->refcount, 1);
 	atomic_set(&ioc->active_ref, 1);
+#ifdef CONFIG_BLK_ICQ
 	spin_lock_init(&ioc->lock);
 	INIT_RADIX_TREE(&ioc->icq_tree, GFP_ATOMIC);
 	INIT_HLIST_HEAD(&ioc->icq_list);
 	INIT_WORK(&ioc->release_work, ioc_release_fn);
+#endif
 	return ioc;
 }
 
@@ -316,6 +328,7 @@ int __copy_io(unsigned long clone_flags, struct task_struct *tsk)
 	return 0;
 }
 
+#ifdef CONFIG_BLK_ICQ
 /**
  * ioc_lookup_icq - lookup io_cq from ioc
  * @q: the associated request_queue
@@ -441,3 +454,4 @@ static int __init blk_ioc_init(void)
 	return 0;
 }
 subsys_initcall(blk_ioc_init);
+#endif /* CONFIG_BLK_ICQ */
diff --git a/block/blk.h b/block/blk.h
index a55d82c3d1c21..39e822537d1a8 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -365,7 +365,13 @@ static inline unsigned int bio_aligned_discard_max_sectors(
  */
 struct io_cq *ioc_find_get_icq(struct request_queue *q);
 struct io_cq *ioc_lookup_icq(struct request_queue *q);
+#ifdef CONFIG_BLK_ICQ
 void ioc_clear_queue(struct request_queue *q);
+#else
+static inline void ioc_clear_queue(struct request_queue *q)
+{
+}
+#endif /* CONFIG_BLK_ICQ */
 
 #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
 extern ssize_t blk_throtl_sample_time_show(struct request_queue *q, char *page);
diff --git a/include/linux/iocontext.h b/include/linux/iocontext.h
index 82c7f4f5f4f59..ef98a994b7b2e 100644
--- a/include/linux/iocontext.h
+++ b/include/linux/iocontext.h
@@ -100,16 +100,18 @@ struct io_context {
 	atomic_long_t refcount;
 	atomic_t active_ref;
 
+	unsigned short ioprio;
+
+#ifdef CONFIG_BLK_ICQ
 	/* all the fields below are protected by this lock */
 	spinlock_t lock;
 
-	unsigned short ioprio;
-
 	struct radix_tree_root	icq_tree;
 	struct io_cq __rcu	*icq_hint;
 	struct hlist_head	icq_list;
 
 	struct work_struct release_work;
+#endif /* CONFIG_BLK_ICQ */
 };
 
 struct task_struct;
-- 
2.30.2

Re: [PATCH 5/7] block: refactor put_io_context

[Jan Kara]

On Tue 30-11-21 13:46:34, Christoph Hellwig wrote:
> Move the code to delay freeing the icqs into a separate helper.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-ioc.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/block/blk-ioc.c b/block/blk-ioc.c
> index 04f3d2b0ca7db..902bca2b273ba 100644
> --- a/block/blk-ioc.c
> +++ b/block/blk-ioc.c
> @@ -143,6 +143,24 @@ static void ioc_release_fn(struct work_struct *work)
>  	kmem_cache_free(iocontext_cachep, ioc);
>  }
>  
> +/*
> + * Releasing icqs requires reverse order double locking and we may already be
> + ( holding a queue_lock.  Do it asynchronously from a workqueue.
    ^^^ typo here

Otherwise looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

									Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/7] block: remove the NULL ioc check in put_io_context

[Jan Kara]

On Tue 30-11-21 13:46:33, Christoph Hellwig wrote:
> No caller passes in a NULL pointer, so remove the check.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/blk-ioc.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/block/blk-ioc.c b/block/blk-ioc.c
> index 0380e33930e31..04f3d2b0ca7db 100644
> --- a/block/blk-ioc.c
> +++ b/block/blk-ioc.c
> @@ -155,9 +155,6 @@ void put_io_context(struct io_context *ioc)
>  	unsigned long flags;
>  	bool free_ioc = false;
>  
> -	if (ioc == NULL)
> -		return;
> -
>  	BUG_ON(atomic_long_read(&ioc->refcount) <= 0);
>  
>  	/*
> -- 
> 2.30.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/7] block: refactor put_iocontext_active

[Jan Kara]

On Tue 30-11-21 13:46:32, Christoph Hellwig wrote:
> Factor out a ioc_exit_icqs helper to tear down the icqs and the fold
> the rest of put_iocontext_active into exit_io_context.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/7] block: simplify struct io_context refcounting

[Jan Kara]

On Tue 30-11-21 13:46:31, Christoph Hellwig wrote:
> Don't hold a reference to ->refcount for each active reference, but
> just one for all active references.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/7] block: remove the nr_task field from struct io_context

[Jan Kara]

On Tue 30-11-21 13:46:30, Christoph Hellwig wrote:
> Nothing ever looks at ->nr_tasks, so remove it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 6/7] block: cleanup ioc_clear_queue

[Jan Kara]

On Tue 30-11-21 13:46:35, Christoph Hellwig wrote:
> Fold __ioc_clear_queue into ioc_clear_queue, remove the spurious RCU
> criticial section and unify the irq locking style.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-ioc.c | 31 +++++++++----------------------
>  1 file changed, 9 insertions(+), 22 deletions(-)
> 
> diff --git a/block/blk-ioc.c b/block/blk-ioc.c
> index 902bca2b273ba..32ae006e1b3e8 100644
> --- a/block/blk-ioc.c
> +++ b/block/blk-ioc.c
> @@ -192,27 +192,6 @@ void exit_io_context(struct task_struct *task)
>  	}
>  }
>  
> -static void __ioc_clear_queue(struct list_head *icq_list)
> -{
> -	unsigned long flags;
> -
> -	rcu_read_lock();
> -	while (!list_empty(icq_list)) {
> -		struct io_cq *icq = list_entry(icq_list->next,
> -						struct io_cq, q_node);
> -		struct io_context *ioc = icq->ioc;
> -
> -		spin_lock_irqsave(&ioc->lock, flags);
> -		if (icq->flags & ICQ_DESTROYED) {
> -			spin_unlock_irqrestore(&ioc->lock, flags);
> -			continue;
> -		}
> -		ioc_destroy_icq(icq);
> -		spin_unlock_irqrestore(&ioc->lock, flags);
> -	}
> -	rcu_read_unlock();
> -}
> -
>  /**
>   * ioc_clear_queue - break any ioc association with the specified queue
>   * @q: request_queue being cleared
> @@ -227,7 +206,15 @@ void ioc_clear_queue(struct request_queue *q)
>  	list_splice_init(&q->icq_list, &icq_list);
>  	spin_unlock_irq(&q->queue_lock);
>  
> -	__ioc_clear_queue(&icq_list);

> +	while (!list_empty(&icq_list)) {
> +		struct io_cq *icq =
> +			list_entry(icq_list.next, struct io_cq, q_node);

I'm not quite sure about dropping the rcu protection here. This function
generally runs without any protection so what guards us against icq being
freed just after we've got its pointer from the list?

								Honza

> +
> +		spin_lock_irq(&icq->ioc->lock);
> +		if (!(icq->flags & ICQ_DESTROYED))
> +			ioc_destroy_icq(icq);
> +		spin_unlock_irq(&icq->ioc->lock);
> +	}
>  }
>  
>  static struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
> -- 
> 2.30.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 7/7] block: only build the icq tracking code when needed

[Jan Kara]

On Tue 30-11-21 13:46:36, Christoph Hellwig wrote:
> Only bfq needs to code to track icq, so make it conditional.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/Kconfig             |  3 ++
>  block/Kconfig.iosched     |  1 +
>  block/blk-ioc.c           | 64 ++++++++++++++++++++++++---------------
>  block/blk.h               |  6 ++++
>  include/linux/iocontext.h |  6 ++--
>  5 files changed, 53 insertions(+), 27 deletions(-)
> 
> diff --git a/block/Kconfig b/block/Kconfig
> index c6ce41a5e5b27..d5d4197b7ed2d 100644
> --- a/block/Kconfig
> +++ b/block/Kconfig
> @@ -35,6 +35,9 @@ config BLK_CGROUP_RWSTAT
>  config BLK_DEV_BSG_COMMON
>  	tristate
>  
> +config BLK_ICQ
> +	bool
> +
>  config BLK_DEV_BSGLIB
>  	bool "Block layer SG support v4 helper lib"
>  	select BLK_DEV_BSG_COMMON
> diff --git a/block/Kconfig.iosched b/block/Kconfig.iosched
> index 885fee86dfcae..6155161460862 100644
> --- a/block/Kconfig.iosched
> +++ b/block/Kconfig.iosched
> @@ -18,6 +18,7 @@ config MQ_IOSCHED_KYBER
>  
>  config IOSCHED_BFQ
>  	tristate "BFQ I/O scheduler"
> +	select BLK_ICQ
>  	help
>  	BFQ I/O scheduler for BLK-MQ. BFQ distributes the bandwidth of
>  	of the device among all processes according to their weights,
> diff --git a/block/blk-ioc.c b/block/blk-ioc.c
> index 32ae006e1b3e8..5f99b9c833328 100644
> --- a/block/blk-ioc.c
> +++ b/block/blk-ioc.c
> @@ -30,6 +30,7 @@ static void get_io_context(struct io_context *ioc)
>  	atomic_long_inc(&ioc->refcount);
>  }
>  
> +#ifdef CONFIG_BLK_ICQ
>  static void icq_free_icq_rcu(struct rcu_head *head)
>  {
>  	struct io_cq *icq = container_of(head, struct io_cq, __rcu_head);
> @@ -161,6 +162,40 @@ static bool ioc_delay_free(struct io_context *ioc)
>  	return false;
>  }
>  
> +/**
> + * ioc_clear_queue - break any ioc association with the specified queue
> + * @q: request_queue being cleared
> + *
> + * Walk @q->icq_list and exit all io_cq's.
> + */
> +void ioc_clear_queue(struct request_queue *q)
> +{
> +	LIST_HEAD(icq_list);
> +
> +	spin_lock_irq(&q->queue_lock);
> +	list_splice_init(&q->icq_list, &icq_list);
> +	spin_unlock_irq(&q->queue_lock);
> +
> +	while (!list_empty(&icq_list)) {
> +		struct io_cq *icq =
> +			list_entry(icq_list.next, struct io_cq, q_node);
> +
> +		spin_lock_irq(&icq->ioc->lock);
> +		if (!(icq->flags & ICQ_DESTROYED))
> +			ioc_destroy_icq(icq);
> +		spin_unlock_irq(&icq->ioc->lock);
> +	}
> +}
> +#else /* CONFIG_BLK_ICQ */
> +static inline void ioc_exit_icqs(struct io_context *ioc)
> +{
> +}
> +static inline bool ioc_delay_free(struct io_context *ioc)
> +{
> +	return false;
> +}
> +#endif /* CONFIG_BLK_ICQ */
> +
>  /**
>   * put_io_context - put a reference of io_context
>   * @ioc: io_context to put
> @@ -192,31 +227,6 @@ void exit_io_context(struct task_struct *task)
>  	}
>  }
>  
> -/**
> - * ioc_clear_queue - break any ioc association with the specified queue
> - * @q: request_queue being cleared
> - *
> - * Walk @q->icq_list and exit all io_cq's.
> - */
> -void ioc_clear_queue(struct request_queue *q)
> -{
> -	LIST_HEAD(icq_list);
> -
> -	spin_lock_irq(&q->queue_lock);
> -	list_splice_init(&q->icq_list, &icq_list);
> -	spin_unlock_irq(&q->queue_lock);
> -
> -	while (!list_empty(&icq_list)) {
> -		struct io_cq *icq =
> -			list_entry(icq_list.next, struct io_cq, q_node);
> -
> -		spin_lock_irq(&icq->ioc->lock);
> -		if (!(icq->flags & ICQ_DESTROYED))
> -			ioc_destroy_icq(icq);
> -		spin_unlock_irq(&icq->ioc->lock);
> -	}
> -}
> -
>  static struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
>  {
>  	struct io_context *ioc;
> @@ -228,10 +238,12 @@ static struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
>  
>  	atomic_long_set(&ioc->refcount, 1);
>  	atomic_set(&ioc->active_ref, 1);
> +#ifdef CONFIG_BLK_ICQ
>  	spin_lock_init(&ioc->lock);
>  	INIT_RADIX_TREE(&ioc->icq_tree, GFP_ATOMIC);
>  	INIT_HLIST_HEAD(&ioc->icq_list);
>  	INIT_WORK(&ioc->release_work, ioc_release_fn);
> +#endif
>  	return ioc;
>  }
>  
> @@ -316,6 +328,7 @@ int __copy_io(unsigned long clone_flags, struct task_struct *tsk)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_BLK_ICQ
>  /**
>   * ioc_lookup_icq - lookup io_cq from ioc
>   * @q: the associated request_queue
> @@ -441,3 +454,4 @@ static int __init blk_ioc_init(void)
>  	return 0;
>  }
>  subsys_initcall(blk_ioc_init);
> +#endif /* CONFIG_BLK_ICQ */
> diff --git a/block/blk.h b/block/blk.h
> index a55d82c3d1c21..39e822537d1a8 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -365,7 +365,13 @@ static inline unsigned int bio_aligned_discard_max_sectors(
>   */
>  struct io_cq *ioc_find_get_icq(struct request_queue *q);
>  struct io_cq *ioc_lookup_icq(struct request_queue *q);
> +#ifdef CONFIG_BLK_ICQ
>  void ioc_clear_queue(struct request_queue *q);
> +#else
> +static inline void ioc_clear_queue(struct request_queue *q)
> +{
> +}
> +#endif /* CONFIG_BLK_ICQ */
>  
>  #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
>  extern ssize_t blk_throtl_sample_time_show(struct request_queue *q, char *page);
> diff --git a/include/linux/iocontext.h b/include/linux/iocontext.h
> index 82c7f4f5f4f59..ef98a994b7b2e 100644
> --- a/include/linux/iocontext.h
> +++ b/include/linux/iocontext.h
> @@ -100,16 +100,18 @@ struct io_context {
>  	atomic_long_t refcount;
>  	atomic_t active_ref;
>  
> +	unsigned short ioprio;
> +
> +#ifdef CONFIG_BLK_ICQ
>  	/* all the fields below are protected by this lock */
>  	spinlock_t lock;
>  
> -	unsigned short ioprio;
> -
>  	struct radix_tree_root	icq_tree;
>  	struct io_cq __rcu	*icq_hint;
>  	struct hlist_head	icq_list;
>  
>  	struct work_struct release_work;
> +#endif /* CONFIG_BLK_ICQ */
>  };
>  
>  struct task_struct;
> -- 
> 2.30.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 6/7] block: cleanup ioc_clear_queue

[Christoph Hellwig]

On Tue, Nov 30, 2021 at 06:26:13PM +0100, Jan Kara wrote:
> I'm not quite sure about dropping the rcu protection here. This function
> generally runs without any protection so what guards us against icq being
> freed just after we've got its pointer from the list?

How does the RCU protection scheme work for the icq lookups?
ioc_lookup_icq takes it and then drops it before getting any kind
of refcount, so this all looks weird.  But I guess you are right that
I should probably keep this cargo culted scheme unless I have an
actual plan on how this could work.

While we're at it:  I don't see how put put_io_context could
be called under q->queue_lock and thus actually need the whole
workqueue scheme.  Then again we really need to do an audit on
queue_lock and split it into actually documented locks now that the
old request code is gone.

Re: [PATCH 6/7] block: cleanup ioc_clear_queue

[Jan Kara]

On Wed 01-12-21 08:27:02, Christoph Hellwig wrote:
> On Tue, Nov 30, 2021 at 06:26:13PM +0100, Jan Kara wrote:
> > I'm not quite sure about dropping the rcu protection here. This function
> > generally runs without any protection so what guards us against icq being
> > freed just after we've got its pointer from the list?
> 
> How does the RCU protection scheme work for the icq lookups?
> ioc_lookup_icq takes it and then drops it before getting any kind
> of refcount, so this all looks weird.  But I guess you are right that
> I should probably keep this cargo culted scheme unless I have an
> actual plan on how this could work.

I agree the RCU there looks like a bit of cargo-cult and would need better
documentation if nothing else. But I think the logic behind RCU protection
inside __ioc_clear_queue() is that you can safely acquire ioc->lock and
check ICQ_DESTROYED flag - which should be set if ioc got already freed, if
not set, you hold the ioc->lock so you won the race to free the ioc.
For ioc_lookup_icq() I'm not sure what's going on there, there RCU looks
completely pointless.

> While we're at it:  I don't see how put put_io_context could
> be called under q->queue_lock and thus actually need the whole
> workqueue scheme.

I don't see that either but I think in the past an equivalent of
blk_mq_free_request() could get called during request merging while holding
all the locks (I have just recently fixed a deadlock due to this in BFQ by
postponing freeing of merged requests to the caller) and
blk_mq_free_request() will call put_io_context(). So at this point I don't
think it is needed anymore.

> Then again we really need to do an audit on queue_lock and split it into
> actually documented locks now that the old request code is gone.

A worthy goal :)
								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR