raid5: improve write performance for fast storage

Add a per-stripe lock to protect stripe specific data, like dev->read, written, ... The purpose is to reduce lock contention of conf->device_lock.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/raid5.c |   17 +++++++++++++++++
 drivers/md/raid5.h |    1 +
 2 files changed, 18 insertions(+)

Index: linux/drivers/md/raid5.c
===================================================================

drivers/md/raid5.c +17 -0

@@ -749,6 +749,7 @@ static void ops_complete_biofill(void *s
 
 	/* clear completed biofills */
 	spin_lock_irq(&conf->device_lock);
+	spin_lock_irq(&sh->stripe_lock);
 	for (i = sh->disks; i--; ) {
 		struct r5dev *dev = &sh->dev[i];
 

@@ -774,6 +775,7 @@ static void ops_complete_biofill(void *s
 			}
 		}
 	}
+	spin_unlock_irq(&sh->stripe_lock);
 	spin_unlock_irq(&conf->device_lock);
 	clear_bit(STRIPE_BIOFILL_RUN, &sh->state);
 

@@ -798,8 +800,10 @@ static void ops_run_biofill(struct strip
 		if (test_bit(R5_Wantfill, &dev->flags)) {
 			struct bio *rbi;
 			spin_lock_irq(&conf->device_lock);
+			spin_lock_irq(&sh->stripe_lock);
 			dev->read = rbi = dev->toread;
 			dev->toread = NULL;
+			spin_unlock_irq(&sh->stripe_lock);
 			spin_unlock_irq(&conf->device_lock);
 			while (rbi && rbi->bi_sector <
 				dev->sector + STRIPE_SECTORS) {

@@ -1137,10 +1141,12 @@ ops_run_biodrain(struct stripe_head *sh,
 			struct bio *wbi;
 
 			spin_lock_irq(&sh->raid_conf->device_lock);
+			spin_lock_irq(&sh->stripe_lock);
 			chosen = dev->towrite;
 			dev->towrite = NULL;
 			BUG_ON(dev->written);
 			wbi = dev->written = chosen;
+			spin_unlock_irq(&sh->stripe_lock);
 			spin_unlock_irq(&sh->raid_conf->device_lock);
 
 			while (wbi && wbi->bi_sector <

@@ -1446,6 +1452,8 @@ static int grow_one_stripe(struct r5conf
 	init_waitqueue_head(&sh->ops.wait_for_ops);
 	#endif
 
+	spin_lock_init(&sh->stripe_lock);
+
 	if (grow_buffers(sh)) {
 		shrink_buffers(sh);
 		kmem_cache_free(conf->slab_cache, sh);

@@ -2327,6 +2335,7 @@ static int add_stripe_bio(struct stripe_
 
 
 	spin_lock_irq(&conf->device_lock);
+	spin_lock_irq(&sh->stripe_lock);
 	if (forwrite) {
 		bip = &sh->dev[dd_idx].towrite;
 		if (*bip == NULL && sh->dev[dd_idx].written == NULL)

@@ -2360,6 +2369,7 @@ static int add_stripe_bio(struct stripe_
 		if (sector >= sh->dev[dd_idx].sector + STRIPE_SECTORS)
 			set_bit(R5_OVERWRITE, &sh->dev[dd_idx].flags);
 	}
+	spin_unlock_irq(&sh->stripe_lock);
 	spin_unlock_irq(&conf->device_lock);
 
 	pr_debug("added bi b#%llu to stripe s#%llu, disk %d.\n",

@@ -2376,6 +2386,7 @@ static int add_stripe_bio(struct stripe_
 
  overlap:
 	set_bit(R5_Overlap, &sh->dev[dd_idx].flags);
+	spin_unlock_irq(&sh->stripe_lock);
 	spin_unlock_irq(&conf->device_lock);
 	return 0;
 }

@@ -2427,6 +2438,7 @@ handle_failed_stripe(struct r5conf *conf
 			}
 		}
 		spin_lock_irq(&conf->device_lock);
+		spin_lock_irq(&sh->stripe_lock);
 		/* fail all writes first */
 		bi = sh->dev[i].towrite;
 		sh->dev[i].towrite = NULL;

@@ -2488,6 +2500,7 @@ handle_failed_stripe(struct r5conf *conf
 				bi = nextbi;
 			}
 		}
+		spin_unlock_irq(&sh->stripe_lock);
 		spin_unlock_irq(&conf->device_lock);
 		if (bitmap_end)
 			bitmap_endwrite(conf->mddev->bitmap, sh->sector,

@@ -2695,6 +2708,7 @@ static void handle_stripe_clean_event(st
 				int bitmap_end = 0;
 				pr_debug("Return write for disc %d\n", i);
 				spin_lock_irq(&conf->device_lock);
+				spin_lock_irq(&sh->stripe_lock);
 				wbi = dev->written;
 				dev->written = NULL;
 				while (wbi && wbi->bi_sector <

@@ -2709,6 +2723,7 @@ static void handle_stripe_clean_event(st
 				}
 				if (dev->towrite == NULL)
 					bitmap_end = 1;
+				spin_unlock_irq(&sh->stripe_lock);
 				spin_unlock_irq(&conf->device_lock);
 				if (bitmap_end)
 					bitmap_endwrite(conf->mddev->bitmap,

@@ -3168,6 +3183,7 @@ static void analyse_stripe(struct stripe
 	/* Now to look around and see what can be done */
 	rcu_read_lock();
 	spin_lock_irq(&conf->device_lock);
+	spin_lock_irq(&sh->stripe_lock);
 	for (i=disks; i--; ) {
 		struct md_rdev *rdev;
 		sector_t first_bad;

@@ -3313,6 +3329,7 @@ static void analyse_stripe(struct stripe
 				do_recovery = 1;
 		}
 	}
+	spin_unlock_irq(&sh->stripe_lock);
 	spin_unlock_irq(&conf->device_lock);
 	if (test_bit(STRIPE_SYNCING, &sh->state)) {
 		/* If there is a failed device being replaced,

Index: linux/drivers/md/raid5.h
===================================================================

drivers/md/raid5.h +1 -0

@@ -210,6 +210,7 @@ struct stripe_head {
 	int			disks;		/* disks in stripe */
 	enum check_states	check_state;
 	enum reconstruct_states reconstruct_state;
+	spinlock_t		stripe_lock;
 	/**
 	 * struct stripe_operations
 	 * @target - STRIPE_OP_COMPUTE_BLK target

Quoted 2 lines · On Mon, 04 Jun 2012 16:01:53 +0800 Shaohua Li <[email protected]> wrote:

Add a per-stripe lock to protect stripe specific data, like dev->read, written, ... The purpose is to reduce lock contention of conf->device_lock.

I'm not convinced that you need to add a lock.
I am convinced that if you do add one you need to explain exactly what it is protecting.

The STRIPE_ACTIVE bit serves as a lock and ensures that only one process can be in handle_stripe at a time.
So I don't think dev->read actually needs any protection (though I haven't checked thoroughly).

I think the only things that device_lock protects are things shared by multiple stripes, so adding a per-stripe spinlock isn't going to help remove device_lock.

Quoted 14 lines · NeilBrown

Quoted 2 lines · On Mon, 04 Jun 2012 16:01:53 +0800 Shaohua Li <[email protected]> wrote:

Add a per-stripe lock to protect stripe specific data, like dev->read, written, ... The purpose is to reduce lock contention of conf->device_lock.

I'm not convinced that you need to add a lock.
I am convinced that if you do add one you need to explain exactly what it is protecting.

The STRIPE_ACTIVE bit serves as a lock and ensures that only one process can be in handle_stripe at a time.
So I don't think dev->read actually needs any protection (though I haven't checked thoroughly).

Quoted 3 lines

I think the only things that device_lock protects are things shared by multiple stripes, so adding a per-stripe spinlock isn't going to help remove device_lock.

This sounds not true to me. both the async callbacks and request completion access stripe data, like dev->read. Such things are not protected by STRIPE_ACTIVE bit. Thought we can delete STRIPE_ACTIVE bit with stripe lock introduced.

Quoted 19 lines · On Thu, 7 Jun 2012 14:29:39 +0800 Shaohua Li <[email protected]> wrote:

Quoted 17 lines · NeilBrown

Quoted 2 lines · On Mon, 04 Jun 2012 16:01:53 +0800 Shaohua Li <[email protected]> wrote:

Add a per-stripe lock to protect stripe specific data, like dev->read, written, ... The purpose is to reduce lock contention of conf->device_lock.

I'm not convinced that you need to add a lock.
I am convinced that if you do add one you need to explain exactly what it is protecting.

The STRIPE_ACTIVE bit serves as a lock and ensures that only one process can be in handle_stripe at a time.
So I don't think dev->read actually needs any protection (though I haven't checked thoroughly).

I think the only things that device_lock protects are things shared by multiple stripes, so adding a per-stripe spinlock isn't going to help remove device_lock.

Quoted 4 lines

This sounds not true to me. both the async callbacks and request completion access stripe data, like dev->read. Such things are not protected by STRIPE_ACTIVE bit. Thought we can delete STRIPE_ACTIVE bit with stripe lock introduced.

Please give specifics. What race do you see with access to dev->read that is not protected by STRIPE_ACTIVE ?

Quoted 26 lines · NeilBrown

Quoted 23 lines · On Thu, 7 Jun 2012 14:29:39 +0800 Shaohua Li <[email protected]> wrote:

Quoted 9 lines · NeilBrown

Quoted 2 lines · On Mon, 04 Jun 2012 16:01:53 +0800 Shaohua Li <[email protected]> wrote:

Add a per-stripe lock to protect stripe specific data, like dev->read, written, ... The purpose is to reduce lock contention of conf->device_lock.

I'm not convinced that you need to add a lock.
I am convinced that if you do add one you need to explain exactly what it is protecting.

Quoted 8 lines

The STRIPE_ACTIVE bit serves as a lock and ensures that only one process can be in handle_stripe at a time.
So I don't think dev->read actually needs any protection (though I haven't checked thoroughly).

I think the only things that device_lock protects are things shared by multiple stripes, so adding a per-stripe spinlock isn't going to help remove device_lock.

This sounds not true to me. both the async callbacks and request completion access stripe data, like dev->read. Such things are not protected by STRIPE_ACTIVE bit. Thought we can delete STRIPE_ACTIVE bit with stripe lock introduced.

Quoted 2 lines

Please give specifics.  What race do you see with access to dev->read that is not protected by STRIPE_ACTIVE ?

For example, ops_complete_biofill() will change dev->read which isn't protected by STRIPE_ACTIVE. add_stripe_bio() checks ->toread ->towrite, which isn't protected by the bit too. Am I missing anything?

Quoted 27 lines · Shaohua Li

Quoted 26 lines · NeilBrown

Quoted 23 lines · On Thu, 7 Jun 2012 14:29:39 +0800 Shaohua Li <[email protected]> wrote:

Quoted 9 lines · NeilBrown

Quoted 2 lines · On Mon, 04 Jun 2012 16:01:53 +0800 Shaohua Li <[email protected]> wrote:

Add a per-stripe lock to protect stripe specific data, like dev->read, written, ... The purpose is to reduce lock contention of conf->device_lock.

I'm not convinced that you need to add a lock.
I am convinced that if you do add one you need to explain exactly what it is protecting.

Quoted 8 lines

The STRIPE_ACTIVE bit serves as a lock and ensures that only one process can be in handle_stripe at a time.
So I don't think dev->read actually needs any protection (though I haven't checked thoroughly).

I think the only things that device_lock protects are things shared by multiple stripes, so adding a per-stripe spinlock isn't going to help remove device_lock.

This sounds not true to me. both the async callbacks and request completion access stripe data, like dev->read. Such things are not protected by STRIPE_ACTIVE bit. Thought we can delete STRIPE_ACTIVE bit with stripe lock introduced.

Quoted 6 lines

Quoted 2 lines

Please give specifics.  What race do you see with access to dev->read that is not protected by STRIPE_ACTIVE ?

For example, ops_complete_biofill() will change dev->read which isn't protected by STRIPE_ACTIVE. add_stripe_bio() checks ->toread ->towrite, which isn't protected by the bit too. Am I missing anything?

STRIPE_ACTIVE is the replacement for the old per-stripe lock.  That lock never was meant/able to synchronize add_stripe_bio() vs ops_run_* (producer vs consumer).  That's always been device_lock's job because an individual bio may be added to several stripes.  If device_lock is gone we need a different scheme.  That's what tripped me up last time I looked at this.

Quoted 35 lines · Shaohua Li

Quoted 27 lines · NeilBrown

Quoted 26 lines · On Thu, 7 Jun 2012 14:29:39 +0800 Shaohua Li <[email protected]> wrote:

Quoted 24 lines · NeilBrown

Quoted 9 lines · On Mon, 04 Jun 2012 16:01:53 +0800 Shaohua Li <[email protected]> wrote:

Quoted 3 lines

Add a per-stripe lock to protect stripe specific data, like dev->read, written, ... The purpose is to reduce lock contention of conf->device_lock.

I'm not convinced that you need to add a lock.
I am convinced that if you do add one you need to explain exactly what it is protecting.

Quoted 8 lines

The STRIPE_ACTIVE bit serves as a lock and ensures that only one process can be in handle_stripe at a time.
So I don't think dev->read actually needs any protection (though I haven't checked thoroughly).

I think the only things that device_lock protects are things shared by multiple stripes, so adding a per-stripe spinlock isn't going to help remove device_lock.

This sounds not true to me. both the async callbacks and request completion access stripe data, like dev->read. Such things are not protected by STRIPE_ACTIVE bit. Thought we can delete STRIPE_ACTIVE bit with stripe lock introduced.

Quoted 6 lines

Quoted 2 lines

Please give specifics.  What race do you see with access to dev->read that is not protected by STRIPE_ACTIVE ?

For example, ops_complete_biofill() will change dev->read which isn't protected by STRIPE_ACTIVE. add_stripe_bio() checks ->toread ->towrite, which isn't protected by the bit too. Am I missing anything?

Quoted 6 lines

STRIPE_ACTIVE is the replacement for the old per-stripe lock.  That lock never was meant/able to synchronize add_stripe_bio() vs ops_run_* (producer vs consumer).  That's always been device_lock's job because an individual bio may be added to several stripes.  If device_lock is gone we need a different scheme.  That's what tripped me up last time I looked at this.

Actually now that I look at add_stripe_bio again, I think it could be made to work if:

1/ bi_phys_segments is incremented prior to publishing the bio on to{read|write} otherwise we potentially race with a consumer without a reference

2/ making sure the overlap checking does not walk off into invalid bios as it may do once we no longer have a global lock

Add a per-stripe lock to protect stripe specific data, like dev->read, written, ... The purpose is to reduce lock contention of conf->device_lock.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/raid5.c |   17 +++++++++++++++++
 drivers/md/raid5.h |    1 +

 2 files changed, 18 insertions(+)

Index: linux/drivers/md/raid5.c
===================================================================

drivers/md/raid5.c    2012-06-01 13:43:05.594056130 +0800 renamed from drivers/md/raid5.c       2012-06-01 13:38:54.705210229 +0800 +1 -0

@@ -749,6 +749,7 @@ static void ops_complete_biofill(void *s
 
        /* clear completed biofills */
        spin_lock_irq(&conf->device_lock);
+       spin_lock_irq(&sh->stripe_lock);
        for (i = sh->disks; i--; ) {
                struct r5dev *dev = &sh->dev[i];
 

Raid5 overrides bio->bi_phys_segments, accessing it is with device_lock hold, which is unnecessary, We can make it lockless actually.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/raid5.c |   37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

Index: linux/drivers/md/raid5.c
===================================================================

drivers/md/raid5.c +24 -13

@@ -101,32 +101,43 @@ static inline struct bio *r5_next_bio(st
  */
 static inline int raid5_bi_phys_segments(struct bio *bio)
 {
-	return bio->bi_phys_segments & 0xffff;
+	atomic_t *segments = (atomic_t *)&bio->bi_phys_segments;
+	return atomic_read(segments) & 0xffff;
 }
 
 static inline int raid5_bi_hw_segments(struct bio *bio)
 {
-	return (bio->bi_phys_segments >> 16) & 0xffff;
+	atomic_t *segments = (atomic_t *)&bio->bi_phys_segments;
+	return (atomic_read(segments) >> 16) & 0xffff;
 }
 
 static inline int raid5_dec_bi_phys_segments(struct bio *bio)
 {
-	--bio->bi_phys_segments;
-	return raid5_bi_phys_segments(bio);
+	atomic_t *segments = (atomic_t *)&bio->bi_phys_segments;
+	return atomic_sub_return(1, segments) & 0xffff;
 }
 
-static inline int raid5_dec_bi_hw_segments(struct bio *bio)
+static inline void raid5_inc_bi_phys_segments(struct bio *bio)
 {
-	unsigned short val = raid5_bi_hw_segments(bio);
-
-	--val;
-	bio->bi_phys_segments = (val << 16) | raid5_bi_phys_segments(bio);
-	return val;
+	atomic_t *segments = (atomic_t *)&bio->bi_phys_segments;
+	atomic_inc(segments);
 }
 
 static inline void raid5_set_bi_hw_segments(struct bio *bio, unsigned int cnt)
 {
-	bio->bi_phys_segments = raid5_bi_phys_segments(bio) | (cnt << 16);
+	atomic_t *segments = (atomic_t *)&bio->bi_phys_segments;
+	int old, new;
+
+	do {
+		old = atomic_read(segments);
+		new = (old & 0xffff) | (cnt << 16);
+	} while (atomic_cmpxchg(segments, old, new) != old);
+}
+
+static inline void raid5_set_bi_segments(struct bio *bio, unsigned int cnt)
+{
+	atomic_t *segments = (atomic_t *)&bio->bi_phys_segments;
+	atomic_set(segments, cnt);
 }
 
 /* Find first data disk in a raid6 stripe */

@@ -2354,7 +2365,7 @@ static int add_stripe_bio(struct stripe_
 	if (*bip)
 		bi->bi_next = *bip;
 	*bip = bi;
-	bi->bi_phys_segments++;
+	raid5_inc_bi_phys_segments(bi);
 
 	if (forwrite) {
 		/* check if page is covered */

@@ -3783,7 +3794,7 @@ static struct bio *remove_bio_from_retry
 		 * this sets the active strip count to 1 and the processed
 		 * strip count to zero (upper 8 bits)
 		 */
-		bi->bi_phys_segments = 1; /* biased count of active stripes */
+		raid5_set_bi_segments(bi, 1); /* biased count of active stripes */
 	}
 
 	return bi;

Quoted 3 lines · On Mon, 04 Jun 2012 16:01:54 +0800 Shaohua Li <[email protected]> wrote:

Raid5 overrides bio->bi_phys_segments, accessing it is with device_lock hold, which is unnecessary, We can make it lockless actually.

Quoted 1 line

Signed-off-by Shaohua Li <[email protected]>

I cannot say that I like this (casting fields in the bio structure), but I can see the value and it should work.  'atomic_t' is currently always the same size as an 'int', and I doubt that will change.

So maybe I'll get used to the idea.

release_stripe() is a place conf->device_lock is heavily contended. We take the lock even stripe count isn't 1, which isn't required. On the on the other hand, decreasing count first and taking lock if count is 0 can expose races:
1. bewteen dec count and taking lock, another thread hits the stripe in cache, so increase count. The stripe will be deleted from any list. In this case stripe count isn't 0.
2. between dec count and taking lock, another thread hits the stripe in cache and release it. In this case the stripe is already in specific list. We do list_move to adjust its position.
So both cases are fixable to me.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/raid5.c |   43 +++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)

Index: linux/drivers/md/raid5.c
===================================================================

drivers/md/raid5.c +29 -14

@@ -201,20 +201,39 @@ static int stripe_operations_active(stru
 	       test_bit(STRIPE_COMPUTE_RUN, &sh->state);
 }
 
-static void __release_stripe(struct r5conf *conf, struct stripe_head *sh)
+static void __release_stripe(struct r5conf *conf, struct stripe_head *sh,
+	int locking)
 {
+	unsigned long uninitialized_var(flags);
+
 	if (atomic_dec_and_test(&sh->count)) {
-		BUG_ON(!list_empty(&sh->lru));
+		/*
+		 * Before we hold device_lock, other thread can hit this stripe
+		 * in cache. It could do:
+		 * 1. just get_active_stripe(). The stripe count isn't 0 then.
+		 * 2. do get_active_stripe() and follow release_stripe(). So the
+		 * stripe might be already released and already in specific
+		 * list. we do list_move to adjust its position in the list.
+		 */
+		if (locking) {
+			spin_lock_irqsave(&conf->device_lock, flags);
+			if (atomic_read(&sh->count) != 0) {
+				spin_unlock_irqrestore(&conf->device_lock,
+							flags);
+				return;
+			}
+		}
+
 		BUG_ON(atomic_read(&conf->active_stripes)==0);
 		if (test_bit(STRIPE_HANDLE, &sh->state)) {
 			if (test_bit(STRIPE_DELAYED, &sh->state))
-				list_add_tail(&sh->lru, &conf->delayed_list);
+				list_move_tail(&sh->lru, &conf->delayed_list);
 			else if (test_bit(STRIPE_BIT_DELAY, &sh->state) &&
 				   sh->bm_seq - conf->seq_write > 0)
-				list_add_tail(&sh->lru, &conf->bitmap_list);
+				list_move_tail(&sh->lru, &conf->bitmap_list);
 			else {
 				clear_bit(STRIPE_BIT_DELAY, &sh->state);
-				list_add_tail(&sh->lru, &conf->handle_list);
+				list_move_tail(&sh->lru, &conf->handle_list);
 			}
 			md_wakeup_thread(conf->mddev->thread);
 		} else {

@@ -225,23 +244,22 @@ static void __release_stripe(struct r5co
 					md_wakeup_thread(conf->mddev->thread);
 			atomic_dec(&conf->active_stripes);
 			if (!test_bit(STRIPE_EXPANDING, &sh->state)) {
-				list_add_tail(&sh->lru, &conf->inactive_list);
+				list_move_tail(&sh->lru, &conf->inactive_list);
 				wake_up(&conf->wait_for_stripe);
 				if (conf->retry_read_aligned)
 					md_wakeup_thread(conf->mddev->thread);
 			}
 		}
+		if (locking)
+			spin_unlock_irqrestore(&conf->device_lock, flags);
 	}
 }
 
 static void release_stripe(struct stripe_head *sh)
 {
 	struct r5conf *conf = sh->raid_conf;
-	unsigned long flags;
 
-	spin_lock_irqsave(&conf->device_lock, flags);
-	__release_stripe(conf, sh);
-	spin_unlock_irqrestore(&conf->device_lock, flags);
+	__release_stripe(conf, sh, 1);
 }
 
 static inline void remove_hash(struct stripe_head *sh)

@@ -484,9 +502,6 @@ get_active_stripe(struct r5conf *conf, s
 			} else {
 				if (!test_bit(STRIPE_HANDLE, &sh->state))
 					atomic_inc(&conf->active_stripes);
-				if (list_empty(&sh->lru) &&
-				    !test_bit(STRIPE_EXPANDING, &sh->state))
-					BUG();
 				list_del_init(&sh->lru);
 			}
 		}

@@ -3672,7 +3687,7 @@ static void activate_bit_delay(struct r5
 		struct stripe_head *sh = list_entry(head.next, struct stripe_head, lru);
 		list_del_init(&sh->lru);
 		atomic_inc(&sh->count);
-		__release_stripe(conf, sh);
+		__release_stripe(conf, sh, 0);
 	}
 }
 

make_request() does stripe release for every stripe and the stripe usually has count 1, which makes previous release_stripe() optimization not work. In my test, this release_stripe() becomes the heaviest pleace to take conf->device_lock after previous patches applied.

Below patch makes stripe release batch. When maxium strips of a batch reach, the batch will be flushed out. Another way to do the flush is when unplug is called.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/raid5.c |   42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

Index: linux/drivers/md/raid5.c
===================================================================

drivers/md/raid5.c +41 -1

@@ -4023,6 +4023,38 @@ static struct stripe_head *__get_priorit
 	return sh;
 }
 
+struct raid5_plug {
+	struct blk_plug_cb cb;
+	struct stripe_head_batch batch;
+};
+static DEFINE_PER_CPU(struct raid5_plug, raid5_plugs);
+
+static void raid5_do_plug(struct blk_plug_cb *cb)
+{
+	struct raid5_plug *plug = container_of(cb, struct raid5_plug, cb);
+
+	release_stripe_flush_batch(&plug->batch);
+	INIT_LIST_HEAD(&plug->cb.list);
+}
+
+static void release_stripe_plug(struct stripe_head *sh)
+{
+	struct blk_plug *plug = current->plug;
+	struct raid5_plug *raid5_plug;
+
+	if (!plug) {
+		release_stripe(sh);
+		return;
+	}
+	preempt_disable();
+	raid5_plug = &__raw_get_cpu_var(raid5_plugs);
+	release_stripe_add_batch(&raid5_plug->batch, sh);
+
+	if (list_empty(&raid5_plug->cb.list))
+		list_add(&raid5_plug->cb.list, &plug->cb_list);
+	preempt_enable();
+}
+
 static void make_request(struct mddev *mddev, struct bio * bi)
 {
 	struct r5conf *conf = mddev->private;

@@ -4153,7 +4185,7 @@ static void make_request(struct mddev *m
 			if ((bi->bi_rw & REQ_SYNC) &&
 			    !test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
 				atomic_inc(&conf->preread_active_stripes);
-			release_stripe(sh);
+			release_stripe_plug(sh);
 		} else {
 			/* cannot get stripe for read-ahead, just give-up */
 			clear_bit(BIO_UPTODATE, &bi->bi_flags);

@@ -6170,6 +6202,14 @@ static struct md_personality raid4_perso
 
 static int __init raid5_init(void)
 {
+	int i;
+
+	for_each_present_cpu(i) {
+		struct raid5_plug *plug = &per_cpu(raid5_plugs, i);
+		plug->batch.count = 0;
+		INIT_LIST_HEAD(&plug->cb.list);
+		plug->cb.callback = raid5_do_plug;
+	}
 	register_md_personality(&raid6_personality);
 	register_md_personality(&raid5_personality);
 	register_md_personality(&raid4_personality);

Quoted 10 lines · On Mon, 04 Jun 2012 16:01:58 +0800 Shaohua Li <[email protected]> wrote:

make_request() does stripe release for every stripe and the stripe usually has count 1, which makes previous release_stripe() optimization not work. In my test, this release_stripe() becomes the heaviest pleace to take conf->device_lock after previous patches applied.

Below patch makes stripe release batch. When maxium strips of a batch reach, the batch will be flushed out. Another way to do the flush is when unplug is called.

Signed-off-by Shaohua Li <[email protected]>

I like the idea of a batched release.
I don't like the per-cpu variables... and I don't think it is safe to only allocate them for_each_present_cpu without support cpu-hot-plug.

I would much rather keep a list of stripes (linked on ->lru) in struct md_plug_cb (or maybe in some structure which contains that) and release them all on unplug - and only on unplug.

Maybe pass a size to mddev_check_unplugged, and it allocates that much more space. Get mddev_check_unplugged to return the md_plug_cb structure. If the new space is NULL, then list_head_init it, and change the cb.callback to a raid5 specific function.
Then add any stripe to the md_plug_cb, and in the unplug function, release them all.

Does that make sense?

Also I would rather the batched stripe release code were defined in the same patch that used it. It isn't big enough to justify a separate patch.

Quoted 28 lines · NeilBrown

Quoted 10 lines · On Mon, 04 Jun 2012 16:01:58 +0800 Shaohua Li <[email protected]> wrote:

make_request() does stripe release for every stripe and the stripe usually has count 1, which makes previous release_stripe() optimization not work. In my test, this release_stripe() becomes the heaviest pleace to take conf->device_lock after previous patches applied.

Below patch makes stripe release batch. When maxium strips of a batch reach, the batch will be flushed out. Another way to do the flush is when unplug is called.

Signed-off-by Shaohua Li <[email protected]>

I like the idea of a batched release.
I don't like the per-cpu variables... and I don't think it is safe to only allocate them for_each_present_cpu without support cpu-hot-plug.

I would much rather keep a list of stripes (linked on ->lru) in struct md_plug_cb (or maybe in some structure which contains that) and release them all on unplug - and only on unplug.

Maybe pass a size to mddev_check_unplugged, and it allocates that much more space. Get mddev_check_unplugged to return the md_plug_cb structure. If the new space is NULL, then list_head_init it, and change the cb.callback to a raid5 specific function.
Then add any stripe to the md_plug_cb, and in the unplug function, release them all.

Quoted 4 lines

Does that make sense?

Also I would rather the batched stripe release code were defined in the same patch that used it. It isn't big enough to justify a separate patch.

The stripe->lru need protection of device_lock, so I can't use a list. An array is preferred. I really didn't like the idea to allocate memory especially when allocating an array. I'll fix the code for cpuhotplug.

Quoted 29 lines · On Thu, 7 Jun 2012 14:33:58 +0800 Shaohua Li <[email protected]> wrote:

Quoted 28 lines · NeilBrown

Quoted 10 lines · On Mon, 04 Jun 2012 16:01:58 +0800 Shaohua Li <[email protected]> wrote:

make_request() does stripe release for every stripe and the stripe usually has count 1, which makes previous release_stripe() optimization not work. In my test, this release_stripe() becomes the heaviest pleace to take conf->device_lock after previous patches applied.

Below patch makes stripe release batch. When maxium strips of a batch reach, the batch will be flushed out. Another way to do the flush is when unplug is called.

Signed-off-by Shaohua Li <[email protected]>

I like the idea of a batched release.
I don't like the per-cpu variables... and I don't think it is safe to only allocate them for_each_present_cpu without support cpu-hot-plug.

I would much rather keep a list of stripes (linked on ->lru) in struct md_plug_cb (or maybe in some structure which contains that) and release them all on unplug - and only on unplug.

Maybe pass a size to mddev_check_unplugged, and it allocates that much more space. Get mddev_check_unplugged to return the md_plug_cb structure. If the new space is NULL, then list_head_init it, and change the cb.callback to a raid5 specific function.
Then add any stripe to the md_plug_cb, and in the unplug function, release them all.

Quoted 8 lines

Quoted 4 lines

Does that make sense?

Also I would rather the batched stripe release code were defined in the same patch that used it. It isn't big enough to justify a separate patch.

The stripe->lru need protection of device_lock, so I can't use a list. An array is preferred. I really didn't like the idea to allocate memory especially when allocating an array. I'll fix the code for cpuhotplug.

You don't need device_lock to use ->lru.
Currently the lru is not used when sh->count is not-zero unless STRIPE_EXPANDING is set - and we never attach IO requests if STRIPE_EXPANDING is set.
So when make_request wants to release a stripe_head, ->lru is currently unused.
So we can use it to put the stripe on a per-thread list without locking.

We need another stripe_head flag to say "is on a per-thread unplug list" to avoid racing between processes, but we don't need a spinlock for that. ie.
if (!test_and_set(STRIPE_ON_UNPLUG_LIST, &sh->state))

           list_add(&plug->list, &sh->lru);

or similar.

Quoted 54 lines · NeilBrown

Quoted 29 lines · On Thu, 7 Jun 2012 14:33:58 +0800 Shaohua Li <[email protected]> wrote:

Quoted 28 lines · NeilBrown

Quoted 10 lines · On Mon, 04 Jun 2012 16:01:58 +0800 Shaohua Li <[email protected]> wrote:

make_request() does stripe release for every stripe and the stripe usually has count 1, which makes previous release_stripe() optimization not work. In my test, this release_stripe() becomes the heaviest pleace to take conf->device_lock after previous patches applied.

Below patch makes stripe release batch. When maxium strips of a batch reach, the batch will be flushed out. Another way to do the flush is when unplug is called.

Signed-off-by Shaohua Li <[email protected]>

I like the idea of a batched release.
I don't like the per-cpu variables... and I don't think it is safe to only allocate them for_each_present_cpu without support cpu-hot-plug.

I would much rather keep a list of stripes (linked on ->lru) in struct md_plug_cb (or maybe in some structure which contains that) and release them all on unplug - and only on unplug.

Maybe pass a size to mddev_check_unplugged, and it allocates that much more space. Get mddev_check_unplugged to return the md_plug_cb structure. If the new space is NULL, then list_head_init it, and change the cb.callback to a raid5 specific function.
Then add any stripe to the md_plug_cb, and in the unplug function, release them all.

Quoted 8 lines

Quoted 4 lines

Does that make sense?

Also I would rather the batched stripe release code were defined in the same patch that used it. It isn't big enough to justify a separate patch.

The stripe->lru need protection of device_lock, so I can't use a list. An array is preferred. I really didn't like the idea to allocate memory especially when allocating an array. I'll fix the code for cpuhotplug.

You don't need device_lock to use ->lru.
Currently the lru is not used when sh->count is not-zero unless STRIPE_EXPANDING is set - and we never attach IO requests if STRIPE_EXPANDING is set.
So when make_request wants to release a stripe_head, ->lru is currently unused.
So we can use it to put the stripe on a per-thread list without locking.

We need another stripe_head flag to say "is on a per-thread unplug list" to avoid racing between processes, but we don't need a spinlock for that. ie.
if (!test_and_set(STRIPE_ON_UNPLUG_LIST, &sh->state))

           list_add(&plug->list, &sh->lru);

Quoted 1 line

or similar.

I did see some BUG_ON trigger when I access ->lru without device_lock hold before, for example get_active_stripe will remove it from list. Maybe can use the same bit to avoid it. Let me try.

Quoted 55 lines · Shaohua Li

Quoted 54 lines · NeilBrown

Quoted 29 lines · On Thu, 7 Jun 2012 14:33:58 +0800 Shaohua Li <[email protected]> wrote:

Quoted 28 lines · NeilBrown

Quoted 10 lines · On Mon, 04 Jun 2012 16:01:58 +0800 Shaohua Li <[email protected]> wrote:

make_request() does stripe release for every stripe and the stripe usually has count 1, which makes previous release_stripe() optimization not work. In my test, this release_stripe() becomes the heaviest pleace to take conf->device_lock after previous patches applied.

Below patch makes stripe release batch. When maxium strips of a batch reach, the batch will be flushed out. Another way to do the flush is when unplug is called.

Signed-off-by Shaohua Li <[email protected]>

I like the idea of a batched release.
I don't like the per-cpu variables... and I don't think it is safe to only allocate them for_each_present_cpu without support cpu-hot-plug.

I would much rather keep a list of stripes (linked on ->lru) in struct md_plug_cb (or maybe in some structure which contains that) and release them all on unplug - and only on unplug.

Maybe pass a size to mddev_check_unplugged, and it allocates that much more space. Get mddev_check_unplugged to return the md_plug_cb structure. If the new space is NULL, then list_head_init it, and change the cb.callback to a raid5 specific function.
Then add any stripe to the md_plug_cb, and in the unplug function, release them all.

Quoted 8 lines

Quoted 4 lines

Does that make sense?

Also I would rather the batched stripe release code were defined in the same patch that used it. It isn't big enough to justify a separate patch.

The stripe->lru need protection of device_lock, so I can't use a list. An array is preferred. I really didn't like the idea to allocate memory especially when allocating an array. I'll fix the code for cpuhotplug.

You don't need device_lock to use ->lru.
Currently the lru is not used when sh->count is not-zero unless STRIPE_EXPANDING is set - and we never attach IO requests if STRIPE_EXPANDING is set.
So when make_request wants to release a stripe_head, ->lru is currently unused.
So we can use it to put the stripe on a per-thread list without locking.

We need another stripe_head flag to say "is on a per-thread unplug list" to avoid racing between processes, but we don't need a spinlock for that. ie.
if (!test_and_set(STRIPE_ON_UNPLUG_LIST, &sh->state))

           list_add(&plug->list, &sh->lru);

Quoted 5 lines

Quoted 1 line

or similar.

I did see some BUG_ON trigger when I access ->lru without device_lock hold before, for example get_active_stripe will remove it from list. Maybe can use the same bit to avoid it. Let me try.

Thinking a bit more, the STRIPE_ON_UNPLUG_LIST bit can't avoid races. For example,
Task 1 hit a stripe, assume stripe count 0 (could not be 0 too):
it does:
1. inc count
2. set STRIPE_ON_UNPLUG_LIST
3. add stripe to plug list
4. unplug to release the stripe
Between 3 and 4, task 2 hit the stripe, it does:
A: inc count. Since the bit set, do nothing more
B: unplug
If the order is 3, A, 4, B. Task1 will not release the stripe, since the count is 2. Tasks2 will not release the stripe, since stripe isn't in its list. The stripe will never be handled.

Let raid5d handle stripe in batch way to reduce conf->device_lock locking.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/raid5.c |   35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

Index: linux/drivers/md/raid5.c
===================================================================

drivers/md/raid5.c +26 -9

@@ -4585,6 +4585,22 @@ static int  retry_aligned_read(struct r5
 	return handled;
 }
 
+static int __get_stripe_batch(struct r5conf *conf,
+		struct stripe_head_batch *batch)
+{
+	struct stripe_head *sh;
+
+	batch->count = 0;
+	do {
+		sh = __get_priority_stripe(conf);
+		if (sh) {
+			batch->stripes[batch->count] = sh;
+			batch->count++;
+		}
+	} while (sh && batch->count < MAX_STRIPE_BATCH);
+
+	return batch->count;
+}
 
 /*
  * This is our raid5 kernel thread.

@@ -4595,10 +4611,10 @@ static int  retry_aligned_read(struct r5
  */
 static void raid5d(struct mddev *mddev)
 {
-	struct stripe_head *sh;
 	struct r5conf *conf = mddev->private;
-	int handled;
+	int handled, i;
 	struct blk_plug plug;
+	struct stripe_head_batch batch;
 
 	pr_debug("+++ raid5d active\n");
 

@@ -4633,15 +4649,16 @@ static void raid5d(struct mddev *mddev)
 			handled++;
 		}
 
-		sh = __get_priority_stripe(conf);
-
-		if (!sh)
+		if (!__get_stripe_batch(conf, &batch))
 			break;
 		spin_unlock_irq(&conf->device_lock);
-		
-		handled++;
-		handle_stripe(sh);
-		release_stripe(sh);
+
+		for (i = 0; i < batch.count; i++) {
+			handled++;
+			handle_stripe(batch.stripes[i]);
+		}
+
+		release_stripe_flush_batch(&batch);
 		cond_resched();
 
 		if (mddev->flags & ~(1<<MD_CHANGE_PENDING))

Quoted 3 lines · On Mon, 04 Jun 2012 16:01:59 +0800 Shaohua Li <[email protected]> wrote:

Let raid5d handle stripe in batch way to reduce conf->device_lock locking.

Signed-off-by Shaohua Li <[email protected]>

I like this.
I don't think it justifies a separate function.

#define MAX_STRIPE_BATCH 8
struct stripe_head *batch[MAX_STRIPE_BATCH]
int batch_size = 0;

...

while (batch_size < MAX_STRPE_BATCH &&

       (sh = __get_priority_stripe(conf)) != NULL)
     batch[batch_size++] = sh;

spin_unlock(&conf->device_lock);
if (batch_size == 0)

     break;

handled += batch_size;

for (i = 0; i < batch_size; i++)

     handle_stripe(batch[i]);

cond_resched();
if (....) md_check_recovery(mddev);

spin_lock_irq(&conf->lock);
for (i = 0; i < batch_size; i++)

     __release_stripe(batch[i]);

something like that?

Quoted 30 lines · NeilBrown

Quoted 3 lines · On Mon, 04 Jun 2012 16:01:59 +0800 Shaohua Li <[email protected]> wrote:

Let raid5d handle stripe in batch way to reduce conf->device_lock locking.

Signed-off-by Shaohua Li <[email protected]>

I like this.
I don't think it justifies a separate function.

#define MAX_STRIPE_BATCH 8
struct stripe_head *batch[MAX_STRIPE_BATCH]
int batch_size = 0;

...

while (batch_size < MAX_STRPE_BATCH &&

       (sh = __get_priority_stripe(conf)) != NULL)
     batch[batch_size++] = sh;

spin_unlock(&conf->device_lock);
if (batch_size == 0)

     break;

handled += batch_size;

for (i = 0; i < batch_size; i++)

     handle_stripe(batch[i]);

cond_resched();
if (....) md_check_recovery(mddev);

Quoted 6 lines

spin_lock_irq(&conf->lock);
for (i = 0; i < batch_size; i++)

     __release_stripe(batch[i]);

something like that?

the 8th patch does the same thing, so I moved the code to a separate function.

Like raid 1/10, raid5 uses one thread to handle stripe. In a fast storage, the thread becomes a bottleneck. raid5 can offload calculation like checksum to async threads. And if storge is fast, scheduling async work and running async work will introduce heavy lock contention of workqueue, which makes such optimization useless. And calculation isn't the only bottleneck. For example, in my test raid5 thread must handle > 450k requests per second. Just doing dispatch and completion will make raid5 thread incapable. The only chance to scale is using several threads to handle stripe.

With this patch, user can create several extra threads to handle stripe. How many threads are better depending on disk number, so the thread number can be changed in userspace. By default, the thread number is 0, which means no extra thread.

In a 3-disk raid5 setup, 2 extra threads can provide 130% throughput improvement (double stripe_cache_size) and the throughput is pretty close to theory value. With >=4 disks, the improvement is even bigger, for example, can improve 200% for 4-disk setup, but the throughput is far less than theory value, which is caused by several factors like request queue lock contention, cache issue, latency introduced by how a stripe is handled in different disks. Those factors need further investigations.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/raid5.c |  118 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/md/raid5.h |    2 
 2 files changed, 120 insertions(+)

Index: linux/drivers/md/raid5.c
===================================================================

drivers/md/raid5.c +118 -0

@@ -4602,6 +4602,41 @@ static int __get_stripe_batch(struct r5c
 	return batch->count;
 }
 
+static void raid5auxd(struct mddev *mddev)
+{
+	struct r5conf *conf = mddev->private;
+	struct blk_plug plug;
+	struct stripe_head_batch batch;
+	int handled, i;
+
+	pr_debug("+++ raid5auxd active\n");
+
+	blk_start_plug(&plug);
+	handled = 0;
+	spin_lock_irq(&conf->device_lock);
+	while (1) {
+		if (!__get_stripe_batch(conf, &batch))
+			break;
+		spin_unlock_irq(&conf->device_lock);
+
+		for (i = 0; i < batch.count; i++) {
+			handled++;
+			handle_stripe(batch.stripes[i]);
+		}
+
+		release_stripe_flush_batch(&batch);
+		cond_resched();
+
+		spin_lock_irq(&conf->device_lock);
+	}
+	pr_debug("%d stripes handled\n", handled);
+
+	spin_unlock_irq(&conf->device_lock);
+	blk_finish_plug(&plug);
+
+	pr_debug("--- raid5auxd inactive\n");
+}
+
 /*
  * This is our raid5 kernel thread.
  *

@@ -4651,6 +4686,8 @@ static void raid5d(struct mddev *mddev)
 
 		if (!__get_stripe_batch(conf, &batch))
 			break;
+		for (i = 0; i < conf->aux_thread_num; i++)
+			md_wakeup_thread(conf->aux_threads[i]);
 		spin_unlock_irq(&conf->device_lock);
 
 		for (i = 0; i < batch.count; i++) {

@@ -4784,10 +4821,86 @@ stripe_cache_active_show(struct mddev *m
 static struct md_sysfs_entry
 raid5_stripecache_active = __ATTR_RO(stripe_cache_active);
 
+static ssize_t
+raid5_show_auxthread_number(struct mddev *mddev, char *page)
+{
+	struct r5conf *conf = mddev->private;
+	if (conf)
+		return sprintf(page, "%d\n", conf->aux_thread_num);
+	else
+		return 0;
+}
+
+static ssize_t
+raid5_store_auxthread_number(struct mddev *mddev, const char *page, size_t len)
+{
+	struct r5conf *conf = mddev->private;
+	unsigned long new;
+	int i;
+	struct md_thread **threads;
+
+	if (len >= PAGE_SIZE)
+		return -EINVAL;
+	if (!conf)
+		return -ENODEV;
+
+	if (strict_strtoul(page, 10, &new))
+		return -EINVAL;
+
+	if (new == conf->aux_thread_num)
+		return len;
+
+	if (new > conf->aux_thread_num) {
+
+		threads = kmalloc(sizeof(struct md_thread *) * new, GFP_KERNEL);
+		if (!threads)
+			return -EFAULT;
+
+		i = conf->aux_thread_num;
+		while (i < new) {
+			char name[10];
+
+			sprintf(name, "aux%d", i);
+			threads[i] = md_register_thread(raid5auxd, mddev, name);
+			if (!threads[i])
+				goto error;
+			i++;
+		}
+		memcpy(threads, conf->aux_threads,
+			sizeof(struct md_thread *) * conf->aux_thread_num);
+		spin_lock_irq(&conf->device_lock);
+		kfree(conf->aux_threads);
+		conf->aux_threads = threads;
+		conf->aux_thread_num = new;
+		spin_unlock_irq(&conf->device_lock);
+	} else {
+		int old = conf->aux_thread_num;
+
+		spin_lock_irq(&conf->device_lock);
+		conf->aux_thread_num = new;
+		spin_unlock_irq(&conf->device_lock);
+		for (i = new; i < old; i++)
+			md_unregister_thread(&conf->aux_threads[i]);
+	}
+
+	return len;
+error:
+	while (--i >= conf->aux_thread_num)
+		md_unregister_thread(&threads[i]);
+	kfree(threads);
+	return -EFAULT;
+}
+
+static struct md_sysfs_entry
+raid5_auxthread_number = __ATTR(auxthread_number, S_IRUGO|S_IWUSR,
+				raid5_show_auxthread_number,
+				raid5_store_auxthread_number);
+
 static struct attribute *raid5_attrs[] =  {
 	&raid5_stripecache_size.attr,
 	&raid5_stripecache_active.attr,
 	&raid5_preread_bypass_threshold.attr,
+	&raid5_auxthread_number.attr,
 	NULL,
 };
 static struct attribute_group raid5_attrs_group = {

@@ -4835,6 +4948,7 @@ static void raid5_free_percpu(struct r5c
 
 static void free_conf(struct r5conf *conf)
 {
+	kfree(conf->aux_threads);
 	shrink_stripes(conf);
 	raid5_free_percpu(conf);
 	kfree(conf->disks);

@@ -5391,6 +5505,10 @@ abort:
 static int stop(struct mddev *mddev)
 {
 	struct r5conf *conf = mddev->private;
+	int i;
+
+	for (i = 0; i < conf->aux_thread_num; i++)
+		md_unregister_thread(&conf->aux_threads[i]);
 
 	md_unregister_thread(&mddev->thread);
 	if (mddev->queue)

Index: linux/drivers/md/raid5.h
===================================================================

drivers/md/raid5.h +2 -0

@@ -463,6 +463,8 @@ struct r5conf {
 	 * the new thread here until we fully activate the array.
 	 */
 	struct md_thread	*thread;
+	int			aux_thread_num;
+	struct md_thread	**aux_threads;
 };
 
 /*

Quoted 22 lines · On Mon, 04 Jun 2012 16:02:00 +0800 Shaohua Li <[email protected]> wrote:

Like raid 1/10, raid5 uses one thread to handle stripe. In a fast storage, the thread becomes a bottleneck. raid5 can offload calculation like checksum to async threads. And if storge is fast, scheduling async work and running async work will introduce heavy lock contention of workqueue, which makes such optimization useless. And calculation isn't the only bottleneck. For example, in my test raid5 thread must handle > 450k requests per second. Just doing dispatch and completion will make raid5 thread incapable. The only chance to scale is using several threads to handle stripe.

With this patch, user can create several extra threads to handle stripe. How many threads are better depending on disk number, so the thread number can be changed in userspace. By default, the thread number is 0, which means no extra thread.

In a 3-disk raid5 setup, 2 extra threads can provide 130% throughput improvement (double stripe_cache_size) and the throughput is pretty close to theory value. With >=4 disks, the improvement is even bigger, for example, can improve 200% for 4-disk setup, but the throughput is far less than theory value, which is caused by several factors like request queue lock contention, cache issue, latency introduced by how a stripe is handled in different disks. Those factors need further investigations.

Quoted 1 line

Signed-off-by Shaohua Li <[email protected]>

I think it is great that you have got RAID5 to the point where multiple threads improve performance.
I really don't like the idea of having to configure that number of threads.

It would be great if it would auto-configure.
Maybe the main thread could fork aux threads when it notices a high load. e.g. if it has been servicing requests for more than 100ms without a break, and the number of threads is less than the number of CPUs, then it forks a new helper and resets the timer.

If a thread has been idle for more than 30 minutes, it exits.

Might that be reasonable?

Quoted 38 lines · NeilBrown

Quoted 22 lines · On Mon, 04 Jun 2012 16:02:00 +0800 Shaohua Li <[email protected]> wrote:

Like raid 1/10, raid5 uses one thread to handle stripe. In a fast storage, the thread becomes a bottleneck. raid5 can offload calculation like checksum to async threads. And if storge is fast, scheduling async work and running async work will introduce heavy lock contention of workqueue, which makes such optimization useless. And calculation isn't the only bottleneck. For example, in my test raid5 thread must handle > 450k requests per second. Just doing dispatch and completion will make raid5 thread incapable. The only chance to scale is using several threads to handle stripe.

With this patch, user can create several extra threads to handle stripe. How many threads are better depending on disk number, so the thread number can be changed in userspace. By default, the thread number is 0, which means no extra thread.

In a 3-disk raid5 setup, 2 extra threads can provide 130% throughput improvement (double stripe_cache_size) and the throughput is pretty close to theory value. With >=4 disks, the improvement is even bigger, for example, can improve 200% for 4-disk setup, but the throughput is far less than theory value, which is caused by several factors like request queue lock contention, cache issue, latency introduced by how a stripe is handled in different disks. Those factors need further investigations.

Quoted 1 line

Signed-off-by Shaohua Li <[email protected]>

I think it is great that you have got RAID5 to the point where multiple threads improve performance.
I really don't like the idea of having to configure that number of threads.

It would be great if it would auto-configure.
Maybe the main thread could fork aux threads when it notices a high load. e.g. if it has been servicing requests for more than 100ms without a break, and the number of threads is less than the number of CPUs, then it forks a new helper and resets the timer.

If a thread has been idle for more than 30 minutes, it exits.

Quoted 1 line

Might that be reasonable?

Yep, I bet this patch needs more discussion. auto-configure is preferred. Your idea is worthy doing. However, the concern is if doing auto fork/kill thread, user can't do numa binding, which is important for high speed storage. Maybe have a reasonable default thread number, like one thread one disk? Need more investigations, I'm open to any suggestion in this side.

Quoted 39 lines · Shaohua Li

Quoted 38 lines · NeilBrown

Quoted 22 lines · On Mon, 04 Jun 2012 16:02:00 +0800 Shaohua Li <[email protected]> wrote:

Like raid 1/10, raid5 uses one thread to handle stripe. In a fast storage, the thread becomes a bottleneck. raid5 can offload calculation like checksum to async threads. And if storge is fast, scheduling async work and running async work will introduce heavy lock contention of workqueue, which makes such optimization useless. And calculation isn't the only bottleneck. For example, in my test raid5 thread must handle > 450k requests per second. Just doing dispatch and completion will make raid5 thread incapable. The only chance to scale is using several threads to handle stripe.

With this patch, user can create several extra threads to handle stripe. How many threads are better depending on disk number, so the thread number can be changed in userspace. By default, the thread number is 0, which means no extra thread.

In a 3-disk raid5 setup, 2 extra threads can provide 130% throughput improvement (double stripe_cache_size) and the throughput is pretty close to theory value. With >=4 disks, the improvement is even bigger, for example, can improve 200% for 4-disk setup, but the throughput is far less than theory value, which is caused by several factors like request queue lock contention, cache issue, latency introduced by how a stripe is handled in different disks. Those factors need further investigations.

Quoted 1 line

Signed-off-by Shaohua Li <[email protected]>

I think it is great that you have got RAID5 to the point where multiple threads improve performance.
I really don't like the idea of having to configure that number of threads.

It would be great if it would auto-configure.
Maybe the main thread could fork aux threads when it notices a high load. e.g. if it has been servicing requests for more than 100ms without a break, and the number of threads is less than the number of CPUs, then it forks a new helper and resets the timer.

If a thread has been idle for more than 30 minutes, it exits.

Quoted 7 lines

Quoted 1 line

Might that be reasonable?

Yep, I bet this patch needs more discussion. auto-configure is preferred. Your idea is worthy doing. However, the concern is if doing auto fork/kill thread, user can't do numa binding, which is important for high speed storage. Maybe have a reasonable default thread number, like one thread one disk? Need more investigations, I'm open to any suggestion in this side.

The last time I looked at this the btrfs thread pool looked like a good candidate:

http://marc.info/?l=linux-raid&m=126944260704907&w=2

...have not looked if Tejun has made this available as a generic workqueue mode.

Quoted 48 lines · Dan Williams

Quoted 39 lines · Shaohua Li

Quoted 38 lines · NeilBrown

Quoted 22 lines · On Mon, 04 Jun 2012 16:02:00 +0800 Shaohua Li <[email protected]> wrote:

Like raid 1/10, raid5 uses one thread to handle stripe. In a fast storage, the thread becomes a bottleneck. raid5 can offload calculation like checksum to async threads. And if storge is fast, scheduling async work and running async work will introduce heavy lock contention of workqueue, which makes such optimization useless. And calculation isn't the only bottleneck. For example, in my test raid5 thread must handle > 450k requests per second. Just doing dispatch and completion will make raid5 thread incapable. The only chance to scale is using several threads to handle stripe.

With this patch, user can create several extra threads to handle stripe. How many threads are better depending on disk number, so the thread number can be changed in userspace. By default, the thread number is 0, which means no extra thread.

In a 3-disk raid5 setup, 2 extra threads can provide 130% throughput improvement (double stripe_cache_size) and the throughput is pretty close to theory value. With >=4 disks, the improvement is even bigger, for example, can improve 200% for 4-disk setup, but the throughput is far less than theory value, which is caused by several factors like request queue lock contention, cache issue, latency introduced by how a stripe is handled in different disks. Those factors need further investigations.

Quoted 1 line

Signed-off-by Shaohua Li <[email protected]>

I think it is great that you have got RAID5 to the point where multiple threads improve performance.
I really don't like the idea of having to configure that number of threads.

It would be great if it would auto-configure.
Maybe the main thread could fork aux threads when it notices a high load. e.g. if it has been servicing requests for more than 100ms without a break, and the number of threads is less than the number of CPUs, then it forks a new helper and resets the timer.

If a thread has been idle for more than 30 minutes, it exits.

Quoted 7 lines

Quoted 1 line

Might that be reasonable?

Yep, I bet this patch needs more discussion. auto-configure is preferred. Your idea is worthy doing. However, the concern is if doing auto fork/kill thread, user can't do numa binding, which is important for high speed storage. Maybe have a reasonable default thread number, like one thread one disk? Need more investigations, I'm open to any suggestion in this side.

Quoted 6 lines

The last time I looked at this the btrfs thread pool looked like a good candidate:

http://marc.info/?l=linux-raid&m=126944260704907&w=2

...have not looked if Tejun has made this available as a generic workqueue mode.

I tried to create a UNBOUND workqueue and set max active to the cpu number, so each cpu will handle one work. In the work, the cpu will handle 8 stripes. The throughput is relative ok, but CPU utilization is very high compared to just create 3 or 4 threads like the patch does. There is heavy lock contention in block queue_lock, since every cpu now dispatches request. There are other issues like cache, raid5 device_lock has more contention too. It appears too many threads to handle stripe isn't as good as expected.