simplify block layer based on immutable biovecs

From: Kent Overstreet <[email protected]>

The way the block layer is currently written, it goes to great lengths to avoid having to split bios; upper layer code (such as bio_add_page()) checks what the underlying device can handle and tries to always create bios that don't need to be split.

But this approach becomes unwieldy and eventually breaks down with stacked devices and devices with dynamic limits, and it adds a lot of complexity. If the block layer could split bios as needed, we could eliminate a lot of complexity elsewhere - particularly in stacked drivers. Code that creates bios can then create whatever size bios are convenient, and more importantly stacked drivers don't have to deal with both their own bio size limitations and the limitations of the (potentially multiple) devices underneath them. In the future this will let us delete merge_bvec_fn and a bunch of other code.

We do this by adding calls to blk_queue_split() to the various make_request functions that need it - a few can already handle arbitrary size bios. Note that we add the call _after_ any call to blk_queue_bounce(); this means that blk_queue_split() and blk_recalc_rq_segments() don't need to be concerned with bouncing affecting segment merging.

Some make_request_fn() callbacks were simple enough to audit and verify they don't need blk_queue_split() calls. The skipped ones are:

* nfhd_make_request (arch/m68k/emu/nfblock.c)
* axon_ram_make_request (arch/powerpc/sysdev/axonram.c)
* simdisk_make_request (arch/xtensa/platforms/iss/simdisk.c)
* brd_make_request (ramdisk - drivers/block/brd.c)
* mtip_submit_request (drivers/block/mtip32xx/mtip32xx.c)
* loop_make_request
* null_queue_bio
* bcache's make_request fns

Some others are almost certainly safe to remove now, but will be left for future patches.

Cc Jens Axboe <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Al Viro <[email protected]>

Cc Ming Lei <[email protected]>

Cc Neil Brown <[email protected]>

Cc Alasdair Kergon <[email protected]>

Cc Mike Snitzer <[email protected]>

Cc [email protected]

Cc Lars Ellenberg <[email protected]>

Cc [email protected]

Cc Jiri Kosina <[email protected]>

Cc Geoff Levand <[email protected]>

Cc Jim Paris <[email protected]>

Cc Joshua Morris <[email protected]>

Cc Philip Kelleher <[email protected]>

Cc Minchan Kim <[email protected]>

Cc Nitin Gupta <[email protected]>

Cc Oleg Drokin <[email protected]>

Cc Andreas Dilger <[email protected]>

Signed-off-by Kent Overstreet <[email protected]>

[dpark: skip more mq-based drivers, resolve merge conflicts, etc.]

Signed-off-by Dongsu Park <[email protected]>

Signed-off-by Ming Lin <[email protected]>

---
 block/blk-core.c                            |  19 ++--
 block/blk-merge.c                           | 159 ++++++++++++++++++++++++++--
 block/blk-mq.c                              |   4 +
 drivers/block/drbd/drbd_req.c               |   2 +
 drivers/block/pktcdvd.c                     |   6 +-
 drivers/block/ps3vram.c                     |   2 +
 drivers/block/rsxx/dev.c                    |   2 +
 drivers/block/umem.c                        |   2 +
 drivers/block/zram/zram_drv.c               |   2 +
 drivers/md/dm.c                             |   2 +
 drivers/md/md.c                             |   2 +
 drivers/s390/block/dcssblk.c                |   2 +
 drivers/s390/block/xpram.c                  |   2 +
 drivers/staging/lustre/lustre/llite/lloop.c |   2 +
 include/linux/blkdev.h                      |   3 +
 15 files changed, 189 insertions(+), 22 deletions(-)

block/blk-core.c +9 -10

index 7871603..fbbb337 100644
@@ -619,6 +619,10 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
 	if (q->id < 0)
 		goto fail_q;
 
+	q->bio_split = bioset_create(BIO_POOL_SIZE, 0);
+	if (!q->bio_split)
+		goto fail_id;
+
 	q->backing_dev_info.ra_pages =
 			(VM_MAX_READAHEAD * 1024) / PAGE_CACHE_SIZE;
 	q->backing_dev_info.state = 0;

@@ -628,7 +632,7 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
 
 	err = bdi_init(&q->backing_dev_info);
 	if (err)
-		goto fail_id;
+		goto fail_split;
 
 	setup_timer(&q->backing_dev_info.laptop_mode_wb_timer,
 		    laptop_mode_timer_fn, (unsigned long) q);

@@ -670,6 +674,8 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
 
 fail_bdi:
 	bdi_destroy(&q->backing_dev_info);
+fail_split:
+	bioset_free(q->bio_split);
 fail_id:
 	ida_simple_remove(&blk_queue_ida, q->id);
 fail_q:

@@ -1586,6 +1592,8 @@ void blk_queue_bio(struct request_queue *q, struct bio *bio)
 	struct request *req;
 	unsigned int request_count = 0;
 
+	blk_queue_split(q, &bio, q->bio_split);
+
 	/*
 	 * low level driver can indicate that it wants pages above a
 	 * certain limit bounced to low memory (ie for highmem, or even

@@ -1809,15 +1817,6 @@ generic_make_request_checks(struct bio *bio)
 		goto end_io;
 	}
 
-	if (likely(bio_is_rw(bio) &&
-		   nr_sectors > queue_max_hw_sectors(q))) {
-		printk(KERN_ERR "bio too big device %s (%u > %u)\n",
-		       bdevname(bio->bi_bdev, b),
-		       bio_sectors(bio),
-		       queue_max_hw_sectors(q));
-		goto end_io;
-	}
-
 	part = bio->bi_bdev->bd_part;
 	if (should_fail_request(part, bio->bi_iter.bi_size) ||
 	    should_fail_request(&part_to_disk(part)->part0,

block/blk-merge.c +149 -10

index fd3fee8..dc14255 100644
@@ -9,12 +9,158 @@
 
 #include "blk.h"
 
+static struct bio *blk_bio_discard_split(struct request_queue *q,
+					 struct bio *bio,
+					 struct bio_set *bs)
+{
+	unsigned int max_discard_sectors, granularity;
+	int alignment;
+	sector_t tmp;
+	unsigned split_sectors;
+
+	/* Zero-sector (unknown) and one-sector granularities are the same.  */
+	granularity = max(q->limits.discard_granularity >> 9, 1U);
+
+	max_discard_sectors = min(q->limits.max_discard_sectors, UINT_MAX >> 9);
+	max_discard_sectors -= max_discard_sectors % granularity;
+
+	if (unlikely(!max_discard_sectors)) {
+		/* XXX: warn */
+		return NULL;
+	}
+
+	if (bio_sectors(bio) <= max_discard_sectors)
+		return NULL;
+
+	split_sectors = max_discard_sectors;
+
+	/*
+	 * If the next starting sector would be misaligned, stop the discard at
+	 * the previous aligned sector.
+	 */
+	alignment = (q->limits.discard_alignment >> 9) % granularity;
+
+	tmp = bio->bi_iter.bi_sector + split_sectors - alignment;
+	tmp = sector_div(tmp, granularity);
+
+	if (split_sectors > tmp)
+		split_sectors -= tmp;
+
+	return bio_split(bio, split_sectors, GFP_NOIO, bs);
+}
+
+static struct bio *blk_bio_write_same_split(struct request_queue *q,
+					    struct bio *bio,
+					    struct bio_set *bs)
+{
+	if (!q->limits.max_write_same_sectors)
+		return NULL;
+
+	if (bio_sectors(bio) <= q->limits.max_write_same_sectors)
+		return NULL;
+
+	return bio_split(bio, q->limits.max_write_same_sectors, GFP_NOIO, bs);
+}
+
+static struct bio *blk_bio_segment_split(struct request_queue *q,
+					 struct bio *bio,
+					 struct bio_set *bs)
+{
+	struct bio *split;
+	struct bio_vec bv, bvprv;
+	struct bvec_iter iter;
+	unsigned seg_size = 0, nsegs = 0;
+	int prev = 0;
+
+	struct bvec_merge_data bvm = {
+		.bi_bdev	= bio->bi_bdev,
+		.bi_sector	= bio->bi_iter.bi_sector,
+		.bi_size	= 0,
+		.bi_rw		= bio->bi_rw,
+	};
+
+	bio_for_each_segment(bv, bio, iter) {
+		if (q->merge_bvec_fn &&
+		    q->merge_bvec_fn(q, &bvm, &bv) < (int) bv.bv_len)
+			goto split;
+
+		bvm.bi_size += bv.bv_len;
+
+		if (bvm.bi_size >> 9 > queue_max_sectors(q))
+			goto split;
+
+		/*
+		 * If the queue doesn't support SG gaps and adding this
+		 * offset would create a gap, disallow it.
+		 */
+		if (q->queue_flags & (1 << QUEUE_FLAG_SG_GAPS) &&
+		    prev && bvec_gap_to_prev(&bvprv, bv.bv_offset))
+			goto split;
+
+		if (prev && blk_queue_cluster(q)) {
+			if (seg_size + bv.bv_len > queue_max_segment_size(q))
+				goto new_segment;
+			if (!BIOVEC_PHYS_MERGEABLE(&bvprv, &bv))
+				goto new_segment;
+			if (!BIOVEC_SEG_BOUNDARY(q, &bvprv, &bv))
+				goto new_segment;
+
+			seg_size += bv.bv_len;
+			bvprv = bv;
+			prev = 1;
+			continue;
+		}
+new_segment:
+		if (nsegs == queue_max_segments(q))
+			goto split;
+
+		nsegs++;
+		bvprv = bv;
+		prev = 1;
+		seg_size = bv.bv_len;
+	}
+
+	return NULL;
+split:
+	split = bio_clone_bioset(bio, GFP_NOIO, bs);
+
+	split->bi_iter.bi_size -= iter.bi_size;
+	bio->bi_iter = iter;
+
+	if (bio_integrity(bio)) {
+		bio_integrity_advance(bio, split->bi_iter.bi_size);
+		bio_integrity_trim(split, 0, bio_sectors(split));
+	}
+
+	return split;
+}
+
+void blk_queue_split(struct request_queue *q, struct bio **bio,
+		     struct bio_set *bs)
+{
+	struct bio *split;
+
+	if ((*bio)->bi_rw & REQ_DISCARD)
+		split = blk_bio_discard_split(q, *bio, bs);
+	else if ((*bio)->bi_rw & REQ_WRITE_SAME)
+		split = blk_bio_write_same_split(q, *bio, bs);
+	else
+		split = blk_bio_segment_split(q, *bio, q->bio_split);
+
+	if (split) {
+		bio_chain(split, *bio);
+		generic_make_request(*bio);
+		*bio = split;
+	}
+}
+EXPORT_SYMBOL(blk_queue_split);
+
 static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
 					     struct bio *bio,
 					     bool no_sg_merge)
 {
 	struct bio_vec bv, bvprv = { NULL };
-	int cluster, high, highprv = 1;
+	int cluster, prev = 0;
 	unsigned int seg_size, nr_phys_segs;
 	struct bio *fbio, *bbio;
 	struct bvec_iter iter;

@@ -36,7 +182,6 @@ static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
 	cluster = blk_queue_cluster(q);
 	seg_size = 0;
 	nr_phys_segs = 0;
-	high = 0;
 	for_each_bio(bio) {
 		bio_for_each_segment(bv, bio, iter) {
 			/*

@@ -46,13 +191,7 @@ static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
 			if (no_sg_merge)
 				goto new_segment;
 
-			/*
-			 * the trick here is making sure that a high page is
-			 * never considered part of another segment, since
-			 * that might change with the bounce page.
-			 */
-			high = page_to_pfn(bv.bv_page) > queue_bounce_pfn(q);
-			if (!high && !highprv && cluster) {
+			if (prev && cluster) {
 				if (seg_size + bv.bv_len
 				    > queue_max_segment_size(q))
 					goto new_segment;

@@ -72,8 +211,8 @@ new_segment:
 
 			nr_phys_segs++;
 			bvprv = bv;
+			prev = 1;
 			seg_size = bv.bv_len;
-			highprv = high;
 		}
 		bbio = bio;
 	}

block/blk-mq.c +4 -0

index e68b71b..e7fae76 100644
@@ -1256,6 +1256,8 @@ static void blk_mq_make_request(struct request_queue *q, struct bio *bio)
 		return;
 	}
 
+	blk_queue_split(q, &bio, q->bio_split);
+
 	rq = blk_mq_map_request(q, bio, &data);
 	if (unlikely(!rq))
 		return;

@@ -1339,6 +1341,8 @@ static void blk_sq_make_request(struct request_queue *q, struct bio *bio)
 		return;
 	}
 
+	blk_queue_split(q, &bio, q->bio_split);
+
 	if (use_plug && !blk_queue_nomerges(q) &&
 	    blk_attempt_plug_merge(q, bio, &request_count))
 		return;

drivers/block/drbd/drbd_req.c +2 -0

index 3907202..a6265bc 100644
@@ -1497,6 +1497,8 @@ void drbd_make_request(struct request_queue *q, struct bio *bio)
 	struct drbd_device *device = (struct drbd_device *) q->queuedata;
 	unsigned long start_jif;
 
+	blk_queue_split(q, &bio, q->bio_split);
+
 	start_jif = jiffies;
 
 	/*

drivers/block/pktcdvd.c +4 -2

index 09e628da..ea10bd9 100644
@@ -2446,6 +2446,10 @@ static void pkt_make_request(struct request_queue *q, struct bio *bio)
 	char b[BDEVNAME_SIZE];
 	struct bio *split;
 
+	blk_queue_bounce(q, &bio);
+
+	blk_queue_split(q, &bio, q->bio_split);
+
 	pd = q->queuedata;
 	if (!pd) {
 		pr_err("%s incorrect request queue\n",

@@ -2476,8 +2480,6 @@ static void pkt_make_request(struct request_queue *q, struct bio *bio)
 		goto end_io;
 	}
 
-	blk_queue_bounce(q, &bio);
-
 	do {
 		sector_t zone = get_zone(bio->bi_iter.bi_sector, pd);
 		sector_t last_zone = get_zone(bio_end_sector(bio) - 1, pd);

drivers/block/ps3vram.c +2 -0

index ef45cfb..e32e799 100644
@@ -605,6 +605,8 @@ static void ps3vram_make_request(struct request_queue *q, struct bio *bio)
 
 	dev_dbg(&dev->core, "%s\n", __func__);
 
+	blk_queue_split(q, &bio, q->bio_split);
+
 	spin_lock_irq(&priv->lock);
 	busy = !bio_list_empty(&priv->list);
 	bio_list_add(&priv->list, bio);

drivers/block/rsxx/dev.c +2 -0

index ac8c62c..50ef199 100644
@@ -148,6 +148,8 @@ static void rsxx_make_request(struct request_queue *q, struct bio *bio)
 	struct rsxx_bio_meta *bio_meta;
 	int st = -EINVAL;
 
+	blk_queue_split(q, &bio, q->bio_split);
+
 	might_sleep();
 
 	if (!card)

drivers/block/umem.c +2 -0

index 4cf81b5..13d577c 100644
@@ -531,6 +531,8 @@ static void mm_make_request(struct request_queue *q, struct bio *bio)
 		 (unsigned long long)bio->bi_iter.bi_sector,
 		 bio->bi_iter.bi_size);
 
+	blk_queue_split(q, &bio, q->bio_split);
+
 	spin_lock_irq(&card->lock);
 	*card->biotail = bio;
 	bio->bi_next = NULL;

drivers/block/zram/zram_drv.c +2 -0

index 8dcbced..36a004e 100644
@@ -981,6 +981,8 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio)
 	if (unlikely(!zram_meta_get(zram)))
 		goto error;
 
+	blk_queue_split(queue, &bio, queue->bio_split);
+
 	if (!valid_io_request(zram, bio->bi_iter.bi_sector,
 					bio->bi_iter.bi_size)) {
 		atomic64_inc(&zram->stats.invalid_io);

drivers/md/dm.c +2 -0

index a930b72..34f6063 100644
@@ -1784,6 +1784,8 @@ static void dm_make_request(struct request_queue *q, struct bio *bio)
 
 	map = dm_get_live_table(md, &srcu_idx);
 
+	blk_queue_split(q, &bio, q->bio_split);
+
 	generic_start_io_acct(rw, bio_sectors(bio), &dm_disk(md)->part0);
 
 	/* if we're suspended, we have to queue this io for later */

drivers/md/md.c +2 -0

index 593a024..046b3c9 100644
@@ -257,6 +257,8 @@ static void md_make_request(struct request_queue *q, struct bio *bio)
 	unsigned int sectors;
 	int cpu;
 
+	blk_queue_split(q, &bio, q->bio_split);
+
 	if (mddev == NULL || mddev->pers == NULL
 	    || !mddev->ready) {
 		bio_io_error(bio);

drivers/s390/block/dcssblk.c +2 -0

index da21281..267ca3a 100644
@@ -826,6 +826,8 @@ dcssblk_make_request(struct request_queue *q, struct bio *bio)
 	unsigned long source_addr;
 	unsigned long bytes_done;
 
+	blk_queue_split(q, &bio, q->bio_split);
+
 	bytes_done = 0;
 	dev_info = bio->bi_bdev->bd_disk->private_data;
 	if (dev_info == NULL)

drivers/s390/block/xpram.c +2 -0

index 7d4e939..1305ed3 100644
@@ -190,6 +190,8 @@ static void xpram_make_request(struct request_queue *q, struct bio *bio)
 	unsigned long page_addr;
 	unsigned long bytes;
 
+	blk_queue_split(q, &bio, q->bio_split);
+
 	if ((bio->bi_iter.bi_sector & 7) != 0 ||
 	    (bio->bi_iter.bi_size & 4095) != 0)
 		/* Request is not page-aligned. */

drivers/staging/lustre/lustre/llite/lloop.c +2 -0

index 413a840..a8645a9 100644
@@ -340,6 +340,8 @@ static void loop_make_request(struct request_queue *q, struct bio *old_bio)
 	int rw = bio_rw(old_bio);
 	int inactive;
 
+	blk_queue_split(q, &old_bio, q->bio_split);
+
 	if (!lo)
 		goto err;
 

include/linux/blkdev.h +3 -0

index 7f9a516..93b81a2 100644
@@ -488,6 +488,7 @@ struct request_queue {
 
 	struct blk_mq_tag_set	*tag_set;
 	struct list_head	tag_set_list;
+	struct bio_set		*bio_split;
 };
 
 #define QUEUE_FLAG_QUEUED	1	/* uses generic tag queueing */

@@ -812,6 +813,8 @@ extern void blk_rq_unprep_clone(struct request *rq);
 extern int blk_insert_cloned_request(struct request_queue *q,
 				     struct request *rq);
 extern void blk_delay_queue(struct request_queue *, unsigned long);
+extern void blk_queue_split(struct request_queue *, struct bio **,
+			    struct bio_set *);
 extern void blk_recount_segments(struct request_queue *, struct bio *);
 extern int scsi_verify_blk_ioctl(struct block_device *, unsigned int);
 extern int scsi_cmd_blk_ioctl(struct block_device *, fmode_t,

From: Kent Overstreet <[email protected]>

The way the block layer is currently written, it goes to great lengths to avoid having to split bios; upper layer code (such as bio_add_page()) checks what the underlying device can handle and tries to always create bios that don't need to be split.

But this approach becomes unwieldy and eventually breaks down with stacked devices and devices with dynamic limits, and it adds a lot of complexity. If the block layer could split bios as needed, we could eliminate a lot of complexity elsewhere - particularly in stacked drivers. Code that creates bios can then create whatever size bios are convenient, and more importantly stacked drivers don't have to deal with both their own bio size limitations and the limitations of the (potentially multiple) devices underneath them. In the future this will let us delete merge_bvec_fn and a bunch of other code.

Quoted 7 lines · Ming Lin

From: Kent Overstreet <[email protected]>

The way the block layer is currently written, it goes to great lengths to avoid having to split bios; upper layer code (such as bio_add_page()) checks what the underlying device can handle and tries to always create bios that don't need to be split.

Quoted 9 lines

But this approach becomes unwieldy and eventually breaks down with stacked devices and devices with dynamic limits, and it adds a lot of complexity. If the block layer could split bios as needed, we could eliminate a lot of complexity elsewhere - particularly in stacked drivers. Code that creates bios can then create whatever size bios are convenient, and more importantly stacked drivers don't have to deal with both their own bio size limitations and the limitations of the (potentially multiple) devices underneath them. In the future this will let us delete merge_bvec_fn and a bunch of other code.

This series doesn't take any steps to train upper layers (e.g. filesystems) to size their bios larger (which is defined as "whatever size bios are convenient" above).

bio_add_page(), and merge_bvec_fn, served as the means for upper layers (and direct IO) to build up optimally sized bios. Without a replacement (that I can see anyway) how is this patchset making forward progress (getting Acks, etc)!?

I like the idea of reduced complexity associated with these late bio splitting changes I'm just not seeing how this is ready given there are no upper layer changes that speak to building larger bios..

What am I missing?

Please advise, thanks!
Mike

Now bio_add_page() can build lager bios.
And blk_queue_split() can split the bios in ->make_request() if needed.

Quoted 2 lines · Ming Lin

Now bio_add_page() can build lager bios.
And blk_queue_split() can split the bios in ->make_request() if needed.

But why not try to make the bio the right size in the first place so you don't have to incur the performance impact of splitting?

What performance testing have you yet done to demonstrate the *actual* impact of this patchset in situations where merge_bvec_fn is currently a net benefit?

Alasdair

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Because we don't know what the "right" size is. And the "right" size can change when array reconfiguration happens.

Splitting has to happen somewhere, if only in bio_addpage where it decides to create a new bio rather than add another page to the current one. So moving the split to a different level of the stack shouldn't necessarily change the performance profile.

It does sometimes make a significant difference to device-mapper stacks. DM only uses it for performance reasons - it can already split bios when it needs to. I tried to remove merge_bvec_fn from DM several years ago but couldn't because of the adverse performance impact of lots of splitting activity.

Quoted 33 lines · Mike Snitzer

Quoted 7 lines · Ming Lin

From: Kent Overstreet <[email protected]>

The way the block layer is currently written, it goes to great lengths to avoid having to split bios; upper layer code (such as bio_add_page()) checks what the underlying device can handle and tries to always create bios that don't need to be split.

Quoted 9 lines

But this approach becomes unwieldy and eventually breaks down with stacked devices and devices with dynamic limits, and it adds a lot of complexity. If the block layer could split bios as needed, we could eliminate a lot of complexity elsewhere - particularly in stacked drivers. Code that creates bios can then create whatever size bios are convenient, and more importantly stacked drivers don't have to deal with both their own bio size limitations and the limitations of the (potentially multiple) devices underneath them. In the future this will let us delete merge_bvec_fn and a bunch of other code.

This series doesn't take any steps to train upper layers (e.g. filesystems) to size their bios larger (which is defined as "whatever size bios are convenient" above).

bio_add_page(), and merge_bvec_fn, served as the means for upper layers (and direct IO) to build up optimally sized bios. Without a replacement (that I can see anyway) how is this patchset making forward progress (getting Acks, etc)!?

I like the idea of reduced complexity associated with these late bio splitting changes I'm just not seeing how this is ready given there are no upper layer changes that speak to building larger bios..

Quoted 1 line

What am I missing?

See: [PATCH v4 02/11] block: simplify bio_add_page()
https://lkml.org/lkml/2015/5/22/754

Now bio_add_page() can build lager bios.
And blk_queue_split() can split the bios in ->make_request() if needed.

Quoted 34 lines · Ming Lin

Quoted 33 lines · Mike Snitzer

Quoted 7 lines · Ming Lin

From: Kent Overstreet <[email protected]>

The way the block layer is currently written, it goes to great lengths to avoid having to split bios; upper layer code (such as bio_add_page()) checks what the underlying device can handle and tries to always create bios that don't need to be split.

Quoted 9 lines

But this approach becomes unwieldy and eventually breaks down with stacked devices and devices with dynamic limits, and it adds a lot of complexity. If the block layer could split bios as needed, we could eliminate a lot of complexity elsewhere - particularly in stacked drivers. Code that creates bios can then create whatever size bios are convenient, and more importantly stacked drivers don't have to deal with both their own bio size limitations and the limitations of the (potentially multiple) devices underneath them. In the future this will let us delete merge_bvec_fn and a bunch of other code.

This series doesn't take any steps to train upper layers (e.g. filesystems) to size their bios larger (which is defined as "whatever size bios are convenient" above).

bio_add_page(), and merge_bvec_fn, served as the means for upper layers (and direct IO) to build up optimally sized bios. Without a replacement (that I can see anyway) how is this patchset making forward progress (getting Acks, etc)!?

I like the idea of reduced complexity associated with these late bio splitting changes I'm just not seeing how this is ready given there are no upper layer changes that speak to building larger bios..

Quoted 7 lines

Quoted 1 line

What am I missing?

See: [PATCH v4 02/11] block: simplify bio_add_page()
https://lkml.org/lkml/2015/5/22/754

Now bio_add_page() can build lager bios.
And blk_queue_split() can split the bios in ->make_request() if needed.

That'll result in quite large bios and always needing splitting.

As Alasdair asked: please provide some performance data that justifies these changes. E.g use a setup like: XFS on a DM striped target. We can iterate on more complex setups once we have established some basic tests.

If you're just punting to reviewers to do the testing for you that isn't going to instill _any_ confidence in me for this patchset as a suitabe replacement relative to performance.

Quoted 34 lines · Ming Lin

Quoted 33 lines · Mike Snitzer

Quoted 7 lines · Ming Lin

From: Kent Overstreet <[email protected]>

The way the block layer is currently written, it goes to great lengths to avoid having to split bios; upper layer code (such as bio_add_page()) checks what the underlying device can handle and tries to always create bios that don't need to be split.

Quoted 9 lines

But this approach becomes unwieldy and eventually breaks down with stacked devices and devices with dynamic limits, and it adds a lot of complexity. If the block layer could split bios as needed, we could eliminate a lot of complexity elsewhere - particularly in stacked drivers. Code that creates bios can then create whatever size bios are convenient, and more importantly stacked drivers don't have to deal with both their own bio size limitations and the limitations of the (potentially multiple) devices underneath them. In the future this will let us delete merge_bvec_fn and a bunch of other code.

This series doesn't take any steps to train upper layers (e.g. filesystems) to size their bios larger (which is defined as "whatever size bios are convenient" above).

bio_add_page(), and merge_bvec_fn, served as the means for upper layers (and direct IO) to build up optimally sized bios. Without a replacement (that I can see anyway) how is this patchset making forward progress (getting Acks, etc)!?

I like the idea of reduced complexity associated with these late bio splitting changes I'm just not seeing how this is ready given there are no upper layer changes that speak to building larger bios..

Quoted 7 lines

Quoted 1 line

What am I missing?

See: [PATCH v4 02/11] block: simplify bio_add_page()
https://lkml.org/lkml/2015/5/22/754

Now bio_add_page() can build lager bios.
And blk_queue_split() can split the bios in ->make_request() if needed.

That'll result in quite large bios and always needing splitting.

As Alasdair asked: please provide some performance data that justifies these changes. E.g use a setup like: XFS on a DM striped target. We can iterate on more complex setups once we have established some basic tests.

Here are fio results of XFS on a DM stripped target with 2 SSDs + 1 HDD. Does it make sense?

Quoted 2 lines · Ming Lin

Here are fio results of XFS on a DM stripped target with 2 SSDs + 1 HDD. Does it make sense?

To stripe across devices with different characteristics?

Some suggestions.

Prepare 3 kernels.
O - Old kernel.
M - Old kernel with merge_bvec_fn disabled.
N - New kernel.

You're trying to search for counter-examples to the hypothesis that "Kernel N always outperforms Kernel O". Then if you find any, trying to show either that the performance impediment is small enough that it doesn't matter or that the cases are sufficiently rare or obscure that they may be ignored because of the greater benefits of N in much more common cases.

(1) You're looking to set up configurations where kernel O performs noticeably better than M. Then you're comparing the performance of O and N in those situations.

(2) You're looking at other sensible configurations where O and M have similar performance, and comparing that with the performance of N.

In each case you find, you expect to be able to vary some parameter (such as stripe size) to show a progression of the effect.

When running tests you've to take care the system is reset into the same initial state before each test, so you'll normally also try to include some baseline test between tests that should give the same results each time and also take the average of a number of runs (while also reporting some measure of the variation within each set to make sure that remains low, typically a low single digit percentage).

Since we're mostly concerned about splitting, you'll want to monitor iostat to see if that gives you enough information to home in on suitable configurations for (1). Failing that, you might need to instrument the kernels to tell you the sizes of the bios being created and the amount of splitting actually happening.

Striping was mentioned because it forces splitting. So show the progression from tiny stripes to huge stripes. (Ensure all the devices providing the stripes have identical characteristics, but you can test with slow and fast underlying devices.)

You may also want to test systems with a restricted amount of available memory to show how the splitting via worker thread performs. (Again, instrument to prove the extent to which the new code is being exercised.)

Alasdair

Quoted 2 lines · Ming Lin

Here are fio results of XFS on a DM stripped target with 2 SSDs + 1 HDD. Does it make sense?

To stripe across devices with different characteristics?

Some suggestions.

Prepare 3 kernels.
O - Old kernel.
M - Old kernel with merge_bvec_fn disabled.

N - New kernel.

Quoted 11 lines · Alasdair G Kergon

Quoted 2 lines · Ming Lin

Here are fio results of XFS on a DM stripped target with 2 SSDs + 1 HDD. Does it make sense?

To stripe across devices with different characteristics?

Some suggestions.

Prepare 3 kernels.
O - Old kernel.
M - Old kernel with merge_bvec_fn disabled.

How to disable it?
Maybe just hack it as below?

void blk_queue_merge_bvec(struct request_queue *q, merge_bvec_fn *mbfn)

{
        //q->merge_bvec_fn = mbfn;
}

Quoted 2 lines · Ming Lin

Here are fio results of XFS on a DM stripped target with 2 SSDs + 1 HDD. Does it make sense?

To stripe across devices with different characteristics?

Some suggestions.

Prepare 3 kernels.
O - Old kernel.
M - Old kernel with merge_bvec_fn disabled.
N - New kernel.

You're trying to search for counter-examples to the hypothesis that "Kernel N always outperforms Kernel O". Then if you find any, trying to show either that the performance impediment is small enough that it doesn't matter or that the cases are sufficiently rare or obscure that they may be ignored because of the greater benefits of N in much more common cases.

(1) You're looking to set up configurations where kernel O performs noticeably better than M. Then you're comparing the performance of O and N in those situations.

(2) You're looking at other sensible configurations where O and M have similar performance, and comparing that with the performance of N.

Quoted 20 lines · Alasdair G Kergon

Quoted 2 lines · Ming Lin

Here are fio results of XFS on a DM stripped target with 2 SSDs + 1 HDD. Does it make sense?

To stripe across devices with different characteristics?

Some suggestions.

Prepare 3 kernels.
O - Old kernel.
M - Old kernel with merge_bvec_fn disabled.
N - New kernel.

You're trying to search for counter-examples to the hypothesis that "Kernel N always outperforms Kernel O". Then if you find any, trying to show either that the performance impediment is small enough that it doesn't matter or that the cases are sufficiently rare or obscure that they may be ignored because of the greater benefits of N in much more common cases.

Quoted 6 lines

(1) You're looking to set up configurations where kernel O performs noticeably better than M. Then you're comparing the performance of O and N in those situations.

(2) You're looking at other sensible configurations where O and M have similar performance, and comparing that with the performance of N.

I didn't find case (1).

But the important thing for this series is to simplify block layer based on immutable biovecs. I don't expect performance improvement.

Here is the changes statistics.

"68 files changed, 336 insertions(+), 1331 deletions(-)"

I run below 3 test cases to make sure it didn't bring any regressions. Test environment: 2 NVMe drives on 2 sockets server.
Each case run for 30 minutes.

2) btrfs radi0

mkfs.btrfs -f -d raid0 /dev/nvme0n1 /dev/nvme1n1
mount /dev/nvme0n1 /mnt

Then run 8K read.

[global]
ioengine=libaio
iodepth=64
direct=1
runtime=1800
time_based
group_reporting
numjobs=4
rw=read

[job1]
bs=8K
directory=/mnt
size=1G

2) ext4 on MD raid5

mdadm --create /dev/md0 --level=5 --raid-devices=2 /dev/nvme0n1 /dev/nvme1n1 mkfs.ext4 /dev/md0
mount /dev/md0 /mnt

fio script same as btrfs test

3) xfs on DM stripped target

pvcreate /dev/nvme0n1 /dev/nvme1n1
vgcreate striped_vol_group /dev/nvme0n1 /dev/nvme1n1 lvcreate -i2 -I4 -L250G -nstriped_logical_volume striped_vol_group mkfs.xfs -f /dev/striped_vol_group/striped_logical_volume mount /dev/striped_vol_group/striped_logical_volume /mnt

fio script same as btrfs test

------

Results:

        4.1-rc4         4.1-rc4-patched
btrfs   1818.6MB/s      1874.1MB/s
ext4    717307KB/s      714030KB/s
xfs     1396.6MB/s      1398.6MB/s

Quoted 25 lines · Ming Lin

Quoted 24 lines · Alasdair G Kergon

Quoted 2 lines · Ming Lin

Here are fio results of XFS on a DM stripped target with 2 SSDs + 1 HDD. Does it make sense?

To stripe across devices with different characteristics?

Some suggestions.

Prepare 3 kernels.
O - Old kernel.
M - Old kernel with merge_bvec_fn disabled.
N - New kernel.

You're trying to search for counter-examples to the hypothesis that "Kernel N always outperforms Kernel O". Then if you find any, trying to show either that the performance impediment is small enough that it doesn't matter or that the cases are sufficiently rare or obscure that they may be ignored because of the greater benefits of N in much more common cases.

(1) You're looking to set up configurations where kernel O performs noticeably better than M. Then you're comparing the performance of O and N in those situations.

Quoted 7 lines

Quoted 2 lines

(2) You're looking at other sensible configurations where O and M have similar performance, and comparing that with the performance of N.

I didn't find case (1).

But the important thing for this series is to simplify block layer based on immutable biovecs. I don't expect performance improvement.

Quoted 48 lines

Here is the changes statistics.

"68 files changed, 336 insertions(+), 1331 deletions(-)"

I run below 3 test cases to make sure it didn't bring any regressions. Test environment: 2 NVMe drives on 2 sockets server.
Each case run for 30 minutes.

2) btrfs radi0

mkfs.btrfs -f -d raid0 /dev/nvme0n1 /dev/nvme1n1
mount /dev/nvme0n1 /mnt

Then run 8K read.

[global]
ioengine=libaio
iodepth=64
direct=1
runtime=1800
time_based
group_reporting
numjobs=4
rw=read

[job1]
bs=8K
directory=/mnt
size=1G

2) ext4 on MD raid5

mdadm --create /dev/md0 --level=5 --raid-devices=2 /dev/nvme0n1 /dev/nvme1n1 mkfs.ext4 /dev/md0
mount /dev/md0 /mnt

fio script same as btrfs test

3) xfs on DM stripped target

pvcreate /dev/nvme0n1 /dev/nvme1n1
vgcreate striped_vol_group /dev/nvme0n1 /dev/nvme1n1 lvcreate -i2 -I4 -L250G -nstriped_logical_volume striped_vol_group mkfs.xfs -f /dev/striped_vol_group/striped_logical_volume mount /dev/striped_vol_group/striped_logical_volume /mnt

fio script same as btrfs test

Quoted 8 lines

------

Results:

        4.1-rc4         4.1-rc4-patched
btrfs   1818.6MB/s      1874.1MB/s
ext4    717307KB/s      714030KB/s
xfs     1396.6MB/s      1398.6MB/s

Hi Alasdair & Mike,

Would you like these numbers?
I'd like to address your concerns to move forward.

Quoted 26 lines · Ming Lin

Quoted 25 lines · Ming Lin

Quoted 24 lines · Alasdair G Kergon

Quoted 2 lines · Ming Lin

Here are fio results of XFS on a DM stripped target with 2 SSDs + 1 HDD. Does it make sense?

To stripe across devices with different characteristics?

Some suggestions.

Prepare 3 kernels.
O - Old kernel.
M - Old kernel with merge_bvec_fn disabled.
N - New kernel.

You're trying to search for counter-examples to the hypothesis that "Kernel N always outperforms Kernel O". Then if you find any, trying to show either that the performance impediment is small enough that it doesn't matter or that the cases are sufficiently rare or obscure that they may be ignored because of the greater benefits of N in much more common cases.

(1) You're looking to set up configurations where kernel O performs noticeably better than M. Then you're comparing the performance of O and N in those situations.

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

(2) You're looking at other sensible configurations where O and M have similar performance, and comparing that with the performance of N.

I didn't find case (1).

But the important thing for this series is to simplify block layer based on immutable biovecs. I don't expect performance improvement.

No simplifying isn't the important thing. Any change to remove the merge_bvec callbacks needs to not introduce performance regressions on enterprise systems with large RAID arrays, etc.

It is fine if there isn't a performance improvement but I really don't think the limited testing you've done on a relatively small storage configuration has come even close to showing these changes don't introduce performance regressions.

Quoted 57 lines

Quoted 48 lines

Here is the changes statistics.

"68 files changed, 336 insertions(+), 1331 deletions(-)"

I run below 3 test cases to make sure it didn't bring any regressions. Test environment: 2 NVMe drives on 2 sockets server.
Each case run for 30 minutes.

2) btrfs radi0

mkfs.btrfs -f -d raid0 /dev/nvme0n1 /dev/nvme1n1
mount /dev/nvme0n1 /mnt

Then run 8K read.

[global]
ioengine=libaio
iodepth=64
direct=1
runtime=1800
time_based
group_reporting
numjobs=4
rw=read

[job1]
bs=8K
directory=/mnt
size=1G

2) ext4 on MD raid5

mdadm --create /dev/md0 --level=5 --raid-devices=2 /dev/nvme0n1 /dev/nvme1n1 mkfs.ext4 /dev/md0
mount /dev/md0 /mnt

fio script same as btrfs test

3) xfs on DM stripped target

pvcreate /dev/nvme0n1 /dev/nvme1n1
vgcreate striped_vol_group /dev/nvme0n1 /dev/nvme1n1 lvcreate -i2 -I4 -L250G -nstriped_logical_volume striped_vol_group mkfs.xfs -f /dev/striped_vol_group/striped_logical_volume mount /dev/striped_vol_group/striped_logical_volume /mnt

fio script same as btrfs test

Quoted 8 lines

------

Results:

        4.1-rc4         4.1-rc4-patched
btrfs   1818.6MB/s      1874.1MB/s
ext4    717307KB/s      714030KB/s
xfs     1396.6MB/s      1398.6MB/s

Quoted 4 lines

Hi Alasdair & Mike,

Would you like these numbers?
I'd like to address your concerns to move forward.

I really don't see that these NVMe results prove much.

We need to test on large HW raid setups like a Netapp filer (or even local SAS drives connected via some SAS controller). Like a 8+2 drive RAID6 or 8+1 RAID5 setup. Testing with MD raid on JBOD setups with 8 devices is also useful. It is larger RAID setups that will be more sensitive to IO sizes being properly aligned on RAID stripe and/or chunk size boundaries.

Quoted 7 lines · Mike Snitzer

We need to test on large HW raid setups like a Netapp filer (or even local SAS drives connected via some SAS controller). Like a 8+2 drive RAID6 or 8+1 RAID5 setup. Testing with MD raid on JBOD setups with 8 devices is also useful. It is larger RAID setups that will be more sensitive to IO sizes being properly aligned on RAID stripe and/or chunk size boundaries.

I'll test it on large HW raid setup.

Here is HW RAID5 setup with 19 278G HDDs on Dell R730xd(2sockets/48 logical cpus/264G mem).
http://minggr.net/pub/20150604/hw_raid5.jpg

The stripe size is 64K.

I'm going to test ext4/btrfs/xfs on it.
"bs" set to 1216k(64K * 19 = 1216k)
and run 48 jobs.

[global]
ioengine=libaio
iodepth=64
direct=1
runtime=1800
time_based
group_reporting
numjobs=48
rw=read

[job1]
bs=1216K
directory=/mnt
size=1G

Or do you have other suggestions of what tests I should run?

Quoted 15 lines · Ming Lin

Quoted 7 lines · Mike Snitzer

We need to test on large HW raid setups like a Netapp filer (or even local SAS drives connected via some SAS controller). Like a 8+2 drive RAID6 or 8+1 RAID5 setup. Testing with MD raid on JBOD setups with 8 devices is also useful. It is larger RAID setups that will be more sensitive to IO sizes being properly aligned on RAID stripe and/or chunk size boundaries.

I'll test it on large HW raid setup.

Here is HW RAID5 setup with 19 278G HDDs on Dell R730xd(2sockets/48 logical cpus/264G mem).
http://minggr.net/pub/20150604/hw_raid5.jpg

Quoted 5 lines

The stripe size is 64K.

I'm going to test ext4/btrfs/xfs on it.
"bs" set to 1216k(64K * 19 = 1216k)
and run 48 jobs.

Definitely an odd blocksize (though 1280K full stripe is pretty common for 10+2 HW RAID6 w/ 128K chunk size).

Quoted 11 lines

[global]
ioengine=libaio
iodepth=64
direct=1
runtime=1800
time_based
group_reporting
numjobs=48
rw=read

Quoted 4 lines

[job1]
bs=1216K
directory=/mnt
size=1G

How does time_based relate to size=1G? It'll rewrite the same 1 gig file repeatedly?

Quoted 2 lines

Or do you have other suggestions of what tests I should run?

You're welcome to run this job but I'll also check with others here to see what fio jobs we used in the recent past when assessing performance of the dm-crypt parallelization changes.

Also, a lot of care needs to be taken to eliminate jitter in the system while the test is running. We got a lot of good insight from Bart Van Assche on that and put it to practice. I'll see if we can (re)summarize that too.

Mike

We need to test on large HW raid setups like a Netapp filer (or even local SAS drives connected via some SAS controller). Like a 8+2 drive RAID6 or 8+1 RAID5 setup. Testing with MD raid on JBOD setups with 8 devices is also useful. It is larger RAID setups that will be more sensitive to IO sizes being properly aligned on RAID stripe and/or chunk size boundaries.

Quoted 6 lines · Mike Snitzer

We need to test on large HW raid setups like a Netapp filer (or even local SAS drives connected via some SAS controller). Like a 8+2 drive RAID6 or 8+1 RAID5 setup. Testing with MD raid on JBOD setups with 8 devices is also useful. It is larger RAID setups that will be more sensitive to IO sizes being properly aligned on RAID stripe and/or chunk size boundaries.

Here are tests results of xfs/ext4/btrfs read/write on HW RAID6/MD RAID6/DM stripe target. Each case run 0.5 hour, so it took 36 hours to finish all the tests on 4.1-rc4 and 4.1-rc4-patched kernels.

No performance regressions were introduced.

Test server: Dell R730xd(2 sockets/48 logical cpus/264G memory) HW RAID6/MD RAID6/DM stripe target were configured with 10 HDDs, each 280G Stripe size 64k and 128k were tested.

devs="/dev/sdb /dev/sdc /dev/sdd /dev/sde /dev/sdf /dev/sdg /dev/sdh /dev/sdi /dev/sdj /dev/sdk" spare_devs="/dev/sdl /dev/sdm"
stripe_size=64 (or 128)

MD RAID6 was created by:
mdadm --create --verbose /dev/md0 --level=6 --raid-devices=10 $devs --spare-devices=2 $spare_devs -c $stripe_size

DM stripe target was created by:
pvcreate $devs
vgcreate striped_vol_group $devs
lvcreate -i10 -I${stripe_size} -L2T -nstriped_logical_volume striped_vol_group

Here is an example of fio script for stripe size 128k:
[global]
ioengine=libaio
iodepth=64
direct=1
runtime=1800
time_based
group_reporting
numjobs=48
gtod_reduce=0
norandommap
write_iops_log=fs

[job1]
bs=1280K
directory=/mnt
size=5G
rw=read

All results here: http://minggr.net/pub/20150608/fio_results/

Results summary:

1. HW RAID6: stripe size 64k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       821.23          812.20  -1.09%
xfs write:      753.16          754.42  +0.16%
ext4 read:      827.80          834.82  +0.84%
ext4 write:     783.08          777.58  -0.70%
btrfs read:     859.26          871.68  +1.44%
btrfs write:    815.63          844.40  +3.52%

2. HW RAID6: stripe size 128k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       948.27          979.11  +3.25%
xfs write:      820.78          819.94  -0.10%
ext4 read:      978.35          997.92  +2.00%
ext4 write:     853.51          847.97  -0.64%
btrfs read:     1013.1          1015.6  +0.24%
btrfs write:    854.43          850.42  -0.46%

3. MD RAID6: stripe size 64k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       847.34          869.43  +2.60%
xfs write:      198.67          199.03  +0.18%
ext4 read:      763.89          767.79  +0.51%
ext4 write:     281.44          282.83  +0.49%
btrfs read:     756.02          743.69  -1.63%
btrfs write:    268.37          265.93  -0.90%

4. MD RAID6: stripe size 128k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       993.04          1014.1  +2.12%
xfs write:      293.06          298.95  +2.00%
ext4 read:      1019.6          1020.9  +0.12%
ext4 write:     371.51          371.47  -0.01%
btrfs read:     1000.4          1020.8  +2.03%
btrfs write:    241.08          246.77  +2.36%

5. DM: stripe size 64k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       1084.4          1080.1  -0.39%
xfs write:      1071.1          1063.4  -0.71%
ext4 read:      991.54          1003.7  +1.22%
ext4 write:     1069.7          1052.2  -1.63%
btrfs read:     1076.1          1082.1  +0.55%
btrfs write:    968.98          965.07  -0.40%

6. DM: stripe size 128k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       1020.4          1066.1  +4.47%
xfs write:      1058.2          1066.6  +0.79%
ext4 read:      990.72          988.19  -0.25%
ext4 write:     1050.4          1070.2  +1.88%
btrfs read:     1080.9          1074.7  -0.57%
btrfs write:    975.10          972.76  -0.23%

Quoted 21 lines · Ming Lin

Quoted 6 lines · Mike Snitzer

We need to test on large HW raid setups like a Netapp filer (or even local SAS drives connected via some SAS controller). Like a 8+2 drive RAID6 or 8+1 RAID5 setup. Testing with MD raid on JBOD setups with 8 devices is also useful. It is larger RAID setups that will be more sensitive to IO sizes being properly aligned on RAID stripe and/or chunk size boundaries.

Here are tests results of xfs/ext4/btrfs read/write on HW RAID6/MD RAID6/DM stripe target. Each case run 0.5 hour, so it took 36 hours to finish all the tests on 4.1-rc4 and 4.1-rc4-patched kernels.

No performance regressions were introduced.

Test server: Dell R730xd(2 sockets/48 logical cpus/264G memory) HW RAID6/MD RAID6/DM stripe target were configured with 10 HDDs, each 280G Stripe size 64k and 128k were tested.

devs="/dev/sdb /dev/sdc /dev/sdd /dev/sde /dev/sdf /dev/sdg /dev/sdh /dev/sdi /dev/sdj /dev/sdk" spare_devs="/dev/sdl /dev/sdm"
stripe_size=64 (or 128)

Quoted 7 lines

MD RAID6 was created by:
mdadm --create --verbose /dev/md0 --level=6 --raid-devices=10 $devs --spare-devices=2 $spare_devs -c $stripe_size

DM stripe target was created by:
pvcreate $devs
vgcreate striped_vol_group $devs
lvcreate -i10 -I${stripe_size} -L2T -nstriped_logical_volume striped_vol_group

Quoted 79 lines

Here is an example of fio script for stripe size 128k:
[global]
ioengine=libaio
iodepth=64
direct=1
runtime=1800
time_based
group_reporting
numjobs=48
gtod_reduce=0
norandommap
write_iops_log=fs

[job1]
bs=1280K
directory=/mnt
size=5G
rw=read

All results here: http://minggr.net/pub/20150608/fio_results/

Results summary:

1. HW RAID6: stripe size 64k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       821.23          812.20  -1.09%
xfs write:      753.16          754.42  +0.16%
ext4 read:      827.80          834.82  +0.84%
ext4 write:     783.08          777.58  -0.70%
btrfs read:     859.26          871.68  +1.44%
btrfs write:    815.63          844.40  +3.52%

2. HW RAID6: stripe size 128k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       948.27          979.11  +3.25%
xfs write:      820.78          819.94  -0.10%
ext4 read:      978.35          997.92  +2.00%
ext4 write:     853.51          847.97  -0.64%
btrfs read:     1013.1          1015.6  +0.24%
btrfs write:    854.43          850.42  -0.46%

3. MD RAID6: stripe size 64k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       847.34          869.43  +2.60%
xfs write:      198.67          199.03  +0.18%
ext4 read:      763.89          767.79  +0.51%
ext4 write:     281.44          282.83  +0.49%
btrfs read:     756.02          743.69  -1.63%
btrfs write:    268.37          265.93  -0.90%

4. MD RAID6: stripe size 128k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       993.04          1014.1  +2.12%
xfs write:      293.06          298.95  +2.00%
ext4 read:      1019.6          1020.9  +0.12%
ext4 write:     371.51          371.47  -0.01%
btrfs read:     1000.4          1020.8  +2.03%
btrfs write:    241.08          246.77  +2.36%

5. DM: stripe size 64k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       1084.4          1080.1  -0.39%
xfs write:      1071.1          1063.4  -0.71%
ext4 read:      991.54          1003.7  +1.22%
ext4 write:     1069.7          1052.2  -1.63%
btrfs read:     1076.1          1082.1  +0.55%
btrfs write:    968.98          965.07  -0.40%

6. DM: stripe size 128k

Quoted 9 lines

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       1020.4          1066.1  +4.47%
xfs write:      1058.2          1066.6  +0.79%
ext4 read:      990.72          988.19  -0.25%
ext4 write:     1050.4          1070.2  +1.88%
btrfs read:     1080.9          1074.7  -0.57%
btrfs write:    975.10          972.76  -0.23%

Hi Mike,

How about these numbers?

I'm also happy to run other fio jobs your team used.

Quoted 22 lines · Ming Lin

Quoted 21 lines · Ming Lin

Quoted 6 lines · Mike Snitzer

We need to test on large HW raid setups like a Netapp filer (or even local SAS drives connected via some SAS controller). Like a 8+2 drive RAID6 or 8+1 RAID5 setup. Testing with MD raid on JBOD setups with 8 devices is also useful. It is larger RAID setups that will be more sensitive to IO sizes being properly aligned on RAID stripe and/or chunk size boundaries.

Here are tests results of xfs/ext4/btrfs read/write on HW RAID6/MD RAID6/DM stripe target. Each case run 0.5 hour, so it took 36 hours to finish all the tests on 4.1-rc4 and 4.1-rc4-patched kernels.

No performance regressions were introduced.

Test server: Dell R730xd(2 sockets/48 logical cpus/264G memory) HW RAID6/MD RAID6/DM stripe target were configured with 10 HDDs, each 280G Stripe size 64k and 128k were tested.

devs="/dev/sdb /dev/sdc /dev/sdd /dev/sde /dev/sdf /dev/sdg /dev/sdh /dev/sdi /dev/sdj /dev/sdk" spare_devs="/dev/sdl /dev/sdm"
stripe_size=64 (or 128)

Quoted 7 lines

Quoted 7 lines

MD RAID6 was created by:
mdadm --create --verbose /dev/md0 --level=6 --raid-devices=10 $devs --spare-devices=2 $spare_devs -c $stripe_size

DM stripe target was created by:
pvcreate $devs
vgcreate striped_vol_group $devs
lvcreate -i10 -I${stripe_size} -L2T -nstriped_logical_volume striped_vol_group

DM had a regression relative to merge_bvec that wasn't fixed until recently (it wasn't in 4.1-rc4), see commit 1c220c69ce0 ("dm: fix casting bug in dm_merge_bvec()"). It was introduced in 4.1.

So your 4.1-rc4 DM stripe testing may have effectively been with merge_bvec disabled.

Quoted 90 lines

Quoted 89 lines

Here is an example of fio script for stripe size 128k:
[global]
ioengine=libaio
iodepth=64
direct=1
runtime=1800
time_based
group_reporting
numjobs=48
gtod_reduce=0
norandommap
write_iops_log=fs

[job1]
bs=1280K
directory=/mnt
size=5G
rw=read

All results here: http://minggr.net/pub/20150608/fio_results/

Results summary:

1. HW RAID6: stripe size 64k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       821.23          812.20  -1.09%
xfs write:      753.16          754.42  +0.16%
ext4 read:      827.80          834.82  +0.84%
ext4 write:     783.08          777.58  -0.70%
btrfs read:     859.26          871.68  +1.44%
btrfs write:    815.63          844.40  +3.52%

2. HW RAID6: stripe size 128k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       948.27          979.11  +3.25%
xfs write:      820.78          819.94  -0.10%
ext4 read:      978.35          997.92  +2.00%
ext4 write:     853.51          847.97  -0.64%
btrfs read:     1013.1          1015.6  +0.24%
btrfs write:    854.43          850.42  -0.46%

3. MD RAID6: stripe size 64k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       847.34          869.43  +2.60%
xfs write:      198.67          199.03  +0.18%
ext4 read:      763.89          767.79  +0.51%
ext4 write:     281.44          282.83  +0.49%
btrfs read:     756.02          743.69  -1.63%
btrfs write:    268.37          265.93  -0.90%

4. MD RAID6: stripe size 128k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       993.04          1014.1  +2.12%
xfs write:      293.06          298.95  +2.00%
ext4 read:      1019.6          1020.9  +0.12%
ext4 write:     371.51          371.47  -0.01%
btrfs read:     1000.4          1020.8  +2.03%
btrfs write:    241.08          246.77  +2.36%

5. DM: stripe size 64k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       1084.4          1080.1  -0.39%
xfs write:      1071.1          1063.4  -0.71%
ext4 read:      991.54          1003.7  +1.22%
ext4 write:     1069.7          1052.2  -1.63%
btrfs read:     1076.1          1082.1  +0.55%
btrfs write:    968.98          965.07  -0.40%

6. DM: stripe size 128k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       1020.4          1066.1  +4.47%
xfs write:      1058.2          1066.6  +0.79%
ext4 read:      990.72          988.19  -0.25%
ext4 write:     1050.4          1070.2  +1.88%
btrfs read:     1080.9          1074.7  -0.57%
btrfs write:    975.10          972.76  -0.23%

Quoted 3 lines

Hi Mike,

How about these numbers?

Looks fairly good. I just am not sure the workload is going to test the code paths in question like we'd hope. I'll have to set aside some time

to think through scenarios to test.

My concern still remains that at some point it the future we'll regret not having merge_bvec but it'll be too late. That is just my own FUD at this point...

Quoted 1 line

I'm also happy to run other fio jobs your team used.

I've been busy getting DM changes for the 4.2 merge window finalized. As such I haven't connected with others on the team to discuss this issue.

I'll see if we can make time in the next 2 days. But I also have RHEL-specific kernel deadlines I'm coming up against.

Seems late to be staging this extensive a change for 4.2... are you pushing for this code to land in the 4.2 merge window? Or do we have time to work this further and target the 4.3 merge?

Mike

Quoted 33 lines · Mike Snitzer

Quoted 29 lines · Ming Lin

Quoted 28 lines · Ming Lin

Quoted 6 lines · Mike Snitzer

We need to test on large HW raid setups like a Netapp filer (or even local SAS drives connected via some SAS controller). Like a 8+2 drive RAID6 or 8+1 RAID5 setup. Testing with MD raid on JBOD setups with 8 devices is also useful. It is larger RAID setups that will be more sensitive to IO sizes being properly aligned on RAID stripe and/or chunk size boundaries.

Here are tests results of xfs/ext4/btrfs read/write on HW RAID6/MD RAID6/DM stripe target. Each case run 0.5 hour, so it took 36 hours to finish all the tests on 4.1-rc4 and 4.1-rc4-patched kernels.

No performance regressions were introduced.

Test server: Dell R730xd(2 sockets/48 logical cpus/264G memory) HW RAID6/MD RAID6/DM stripe target were configured with 10 HDDs, each 280G Stripe size 64k and 128k were tested.

devs="/dev/sdb /dev/sdc /dev/sdd /dev/sde /dev/sdf /dev/sdg /dev/sdh /dev/sdi /dev/sdj /dev/sdk" spare_devs="/dev/sdl /dev/sdm"
stripe_size=64 (or 128)

MD RAID6 was created by:
mdadm --create --verbose /dev/md0 --level=6 --raid-devices=10 $devs --spare-devices=2 $spare_devs -c $stripe_size

DM stripe target was created by:
pvcreate $devs
vgcreate striped_vol_group $devs
lvcreate -i10 -I${stripe_size} -L2T -nstriped_logical_volume striped_vol_group

Quoted 6 lines

DM had a regression relative to merge_bvec that wasn't fixed until recently (it wasn't in 4.1-rc4), see commit 1c220c69ce0 ("dm: fix casting bug in dm_merge_bvec()"). It was introduced in 4.1.

So your 4.1-rc4 DM stripe testing may have effectively been with merge_bvec disabled.

I'l rebase it to latest Linus tree and re-run DM stripe testing.

Quoted 22 lines · Ming Lin

Quoted 21 lines · Ming Lin

Quoted 6 lines · Mike Snitzer

We need to test on large HW raid setups like a Netapp filer (or even local SAS drives connected via some SAS controller). Like a 8+2 drive RAID6 or 8+1 RAID5 setup. Testing with MD raid on JBOD setups with 8 devices is also useful. It is larger RAID setups that will be more sensitive to IO sizes being properly aligned on RAID stripe and/or chunk size boundaries.

Here are tests results of xfs/ext4/btrfs read/write on HW RAID6/MD RAID6/DM stripe target. Each case run 0.5 hour, so it took 36 hours to finish all the tests on 4.1-rc4 and 4.1-rc4-patched kernels.

No performance regressions were introduced.

Test server: Dell R730xd(2 sockets/48 logical cpus/264G memory) HW RAID6/MD RAID6/DM stripe target were configured with 10 HDDs, each 280G Stripe size 64k and 128k were tested.

devs="/dev/sdb /dev/sdc /dev/sdd /dev/sde /dev/sdf /dev/sdg /dev/sdh /dev/sdi /dev/sdj /dev/sdk" spare_devs="/dev/sdl /dev/sdm"
stripe_size=64 (or 128)

Quoted 7 lines

Quoted 7 lines

MD RAID6 was created by:
mdadm --create --verbose /dev/md0 --level=6 --raid-devices=10 $devs --spare-devices=2 $spare_devs -c $stripe_size

DM stripe target was created by:
pvcreate $devs
vgcreate striped_vol_group $devs
lvcreate -i10 -I${stripe_size} -L2T -nstriped_logical_volume striped_vol_group

DM had a regression relative to merge_bvec that wasn't fixed until recently (it wasn't in 4.1-rc4), see commit 1c220c69ce0 ("dm: fix casting bug in dm_merge_bvec()"). It was introduced in 4.1.

So your 4.1-rc4 DM stripe testing may have effectively been with merge_bvec disabled.

Quoted 89 lines

Quoted 88 lines

Here is an example of fio script for stripe size 128k:
[global]
ioengine=libaio
iodepth=64
direct=1
runtime=1800
time_based
group_reporting
numjobs=48
gtod_reduce=0
norandommap
write_iops_log=fs

[job1]
bs=1280K
directory=/mnt
size=5G
rw=read

All results here: http://minggr.net/pub/20150608/fio_results/

Results summary:

1. HW RAID6: stripe size 64k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       821.23          812.20  -1.09%
xfs write:      753.16          754.42  +0.16%
ext4 read:      827.80          834.82  +0.84%
ext4 write:     783.08          777.58  -0.70%
btrfs read:     859.26          871.68  +1.44%
btrfs write:    815.63          844.40  +3.52%

2. HW RAID6: stripe size 128k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       948.27          979.11  +3.25%
xfs write:      820.78          819.94  -0.10%
ext4 read:      978.35          997.92  +2.00%
ext4 write:     853.51          847.97  -0.64%
btrfs read:     1013.1          1015.6  +0.24%
btrfs write:    854.43          850.42  -0.46%

3. MD RAID6: stripe size 64k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       847.34          869.43  +2.60%
xfs write:      198.67          199.03  +0.18%
ext4 read:      763.89          767.79  +0.51%
ext4 write:     281.44          282.83  +0.49%
btrfs read:     756.02          743.69  -1.63%
btrfs write:    268.37          265.93  -0.90%

4. MD RAID6: stripe size 128k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       993.04          1014.1  +2.12%
xfs write:      293.06          298.95  +2.00%
ext4 read:      1019.6          1020.9  +0.12%
ext4 write:     371.51          371.47  -0.01%
btrfs read:     1000.4          1020.8  +2.03%
btrfs write:    241.08          246.77  +2.36%

5. DM: stripe size 64k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       1084.4          1080.1  -0.39%
xfs write:      1071.1          1063.4  -0.71%
ext4 read:      991.54          1003.7  +1.22%
ext4 write:     1069.7          1052.2  -1.63%
btrfs read:     1076.1          1082.1  +0.55%
btrfs write:    968.98          965.07  -0.40%

6. DM: stripe size 128k

                4.1-rc4         4.1-rc4-patched
                -------         ---------------
                (MB/s)          (MB/s)
xfs read:       1020.4          1066.1  +4.47%
xfs write:      1058.2          1066.6  +0.79%
ext4 read:      990.72          988.19  -0.25%
ext4 write:     1050.4          1070.2  +1.88%
btrfs read:     1080.9          1074.7  -0.57%
btrfs write:    975.10          972.76  -0.23%

Quoted 3 lines

Hi Mike,

How about these numbers?

Looks fairly good. I just am not sure the workload is going to test the code paths in question like we'd hope. I'll have to set aside some time to think through scenarios to test.

My concern still remains that at some point it the future we'll regret not having merge_bvec but it'll be too late. That is just my own FUD at this point...

Quoted 1 line

I'm also happy to run other fio jobs your team used.

I've been busy getting DM changes for the 4.2 merge window finalized. As such I haven't connected with others on the team to discuss this issue.

I'll see if we can make time in the next 2 days. But I also have RHEL-specific kernel deadlines I'm coming up against.

Seems late to be staging this extensive a change for 4.2... are you pushing for this code to land in the 4.2 merge window? Or do we have time to work this further and target the 4.3 merge?

Mike

From: Kent Overstreet <[email protected]>

Remove bio_fits_rdev() completely, because ->merge_bvec_fn() has now gone. There's no point in calling bio_fits_rdev() only for ensuring aligned read from rdev.

Cc Neil Brown <[email protected]>

Cc [email protected]

Signed-off-by Kent Overstreet <[email protected]>

[dpark: add more description in commit message]

Signed-off-by Dongsu Park <[email protected]>

Signed-off-by Ming Lin <[email protected]>

---
 drivers/md/raid5.c | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

drivers/md/raid5.c +1 -22

index 1ba97fd..b303ded 100644
@@ -4743,25 +4743,6 @@ static void raid5_align_endio(struct bio *bi, int error)
 	add_bio_to_retry(raid_bi, conf);
 }
 
-static int bio_fits_rdev(struct bio *bi)
-{
-	struct request_queue *q = bdev_get_queue(bi->bi_bdev);
-
-	if (bio_sectors(bi) > queue_max_sectors(q))
-		return 0;
-	blk_recount_segments(q, bi);
-	if (bi->bi_phys_segments > queue_max_segments(q))
-		return 0;
-
-	if (q->merge_bvec_fn)
-		/* it's too hard to apply the merge_bvec_fn at this stage,
-		 * just just give up
-		 */
-		return 0;
-
-	return 1;
-}
-
 static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio)
 {
 	struct r5conf *conf = mddev->private;

@@ -4815,11 +4796,9 @@ static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio)
 		align_bi->bi_bdev =  rdev->bdev;
 		__clear_bit(BIO_SEG_VALID, &align_bi->bi_flags);
 
-		if (!bio_fits_rdev(align_bi) ||
-		    is_badblock(rdev, align_bi->bi_iter.bi_sector,
+		if (is_badblock(rdev, align_bi->bi_iter.bi_sector,
 				bio_sectors(align_bi),
 				&first_bad, &bad_sectors)) {
-			/* too big in some way, or has a known bad block */
 			bio_put(align_bi);
 			rdev_dec_pending(rdev, mddev);
 			return 0;

Quoted 5 lines · On Fri, 22 May 2015 11:18:38 -0700 Ming Lin <[email protected]> wrote:

From: Kent Overstreet <[email protected]>

Remove bio_fits_rdev() completely, because ->merge_bvec_fn() has now gone. There's no point in calling bio_fits_rdev() only for ensuring aligned read from rdev.

Surely this patch should come *before*
[PATCH v4 07/11] md/raid5: split bio for chunk_aligned_read

and the comment says ->merge_bvec_fn() has gone, but that isn't until
[PATCH v4 08/11] block: kill merge_bvec_fn() completely

If those issues are resolved, then

Acked-by NeilBrown <[email protected]>

Thanks,
NeilBrown

Quoted 54 lines

Cc Neil Brown <[email protected]>

Cc [email protected]

Signed-off-by Kent Overstreet <[email protected]>

[dpark: add more description in commit message]

Signed-off-by Dongsu Park <[email protected]>

Signed-off-by Ming Lin <[email protected]>

---
 drivers/md/raid5.c | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

drivers/md/raid5.c +1 -22

index 1ba97fd..b303ded 100644
@@ -4743,25 +4743,6 @@ static void raid5_align_endio(struct bio *bi, int error)
      add_bio_to_retry(raid_bi, conf);
 }
 
-static int bio_fits_rdev(struct bio *bi)
-{
-     struct request_queue *q = bdev_get_queue(bi->bi_bdev);
-
-     if (bio_sectors(bi) > queue_max_sectors(q))
-             return 0;
-     blk_recount_segments(q, bi);
-     if (bi->bi_phys_segments > queue_max_segments(q))
-             return 0;
-
-     if (q->merge_bvec_fn)
-             /* it's too hard to apply the merge_bvec_fn at this stage,
-              * just just give up
-              */
-             return 0;
-
-     return 1;
-}
-
 static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio)
 {
      struct r5conf *conf = mddev->private;

@@ -4815,11 +4796,9 @@ static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio)
              align_bi->bi_bdev =  rdev->bdev;
              __clear_bit(BIO_SEG_VALID, &align_bi->bi_flags);
 
-             if (!bio_fits_rdev(align_bi) ||
-                 is_badblock(rdev, align_bi->bi_iter.bi_sector,
+             if (is_badblock(rdev, align_bi->bi_iter.bi_sector,
                              bio_sectors(align_bi),
                              &first_bad, &bad_sectors)) {
-                     /* too big in some way, or has a known bad block */
                      bio_put(align_bi);
                      rdev_dec_pending(rdev, mddev);
                      return 0;

Quoted 4 lines · NeilBrown

Quoted 2 lines · On Fri, 22 May 2015 11:18:38 -0700 Ming Lin <[email protected]> wrote:

From: Kent Overstreet <[email protected]>

Quoted 6 lines

Quoted 3 lines

Remove bio_fits_rdev() completely, because ->merge_bvec_fn() has now gone. There's no point in calling bio_fits_rdev() only for ensuring aligned read from rdev.

Surely this patch should come *before*
[PATCH v4 07/11] md/raid5: split bio for chunk_aligned_read

PATCH 6, then PATCH 7, isn't it already *before*?

Quoted 6 lines

and the comment says ->merge_bvec_fn() has gone, but that isn't until
[PATCH v4 08/11] block: kill merge_bvec_fn() completely

Quoted 1 line

If those issues are resolved, then

How about this?

PATCH 6: md/raid5: split bio for chunk_aligned_read PATCH 7: block: kill merge_bvec_fn() completely PATCH 8: md/raid5: get rid of bio_fits_rdev()

Thanks.

Quoted 52 lines

Acked-by NeilBrown <[email protected]>

Thanks,
NeilBrown

Quoted 45 lines

Cc Neil Brown <[email protected]>

Cc [email protected]

Signed-off-by Kent Overstreet <[email protected]>

[dpark: add more description in commit message]

Signed-off-by Dongsu Park <[email protected]>

Signed-off-by Ming Lin <[email protected]>

---
 drivers/md/raid5.c | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

drivers/md/raid5.c +0 -19

index 1ba97fd..b303ded 100644
@@ -4743,25 +4743,6 @@ static void raid5_align_endio(struct bio *bi, int error)
      add_bio_to_retry(raid_bi, conf);
 }
 
-static int bio_fits_rdev(struct bio *bi)
-{
-     struct request_queue *q = bdev_get_queue(bi->bi_bdev);
-
-     if (bio_sectors(bi) > queue_max_sectors(q))
-             return 0;
-     blk_recount_segments(q, bi);
-     if (bi->bi_phys_segments > queue_max_segments(q))
-             return 0;
-
-     if (q->merge_bvec_fn)
-             /* it's too hard to apply the merge_bvec_fn at this stage,
-              * just just give up
-              */
-             return 0;
-
-     return 1;
-}
-
 static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio)
 {
      struct r5conf *conf = mddev->private;

@@ -4815,11 +4796,9 @@ static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio)
              align_bi->bi_bdev =  rdev->bdev;
              __clear_bit(BIO_SEG_VALID, &align_bi->bi_flags);
 

Quoted 10 lines

drivers/md/raid5.c static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio) · line 4804

Quoted 9 lines

drivers/md/raid5.c +1 -3

-             if (!bio_fits_rdev(align_bi) ||
-                 is_badblock(rdev, align_bi->bi_iter.bi_sector,
+             if (is_badblock(rdev, align_bi->bi_iter.bi_sector,
                              bio_sectors(align_bi),
                              &first_bad, &bad_sectors)) {
-                     /* too big in some way, or has a known bad block */
                      bio_put(align_bi);
                      rdev_dec_pending(rdev, mddev);
                      return 0;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Did I write that? I guess I did :-(
I meant *after*. Don't get rid of bio_fits_rdev until split_bio is in
chunk_aligned_read().

Quoted 3 lines · NeilBrown

Did I write that? I guess I did :-(
I meant *after*. Don't get rid of bio_fits_rdev until split_bio is in
chunk_aligned_read().

I suspect the whole series could use some reordering.

patch 1:

add ->bio_split and blk_queue_split

patch 2..n:

one for each non-trivial driver that implements ->merge_bvec_fn to remove it and instead split bios in ->make_request. The md patch to do the right thing in chunk_aligned_read goes into the general md patch here. The bcache patch also goes into this series.

patch n+1:

- add blk_queue_split calls for remaining trivial drivers

patch n+2:

- remove ->merge_bvec_fn and checking of max_sectors a for all drivers, simplify bio_add_page

patch n+2:

- remove splitting in blkdev_issue_discard

patch n+3

- remove bio_fits_rdev

patch n+4

- remove bio_get_nr_vecs

patch n+4

- use bio_add_page

patch n+5

- update documentation

Quoted 6 lines · Christoph Hellwig

Quoted 3 lines · NeilBrown

Did I write that? I guess I did :-(
I meant *after*. Don't get rid of bio_fits_rdev until split_bio is in
chunk_aligned_read().

I suspect the whole series could use some reordering.

Nice reordering.
I'll do this.

Thanks.

Quoted 40 lines

patch 1:

add ->bio_split and blk_queue_split

patch 2..n:

one for each non-trivial driver that implements ->merge_bvec_fn to remove it and instead split bios in ->make_request. The md patch to do the right thing in chunk_aligned_read goes into the general md patch here. The bcache patch also goes into this series.

patch n+1:

- add blk_queue_split calls for remaining trivial drivers

patch n+2:

- remove ->merge_bvec_fn and checking of max_sectors a for all drivers, simplify bio_add_page

patch n+2:

- remove splitting in blkdev_issue_discard

patch n+3

- remove bio_fits_rdev

patch n+4

- remove bio_get_nr_vecs

patch n+4

- use bio_add_page

patch n+5

- update documentation

Quoted 10 lines · Ming Lin

Quoted 6 lines · Christoph Hellwig

Quoted 3 lines · NeilBrown

Did I write that? I guess I did :-(
I meant *after*. Don't get rid of bio_fits_rdev until split_bio is in
chunk_aligned_read().

I suspect the whole series could use some reordering.

Nice reordering.
I'll do this.

Here is the reordering.
https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=block-generic-req

I'll post it if you are OK.

[PATCH 01/15] block: add blk_queue_split()
[PATCH 02/15] md: remove ->merge_bvec_fn
[PATCH 03/15] dm: remov merge functions
[PATCH 04/15] drbd: remove ->merge_bvec_fn
[PATCH 05/15] pktcdvd: remove ->merge_bvec_fn
[PATCH 06/15] rbd: remove ->merge_bvec_fn
[PATCH 07/15] bcache: remove driver private bio splitting code [PATCH 08/15] btrfs: remove bio splitting and merge_bvec_fn() calls [PATCH 09/15] block: call blk_queue_split() in make_request functions [PATCH 10/15] block: kill ->merge_bvec_fn and simplify bio_add_page [PATCH 11/15] block: remove split code in blkdev_issue_discard [PATCH 12/15] md/raid5: get rid of bio_fits_rdev()
[PATCH 13/15] block: remove bio_get_nr_vecs()
[PATCH 14/15] fs: use helper bio_add_page() instead of open coding on [PATCH 15/15] Documentation: update notes in biovecs about

Quoted 44 lines

Thanks.

Quoted 40 lines

patch 1:

add ->bio_split and blk_queue_split

patch 2..n:

one for each non-trivial driver that implements ->merge_bvec_fn to remove it and instead split bios in ->make_request. The md patch to do the right thing in chunk_aligned_read goes into the general md patch here. The bcache patch also goes into this series.

patch n+1:

- add blk_queue_split calls for remaining trivial drivers

patch n+2:

- remove ->merge_bvec_fn and checking of max_sectors a for all drivers, simplify bio_add_page

patch n+2:

- remove splitting in blkdev_issue_discard

patch n+3

- remove bio_fits_rdev

patch n+4

- remove bio_get_nr_vecs

patch n+4

- use bio_add_page

patch n+5

- update documentation

Quoted 17 lines · On Tue, 26 May 2015 15:32:38 -0700 Ming Lin <[email protected]> wrote:

Quoted 10 lines · Ming Lin

Quoted 6 lines · Christoph Hellwig

Quoted 3 lines · NeilBrown

Did I write that? I guess I did :-(
I meant *after*. Don't get rid of bio_fits_rdev until split_bio is in
chunk_aligned_read().

I suspect the whole series could use some reordering.

Nice reordering.
I'll do this.

Here is the reordering.
https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=block-generic-req

I'll post it if you are OK.

Quoted 15 lines

[PATCH 01/15] block: add blk_queue_split()
[PATCH 02/15] md: remove ->merge_bvec_fn
[PATCH 03/15] dm: remov merge functions
[PATCH 04/15] drbd: remove ->merge_bvec_fn
[PATCH 05/15] pktcdvd: remove ->merge_bvec_fn
[PATCH 06/15] rbd: remove ->merge_bvec_fn
[PATCH 07/15] bcache: remove driver private bio splitting code [PATCH 08/15] btrfs: remove bio splitting and merge_bvec_fn() calls [PATCH 09/15] block: call blk_queue_split() in make_request functions [PATCH 10/15] block: kill ->merge_bvec_fn and simplify bio_add_page [PATCH 11/15] block: remove split code in blkdev_issue_discard [PATCH 12/15] md/raid5: get rid of bio_fits_rdev()
[PATCH 13/15] block: remove bio_get_nr_vecs()
[PATCH 14/15] fs: use helper bio_add_page() instead of open coding on [PATCH 15/15] Documentation: update notes in biovecs about

The changes to dm.c and dm.h should be in the "dm:" patch, not "md:".

But I don't think the sequence is right.

You cannot remove ->merge_bvec_fn for *any* stacked device until *all* devices make use of blk_queue_split() (or otherwise handle arbitrarily large bios).

I think it would be easiest to:
- add blk_queue_split() and call it from common code before ->make_request_fn is called. The ensure all devices can accept arbitrarily large bios.

- driver-by-driver remove merge_bvec_fn and make sure the the driver can cope with arbitrary bios themselve, calling blk_queue_split in the make_request function only if needed
- finally remove the call to blk_queue_split from the common code.

Does that make sense to others?

Thanks,
NeilBrown

Quoted 44 lines

Quoted 44 lines

Thanks.

Quoted 40 lines

patch 1:

add ->bio_split and blk_queue_split

patch 2..n:

one for each non-trivial driver that implements ->merge_bvec_fn to remove it and instead split bios in ->make_request. The md patch to do the right thing in chunk_aligned_read goes into the general md patch here. The bcache patch also goes into this series.

patch n+1:

- add blk_queue_split calls for remaining trivial drivers

patch n+2:

- remove ->merge_bvec_fn and checking of max_sectors a for all drivers, simplify bio_add_page

patch n+2:

- remove splitting in blkdev_issue_discard

patch n+3

- remove bio_fits_rdev

patch n+4

- remove bio_get_nr_vecs

patch n+4

- use bio_add_page

patch n+5

- update documentation

Quoted 35 lines · NeilBrown

Quoted 17 lines · On Tue, 26 May 2015 15:32:38 -0700 Ming Lin <[email protected]> wrote:

Quoted 10 lines · Ming Lin

Quoted 6 lines · Christoph Hellwig

Quoted 3 lines · NeilBrown

Did I write that? I guess I did :-(
I meant *after*. Don't get rid of bio_fits_rdev until split_bio is in
chunk_aligned_read().

I suspect the whole series could use some reordering.

Nice reordering.
I'll do this.

Here is the reordering.
https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=block-generic-req

I'll post it if you are OK.

Quoted 15 lines

[PATCH 01/15] block: add blk_queue_split()
[PATCH 02/15] md: remove ->merge_bvec_fn
[PATCH 03/15] dm: remov merge functions
[PATCH 04/15] drbd: remove ->merge_bvec_fn
[PATCH 05/15] pktcdvd: remove ->merge_bvec_fn
[PATCH 06/15] rbd: remove ->merge_bvec_fn
[PATCH 07/15] bcache: remove driver private bio splitting code [PATCH 08/15] btrfs: remove bio splitting and merge_bvec_fn() calls [PATCH 09/15] block: call blk_queue_split() in make_request functions [PATCH 10/15] block: kill ->merge_bvec_fn and simplify bio_add_page [PATCH 11/15] block: remove split code in blkdev_issue_discard [PATCH 12/15] md/raid5: get rid of bio_fits_rdev()
[PATCH 13/15] block: remove bio_get_nr_vecs()
[PATCH 14/15] fs: use helper bio_add_page() instead of open coding on [PATCH 15/15] Documentation: update notes in biovecs about

Quoted 1 line

The changes to dm.c and dm.h should be in the "dm:" patch, not "md:".

Will move it.

Quoted 9 lines

But I don't think the sequence is right.

You cannot remove ->merge_bvec_fn for *any* stacked device until *all* devices make use of blk_queue_split() (or otherwise handle arbitrarily large bios).

I think it would be easiest to:
- add blk_queue_split() and call it from common code before ->make_request_fn is called. The ensure all devices can accept arbitrarily large bios.

For "common code", do you mean "generic_make_request()"

block/blk-core.c +0 -0

index fbbb337..bb6455b 100644

But I don't think the sequence is right.

You cannot remove ->merge_bvec_fn for *any* stacked device until *all* devices make use of blk_queue_split() (or otherwise handle arbitrarily large bios).

I think it would be easiest to:
- add blk_queue_split() and call it from common code before ->make_request_fn is called. The ensure all devices can accept arbitrarily large bios.
- driver-by-driver remove merge_bvec_fn and make sure the the driver can cope with arbitrary bios themselve, calling blk_queue_split in the make_request function only if needed
- finally remove the call to blk_queue_split from the common code.

Does that make sense to others?