cleanup and optimize block plug handling

cleanup and optimize block plug handling

[Christoph Hellwig]

Hi Jens,

this is a spinoff from Pavel's misc optimizations.  It shuld take care
of his two painpoints and also cleans up the interface for flushing
plugs a bit.

Diffstat:
 block/blk-core.c       |   17 ++++++++---------
 block/blk-mq.c         |    2 +-
 block/blk-mq.h         |    1 +
 fs/fs-writeback.c      |    5 +++--
 include/linux/blk-mq.h |    2 --
 include/linux/blkdev.h |   29 ++++-------------------------
 kernel/sched/core.c    |    5 +++--
 7 files changed, 20 insertions(+), 41 deletions(-)

[PATCH 1/4] blk-mq: only flush requests from the plug in blk_mq_submit_bio

[Christoph Hellwig]

Replace the call to blk_flush_plug_list in blk_mq_submit_bio with a
direct call to blk_mq_flush_plug_list.  This means we do not flush
plug callback from stackable devices, which doesn't really help with
the accumulated requests anyway, and it also means the cached requests
aren't freed here as they can still be used later on.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-mq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 31d9e612d2360..9bb6ece419a5b 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2571,7 +2571,7 @@ void blk_mq_submit_bio(struct bio *bio)
 		}
 
 		if (request_count >= blk_plug_max_rq_count(plug) || last) {
-			blk_flush_plug_list(plug, false);
+			blk_mq_flush_plug_list(plug, false);
 			trace_block_plug(q);
 		}
 
-- 
2.30.2

[PATCH 2/4] blk-mq: move blk_mq_flush_plug_list to block/blk-mq.h

[Christoph Hellwig]

This helper is internal to the block layer.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-mq.h         | 1 +
 include/linux/blk-mq.h | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/blk-mq.h b/block/blk-mq.h
index d8ccb341e82e9..08fb5922e611b 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -122,6 +122,7 @@ extern int blk_mq_sysfs_register(struct request_queue *q);
 extern void blk_mq_sysfs_unregister(struct request_queue *q);
 extern void blk_mq_hctx_kobj_init(struct blk_mq_hw_ctx *hctx);
 void blk_mq_free_plug_rqs(struct blk_plug *plug);
+void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule);
 
 void blk_mq_release(struct request_queue *q);
 
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 6cf35de151a95..e137802365509 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -656,8 +656,6 @@ int blk_mq_alloc_sq_tag_set(struct blk_mq_tag_set *set,
 		unsigned int set_flags);
 void blk_mq_free_tag_set(struct blk_mq_tag_set *set);
 
-void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule);
-
 void blk_mq_free_request(struct request *rq);
 
 bool blk_mq_queue_inflight(struct request_queue *q);
-- 
2.30.2

[PATCH 3/4] block: optimise blk_flush_plug_list

[Christoph Hellwig]

From: Pavel Begunkov <asml.silence@gmail.com>

Don't call flush_plug_callbacks if there are no plug callbacks.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
[hch: split from a larger patch]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index e6ad5b51d0c3d..63fee1f82bd7d 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1639,8 +1639,8 @@ EXPORT_SYMBOL(blk_check_plugged);
 
 void blk_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 {
-	flush_plug_callbacks(plug, from_schedule);
-
+	if (!list_empty(&plug->cb_list))
+		flush_plug_callbacks(plug, from_schedule);
 	if (!rq_list_empty(plug->mq_list))
 		blk_mq_flush_plug_list(plug, from_schedule);
 	if (unlikely(!from_schedule && plug->cached_rq))
-- 
2.30.2

[PATCH 4/4] block: cleanup the flush plug helpers

[Christoph Hellwig]

Consolidate the various helpers into a single blk_flush_plug helper that
takes a plk_plug and the from_scheduler bool and switch all callsites to
call it directly.  Checks that the plug is non-NULL must be performed by
the caller, something that most already do anyway.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c       | 13 ++++++-------
 fs/fs-writeback.c      |  5 +++--
 include/linux/blkdev.h | 29 ++++-------------------------
 kernel/sched/core.c    |  5 +++--
 4 files changed, 16 insertions(+), 36 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 63fee1f82bd7d..bb25934c9408a 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1089,7 +1089,7 @@ int bio_poll(struct bio *bio, struct io_comp_batch *iob, unsigned int flags)
 		return 0;
 
 	if (current->plug)
-		blk_flush_plug_list(current->plug, false);
+		blk_flush_plug(current->plug, false);
 
 	if (blk_queue_enter(q, BLK_MQ_REQ_NOWAIT))
 		return 0;
@@ -1637,7 +1637,7 @@ struct blk_plug_cb *blk_check_plugged(blk_plug_cb_fn unplug, void *data,
 }
 EXPORT_SYMBOL(blk_check_plugged);
 
-void blk_flush_plug_list(struct blk_plug *plug, bool from_schedule)
+void blk_flush_plug(struct blk_plug *plug, bool from_schedule)
 {
 	if (!list_empty(&plug->cb_list))
 		flush_plug_callbacks(plug, from_schedule);
@@ -1659,11 +1659,10 @@ void blk_flush_plug_list(struct blk_plug *plug, bool from_schedule)
  */
 void blk_finish_plug(struct blk_plug *plug)
 {
-	if (plug != current->plug)
-		return;
-	blk_flush_plug_list(plug, false);
-
-	current->plug = NULL;
+	if (plug == current->plug) {
+		blk_flush_plug(plug, false);
+		current->plug = NULL;
+	}
 }
 EXPORT_SYMBOL(blk_finish_plug);
 
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 81ec192ce0673..4124a89a1a5df 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1893,7 +1893,8 @@ static long writeback_sb_inodes(struct super_block *sb,
 			 * unplug, so get our IOs out the door before we
 			 * give up the CPU.
 			 */
-			blk_flush_plug(current);
+			if (current->plug)
+				blk_flush_plug(current->plug, false);
 			cond_resched();
 		}
 
@@ -2291,7 +2292,7 @@ void wakeup_flusher_threads(enum wb_reason reason)
 	 * If we are expecting writeback progress we must submit plugged IO.
 	 */
 	if (blk_needs_flush_plug(current))
-		blk_schedule_flush_plug(current);
+		blk_flush_plug(current->plug, true);
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(bdi, &bdi_list, bdi_list)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c95aee920bbb4..16647fbd1c2c4 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -708,9 +708,8 @@ extern void blk_set_queue_dying(struct request_queue *);
  * as the lock contention for request_queue lock is reduced.
  *
  * It is ok not to disable preemption when adding the request to the plug list
- * or when attempting a merge, because blk_schedule_flush_list() will only flush
- * the plug list when the task sleeps by itself. For details, please see
- * schedule() where blk_schedule_flush_plug() is called.
+ * or when attempting a merge. For details, please see schedule() where
+ * blk_flush_plug() is called.
  */
 struct blk_plug {
 	struct request *mq_list; /* blk-mq requests */
@@ -740,23 +739,8 @@ extern struct blk_plug_cb *blk_check_plugged(blk_plug_cb_fn unplug,
 extern void blk_start_plug(struct blk_plug *);
 extern void blk_start_plug_nr_ios(struct blk_plug *, unsigned short);
 extern void blk_finish_plug(struct blk_plug *);
-extern void blk_flush_plug_list(struct blk_plug *, bool);
 
-static inline void blk_flush_plug(struct task_struct *tsk)
-{
-	struct blk_plug *plug = tsk->plug;
-
-	if (plug)
-		blk_flush_plug_list(plug, false);
-}
-
-static inline void blk_schedule_flush_plug(struct task_struct *tsk)
-{
-	struct blk_plug *plug = tsk->plug;
-
-	if (plug)
-		blk_flush_plug_list(plug, true);
-}
+void blk_flush_plug(struct blk_plug *plug, bool from_schedule);
 
 static inline bool blk_needs_flush_plug(struct task_struct *tsk)
 {
@@ -785,15 +769,10 @@ static inline void blk_finish_plug(struct blk_plug *plug)
 {
 }
 
-static inline void blk_flush_plug(struct task_struct *task)
-{
-}
-
-static inline void blk_schedule_flush_plug(struct task_struct *task)
+static inline void blk_flush_plug(struct blk_plug *plug, bool async)
 {
 }
 
-
 static inline bool blk_needs_flush_plug(struct task_struct *tsk)
 {
 	return false;
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 92ef7b68198c4..34f37502c27e1 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6343,7 +6343,7 @@ static inline void sched_submit_work(struct task_struct *tsk)
 	 * make sure to submit it to avoid deadlocks.
 	 */
 	if (blk_needs_flush_plug(tsk))
-		blk_schedule_flush_plug(tsk);
+		blk_flush_plug(tsk->plug, true);
 }
 
 static void sched_update_worker(struct task_struct *tsk)
@@ -8354,7 +8354,8 @@ int io_schedule_prepare(void)
 	int old_iowait = current->in_iowait;
 
 	current->in_iowait = 1;
-	blk_schedule_flush_plug(current);
+	if (current->plug)
+		blk_flush_plug(current->plug, true);
 
 	return old_iowait;
 }
-- 
2.30.2

Re: cleanup and optimize block plug handling

[Jens Axboe]

On Wed, 20 Oct 2021 16:41:15 +0200, Christoph Hellwig wrote:
> this is a spinoff from Pavel's misc optimizations.  It shuld take care
> of his two painpoints and also cleans up the interface for flushing
> plugs a bit.
> 
> Diffstat:
>  block/blk-core.c       |   17 ++++++++---------
>  block/blk-mq.c         |    2 +-
>  block/blk-mq.h         |    1 +
>  fs/fs-writeback.c      |    5 +++--
>  include/linux/blk-mq.h |    2 --
>  include/linux/blkdev.h |   29 ++++-------------------------
>  kernel/sched/core.c    |    5 +++--
>  7 files changed, 20 insertions(+), 41 deletions(-)
> 
> [...]

Applied, thanks!

[1/4] blk-mq: only flush requests from the plug in blk_mq_submit_bio
      commit: a214b949d8e365583dd67441f6f608f0b20f7f52
[2/4] blk-mq: move blk_mq_flush_plug_list to block/blk-mq.h
      commit: dbb6f764a079d1dea883c6f2439d91db4f0fb2f2
[3/4] block: optimise blk_flush_plug_list
      commit: b600455d84307696b3cb7debdaf3081080748409
[4/4] block: cleanup the flush plug helpers
      commit: 008f75a20e7072d0840ec323c39b42206f3fa8a0

Best regards,
-- 
Jens Axboe

Re: [PATCH 4/4] block: cleanup the flush plug helpers

[Nathan Chancellor]

On Wed, Oct 20, 2021 at 04:41:19PM +0200, Christoph Hellwig wrote:
> Consolidate the various helpers into a single blk_flush_plug helper that
> takes a plk_plug and the from_scheduler bool and switch all callsites to
> call it directly.  Checks that the plug is non-NULL must be performed by
> the caller, something that most already do anyway.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

This patch as commit 008f75a20e70 ("block: cleanup the flush plug
helpers") in -next causes the following errors with CONFIG_BLOCK=n
(tinyconfig):

kernel/sched/core.c: In function ‘sched_submit_work’:
kernel/sched/core.c:6346:35: error: ‘struct task_struct’ has no member named ‘plug’
 6346 |                 blk_flush_plug(tsk->plug, true);
      |                                   ^~
kernel/sched/core.c: In function ‘io_schedule_prepare’:
kernel/sched/core.c:8357:20: error: ‘struct task_struct’ has no member named ‘plug’
 8357 |         if (current->plug)
      |                    ^~
kernel/sched/core.c:8358:39: error: ‘struct task_struct’ has no member named ‘plug’
 8358 |                 blk_flush_plug(current->plug, true);
      |                                       ^~

I tested the latest block tree and did not see it fixed nor did I see it
reported or fixed elsewhere.

Cheers,
Nathan

Re: [PATCH 4/4] block: cleanup the flush plug helpers

[Jens Axboe]

On 10/22/21 2:09 PM, Nathan Chancellor wrote:
> On Wed, Oct 20, 2021 at 04:41:19PM +0200, Christoph Hellwig wrote:
>> Consolidate the various helpers into a single blk_flush_plug helper that
>> takes a plk_plug and the from_scheduler bool and switch all callsites to
>> call it directly.  Checks that the plug is non-NULL must be performed by
>> the caller, something that most already do anyway.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> This patch as commit 008f75a20e70 ("block: cleanup the flush plug
> helpers") in -next causes the following errors with CONFIG_BLOCK=n
> (tinyconfig):
> 
> kernel/sched/core.c: In function ‘sched_submit_work’:
> kernel/sched/core.c:6346:35: error: ‘struct task_struct’ has no member named ‘plug’
>  6346 |                 blk_flush_plug(tsk->plug, true);
>       |                                   ^~
> kernel/sched/core.c: In function ‘io_schedule_prepare’:
> kernel/sched/core.c:8357:20: error: ‘struct task_struct’ has no member named ‘plug’
>  8357 |         if (current->plug)
>       |                    ^~
> kernel/sched/core.c:8358:39: error: ‘struct task_struct’ has no member named ‘plug’
>  8358 |                 blk_flush_plug(current->plug, true);
>       |                                       ^~
> 
> I tested the latest block tree and did not see it fixed nor did I see it
> reported or fixed elsewhere.

This should fix it, thanks for reporting.

commit 599593a82fc57f5e9453c8ef7420df3206934a0c
Author: Jens Axboe <axboe@kernel.dk>
Date:   Fri Oct 22 19:35:45 2021 -0600

    sched: make task_struct->plug always defined
    
    If CONFIG_BLOCK isn't set, then it's an empty struct anyway. Just make
    it generally available, so we don't break the compile:
    
    kernel/sched/core.c: In function ‘sched_submit_work’:
    kernel/sched/core.c:6346:35: error: ‘struct task_struct’ has no member named ‘plug’
     6346 |                 blk_flush_plug(tsk->plug, true);
          |                                   ^~
    kernel/sched/core.c: In function ‘io_schedule_prepare’:
    kernel/sched/core.c:8357:20: error: ‘struct task_struct’ has no member named ‘plug’
     8357 |         if (current->plug)
          |                    ^~
    kernel/sched/core.c:8358:39: error: ‘struct task_struct’ has no member named ‘plug’
     8358 |                 blk_flush_plug(current->plug, true);
          |                                       ^~
    
    Reported-by: Nathan Chancellor <nathan@kernel.org>
    Fixes: 008f75a20e70 ("block: cleanup the flush plug helpers")
    Signed-off-by: Jens Axboe <axboe@kernel.dk>

diff --git a/include/linux/sched.h b/include/linux/sched.h
index c1a927ddec64..e0454e60fe8f 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1160,10 +1160,8 @@ struct task_struct {
 	/* Stacked block device info: */
 	struct bio_list			*bio_list;
 
-#ifdef CONFIG_BLOCK
 	/* Stack plugging: */
 	struct blk_plug			*plug;
-#endif
 
 	/* VM state: */
 	struct reclaim_state		*reclaim_state;

-- 
Jens Axboe