diff mbox series

block: handle fast path of bio splitting inline

Message ID 30045b11-0064-1849-5b10-f8fa05c6958d@kernel.dk (mailing list archive)
State New, archived
Headers show
Series block: handle fast path of bio splitting inline | expand

Commit Message

Jens Axboe Oct. 13, 2021, 8:44 p.m. UTC 
The fast path is no splitting needed. Separate the handling into a
check part we can inline, and an out-of-line handling path if we do
need to split.

Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

Comments

Christoph Hellwig Oct. 14, 2021, 5:38 a.m. UTC | #1 
On Wed, Oct 13, 2021 at 02:44:14PM -0600, Jens Axboe wrote:
> The fast path is no splitting needed. Separate the handling into a
> check part we can inline, and an out-of-line handling path if we do
> need to split.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>

What about something like this version instead?

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 9a55b50708293..f333afb45eb15 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -353,21 +353,6 @@ void __blk_queue_split(struct bio **bio, unsigned int *nr_segs)
 				nr_segs);
 		break;
 	default:
-		/*
-		 * All drivers must accept single-segments bios that are <=
-		 * PAGE_SIZE.  This is a quick and dirty check that relies on
-		 * the fact that bi_io_vec[0] is always valid if a bio has data.
-		 * The check might lead to occasional false negatives when bios
-		 * are cloned, but compared to the performance impact of cloned
-		 * bios themselves the loop below doesn't matter anyway.
-		 */
-		if (!q->limits.chunk_sectors &&
-		    (*bio)->bi_vcnt == 1 &&
-		    ((*bio)->bi_io_vec[0].bv_len +
-		     (*bio)->bi_io_vec[0].bv_offset) <= PAGE_SIZE) {
-			*nr_segs = 1;
-			break;
-		}
 		split = blk_bio_segment_split(q, *bio, &q->bio_split, nr_segs);
 		break;
 	}
@@ -399,7 +384,8 @@ void blk_queue_split(struct bio **bio)
 {
 	unsigned int nr_segs;
 
-	__blk_queue_split(bio, &nr_segs);
+	if (blk_may_split(*bio))
+		__blk_queue_split(bio, &nr_segs);
 }
 EXPORT_SYMBOL(blk_queue_split);
 
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 608b270a7f6b8..7c82e052ca83f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2251,11 +2251,12 @@ void blk_mq_submit_bio(struct bio *bio)
 	const int is_flush_fua = op_is_flush(bio->bi_opf);
 	struct request *rq;
 	struct blk_plug *plug;
-	unsigned int nr_segs;
+	unsigned int nr_segs = 1;
 	blk_status_t ret;
 
 	blk_queue_bounce(q, &bio);
-	__blk_queue_split(&bio, &nr_segs);
+	if (blk_may_split(bio))
+		__blk_queue_split(&bio, &nr_segs);
 
 	if (!bio_integrity_prep(bio))
 		goto queue_exit;
diff --git a/block/blk.h b/block/blk.h
index 0afee3e6a7c1e..34b31baf51324 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -262,6 +262,31 @@ ssize_t part_timeout_show(struct device *, struct device_attribute *, char *);
 ssize_t part_timeout_store(struct device *, struct device_attribute *,
 				const char *, size_t);
 
+static inline bool blk_may_split(struct bio *bio)
+{
+	switch (bio_op(bio)) {
+	case REQ_OP_DISCARD:
+	case REQ_OP_SECURE_ERASE:
+	case REQ_OP_WRITE_ZEROES:
+	case REQ_OP_WRITE_SAME:
+		return true; /* non-trivial splitting decisions */
+	default:
+		break;
+	}
+
+	/*
+	 * All drivers must accept single-segments bios that are <= PAGE_SIZE.
+	 * This is a quick and dirty check that relies on the fact that
+	 * bi_io_vec[0] is always valid if a bio has data.  The check might
+	 * lead to occasional false negatives when bios are cloned, but compared
+	 * to the performance impact of cloned bios themselves the loop below
+	 * doesn't matter anyway.
+	 */
+	return bio->bi_bdev->bd_disk->queue->limits.chunk_sectors ||
+		bio->bi_vcnt != 1 ||
+		bio->bi_io_vec->bv_len + bio->bi_io_vec->bv_offset >PAGE_SIZE;
+}
+
 void __blk_queue_split(struct bio **bio, unsigned int *nr_segs);
 int ll_back_merge_fn(struct request *req, struct bio *bio,
 		unsigned int nr_segs);
Jens Axboe Oct. 14, 2021, 1:16 p.m. UTC | #2 
On 10/13/21 11:38 PM, Christoph Hellwig wrote:
> On Wed, Oct 13, 2021 at 02:44:14PM -0600, Jens Axboe wrote:
>> The fast path is no splitting needed. Separate the handling into a
>> check part we can inline, and an out-of-line handling path if we do
>> need to split.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> What about something like this version instead?

A bit of a combo, I think this will do fine.


commit d997c5f4001031863de1c8c437bd2fcc6a4f79a2
Author: Jens Axboe <axboe@kernel.dk>
Date:   Wed Oct 13 12:43:41 2021 -0600

    block: handle fast path of bio splitting inline
    
    The fast path is no splitting needed. Separate the handling into a
    check part we can inline, and an out-of-line handling path if we do
    need to split.
    
    Signed-off-by: Jens Axboe <axboe@kernel.dk>

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 4da2bf18fa4d..f390a8753268 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -324,6 +324,7 @@ static struct bio *blk_bio_segment_split(struct request_queue *q,
 
 /**
  * __blk_queue_split - split a bio and submit the second half
+ * @q:       [in] request_queue new bio is being queued at
  * @bio:     [in, out] bio to be split
  * @nr_segs: [out] number of segments in the first bio
  *
@@ -334,9 +335,9 @@ static struct bio *blk_bio_segment_split(struct request_queue *q,
  * of the caller to ensure that q->bio_split is only released after processing
  * of the split bio has finished.
  */
-void __blk_queue_split(struct bio **bio, unsigned int *nr_segs)
+void __blk_queue_split(struct request_queue *q, struct bio **bio,
+		       unsigned int *nr_segs)
 {
-	struct request_queue *q = (*bio)->bi_bdev->bd_disk->queue;
 	struct bio *split = NULL;
 
 	switch (bio_op(*bio)) {
@@ -353,21 +354,6 @@ void __blk_queue_split(struct bio **bio, unsigned int *nr_segs)
 				nr_segs);
 		break;
 	default:
-		/*
-		 * All drivers must accept single-segments bios that are <=
-		 * PAGE_SIZE.  This is a quick and dirty check that relies on
-		 * the fact that bi_io_vec[0] is always valid if a bio has data.
-		 * The check might lead to occasional false negatives when bios
-		 * are cloned, but compared to the performance impact of cloned
-		 * bios themselves the loop below doesn't matter anyway.
-		 */
-		if (!q->limits.chunk_sectors &&
-		    (*bio)->bi_vcnt == 1 &&
-		    ((*bio)->bi_io_vec[0].bv_len +
-		     (*bio)->bi_io_vec[0].bv_offset) <= PAGE_SIZE) {
-			*nr_segs = 1;
-			break;
-		}
 		split = blk_bio_segment_split(q, *bio, &q->bio_split, nr_segs);
 		break;
 	}
@@ -397,9 +383,11 @@ void __blk_queue_split(struct bio **bio, unsigned int *nr_segs)
  */
 void blk_queue_split(struct bio **bio)
 {
+	struct request_queue *q = (*bio)->bi_bdev->bd_disk->queue;
 	unsigned int nr_segs;
 
-	__blk_queue_split(bio, &nr_segs);
+	if (blk_may_split(q, *bio))
+		__blk_queue_split(q, bio, &nr_segs);
 }
 EXPORT_SYMBOL(blk_queue_split);
 
diff --git a/block/blk-mq.c b/block/blk-mq.c
index dd4121dcd3ce..0cca4b7a4d16 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2521,11 +2521,12 @@ void blk_mq_submit_bio(struct bio *bio)
 	struct request *rq;
 	struct blk_plug *plug;
 	struct request *same_queue_rq = NULL;
-	unsigned int nr_segs;
+	unsigned int nr_segs = 1;
 	blk_status_t ret;
 
 	blk_queue_bounce(q, &bio);
-	__blk_queue_split(&bio, &nr_segs);
+	if (blk_may_split(q, bio))
+		__blk_queue_split(q, &bio, &nr_segs);
 
 	if (!bio_integrity_prep(bio))
 		goto queue_exit;
diff --git a/block/blk.h b/block/blk.h
index fa23338449ed..f6e61cebd6ae 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -264,7 +264,32 @@ ssize_t part_timeout_show(struct device *, struct device_attribute *, char *);
 ssize_t part_timeout_store(struct device *, struct device_attribute *,
 				const char *, size_t);
 
-void __blk_queue_split(struct bio **bio, unsigned int *nr_segs);
+static inline bool blk_may_split(struct request_queue *q, struct bio *bio)
+{
+	switch (bio_op(bio)) {
+	case REQ_OP_DISCARD:
+	case REQ_OP_SECURE_ERASE:
+	case REQ_OP_WRITE_ZEROES:
+	case REQ_OP_WRITE_SAME:
+		return true; /* non-trivial splitting decisions */
+	default:
+		break;
+	}
+
+	/*
+	 * All drivers must accept single-segments bios that are <= PAGE_SIZE.
+	 * This is a quick and dirty check that relies on the fact that
+	 * bi_io_vec[0] is always valid if a bio has data.  The check might
+	 * lead to occasional false negatives when bios are cloned, but compared
+	 * to the performance impact of cloned bios themselves the loop below
+	 * doesn't matter anyway.
+	 */
+	return q->limits.chunk_sectors || bio->bi_vcnt != 1 ||
+		bio->bi_io_vec->bv_len + bio->bi_io_vec->bv_offset > PAGE_SIZE;
+}
+
+void __blk_queue_split(struct request_queue *q, struct bio **bio,
+			unsigned int *nr_segs);
 int ll_back_merge_fn(struct request *req, struct bio *bio,
 		unsigned int nr_segs);
 bool blk_attempt_req_merge(struct request_queue *q, struct request *rq,
Christoph Hellwig Oct. 16, 2021, 4:40 a.m. UTC | #3 
On Thu, Oct 14, 2021 at 07:16:50AM -0600, Jens Axboe wrote:
> On 10/13/21 11:38 PM, Christoph Hellwig wrote:
> > On Wed, Oct 13, 2021 at 02:44:14PM -0600, Jens Axboe wrote:
> >> The fast path is no splitting needed. Separate the handling into a
> >> check part we can inline, and an out-of-line handling path if we do
> >> need to split.
> >>
> >> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> > 
> > What about something like this version instead?
> 
> A bit of a combo, I think this will do fine.

Fine with me:

Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 9a55b5070829..20ec9f00801a 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -14,6 +14,7 @@ 
 #include "blk.h"
 #include "blk-rq-qos.h"
 #include "blk-throttle.h"
+#include "blk-merge.h"
 
 static inline void bio_get_first_bvec(struct bio *bio, struct bio_vec *bv)
 {
@@ -92,10 +93,8 @@  static inline bool req_gap_front_merge(struct request *req, struct bio *bio)
 	return bio_will_gap(req->q, NULL, bio, req->bio);
 }
 
-static struct bio *blk_bio_discard_split(struct request_queue *q,
-					 struct bio *bio,
-					 struct bio_set *bs,
-					 unsigned *nsegs)
+struct bio *blk_bio_discard_split(struct request_queue *q, struct bio *bio,
+				  struct bio_set *bs, unsigned *nsegs)
 {
 	unsigned int max_discard_sectors, granularity;
 	int alignment;
@@ -136,8 +135,8 @@  static struct bio *blk_bio_discard_split(struct request_queue *q,
 	return bio_split(bio, split_sectors, GFP_NOIO, bs);
 }
 
-static struct bio *blk_bio_write_zeroes_split(struct request_queue *q,
-		struct bio *bio, struct bio_set *bs, unsigned *nsegs)
+struct bio *blk_bio_write_zeroes_split(struct request_queue *q, struct bio *bio,
+				       struct bio_set *bs, unsigned *nsegs)
 {
 	*nsegs = 0;
 
@@ -150,10 +149,8 @@  static struct bio *blk_bio_write_zeroes_split(struct request_queue *q,
 	return bio_split(bio, q->limits.max_write_zeroes_sectors, GFP_NOIO, bs);
 }
 
-static struct bio *blk_bio_write_same_split(struct request_queue *q,
-					    struct bio *bio,
-					    struct bio_set *bs,
-					    unsigned *nsegs)
+struct bio *blk_bio_write_same_split(struct request_queue *q, struct bio *bio,
+				     struct bio_set *bs, unsigned *nsegs)
 {
 	*nsegs = 1;
 
@@ -275,10 +272,8 @@  static bool bvec_split_segs(const struct request_queue *q,
  * responsible for ensuring that @bs is only destroyed after processing of the
  * split bio has finished.
  */
-static struct bio *blk_bio_segment_split(struct request_queue *q,
-					 struct bio *bio,
-					 struct bio_set *bs,
-					 unsigned *segs)
+struct bio *blk_bio_segment_split(struct request_queue *q, struct bio *bio,
+				  struct bio_set *bs, unsigned *segs)
 {
 	struct bio_vec bv, bvprv, *bvprvp = NULL;
 	struct bvec_iter iter;
@@ -322,67 +317,17 @@  static struct bio *blk_bio_segment_split(struct request_queue *q,
 	return bio_split(bio, sectors, GFP_NOIO, bs);
 }
 
-/**
- * __blk_queue_split - split a bio and submit the second half
- * @bio:     [in, out] bio to be split
- * @nr_segs: [out] number of segments in the first bio
- *
- * Split a bio into two bios, chain the two bios, submit the second half and
- * store a pointer to the first half in *@bio. If the second bio is still too
- * big it will be split by a recursive call to this function. Since this
- * function may allocate a new bio from q->bio_split, it is the responsibility
- * of the caller to ensure that q->bio_split is only released after processing
- * of the split bio has finished.
- */
-void __blk_queue_split(struct bio **bio, unsigned int *nr_segs)
+void blk_bio_handle_split(struct bio **bio, struct bio *split)
 {
-	struct request_queue *q = (*bio)->bi_bdev->bd_disk->queue;
-	struct bio *split = NULL;
-
-	switch (bio_op(*bio)) {
-	case REQ_OP_DISCARD:
-	case REQ_OP_SECURE_ERASE:
-		split = blk_bio_discard_split(q, *bio, &q->bio_split, nr_segs);
-		break;
-	case REQ_OP_WRITE_ZEROES:
-		split = blk_bio_write_zeroes_split(q, *bio, &q->bio_split,
-				nr_segs);
-		break;
-	case REQ_OP_WRITE_SAME:
-		split = blk_bio_write_same_split(q, *bio, &q->bio_split,
-				nr_segs);
-		break;
-	default:
-		/*
-		 * All drivers must accept single-segments bios that are <=
-		 * PAGE_SIZE.  This is a quick and dirty check that relies on
-		 * the fact that bi_io_vec[0] is always valid if a bio has data.
-		 * The check might lead to occasional false negatives when bios
-		 * are cloned, but compared to the performance impact of cloned
-		 * bios themselves the loop below doesn't matter anyway.
-		 */
-		if (!q->limits.chunk_sectors &&
-		    (*bio)->bi_vcnt == 1 &&
-		    ((*bio)->bi_io_vec[0].bv_len +
-		     (*bio)->bi_io_vec[0].bv_offset) <= PAGE_SIZE) {
-			*nr_segs = 1;
-			break;
-		}
-		split = blk_bio_segment_split(q, *bio, &q->bio_split, nr_segs);
-		break;
-	}
-
-	if (split) {
-		/* there isn't chance to merge the splitted bio */
-		split->bi_opf |= REQ_NOMERGE;
+	/* there isn't chance to merge the splitted bio */
+	split->bi_opf |= REQ_NOMERGE;
 
-		bio_chain(split, *bio);
-		trace_block_split(split, (*bio)->bi_iter.bi_sector);
-		submit_bio_noacct(*bio);
-		*bio = split;
+	bio_chain(split, *bio);
+	trace_block_split(split, (*bio)->bi_iter.bi_sector);
+	submit_bio_noacct(*bio);
+	*bio = split;
 
-		blk_throtl_charge_bio_split(*bio);
-	}
+	blk_throtl_charge_bio_split(*bio);
 }
 
 /**
@@ -397,9 +342,10 @@  void __blk_queue_split(struct bio **bio, unsigned int *nr_segs)
  */
 void blk_queue_split(struct bio **bio)
 {
+	struct request_queue *q = (*bio)->bi_bdev->bd_disk->queue;
 	unsigned int nr_segs;
 
-	__blk_queue_split(bio, &nr_segs);
+	__blk_queue_split(q, bio, &nr_segs);
 }
 EXPORT_SYMBOL(blk_queue_split);
 
diff --git a/block/blk-merge.h b/block/blk-merge.h
new file mode 100644
index 000000000000..8f0b7dec2dd2
--- /dev/null
+++ b/block/blk-merge.h
@@ -0,0 +1,71 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef BLK_MERGE_H
+#define BLK_MERGE_H
+
+#include "blk-mq.h"
+
+struct bio *blk_bio_discard_split(struct request_queue *q, struct bio *bio,
+				  struct bio_set *bs, unsigned *nsegs);
+struct bio *blk_bio_write_zeroes_split(struct request_queue *q, struct bio *bio,
+				       struct bio_set *bs, unsigned *nsegs);
+struct bio *blk_bio_write_same_split(struct request_queue *q, struct bio *bio,
+				     struct bio_set *bs, unsigned *nsegs);
+struct bio *blk_bio_segment_split(struct request_queue *q, struct bio *bio,
+				  struct bio_set *bs, unsigned *segs);
+void blk_bio_handle_split(struct bio **bio, struct bio *split);
+
+/**
+ * blk_queue_split - split a bio and submit the second half
+ * @bio:     [in, out] bio to be split
+ * @nr_segs: [out] number of segments in the first bio
+ *
+ * Split a bio into two bios, chain the two bios, submit the second half and
+ * store a pointer to the first half in *@bio. If the second bio is still too
+ * big it will be split by a recursive call to this function. Since this
+ * function may allocate a new bio from q->bio_split, it is the responsibility
+ * of the caller to ensure that q->bio_split is only released after processing
+ * of the split bio has finished.
+ */
+static inline void __blk_queue_split(struct request_queue *q, struct bio **bio,
+				     unsigned int *nr_segs)
+{
+	struct bio *split = NULL;
+
+	switch (bio_op(*bio)) {
+	case REQ_OP_DISCARD:
+	case REQ_OP_SECURE_ERASE:
+		split = blk_bio_discard_split(q, *bio, &q->bio_split, nr_segs);
+		break;
+	case REQ_OP_WRITE_ZEROES:
+		split = blk_bio_write_zeroes_split(q, *bio, &q->bio_split,
+				nr_segs);
+		break;
+	case REQ_OP_WRITE_SAME:
+		split = blk_bio_write_same_split(q, *bio, &q->bio_split,
+				nr_segs);
+		break;
+	default:
+		/*
+		 * All drivers must accept single-segments bios that are <=
+		 * PAGE_SIZE.  This is a quick and dirty check that relies on
+		 * the fact that bi_io_vec[0] is always valid if a bio has data.
+		 * The check might lead to occasional false negatives when bios
+		 * are cloned, but compared to the performance impact of cloned
+		 * bios themselves the loop below doesn't matter anyway.
+		 */
+		if (!q->limits.chunk_sectors &&
+		    (*bio)->bi_vcnt == 1 &&
+		    ((*bio)->bi_io_vec[0].bv_len +
+		     (*bio)->bi_io_vec[0].bv_offset) <= PAGE_SIZE) {
+			*nr_segs = 1;
+			break;
+		}
+		split = blk_bio_segment_split(q, *bio, &q->bio_split, nr_segs);
+		break;
+	}
+
+	if (split)
+		blk_bio_handle_split(bio, split);
+}
+
+#endif
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 7dadfac5bad0..54c510c0f3ee 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -42,6 +42,7 @@ 
 #include "blk-stat.h"
 #include "blk-mq-sched.h"
 #include "blk-rq-qos.h"
+#include "blk-merge.h"
 
 static DEFINE_PER_CPU(struct llist_head, blk_cpu_done);
 
@@ -2514,7 +2515,7 @@  void blk_mq_submit_bio(struct bio *bio)
 	blk_status_t ret;
 
 	blk_queue_bounce(q, &bio);
-	__blk_queue_split(&bio, &nr_segs);
+	__blk_queue_split(q, &bio, &nr_segs);
 
 	if (!bio_integrity_prep(bio))
 		goto queue_exit;
diff --git a/block/blk.h b/block/blk.h
index 0afee3e6a7c1..c0b59bc38a2e 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -262,7 +262,6 @@  ssize_t part_timeout_show(struct device *, struct device_attribute *, char *);
 ssize_t part_timeout_store(struct device *, struct device_attribute *,
 				const char *, size_t);
 
-void __blk_queue_split(struct bio **bio, unsigned int *nr_segs);
 int ll_back_merge_fn(struct request *req, struct bio *bio,
 		unsigned int nr_segs);
 bool blk_attempt_req_merge(struct request_queue *q, struct request *rq,