a few small bio related cleanups

a few small bio related cleanups

[Christoph Hellwig]

Hi Jens,

below a few patches to clean up the bio submission path.

[PATCH 1/4] block: improve the submit_bio and generic_make_request documentation

[Christoph Hellwig]

The current documentation is a little weird, as it doesn't clearly
explain which function to use, and also has the guts of the information
on generic_make_request, which is the internal interface for stacking
drivers.

Fix this up by properly documenting submit_bio, and only documenting
the differences and the use case for generic_make_request.

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

diff --git a/block/blk-core.c b/block/blk-core.c
index dffff21008886..68351ee94ad2e 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -992,28 +992,13 @@ generic_make_request_checks(struct bio *bio)
 }
 
 /**
- * generic_make_request - hand a buffer to its device driver for I/O
+ * generic_make_request - re-submit a bio to the block device layer for I/O
  * @bio:  The bio describing the location in memory and on the device.
  *
- * generic_make_request() is used to make I/O requests of block
- * devices. It is passed a &struct bio, which describes the I/O that needs
- * to be done.
- *
- * generic_make_request() does not return any status.  The
- * success/failure status of the request, along with notification of
- * completion, is delivered asynchronously through the bio->bi_end_io
- * function described (one day) else where.
- *
- * The caller of generic_make_request must make sure that bi_io_vec
- * are set to describe the memory buffer, and that bi_dev and bi_sector are
- * set to describe the device address, and the
- * bi_end_io and optionally bi_private are set to describe how
- * completion notification should be signaled.
- *
- * generic_make_request and the drivers it calls may use bi_next if this
- * bio happens to be merged with someone else, and may resubmit the bio to
- * a lower device by calling into generic_make_request recursively, which
- * means the bio should NOT be touched after the call to ->make_request_fn.
+ * This is a version of submit_bio() that shall only be used for I/O that is
+ * resubmitted to lower level drivers by stacking block drivers.  All file
+ * systems and other upper level users of the block layer should use
+ * submit_bio() instead.
  */
 blk_qc_t generic_make_request(struct bio *bio)
 {
@@ -1152,10 +1137,14 @@ EXPORT_SYMBOL_GPL(direct_make_request);
  * submit_bio - submit a bio to the block device layer for I/O
  * @bio: The &struct bio which describes the I/O
  *
- * submit_bio() is very similar in purpose to generic_make_request(), and
- * uses that function to do most of the work. Both are fairly rough
- * interfaces; @bio must be presetup and ready for I/O.
+ * submit_bio() is used to submit I/O requests to block devices.  It is passed a
+ * fully set up &struct bio that describes the I/O that needs to be done.  The
+ * bio will be send to the device described by the bi_disk and bi_partno fields.
  *
+ * The success/failure status of the request, along with notification of
+ * completion, is delivered asynchronously through the ->bi_end_io() callback
+ * in @bio.  The bio must NOT be touched by thecaller until ->bi_end_io() has
+ * been called.
  */
 blk_qc_t submit_bio(struct bio *bio)
 {
-- 
2.26.2

Re: [PATCH 1/4] block: improve the submit_bio and generic_make_request documentation

[Johannes Thumshirn]

OK for me,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 1/4] block: improve the submit_bio and generic_make_request documentation

[Keith Busch]

On Tue, Apr 28, 2020 at 01:27:53PM +0200, Christoph Hellwig wrote:
> + * fully set up &struct bio that describes the I/O that needs to be done.  The
> + * bio will be send to the device described by the bi_disk and bi_partno fields.

s/send/sent

Otherwise looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>

[PATCH 2/4] block: cleanup the memory stall accounting in submit_bio

[Christoph Hellwig]

Instead of a convoluted chain just check for REQ_OP_READ directly,
and keep all the memory stall code together in a single unlikely
branch.

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

diff --git a/block/blk-core.c b/block/blk-core.c
index 68351ee94ad2e..81a291085c6ca 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1148,10 +1148,6 @@ EXPORT_SYMBOL_GPL(direct_make_request);
  */
 blk_qc_t submit_bio(struct bio *bio)
 {
-	bool workingset_read = false;
-	unsigned long pflags;
-	blk_qc_t ret;
-
 	if (blkcg_punt_bio_submit(bio))
 		return BLK_QC_T_NONE;
 
@@ -1170,8 +1166,6 @@ blk_qc_t submit_bio(struct bio *bio)
 		if (op_is_write(bio_op(bio))) {
 			count_vm_events(PGPGOUT, count);
 		} else {
-			if (bio_flagged(bio, BIO_WORKINGSET))
-				workingset_read = true;
 			task_io_account_read(bio->bi_iter.bi_size);
 			count_vm_events(PGPGIN, count);
 		}
@@ -1187,20 +1181,24 @@ blk_qc_t submit_bio(struct bio *bio)
 	}
 
 	/*
-	 * If we're reading data that is part of the userspace
-	 * workingset, count submission time as memory stall. When the
-	 * device is congested, or the submitting cgroup IO-throttled,
-	 * submission can be a significant part of overall IO time.
+	 * If we're reading data that is part of the userspace workingset, count
+	 * submission time as memory stall.  When the device is congested, or
+	 * the submitting cgroup IO-throttled, submission can be a significant
+	 * part of overall IO time.
 	 */
-	if (workingset_read)
-		psi_memstall_enter(&pflags);
-
-	ret = generic_make_request(bio);
+	if (unlikely(bio_op(bio) == REQ_OP_READ &&
+	    bio_flagged(bio, BIO_WORKINGSET))) {
+		unsigned long pflags;
+		blk_qc_t ret;
 
-	if (workingset_read)
+		psi_memstall_enter(&pflags);
+		ret = generic_make_request(bio);
 		psi_memstall_leave(&pflags);
 
-	return ret;
+		return ret;
+	}
+
+	return generic_make_request(bio);
 }
 EXPORT_SYMBOL(submit_bio);
 
-- 
2.26.2

Re: [PATCH 2/4] block: cleanup the memory stall accounting in submit_bio

[Johannes Thumshirn]

On 28/04/2020 13:28, Christoph Hellwig wrote:
> Instead of a convoluted chain just check for REQ_OP_READ directly,
> and keep all the memory stall code together in a single unlikely
> branch.

Looks good, I just have one question, why the 'unlikely' annotation for 
the psi code?

In the current code, the psi path is not "unlikely" (only 
REQ_OP_WRITE_SAME is). Is there any measurable benefit coming from the 
annotation?

Anyways,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 2/4] block: cleanup the memory stall accounting in submit_bio

[Christoph Hellwig]

On Tue, Apr 28, 2020 at 02:12:49PM +0000, Johannes Thumshirn wrote:
> On 28/04/2020 13:28, Christoph Hellwig wrote:
> > Instead of a convoluted chain just check for REQ_OP_READ directly,
> > and keep all the memory stall code together in a single unlikely
> > branch.
> 
> Looks good, I just have one question, why the 'unlikely' annotation for 
> the psi code?
> 
> In the current code, the psi path is not "unlikely" (only 
> REQ_OP_WRITE_SAME is). Is there any measurable benefit coming from the 
> annotation?

Following the unlikely check in __bio_add_page that Johanness added
there with the original addition.

Re: [PATCH 2/4] block: cleanup the memory stall accounting in submit_bio

[Johannes Weiner]

On Tue, Apr 28, 2020 at 01:27:54PM +0200, Christoph Hellwig wrote:
> Instead of a convoluted chain just check for REQ_OP_READ directly,
> and keep all the memory stall code together in a single unlikely
> branch.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Hm, I have to say I don't prefer one version over the other here.

Keeping the the stall annotation code more compact is nice, but the
duplicate generic_make_request() seems to suggest that the requests
themselves are also somehow made differently, when it's really just
the annotation that's conditional.

That said, the patch looks correct to me.

Reviewed-by: Johannes Weiner <hannes@cmpxchg.org>

[PATCH 3/4] block: replace BIO_QUEUE_ENTERED with BIO_CGROUP_ACCT

[Christoph Hellwig]

BIO_QUEUE_ENTERED is only used for cgroup accounting now, so rename
the flag and move setting it into the cgroup code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-merge.c          | 10 ----------
 include/linux/blk-cgroup.h | 10 ++++++----
 include/linux/blk_types.h  |  2 +-
 3 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index c49eb3bdd0be8..a04e991b5ded9 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -336,16 +336,6 @@ void __blk_queue_split(struct request_queue *q, struct bio **bio,
 		/* there isn't chance to merge the splitted bio */
 		split->bi_opf |= REQ_NOMERGE;
 
-		/*
-		 * Since we're recursing into make_request here, ensure
-		 * that we mark this bio as already having entered the queue.
-		 * If not, and the queue is going away, we can get stuck
-		 * forever on waiting for the queue reference to drop. But
-		 * that will never happen, as we're already holding a
-		 * reference to it.
-		 */
-		bio_set_flag(*bio, BIO_QUEUE_ENTERED);
-
 		bio_chain(split, *bio);
 		trace_block_split(q, split, (*bio)->bi_iter.bi_sector);
 		generic_make_request(*bio);
diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
index 35f8ffe92b702..4deb8bb7b6afa 100644
--- a/include/linux/blk-cgroup.h
+++ b/include/linux/blk-cgroup.h
@@ -607,12 +607,14 @@ static inline bool blkcg_bio_issue_check(struct request_queue *q,
 		u64_stats_update_begin(&bis->sync);
 
 		/*
-		 * If the bio is flagged with BIO_QUEUE_ENTERED it means this
-		 * is a split bio and we would have already accounted for the
-		 * size of the bio.
+		 * If the bio is flagged with BIO_CGROUP_ACCT it means this is a
+		 * split bio and we would have already accounted for the size of
+		 * the bio.
 		 */
-		if (!bio_flagged(bio, BIO_QUEUE_ENTERED))
+		if (!bio_flagged(bio, BIO_CGROUP_ACCT)) {
+			bio_set_flag(bio, BIO_CGROUP_ACCT);
 			bis->cur.bytes[rwd] += bio->bi_iter.bi_size;
+		}
 		bis->cur.ios[rwd]++;
 
 		u64_stats_update_end(&bis->sync);
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 31eb92876be7c..90895d594e647 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -220,7 +220,7 @@ enum {
 				 * throttling rules. Don't do it again. */
 	BIO_TRACE_COMPLETION,	/* bio_endio() should trace the final completion
 				 * of this bio. */
-	BIO_QUEUE_ENTERED,	/* can use blk_queue_enter_live() */
+	BIO_CGROUP_ACCT,	/* has been accounted to a cgroup */
 	BIO_TRACKED,		/* set if bio goes through the rq_qos path */
 	BIO_FLAG_LAST
 };
-- 
2.26.2

Re: [PATCH 3/4] block: replace BIO_QUEUE_ENTERED with BIO_CGROUP_ACCT

[Johannes Thumshirn]

On 28/04/2020 13:28, Christoph Hellwig wrote:
> @@ -607,12 +607,14 @@ static inline bool blkcg_bio_issue_check(struct request_queue *q,
>   		u64_stats_update_begin(&bis->sync);
>   
>   		/*
> -		 * If the bio is flagged with BIO_QUEUE_ENTERED it means this
> -		 * is a split bio and we would have already accounted for the
> -		 * size of the bio.
> +		 * If the bio is flagged with BIO_CGROUP_ACCT it means this is a
> +		 * split bio and we would have already accounted for the size of
> +		 * the bio.
>   		 */
> -		if (!bio_flagged(bio, BIO_QUEUE_ENTERED))
> +		if (!bio_flagged(bio, BIO_CGROUP_ACCT)) {
> +			bio_set_flag(bio, BIO_CGROUP_ACCT);
>   			bis->cur.bytes[rwd] += bio->bi_iter.bi_size;
> +		}
>   		bis->cur.ios[rwd]++;
>   
>   		u64_stats_update_end(&bis->sync);


This is completely unrelated to the patch, but why is 
blkcg_bio_issue_check() static inline? It's only called in 
generic_make_request_checks().

Re: [PATCH 3/4] block: replace BIO_QUEUE_ENTERED with BIO_CGROUP_ACCT

[Christoph Hellwig]

On Tue, Apr 28, 2020 at 02:23:52PM +0000, Johannes Thumshirn wrote:
> This is completely unrelated to the patch, but why is 
> blkcg_bio_issue_check() static inline? It's only called in 
> generic_make_request_checks().

The whole blk-cgroup group is just crazy :(  This isn't even the worst
part of it.

Re: [PATCH 3/4] block: replace BIO_QUEUE_ENTERED with BIO_CGROUP_ACCT

[Johannes Thumshirn]

On 28/04/2020 16:25, Christoph Hellwig wrote:
> On Tue, Apr 28, 2020 at 02:23:52PM +0000, Johannes Thumshirn wrote:
>> This is completely unrelated to the patch, but why is
>> blkcg_bio_issue_check() static inline? It's only called in
>> generic_make_request_checks().
> 
> The whole blk-cgroup group is just crazy :(  This isn't even the worst
> part of it.
> 

Uff, sounds like the next cleanup desperately needed.

[PATCH 4/4] block: add a bio_queue_enter helper

[Christoph Hellwig]

Add a little helper that passes the right nowait flag to blk_queue_enter
based on the bio flag, and terminates the bio with the right error code
if entering the queue fails.

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

diff --git a/block/blk-core.c b/block/blk-core.c
index 81a291085c6ca..7f11560bfddbb 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -440,6 +440,23 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags)
 	}
 }
 
+static inline int bio_queue_enter(struct bio *bio)
+{
+	struct request_queue *q = bio->bi_disk->queue;
+	bool nowait = bio->bi_opf & REQ_NOWAIT;
+	int ret;
+
+	ret = blk_queue_enter(q, nowait ? BLK_MQ_REQ_NOWAIT : 0);
+	if (unlikely(ret)) {
+		if (nowait && !blk_queue_dying(q))
+			bio_wouldblock_error(bio);
+		else
+			bio_io_error(bio);
+	}
+
+	return ret;
+}
+
 void blk_queue_exit(struct request_queue *q)
 {
 	percpu_ref_put(&q->q_usage_counter);
@@ -1049,10 +1066,8 @@ blk_qc_t generic_make_request(struct bio *bio)
 	current->bio_list = bio_list_on_stack;
 	do {
 		struct request_queue *q = bio->bi_disk->queue;
-		blk_mq_req_flags_t flags = bio->bi_opf & REQ_NOWAIT ?
-			BLK_MQ_REQ_NOWAIT : 0;
 
-		if (likely(blk_queue_enter(q, flags) == 0)) {
+		if (likely(bio_queue_enter(bio) == 0)) {
 			struct bio_list lower, same;
 
 			/* Create a fresh bio_list for all subordinate requests */
@@ -1079,12 +1094,6 @@ blk_qc_t generic_make_request(struct bio *bio)
 			bio_list_merge(&bio_list_on_stack[0], &lower);
 			bio_list_merge(&bio_list_on_stack[0], &same);
 			bio_list_merge(&bio_list_on_stack[0], &bio_list_on_stack[1]);
-		} else {
-			if (unlikely(!blk_queue_dying(q) &&
-					(bio->bi_opf & REQ_NOWAIT)))
-				bio_wouldblock_error(bio);
-			else
-				bio_io_error(bio);
 		}
 		bio = bio_list_pop(&bio_list_on_stack[0]);
 	} while (bio);
@@ -1106,30 +1115,19 @@ EXPORT_SYMBOL(generic_make_request);
 blk_qc_t direct_make_request(struct bio *bio)
 {
 	struct request_queue *q = bio->bi_disk->queue;
-	bool nowait = bio->bi_opf & REQ_NOWAIT;
 	blk_qc_t ret;
 
-	if (WARN_ON_ONCE(q->make_request_fn))
-		goto io_error;
-	if (!generic_make_request_checks(bio))
+	if (WARN_ON_ONCE(q->make_request_fn)) {
+		bio_io_error(bio);
 		return BLK_QC_T_NONE;
-
-	if (unlikely(blk_queue_enter(q, nowait ? BLK_MQ_REQ_NOWAIT : 0))) {
-		if (nowait && !blk_queue_dying(q))
-			goto would_block;
-		goto io_error;
 	}
-
+	if (!generic_make_request_checks(bio))
+		return BLK_QC_T_NONE;
+	if (unlikely(bio_queue_enter(bio)))
+		return BLK_QC_T_NONE;
 	ret = blk_mq_make_request(q, bio);
 	blk_queue_exit(q);
 	return ret;
-
-would_block:
-	bio_wouldblock_error(bio);
-	return BLK_QC_T_NONE;
-io_error:
-	bio_io_error(bio);
-	return BLK_QC_T_NONE;
 }
 EXPORT_SYMBOL_GPL(direct_make_request);
 
-- 
2.26.2

Re: [PATCH 4/4] block: add a bio_queue_enter helper

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: a few small bio related cleanups

[Jens Axboe]

On 4/28/20 5:27 AM, Christoph Hellwig wrote:
> Hi Jens,
> 
> below a few patches to clean up the bio submission path.

Applied, thanks.

-- 
Jens Axboe