dm: improve read performance

From: Mustafa Mesanovic <[email protected]>

A short explanation in prior: in this case we have "stacked" dm devices. Two multipathed luns combined together to one striped logical volume.

I/O throughput degradation happens at __bio_add_page when bio's get checked upon max_sectors. In this setup max_sectors is always set to 8 -> what is 4KiB.
A standalone striped logical volume on luns which are not multipathed do not have the problem: the logical volume will take over the max_sectors from luns below.

Same happens with luns which are multipathed -> the multipathed targets have the same max_sectors as the luns below.

So "magic" happens only when target has no own merge_fn and below lying devices
have a merge function -> we got then max_sectors=PAGE_SIZE >> 9. This patch prevents that max_sectors will be set to PAGE_SIZE >> 9. Instead it will use the minimum max_sectors value from below devices.

Using the patch improves read I/O up to 3x. In this specific case from 600MiB/s up to 1800MiB/s.

Signed-off-by Mustafa Mesanovic <[email protected]>

---

 dm-table.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/drivers/md/dm-table.c
===================================================================

drivers/md/dm-table.c +1 -1

@@ -518,7 +518,7 @@
 
 	if (q->merge_bvec_fn && !ti->type->merge)
 		blk_limits_max_hw_sectors(limits,
-					  (unsigned int) (PAGE_SIZE >> 9));
+					  q->limits.max_sectors);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(dm_set_device_limits);

On Mon, 27 Dec 2010 12:19:55 +0100 Mustafa Mesanovic

Quoted 15 lines · <[email protected]> wrote:

From: Mustafa Mesanovic <[email protected]>

A short explanation in prior: in this case we have "stacked" dm devices. Two multipathed luns combined together to one striped logical volume.

I/O throughput degradation happens at __bio_add_page when bio's get checked upon max_sectors. In this setup max_sectors is always set to 8 -> what is 4KiB.
A standalone striped logical volume on luns which are not multipathed do not have the problem: the logical volume will take over the max_sectors from luns below.

Same happens with luns which are multipathed -> the multipathed targets have the same max_sectors as the luns below.

Quoted 8 lines

So "magic" happens only when target has no own merge_fn and below lying devices
have a merge function -> we got then max_sectors=PAGE_SIZE >> 9. This patch prevents that max_sectors will be set to PAGE_SIZE >> 9. Instead it will use the minimum max_sectors value from below devices.

Using the patch improves read I/O up to 3x. In this specific case from 600MiB/s up to 1800MiB/s.

and using this patch will cause IO to fail sometimes.
If an IO request which is larger than a page crosses a device boundary in the underlying e.g. RAID0, the RAID0 will return an error as such things should not happen - they are prevented by merge_bvec_fn.

If merge_bvec_fn is not being honoured, then you MUST limit requests to a single entry iovec of at most one page.

NeilBrown

Quoted 13 lines

Signed-off-by Mustafa Mesanovic <[email protected]>

---

 dm-table.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/drivers/md/dm-table.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-table.c 2010-12-23 13:49:18.000000000
+0100 +++ linux-2.6/drivers/md/dm-table.c 2010-12-23 13:50:22.000000000
+0100 @@ -518,7 +518,7 @@

drivers/md/dm-table.c +0 -0

 	if (q->merge_bvec_fn && !ti->type->merge)

drivers/md/dm-table.c line 524

Quoted 10 lines

drivers/md/dm-table.c +1 -1

 
 		blk_limits_max_hw_sectors(limits,
 
-					  (unsigned int) (PAGE_SIZE >> 9));
+					  q->limits.max_sectors);
 
 	return 0;
 
 }
 EXPORT_SYMBOL_GPL(dm_set_device_limits);

--
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/

Quoted 14 lines · On Mon December 27 2010 12:54:59 Neil Brown wrote:

On Mon, 27 Dec 2010 12:19:55 +0100 Mustafa Mesanovic

Quoted 11 lines · <[email protected]> wrote:

From: Mustafa Mesanovic<[email protected]>

A short explanation in prior: in this case we have "stacked" dm devices. Two multipathed luns combined together to one striped logical volume.

I/O throughput degradation happens at __bio_add_page when bio's get checked upon max_sectors. In this setup max_sectors is always set to 8 -> what is 4KiB.
A standalone striped logical volume on luns which are not multipathed do not have the problem: the logical volume will take over the max_sectors from luns below.

Quoted 10 lines

Quoted 2 lines

Using the patch improves read I/O up to 3x. In this specific case from 600MiB/s up to 1800MiB/s.

and using this patch will cause IO to fail sometimes.
If an IO request which is larger than a page crosses a device boundary in the underlying e.g. RAID0, the RAID0 will return an error as such things should not happen - they are prevented by merge_bvec_fn.

If merge_bvec_fn is not being honoured, then you MUST limit requests to a single entry iovec of at most one page.

Quoted 2 lines

NeilBrown

Thank you for that hint, I will try to write a merge_bvec_fn for dm-stripe.c which solves the problem, if that is ok?

Mustafa Mesanovic

Quoted 15 lines · Mustafa Mesanovic

Quoted 14 lines · On Mon December 27 2010 12:54:59 Neil Brown wrote:

On Mon, 27 Dec 2010 12:19:55 +0100 Mustafa Mesanovic

Quoted 11 lines · <[email protected]> wrote:

From: Mustafa Mesanovic<[email protected]>

A short explanation in prior: in this case we have "stacked" dm devices. Two multipathed luns combined together to one striped logical volume.

I/O throughput degradation happens at __bio_add_page when bio's get checked upon max_sectors. In this setup max_sectors is always set to 8 -> what is 4KiB.
A standalone striped logical volume on luns which are not multipathed do not have the problem: the logical volume will take over the max_sectors from luns below.

[...]

Quoted 10 lines

Quoted 10 lines

Quoted 2 lines

Using the patch improves read I/O up to 3x. In this specific case from 600MiB/s up to 1800MiB/s.

and using this patch will cause IO to fail sometimes.
If an IO request which is larger than a page crosses a device boundary in the underlying e.g. RAID0, the RAID0 will return an error as such things should not happen - they are prevented by merge_bvec_fn.

If merge_bvec_fn is not being honoured, then you MUST limit requests to a single entry iovec of at most one page.

Quoted 7 lines

Quoted 2 lines

NeilBrown

Thank you for that hint, I will try to write a merge_bvec_fn for dm-stripe.c which solves the problem, if that is ok?

Mustafa Mesanovic

Now here my new suggestion to fix this issue, what is your opinion? I tested this with different setups, and it worked fine and I had very good performance improvements.

[RFC][PATCH] dm: improve read performance - v2

This patch adds a merge_fn for the dm stripe target. This merge_fn prevents dm_set_device_limits() setting the max_sectors to 4KiB (PAGE_SIZE). (As in a prior patch already mentioned.)

Now the read performance improved up to 3x higher compared to before.

What happened before:
I/O throughput degradation happened at __bio_add_page() when bio's got checked at the very beginning upon max_sectors. In this setup max_sectors is always set to 8. So bio's entered the dm target with a max of 4KiB.

Now dm-stripe target will have its own merge_fn so max_sectors will not pushed down to 8 (4KiB), and bio's can get bigger than 4KiB.

Signed-off-by Mustafa Mesanovic<[email protected]>

Hello Mustafa,

Quoted 45 lines · Mustafa Mesanovic

Quoted 15 lines · Mustafa Mesanovic

Quoted 14 lines · On Mon December 27 2010 12:54:59 Neil Brown wrote:

On Mon, 27 Dec 2010 12:19:55 +0100 Mustafa Mesanovic

Quoted 11 lines · <[email protected]> wrote:

From: Mustafa Mesanovic<[email protected]>

A short explanation in prior: in this case we have "stacked" dm devices. Two multipathed luns combined together to one striped logical volume.

I/O throughput degradation happens at __bio_add_page when bio's get checked upon max_sectors. In this setup max_sectors is always set to 8
-> what is 4KiB.
A standalone striped logical volume on luns which are not multipathed do not have the problem: the logical volume will take over the max_sectors from luns below.

[...]

Quoted 10 lines

Quoted 10 lines

Quoted 2 lines

Using the patch improves read I/O up to 3x. In this specific case from 600MiB/s up to 1800MiB/s.

and using this patch will cause IO to fail sometimes.
If an IO request which is larger than a page crosses a device boundary in the underlying e.g. RAID0, the RAID0 will return an error as such things should not happen - they are prevented by merge_bvec_fn.

If merge_bvec_fn is not being honoured, then you MUST limit requests to a single entry iovec of at most one page.

Quoted 7 lines

Quoted 2 lines

NeilBrown

Thank you for that hint, I will try to write a merge_bvec_fn for dm-stripe.c which solves the problem, if that is ok?

Mustafa Mesanovic

Now here my new suggestion to fix this issue, what is your opinion? I tested this with different setups, and it worked fine and I had very good performance improvements.

[RFC][PATCH] dm: improve read performance - v2

This patch adds a merge_fn for the dm stripe target. This merge_fn prevents dm_set_device_limits() setting the max_sectors to 4KiB (PAGE_SIZE). (As in a prior patch already mentioned.)

Quoted 6 lines

Now the read performance improved up to 3x higher compared to before.

What happened before:
I/O throughput degradation happened at __bio_add_page() when bio's got checked at the very beginning upon max_sectors. In this setup max_sectors is always set to 8. So bio's entered the dm target with a max of 4KiB.

So to recap:
- you are stacking dm-stripe on DM multipath devices
- dm_set_device_limits() will set max_hw_sectors to PAGE_SIZE if:
1) q->merge_bvec_fn is defined (as is the case for all DM devices)
2) target does not define a merge_fn (as was the case for dm-stripe)
- dm_merge_bvec (aka DM's q->merge_bvec_fn) will only allow a single page for the situation described in the previous point.

Quoted 4 lines

Now dm-stripe target will have its own merge_fn so max_sectors will not pushed down to 8 (4KiB), and bio's can get bigger than 4KiB.

Signed-off-by Mustafa Mesanovic<[email protected]>

Here is a revised version of your patch that uses the relatively new stripe_map_sector(), removes an unnecessary cast, and a few other cleanups.

This v3 should work as well as your v2 but should be suitable for upstream inclusion -- authorship of the change is still attributed to you as I only updated the code slightly. I'd appreciate it if you could verify your config still performs as expected.

Thanks,
Mike

From: Mustafa Mesanovic<[email protected]>

When the stripe target's underlying devices provide a merge_bvec_fn (e.g. DM) it is important to call down to them when building a biovec that doesn't span a stripe boundary.

Without the merge method, a striped DM device stacked on DM devices would cause bios with a single page to be submitted. The small bios that were built resulted in unnecessary overhead that hurt performance.

In one setup, striped DM multipath devices, read performance was improved from 600MB/s to 1800MB/s.

Signed-off-by Mustafa Mesanovic<[email protected]>

Signed-off-by Mike Snitzer<[email protected]>

---
  drivers/md/dm-stripe.c |   23 ++++++++++++++++++++++-
  1 files changed, 22 insertions(+), 1 deletions(-)

drivers/md/dm-stripe.c +22 -2

index dddfa14..fdfc1c6 100644
@@ -396,9 +396,29 @@ static void stripe_io_hints(struct dm_target *ti,
  	blk_limits_io_opt(limits, chunk_size * sc->stripes);
  }
 
+static int stripe_merge(struct dm_target *ti, struct bvec_merge_data *bvm,
+			struct bio_vec *biovec, int max_size)
+{
+	struct stripe_c *sc = ti->private;
+	sector_t bvm_sector = bvm->bi_sector;
+	uint32_t stripe;
+	struct request_queue *q;
+
+	stripe_map_sector(sc, bvm_sector,&stripe,&bvm_sector);
+
+	q = bdev_get_queue(sc->stripe[stripe].dev->bdev);
+	if (!q->merge_bvec_fn)
+		return max_size;
+
+	bvm->bi_bdev = sc->stripe[stripe].dev->bdev;
+	bvm->bi_sector = sc->stripe[stripe].physical_start + bvm_sector;
+
+	return min(max_size, q->merge_bvec_fn(q, bvm, biovec));
+}
+
  static struct target_type stripe_target = {
  	.name   = "striped",
-	.version = {1, 3, 1},
+	.version = {1, 3, 2},
  	.module = THIS_MODULE,
  	.ctr    = stripe_ctr,
  	.dtr    = stripe_dtr,

@@ -407,6 +427,7 @@ static struct target_type stripe_target = {
  	.status = stripe_status,
  	.iterate_devices = stripe_iterate_devices,
  	.io_hints = stripe_io_hints,
+	.merge  = stripe_merge,
  };
 
  int __init dm_stripe_init(void)
--

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/

Quoted 4 lines · Mike Snitzer

Here is a revised version of your patch that uses the relatively new stripe_map_sector(), removes an unnecessary cast, and a few other cleanups.

Quoted 4 lines

This v3 should work as well as your v2 but should be suitable for upstream inclusion -- authorship of the change is still attributed to you as I only updated the code slightly. I'd appreciate it if you could verify your config still performs as expected.

Mike,

your changes are working well and performing even a bit better.

Are there any further comments from others, or can consider putting it upstream.

Quoted 6 lines · Mustafa Mesanovic

Quoted 4 lines · Mike Snitzer

Here is a revised version of your patch that uses the relatively new stripe_map_sector(), removes an unnecessary cast, and a few other cleanups.

Quoted 7 lines

Quoted 4 lines

This v3 should work as well as your v2 but should be suitable for upstream inclusion -- authorship of the change is still attributed to you as I only updated the code slightly. I'd appreciate it if you could verify your config still performs as expected.

Mike,

your changes are working well and performing even a bit better.

Good to know.

Quoted 2 lines

Are there any further comments from others, or can consider putting it upstream.

I chatted with Alasdair and one concern he had was: does the existence of stripe_merge() ever hurt due to the fact that stripe_map_sector() is performed twice (once for .merge and again for .map)? May be that a really small chuck_size proves to be more problematic but using a small chunk_size generally isn't productive to begin with.

In any case, it clearly helps your workload.

Could you explain your config in more detail?
- what is your chunk_size?
- how many stripes (how many mpath devices)?
- what is the performance, of your test workload, of a single underlying mpath device?

And, in particular, what is your test workload?
- What is the nature of your IO (are you using a particular tool)?
- Are you using AIO?
- How many threads?
- Are you driving deep queue depths? Etc.

I have various configs that I'll be testing to help verify the benefit. The only other change Alasdair request is that the target version should be bumped to 1.4 (rather than 1.3.2).

Given that I can put some time to this now: we should be able to sort all this out for upstream inclusion in 2.6.39.

Thanks,
Mike

Quoted 18 lines · Mike Snitzer

In any case, it clearly helps your workload.

Could you explain your config in more detail?
- what is your chunk_size?
- how many stripes (how many mpath devices)?
- what is the performance, of your test workload, of a single underlying mpath device?

And, in particular, what is your test workload?
- What is the nature of your IO (are you using a particular tool)?
- Are you using AIO?
- How many threads?
- Are you driving deep queue depths? Etc.

I have various configs that I'll be testing to help verify the benefit. The only other change Alasdair request is that the target version should be bumped to 1.4 (rather than 1.3.2).

Quoted 5 lines

Given that I can put some time to this now: we should be able to sort all this out for upstream inclusion in 2.6.39.

Thanks,
Mike

Mike,

the setup that I have used to verify and check upon the changes consisted of:

- Benchmark
iozone (seq write, seq read, random read and write), filesize 2000m, with 32 processes (no AIO used).

- Disk-Setup
2 disks (queue_depth=192) -> each disk with 8 paths -> multipathed (multibus, rr_min_io=1)

And a striped LVM out of these two (chunk_size=64KiB).

The benchmark then runs on this LV.

Hi Mustafa,

Quoted 33 lines · Mustafa Mesanovic

Quoted 18 lines · Mike Snitzer

In any case, it clearly helps your workload.

Could you explain your config in more detail?
- what is your chunk_size?
- how many stripes (how many mpath devices)?
- what is the performance, of your test workload, of a single underlying mpath device?

And, in particular, what is your test workload?
- What is the nature of your IO (are you using a particular tool)?
- Are you using AIO?
- How many threads?
- Are you driving deep queue depths? Etc.

I have various configs that I'll be testing to help verify the benefit. The only other change Alasdair request is that the target version should be bumped to 1.4 (rather than 1.3.2).

Quoted 5 lines

Given that I can put some time to this now: we should be able to sort all this out for upstream inclusion in 2.6.39.

Thanks,
Mike

Mike,

the setup that I have used to verify and check upon the changes consisted of:

- Benchmark
iozone (seq write, seq read, random read and write), filesize 2000m, with 32 processes (no AIO used).

Quoted 7 lines

- Disk-Setup
2 disks (queue_depth=192) -> each disk with 8 paths
-> multipathed (multibus, rr_min_io=1)

And a striped LVM out of these two (chunk_size=64KiB).

The benchmark then runs on this LV.

What record size are you using?
Which filesystem are you using?
Also, were you using O_DIRECT? If not then I'm having a hard time understanding why implementing stripe_merge was so beneficial for you. stripe_merge doesn't help buffered IO.

Please share your exact iozone command line.

In my testing with aio-stress I have seen the number of calls to stripe_map be inversely proportional to the record size (when record size is<= chunk_size).

That is, with the following aio-stress commandline:
aio-stress -O -o 0 -o 1 -r $RECORD_SIZE -d 64 -b 16 -i 16 -s 2048 /dev/snitm/striped_lv

I varied the $RECORD_SIZE from 4k to 256k (striped_lv is using a 64k chunk_size across 8 mpath devices).

The number of stripe_map_sector() calls resulting from having implemented stripe_merge is fixed at 1048560 (when reading and then writing 2048m). And there is one stripe_map_sector() call for each stripe_map() call.

The following table shows the stripe_map_sector and stripe_map call count for writes then reads of 2048m (using $record_size AIO). AIO does make use of dm_merge_bvec and stripe_merge.

record_size    stripe_map_sector calls    stripe_map calls
4k             2097152                    1048592
8k             1572864                    524304
16k            1310720                    262160
32k            1179648                    131088
64k            1114112                    65552
128k           1114112                    65552
256k           1114112                    65552

The above shows that bios are being assembled using larger payloads (up to chunk_size) given that AIO does make use of stripe_merge.

When I did the same accounting (via attached systemtap script) for a buffered iozone run with a file size of 2000m (using -i 0 -i 1 -i 2) I saw that dm_merge_bvec() was _never_ called and the number of stripe_map_sector calls was very close to the stripe_map calls.

Mike

p.s.
All the above aside, one of our more elaborate benchmarks against XFS has seen a significant benefit from stripe_merge() being present... I still need to understand that benchmark's IO workload though.

Quoted 5 lines · Mike Snitzer

What record size are you using?
Which filesystem are you using?
Also, were you using O_DIRECT? If not then I'm having a hard time understanding why implementing stripe_merge was so beneficial for you. stripe_merge doesn't help buffered IO.

I used 64k record size, and ext3 as filesystem.

No, I was not using O_DIRECT. But I have measured as well with O_DIRECT, and the benefits there are significant too.

stripe_merge() helps a lot. The reason of splitting I/O records into 4KiB chunks happens at dm_set_device_limits(), thats what I explained in my v1 patch. If the target has no own merge_fn, max_sectors will be set to PAGE_SIZE, what in my case is 4KiB. Then __bio_add_page checks upon max_sectors and does not add any more pages to a bio. The bio stays at 4KiB.

Now by avoiding the "wrong" setting of max_sectors for the dm target, __bio_add_page will be able to add more than one page to the bios.

So this is my iozone call:

 # iozone -s 2000m -r 64k -t 32 -e -w -R -C -i 0
                                        -F<mntpt>/Child0 ....<mntpt>/Child31
For direct I/O (O_DIRECT) add '-I'.

dm_merge_bvec/stripe_merge is being called only on reads, thats what I have observed when I was testing the patch on my 2.6.32.x-stable kernel. Maybe it depends if the I/O is page cached or aio based...this might be worth a further analysis. On writes another path must be walked through, but I have not further analysed it so far.

In think it helps to avoid "overhead" in passing always 4KiB bios to the dm target. In my opinion it is "cheaper"/"faster" to pass one big bio down to the dm target instead of passing 4KiB max each bio.

I used iostat to check on the devices and the sizes of the requests, just try to start an iostat process which collects I/O statistics during your runs. e.g. 'iostat -dmx 2> outfile&' - check out "avgrq-sz".

And yes during my iostat runs I figured out that the writes are still dropping into the dm in 4KiB chunks, this is what I will analyse next.

Maybe there will be another patch(es) to fix that.

Mustafa

ps:
aio-stress did not work for me, sorry but I did not have the time to check on that and to search where the error might be...

Quoted 16 lines · Mustafa Mesanovic

Quoted 15 lines · On Mon December 27 2010 12:54:59 Neil Brown wrote:

On Mon, 27 Dec 2010 12:19:55 +0100 Mustafa Mesanovic

Quoted 12 lines · <[email protected]> wrote:

From: Mustafa Mesanovic<[email protected]>

A short explanation in prior: in this case we have "stacked" dm devices. Two multipathed luns combined together to one striped logical volume.

I/O throughput degradation happens at __bio_add_page when bio's get checked upon max_sectors. In this setup max_sectors is always set to 8 -> what is 4KiB.
A standalone striped logical volume on luns which are not multipathed do not have the problem: the logical volume will take over the max_sectors from luns below.

[...]

Quoted 13 lines

Quoted 12 lines

Quoted 2 lines

Using the patch improves read I/O up to 3x. In this specific case from 600MiB/s up to 1800MiB/s.

and using this patch will cause IO to fail sometimes.
If an IO request which is larger than a page crosses a device boundary in the underlying e.g. RAID0, the RAID0 will return an error as such things should not happen - they are prevented by merge_bvec_fn.

If merge_bvec_fn is not being honoured, then you MUST limit requests to a single entry iovec of at most one page.

NeilBrown

Quoted 4 lines

Thank you for that hint, I will try to write a merge_bvec_fn for dm-stripe.c which solves the problem, if that is ok?

Mustafa Mesanovic

Now here my new suggestion to fix this issue, what is your opinion? I tested this with different setups, and it worked fine and I had very good performance improvements.

[RFC][PATCH] dm: improve read performance - v2

This patch adds a merge_fn for the dm stripe target. This merge_fn prevents dm_set_device_limits() setting the max_sectors to 4KiB (PAGE_SIZE). (As in a prior patch already mentioned.)
Now the read performance improved up to 3x higher compared to before.

What happened before:
I/O throughput degradation happened at __bio_add_page() when bio's got checked at the very beginning upon max_sectors. In this setup max_sectors is always set to 8. So bio's entered the dm target with a max of 4KiB.

Now dm-stripe target will have its own merge_fn so max_sectors will not pushed down to 8 (4KiB), and bio's can get bigger than 4KiB.

Signed-off-by Mustafa Mesanovic<[email protected]>

---

  dm-stripe.c |   24 ++++++++++++++++++++++++
  1 file changed, 24 insertions(+)

Index: linux-2.6/drivers/md/dm-stripe.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-stripe.c 2011-02-28 10:23:37.000000000
+0100 +++ linux-2.6/drivers/md/dm-stripe.c 2011-02-28 10:24:29.000000000
+0100 @@ -396,6 +396,29 @@
blk_limits_io_opt(limits, chunk_size * sc->stripes);
}

drivers/md/dm-stripe.c +7 -0

+static int stripe_merge(struct dm_target *ti, struct bvec_merge_data *bvm,
+			struct bio_vec *biovec, int max_size)
+{
+	struct stripe_c *sc = (struct stripe_c *) ti->private;
+	sector_t offset, chunk;
+	uint32_t stripe;
+	struct request_queue *q;

drivers/md/dm-stripe.c +3 -0

+	return min(max_size, q->merge_bvec_fn(q, bvm, biovec));
+}
+
  static struct target_type stripe_target = {
  	.name   = "striped",
  	.version = {1, 3, 1},

Quoted 33 lines · Mustafa Mesanovic

Quoted 16 lines · Mustafa Mesanovic

Quoted 15 lines · On Mon December 27 2010 12:54:59 Neil Brown wrote:

On Mon, 27 Dec 2010 12:19:55 +0100 Mustafa Mesanovic

Quoted 12 lines · <[email protected]> wrote:

From: Mustafa Mesanovic<[email protected]>

A short explanation in prior: in this case we have "stacked" dm devices. Two multipathed luns combined together to one striped logical volume.

I/O throughput degradation happens at __bio_add_page when bio's get checked upon max_sectors. In this setup max_sectors is always set to 8 -> what is 4KiB.
A standalone striped logical volume on luns which are not multipathed do not have the problem: the logical volume will take over the max_sectors from luns below.

[...]

Quoted 13 lines

Quoted 12 lines

Quoted 2 lines

Using the patch improves read I/O up to 3x. In this specific case from 600MiB/s up to 1800MiB/s.

and using this patch will cause IO to fail sometimes.
If an IO request which is larger than a page crosses a device boundary in the underlying e.g. RAID0, the RAID0 will return an error as such things should not happen - they are prevented by merge_bvec_fn.

If merge_bvec_fn is not being honoured, then you MUST limit requests to a single entry iovec of at most one page.

NeilBrown

Quoted 9 lines

Quoted 4 lines

Thank you for that hint, I will try to write a merge_bvec_fn for dm-stripe.c which solves the problem, if that is ok?

Mustafa Mesanovic

Now here my new suggestion to fix this issue, what is your opinion? I tested this with different setups, and it worked fine and I had very good performance improvements.

Some minor style nitpicks.

Quoted 36 lines

[RFC][PATCH] dm: improve read performance - v2

This patch adds a merge_fn for the dm stripe target. This merge_fn prevents dm_set_device_limits() setting the max_sectors to 4KiB (PAGE_SIZE). (As in a prior patch already mentioned.)
Now the read performance improved up to 3x higher compared to before.

What happened before:
I/O throughput degradation happened at __bio_add_page() when bio's got checked at the very beginning upon max_sectors. In this setup max_sectors is always set to 8. So bio's entered the dm target with a max of 4KiB.

Now dm-stripe target will have its own merge_fn so max_sectors will not pushed down to 8 (4KiB), and bio's can get bigger than 4KiB.

Signed-off-by Mustafa Mesanovic<[email protected]>

---

  dm-stripe.c |   24 ++++++++++++++++++++++++
  1 file changed, 24 insertions(+)

Index: linux-2.6/drivers/md/dm-stripe.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-stripe.c 2011-02-28 10:23:37.000000000
+0100 +++ linux-2.6/drivers/md/dm-stripe.c 2011-02-28 10:24:29.000000000
+0100 @@ -396,6 +396,29 @@
blk_limits_io_opt(limits, chunk_size * sc->stripes);
}

drivers/md/dm-stripe.c +7 -0

+static int stripe_merge(struct dm_target *ti, struct bvec_merge_data *bvm,
+			struct bio_vec *biovec, int max_size)
+{
+	struct stripe_c *sc = (struct stripe_c *) ti->private;
+	sector_t offset, chunk;
+	uint32_t stripe;
+	struct request_queue *q;

drivers/md/dm-stripe.c line 415

Quoted 10 lines

drivers/md/dm-stripe.c +10 -0

+
+	offset = bvm->bi_sector - ti->begin;
+	chunk = offset>>  sc->chunk_shift;
+	stripe = sector_div(chunk, sc->stripes);
+
+	if (!bdev_get_queue(sc->stripe[stripe].dev->bdev)->merge_bvec_fn)
+			return max_size;
+
+	bvm->bi_bdev = sc->stripe[stripe].dev->bdev;
+	q = bdev_get_queue(bvm->bi_bdev);

Initializing q at the top would simplify the check fro merge_bvec_fn above.

drivers/md/dm-stripe.c line 418

Quoted 3 lines

drivers/md/dm-stripe.c +3 -0

+	bvm->bi_sector = sc->stripe[stripe].physical_start +
+			(chunk<<  sc->chunk_shift) + (offset&  sc->chunk_mask);
+

Can this be written as

bvm->bi_sector = sc->stripe[stripe].physical_start +

			bvm->bi_sector - ti->begin;

or even better
bvm->bi_sector = sc->stripe[stripe].physical_start +

			dm_target_offset(ti, bvm->bi_sector);

Quoted 13 lines

drivers/md/dm-stripe.c +4 -0

+	return min(max_size, q->merge_bvec_fn(q, bvm, biovec));
+}
+
  static struct target_type stripe_target = {
  	.name   = "striped",
  	.version = {1, 3, 1},

@@ -403,6 +426,7 @@
  	.ctr    = stripe_ctr,
  	.dtr    = stripe_dtr,
  	.map    = stripe_map,
+	.merge  = stripe_merge,
  	.end_io = stripe_end_io,

drivers/md/dm-stripe.c line 432

Quoted 5 lines

drivers/md/dm-stripe.c +0 -0

  	.status = stripe_status,
  	.iterate_devices = stripe_iterate_devices,

Reviewed-by Nikanth Karthikesan <[email protected]>

On Thu, Mar 17 2011 at 1:12am -0400,

Quoted 71 lines · Nikanth Karthikesan

Quoted 34 lines · Mustafa Mesanovic

Quoted 16 lines · Mustafa Mesanovic

Quoted 15 lines · On Mon December 27 2010 12:54:59 Neil Brown wrote:

On Mon, 27 Dec 2010 12:19:55 +0100 Mustafa Mesanovic

Quoted 12 lines · <[email protected]> wrote:

From: Mustafa Mesanovic<[email protected]>

A short explanation in prior: in this case we have "stacked" dm devices. Two multipathed luns combined together to one striped logical volume.

I/O throughput degradation happens at __bio_add_page when bio's get checked upon max_sectors. In this setup max_sectors is always set to 8 -> what is 4KiB.
A standalone striped logical volume on luns which are not multipathed do not have the problem: the logical volume will take over the max_sectors from luns below.

[...]

Quoted 14 lines

Quoted 13 lines

Quoted 2 lines

Using the patch improves read I/O up to 3x. In this specific case from 600MiB/s up to 1800MiB/s.

and using this patch will cause IO to fail sometimes. If an IO request which is larger than a page crosses a device boundary in the underlying e.g. RAID0, the RAID0 will return an error as such things should not happen - they are prevented by merge_bvec_fn.

If merge_bvec_fn is not being honoured, then you MUST limit requests to a single entry iovec of at most one page.

NeilBrown

Quoted 8 lines

Quoted 4 lines

Thank you for that hint, I will try to write a merge_bvec_fn for dm-stripe.c which solves the problem, if that is ok?

Mustafa Mesanovic

Now here my new suggestion to fix this issue, what is your opinion? I tested this with different setups, and it worked fine and I had very good performance improvements.

Some minor style nitpicks.

Quoted 25 lines

[RFC][PATCH] dm: improve read performance - v2

This patch adds a merge_fn for the dm stripe target. This merge_fn prevents dm_set_device_limits() setting the max_sectors to 4KiB (PAGE_SIZE). (As in a prior patch already mentioned.)
Now the read performance improved up to 3x higher compared to before.

What happened before:
I/O throughput degradation happened at __bio_add_page() when bio's got checked at the very beginning upon max_sectors. In this setup max_sectors is always set to 8. So bio's entered the dm target with a max of 4KiB.

Now dm-stripe target will have its own merge_fn so max_sectors will not pushed down to 8 (4KiB), and bio's can get bigger than 4KiB.

Signed-off-by Mustafa Mesanovic<[email protected]>

---

  dm-stripe.c |   24 ++++++++++++++++++++++++
  1 file changed, 24 insertions(+)

Index: linux-2.6/drivers/md/dm-stripe.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-stripe.c 2011-02-28

Quoted 60 lines

Quoted 14 lines

+0100 +++ linux-2.6/drivers/md/dm-stripe.c 2011-02-28
10:24:29.000000000 +0100 @@ -396,6 +396,29 @@

  	blk_limits_io_opt(limits, chunk_size * sc->stripes);
  
  }

+static int stripe_merge(struct dm_target *ti, struct bvec_merge_data
*bvm, + struct bio_vec *biovec, int max_size)

drivers/md/dm-stripe.c +5 -0

+{
+	struct stripe_c *sc = (struct stripe_c *) ti->private;
+	sector_t offset, chunk;
+	uint32_t stripe;
+	struct request_queue *q;

drivers/md/dm-stripe.c line 415

Quoted 10 lines

drivers/md/dm-stripe.c +10 -0

+
+	offset = bvm->bi_sector - ti->begin;
+	chunk = offset>>  sc->chunk_shift;
+	stripe = sector_div(chunk, sc->stripes);
+
+	if (!bdev_get_queue(sc->stripe[stripe].dev->bdev)->merge_bvec_fn)
+			return max_size;
+
+	bvm->bi_bdev = sc->stripe[stripe].dev->bdev;
+	q = bdev_get_queue(bvm->bi_bdev);

Initializing q at the top would simplify the check fro merge_bvec_fn above.

drivers/md/dm-stripe.c line 418

Quoted 3 lines

drivers/md/dm-stripe.c +3 -0

+	bvm->bi_sector = sc->stripe[stripe].physical_start +
+			(chunk<<  sc->chunk_shift) + (offset&  sc->chunk_mask);
+

Can this be written as

bvm->bi_sector = sc->stripe[stripe].physical_start +

			bvm->bi_sector - ti->begin;

or even better
bvm->bi_sector = sc->stripe[stripe].physical_start +

			dm_target_offset(ti, bvm->bi_sector);

Quoted 18 lines

drivers/md/dm-stripe.c +4 -0

+	return min(max_size, q->merge_bvec_fn(q, bvm, biovec));
+}
+
 
  static struct target_type stripe_target = {
 
  	.name   = "striped",
  	.version = {1, 3, 1},

@@ -403,6 +426,7 @@
 
  	.ctr    = stripe_ctr,
  	.dtr    = stripe_dtr,
  	.map    = stripe_map,
 
+	.merge  = stripe_merge,
 

Quoted 5 lines

Quoted 3 lines

.end_io = stripe_end_io,
.status = stripe_status, .iterate_devices = stripe_iterate_devices,

Reviewed-by Nikanth Karthikesan <[email protected]>

You reviewed an old version, v4 was posted to dm-devel and is available here: https://patchwork.kernel.org/patch/639801/

It should address all your concerns.