Skip to contentSkip to conversation

md: Subject: introduce get_priority_stripe() to improve raid456 write performance

PATCH In review NeilBrown 3 participants 5 messages Mar 28, 2008 – Mar 29, 2008 1 message not in the archive

Subsystems (2)
Documentation MD RAID
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): 20080328164351.30557.patches@notabene

NeilBrown

author
#1 lore raw

PATCH 1/1 md: Subject: introduce get_priority_stripe() to improve raid456 write performance

This patch is good for 2.6.26 (*not*) 2.6.25.
It substantially improves sequential write throughput for md/raid5.

Thanks,
NeilBrown

### Comments for Changeset

From: Dan Williams <[email protected]>

Improve write performance by preventing the delayed_list from dumping all its stripes onto the handle_list in one shot. Delayed stripes are now further delayed by being held on the 'hold_list'. The 'hold_list' is bypassed when:
* a STRIPE_IO_STARTED stripe is found at the head of 'handle_list'
* 'handle_list' is empty and i/o is being done to satisfy full stripe-width write requests
* 'bypass_count' is less than 'bypass_threshold'. By default the threshold is 1, i.e. every other stripe handled is a preread stripe provided the top two conditions are false.

Benchmark data:
System: 2x Xeon 5150, 4x SATA, mem=1GB
Baseline: 2.6.24-rc7
Configuration: mdadm --create /dev/md0 /dev/sd[b-e] -n 4 -l 5 --assume-clean
Test1: dd if=/dev/zero of=/dev/md0 bs=1024k count=2048
* patched: +33% (stripe_cache_size = 256), +25% (stripe_cache_size = 512)

Test2: tiobench --size 2048 --numruns 5 --block 4096 --block 131072 (XFS)
* patched: +13%
* patched + preread_bypass_threshold = 0: +37%

Changes since v1:
* reduce bypass_threshold from (chunk_size / sectors_per_chunk) to (1) and make it configurable. This defaults to fairness and modest performance gains out of the box.
Changes since v2:
* [[email protected]]: kill STRIPE_PRIO_HI and preread_needed as they are not necessary, the important change was clearing STRIPE_DELAYED in add_stripe_bio and this has been moved out to make_request for the hang fix.
* [[email protected]]: simplify get_priority_stripe
* [[email protected]]: reset the bypass_count when ->hold_list is sampled empty (+11%)
* [[email protected]]: decrement the bypass_count at the detection of stripes being naturally promoted off of hold_list +2%. Note, resetting bypass_count instead of decrementing on these events yields +4% but that is probably too aggressive.
Changes since v3:
* cosmetic fixups

Tested-by James W. Laferriere <[email protected]>
Signed-off-by Dan Williams <[email protected]>
Signed-off-by Neil Brown <[email protected]>

### Diffstat output

 ./Documentation/md.txt       |    6 ++
 ./drivers/md/raid5.c         |  122 +++++++++++++++++++++++++++++++++++++++----
 ./include/linux/raid/raid5.h |    7 ++
 3 files changed, 125 insertions(+), 10 deletions(-)
Documentation/md.txt +6 -0
diff .prev/Documentation/md.txt ./Documentation/md.txt
@@ -450,3 +450,9 @@ These currently include
       there are upper and lower limits (32768, 16).  Default is 128.
   strip_cache_active (currently raid5 only)
       number of active entries in the stripe cache
+  preread_bypass_threshold (currently raid5 only)
+      number of times a stripe requiring preread will be bypassed by
+      a stripe that does not require preread.  For fairness defaults
+      to 1.  Setting this to 0 disables bypass accounting and
+      requires preread stripes to wait until all full-width stripe-
+      writes are complete.  Valid values are 0 to stripe_cache_size.
drivers/md/raid5.c +112 -10

215 lines not on this page · raw message

include/linux/raid/raid5.h +7 -0
diff .prev/include/linux/raid/raid5.h ./include/linux/raid/raid5.h
@@ -252,6 +252,8 @@ struct r6_state {
 #define	STRIPE_EXPANDING	9
 #define	STRIPE_EXPAND_SOURCE	10
 #define	STRIPE_EXPAND_READY	11
+#define	STRIPE_IO_STARTED	12 /* do not count towards 'bypass_count' */
+#define	STRIPE_FULL_WRITE	13 /* all blocks are set to be overwritten */
 /*
  * Operations flags (in issue order)
  */

@@ -316,12 +318,17 @@ struct raid5_private_data {
 	int			previous_raid_disks;
 
 	struct list_head	handle_list; /* stripes needing handling */
+	struct list_head	hold_list; /* preread ready stripes */
 	struct list_head	delayed_list; /* stripes that have plugged requests */
 	struct list_head	bitmap_list; /* stripes delaying awaiting bitmap update */
 	struct bio		*retry_read_aligned; /* currently retrying aligned bios   */
 	struct bio		*retry_read_aligned_list; /* aligned bios retry list  */
 	atomic_t		preread_active_stripes; /* stripes with scheduled io */
 	atomic_t		active_aligned_reads;
+	atomic_t		pending_full_writes; /* full write backlog */
+	int			bypass_count; /* bypassed prereads */
+	int			bypass_threshold; /* preread nice */
+	struct list_head	*last_hold; /* detect hold_list promotions */
 
 	atomic_t		reshape_stripes; /* stripes with pending writes for reshape */
 	/* unfortunately we need two cache names as we temporarily have
#2 lore raw
Quoted 9 lines · On Fri, 28 Mar 2008 16:45:28 +1100 NeilBrown <[email protected]> wrote:
drivers/md/raid5.c +9 -0
+static ssize_t
+raid5_store_preread_threshold(mddev_t *mddev, const char *page, size_t len)
+{
+	raid5_conf_t *conf = mddev_to_conf(mddev);
+	char *end;
+	int new;
+	if (len >= PAGE_SIZE)
+		return -EINVAL;
+	if (!conf)

drivers/md/raid5.c raid5_stripecache_size = __ATTR(stripe_c · line 4097

Quoted 10 lines
drivers/md/raid5.c +10 -0
+		return -ENODEV;
+
+	new = simple_strtoul(page, &end, 10);
+	if (!*page || (*end && *end != '\n'))
+		return -EINVAL;
+	if (new > conf->max_nr_stripes || new < 0)
+		return -EINVAL;
+	conf->bypass_threshold = new;
+	return len;
+}

checkpatch 0.16 (which I misfiled and have thus far failed to merge up) sayeth:

WARNING: consider using strict_strtoul in preference to simple_strtoul #258: FILE: drivers/md/raid5.c:4090:
+ new = simple_strtoul(page, &end, 10);

the reason being that code which uses simple_strtoul() can treat "42-what-a-todo" as "42", which seems a bit sloppy.

Your code won't have that failing, because it explicitly checks that the input ended in \0 or \n. But strict_strtoul() internally does that, so this open-coded test could be removed.

#3 lore raw
Quoted 29 lines · Andrew Morton
Quoted 9 lines · On Fri, 28 Mar 2008 16:45:28 +1100 NeilBrown <[email protected]> wrote:
(quoted diff) +9 -0
+static ssize_t
+raid5_store_preread_threshold(mddev_t *mddev, const char *page, size_t len)
+{
+     raid5_conf_t *conf = mddev_to_conf(mddev);
+     char *end;
+     int new;
+     if (len >= PAGE_SIZE)
+             return -EINVAL;
+     if (!conf)

Review

Quoted 10 lines
(quoted diff) +10 -0
+             return -ENODEV;
+
+     new = simple_strtoul(page, &end, 10);
+     if (!*page || (*end && *end != '\n'))
+             return -EINVAL;
+     if (new > conf->max_nr_stripes || new < 0)
+             return -EINVAL;
+     conf->bypass_threshold = new;
+     return len;
+}

checkpatch 0.16 (which I misfiled and have thus far failed to merge up) sayeth:

WARNING: consider using strict_strtoul in preference to simple_strtoul #258: FILE: drivers/md/raid5.c:4090:
+ new = simple_strtoul(page, &end, 10);

Quoted 6 lines

the reason being that code which uses simple_strtoul() can treat "42-what-a-todo" as "42", which seems a bit sloppy.

Your code won't have that failing, because it explicitly checks that the input ended in \0 or \n. But strict_strtoul() internally does that, so this open-coded test could be removed.

How about the following:
-------snip------>

Subject: md: raid5.c convert simple_strtoul to strict_strtoul

From: Dan Williams <[email protected]>

strict_strtoul handles the open-coded sanity checks in raid5_store_stripe_cache_size and raid5_store_preread_threshold

Cc Neil Brown <[email protected]>
Signed-off-by Dan Williams <[email protected]>

---

 drivers/md/raid5.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)
drivers/md/raid5.c +5 -9
index bc39369..49f1265 100644
@@ -4034,15 +4034,13 @@ static ssize_t
 raid5_store_stripe_cache_size(mddev_t *mddev, const char *page, size_t len)
 {
 	raid5_conf_t *conf = mddev_to_conf(mddev);
-	char *end;
-	int new;
+	unsigned long new;
 	if (len >= PAGE_SIZE)
 		return -EINVAL;
 	if (!conf)
 		return -ENODEV;
 
-	new = simple_strtoul(page, &end, 10);
-	if (!*page || (*end && *end != '\n') )
+	if (strict_strtoul(page, 10, &new))
 		return -EINVAL;
 	if (new <= 16 || new > 32768)
 		return -EINVAL;

@@ -4080,17 +4078,15 @@ static ssize_t
 raid5_store_preread_threshold(mddev_t *mddev, const char *page, size_t len)
 {
 	raid5_conf_t *conf = mddev_to_conf(mddev);
-	char *end;
-	int new;
+	unsigned long new;
 	if (len >= PAGE_SIZE)
 		return -EINVAL;
 	if (!conf)
 		return -ENODEV;
 
-	new = simple_strtoul(page, &end, 10);
-	if (!*page || (*end && *end != '\n'))
+	if (strict_strtoul(page, 10, &new))
 		return -EINVAL;
-	if (new > conf->max_nr_stripes || new < 0)
+	if (new > conf->max_nr_stripes || (int) new < 0)
 		return -EINVAL;
 	conf->bypass_threshold = new;
 	return len;
author Acked-by
#4 lore raw
Quoted 8 lines · Dan Williams

raid5_store_preread_threshold(mddev_t *mddev, const char *page, size_t
len)
{

drivers/md/raid5.c +1 -2
 	raid5_conf_t *conf = mddev_to_conf(mddev);
-	char *end;
-	int new;
+	unsigned long new;
 	if (len >= PAGE_SIZE)

drivers/md/raid5.c static ssize_t · line 4089

Quoted 10 lines
drivers/md/raid5.c +2 -3
 		return -EINVAL;
 	if (!conf)
 		return -ENODEV;
 
-	new = simple_strtoul(page, &end, 10);
-	if (!*page || (*end && *end != '\n'))
+	if (strict_strtoul(page, 10, &new))
 		return -EINVAL;
-	if (new > conf->max_nr_stripes || new < 0)
+	if (new > conf->max_nr_stripes || (int) new < 0)

I had suggested that "new < 0" test when I saw that 'new' was an 'int'. A better suggestion would have been to make 'new' 'unsigned'. Now that you have done that, the "< 0" it pointless and should go.
Otherwise

Acked-by NeilBrown <[email protected]>

Thanks,
NeilBrown

Acked-by NeilBrown
#5 lore raw
Quoted 20 lines · NeilBrown
Quoted 8 lines · Dan Williams

raid5_store_preread_threshold(mddev_t *mddev, const char *page, size_t
len)

 {
      raid5_conf_t *conf = mddev_to_conf(mddev);
(quoted diff) +1 -2
-     char *end;
-     int new;
+     unsigned long new;
      if (len >= PAGE_SIZE)

Review

Quoted 10 lines
(quoted diff) +2 -3
              return -EINVAL;
      if (!conf)
              return -ENODEV;
 
-     new = simple_strtoul(page, &end, 10);
-     if (!*page || (*end && *end != '\n'))
+     if (strict_strtoul(page, 10, &new))
              return -EINVAL;
-     if (new > conf->max_nr_stripes || new < 0)
+     if (new > conf->max_nr_stripes || (int) new < 0)
Quoted 4 lines

I had suggested that "new < 0" test when I saw that 'new' was an 'int'. A better suggestion would have been to make 'new' 'unsigned'. Now that you have done that, the "< 0" it pointless and should go.

Yeah, ->max_nr_stripes will always be less than (1 << 63).

Quoted 2 lines

Otherwise

Acked-by NeilBrown <[email protected]>

Thanks, new patch follows,
Dan

------snip------>
Subject: md: raid5.c convert simple_strtoul to strict_strtoul

From: Dan Williams <[email protected]>

strict_strtoul handles the open-coded sanity checks in raid5_store_stripe_cache_size and raid5_store_preread_threshold

Acked-by NeilBrown <[email protected]>
Signed-off-by Dan Williams <[email protected]>

---

 drivers/md/raid5.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)
drivers/md/raid5.c +5 -9
index bc39369..e43e27a 100644
@@ -4034,15 +4034,13 @@ static ssize_t
 raid5_store_stripe_cache_size(mddev_t *mddev, const char *page, size_t len)
 {
 	raid5_conf_t *conf = mddev_to_conf(mddev);
-	char *end;
-	int new;
+	unsigned long new;
 	if (len >= PAGE_SIZE)
 		return -EINVAL;
 	if (!conf)
 		return -ENODEV;
 
-	new = simple_strtoul(page, &end, 10);
-	if (!*page || (*end && *end != '\n') )
+	if (strict_strtoul(page, 10, &new))
 		return -EINVAL;
 	if (new <= 16 || new > 32768)
 		return -EINVAL;

@@ -4080,17 +4078,15 @@ static ssize_t
 raid5_store_preread_threshold(mddev_t *mddev, const char *page, size_t len)
 {
 	raid5_conf_t *conf = mddev_to_conf(mddev);
-	char *end;
-	int new;
+	unsigned long new;
 	if (len >= PAGE_SIZE)
 		return -EINVAL;
 	if (!conf)
 		return -ENODEV;
 
-	new = simple_strtoul(page, &end, 10);
-	if (!*page || (*end && *end != '\n'))
+	if (strict_strtoul(page, 10, &new))
 		return -EINVAL;
-	if (new > conf->max_nr_stripes || new < 0)
+	if (new > conf->max_nr_stripes)
 		return -EINVAL;
 	conf->bypass_threshold = new;
 	return len;