raid5: improve write performance for fast storage

It's possible several tasks are waiting for stripe overlap. We clear R5_Overlap bit and wake_up, but wake_up just wakes one task. So if there are several tasks in the wait queue, some tasks will not be woken up even its strip R5_Overlap clear. The end result is tasks hang in make_request.

wake_up_all should not introduce performance issue here, since overlap case is rare.

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

---
 drivers/md/raid5.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

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

drivers/md/raid5.c +10 -10

@@ -1399,7 +1399,7 @@ static void __raid_run_ops(struct stripe
 		for (i = disks; i--; ) {
 			struct r5dev *dev = &sh->dev[i];
 			if (test_and_clear_bit(R5_Overlap, &dev->flags))
-				wake_up(&sh->raid_conf->wait_for_overlap);
+				wake_up_all(&sh->raid_conf->wait_for_overlap);
 		}
 	put_cpu();
 }

@@ -2436,7 +2436,7 @@ handle_failed_stripe(struct r5conf *conf
 		}
 
 		if (test_and_clear_bit(R5_Overlap, &sh->dev[i].flags))
-			wake_up(&conf->wait_for_overlap);
+			wake_up_all(&conf->wait_for_overlap);
 
 		while (bi && bi->bi_sector <
 			sh->dev[i].sector + STRIPE_SECTORS) {

@@ -2474,7 +2474,7 @@ handle_failed_stripe(struct r5conf *conf
 			bi = sh->dev[i].toread;
 			sh->dev[i].toread = NULL;
 			if (test_and_clear_bit(R5_Overlap, &sh->dev[i].flags))
-				wake_up(&conf->wait_for_overlap);
+				wake_up_all(&conf->wait_for_overlap);
 			if (bi) s->to_read--;
 			while (bi && bi->bi_sector <
 			       sh->dev[i].sector + STRIPE_SECTORS) {

@@ -3572,7 +3572,7 @@ static void handle_stripe(struct stripe_
 	} else if (s.expanded && !sh->reconstruct_state && s.locked == 0) {
 		clear_bit(STRIPE_EXPAND_READY, &sh->state);
 		atomic_dec(&conf->reshape_stripes);
-		wake_up(&conf->wait_for_overlap);
+		wake_up_all(&conf->wait_for_overlap);
 		md_done_sync(conf->mddev, STRIPE_SECTORS, 1);
 	}
 

@@ -4249,7 +4249,7 @@ static sector_t reshape_request(struct m
 		spin_lock_irq(&conf->device_lock);
 		conf->reshape_safe = mddev->reshape_position;
 		spin_unlock_irq(&conf->device_lock);
-		wake_up(&conf->wait_for_overlap);
+		wake_up_all(&conf->wait_for_overlap);
 		sysfs_notify(&mddev->kobj, NULL, "sync_completed");
 	}
 

@@ -4340,7 +4340,7 @@ static sector_t reshape_request(struct m
 		spin_lock_irq(&conf->device_lock);
 		conf->reshape_safe = mddev->reshape_position;
 		spin_unlock_irq(&conf->device_lock);
-		wake_up(&conf->wait_for_overlap);
+		wake_up_all(&conf->wait_for_overlap);
 		sysfs_notify(&mddev->kobj, NULL, "sync_completed");
 	}
 	return reshape_sectors;

@@ -5718,7 +5718,7 @@ static void end_reshape(struct r5conf *c
 		smp_wmb();
 		conf->reshape_progress = MaxSector;
 		spin_unlock_irq(&conf->device_lock);
-		wake_up(&conf->wait_for_overlap);
+		wake_up_all(&conf->wait_for_overlap);
 
 		/* read-ahead size must cover two whole stripes, which is
 		 * 2 * (datadisks) * chunksize where 'n' is the number of raid devices

@@ -5776,7 +5776,7 @@ static void raid5_quiesce(struct mddev *
 
 	switch(state) {
 	case 2: /* resume for a suspend */
-		wake_up(&conf->wait_for_overlap);
+		wake_up_all(&conf->wait_for_overlap);
 		break;
 
 	case 1: /* stop all writes */

@@ -5792,14 +5792,14 @@ static void raid5_quiesce(struct mddev *
 		conf->quiesce = 1;
 		spin_unlock_irq(&conf->device_lock);
 		/* allow reshape to continue */
-		wake_up(&conf->wait_for_overlap);
+		wake_up_all(&conf->wait_for_overlap);
 		break;
 
 	case 0: /* re-enable writes */
 		spin_lock_irq(&conf->device_lock);
 		conf->quiesce = 0;
 		wake_up(&conf->wait_for_stripe);
-		wake_up(&conf->wait_for_overlap);
+		wake_up_all(&conf->wait_for_overlap);
 		spin_unlock_irq(&conf->device_lock);
 		break;
 	}

Quoted 5 lines · On Mon, 25 Jun 2012 15:24:48 +0800 Shaohua Li <[email protected]> wrote:

It's possible several tasks are waiting for stripe overlap. We clear R5_Overlap bit and wake_up, but wake_up just wakes one task. So if there are several tasks in the wait queue, some tasks will not be woken up even its strip R5_Overlap clear. The end result is tasks hang in make_request.

Quoted 2 lines

wake_up_all should not introduce performance issue here, since overlap case is rare.

This is not necessary.
wake_up_all is only different from wake_up if WQ_FLAG_EXCLUSIVE it set, e.g. by prepare_to_wait_exclusive.
As we don't use an exclusive wait to wait on wait_for_overlap, there is no point in using wake_up_all, wake_up already wakes everything up.

There isn't locking setting STRIPE_DELAYED and STRIPE_PREREAD_ACTIVE bits, but the two bits have relationship. A delayed stripe can be moved to hold list only when preread active stripe count is below IO_THRESHOLD. If a stripe has both the bits set, such stripe will be in delayed list and preread count not 0, which will make such stripe never leave delayed list.

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

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

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

drivers/md/raid5.c +1 -1

@@ -196,12 +196,14 @@ static void __release_stripe(struct r5co
 		BUG_ON(!list_empty(&sh->lru));
 		BUG_ON(atomic_read(&conf->active_stripes)==0);
 		if (test_bit(STRIPE_HANDLE, &sh->state)) {
-			if (test_bit(STRIPE_DELAYED, &sh->state))
+			if (test_bit(STRIPE_DELAYED, &sh->state) &&

Quoted 21 lines · On Mon, 25 Jun 2012 15:24:49 +0800 Shaohua Li <[email protected]> wrote:

There isn't locking setting STRIPE_DELAYED and STRIPE_PREREAD_ACTIVE bits, but the two bits have relationship. A delayed stripe can be moved to hold list only when preread active stripe count is below IO_THRESHOLD. If a stripe has both the bits set, such stripe will be in delayed list and preread count not 0, which will make such stripe never leave delayed list.

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

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

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

drivers/md/raid5.c +1 -1

@@ -196,12 +196,14 @@ static void __release_stripe(struct r5co
 		BUG_ON(!list_empty(&sh->lru));
 		BUG_ON(atomic_read(&conf->active_stripes)==0);
 		if (test_bit(STRIPE_HANDLE, &sh->state)) {
-			if (test_bit(STRIPE_DELAYED, &sh->state))
+			if (test_bit(STRIPE_DELAYED, &sh->state) &&

drivers/md/raid5.c static void __release_stripe(struct r5co · line 209

Quoted 10 lines

drivers/md/raid5.c +2 -0

+			    !test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
 				list_add_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);
 			else {
+				clear_bit(STRIPE_DELAYED, &sh->state);
 				clear_bit(STRIPE_BIT_DELAY, &sh->state);
 				list_add_tail(&sh->lru, &conf->handle_list);
 			}

Thanks. I've applied this patch and will submit it upstream shortly.

Have you actually seen a stripe get trapped with both bits set, or is this just a theoretical problem discovered by code inspection?

Quoted 36 lines · NeilBrown

Quoted 21 lines · On Mon, 25 Jun 2012 15:24:49 +0800 Shaohua Li <[email protected]> wrote:

There isn't locking setting STRIPE_DELAYED and STRIPE_PREREAD_ACTIVE bits, but the two bits have relationship. A delayed stripe can be moved to hold list only when preread active stripe count is below IO_THRESHOLD. If a stripe has both the bits set, such stripe will be in delayed list and preread count not 0, which will make such stripe never leave delayed list.

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

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

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

drivers/md/raid5.c +1 -1

@@ -196,12 +196,14 @@ static void __release_stripe(struct r5co
 		BUG_ON(!list_empty(&sh->lru));
 		BUG_ON(atomic_read(&conf->active_stripes)==0);
 		if (test_bit(STRIPE_HANDLE, &sh->state)) {
-			if (test_bit(STRIPE_DELAYED, &sh->state))
+			if (test_bit(STRIPE_DELAYED, &sh->state) &&

drivers/md/raid5.c static void __release_stripe(struct r5co · line 209

Quoted 10 lines

drivers/md/raid5.c +2 -0

+			    !test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
 				list_add_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);
 			else {
+				clear_bit(STRIPE_DELAYED, &sh->state);
 				clear_bit(STRIPE_BIT_DELAY, &sh->state);
 				list_add_tail(&sh->lru, &conf->handle_list);
 			}

Thanks. I've applied this patch and will submit it upstream shortly.

Quoted 2 lines

Have you actually seen a stripe get trapped with both bits set, or is this just a theoretical problem discovered by code inspection?

I print the flags of strip when there is overlap sleep in make_request(), and found this case, so this is real.

How do you think about the other patches in the series?

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

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

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

@@ -800,8 +802,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(&sh->stripe_lock);
 			dev->read = rbi = dev->toread;
 			dev->toread = NULL;
+			spin_unlock(&sh->stripe_lock);
 			spin_unlock_irq(&conf->device_lock);
 			while (rbi && rbi->bi_sector <
 				dev->sector + STRIPE_SECTORS) {

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

@@ -1448,6 +1454,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);

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

@@ -2362,6 +2371,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(&sh->stripe_lock);
 	spin_unlock_irq(&conf->device_lock);
 
 	pr_debug("added bi b#%llu to stripe s#%llu, disk %d.\n",

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

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

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

@@ -2697,6 +2710,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(&sh->stripe_lock);
 				wbi = dev->written;
 				dev->written = NULL;
 				while (wbi && wbi->bi_sector <

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

@@ -3170,6 +3185,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(&sh->stripe_lock);
 	for (i=disks; i--; ) {
 		struct md_rdev *rdev;
 		sector_t first_bad;

@@ -3315,6 +3331,7 @@ static void analyse_stripe(struct stripe
 				do_recovery = 1;
 		}
 	}
+	spin_unlock(&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 4 lines · On Mon, 25 Jun 2012 15:24:50 +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.

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

I had hoped to avoid having a per-stripe lock again, but it does look like it is needed.
However I don't like the way you have split up these three patches - it makes them a little hard to review.

I would like to see one patch which converts the bi_phys_segments access to be atomic and also removes all the spin_lock calls that were just for protecting that.

Then another patch which adds the new stripe_lock, clearly documenting exactly what is protects (not just "like dev->read" but an explicit list) and also removes any spin_lock of device_lock that is no longer needed.

Then I could see what is being added and what is being removed all in the one patch and I can be sure that they balance.

Quoted 2 lines

Then I could see what is being added and what is being removed all in the one patch and I can be sure that they balance.

reworked the patch 3-5 to two patches as you suggested, and sent to you. please check.

Quoted 4 lines · On Mon, 2 Jul 2012 11:16:26 +0800 Shaohua Li <[email protected]> wrote:

Quoted 2 lines

Then I could see what is being added and what is being removed all in the one patch and I can be sure that they balance.

reworked the patch 3-5 to two patches as you suggested, and sent to you. please check.

Thanks. That's looking really good.

However I think we can do better. I've been looking more closely at the code and I think that the only things that we need stripe_lock to protect are ->toread and ->towrite, together with the following bios. e.g. ->toread->bi_next etc.

->read and ->written don't need stripe_lock protection, as they are only manipulated by the handle_stripe state machine which uses STRIPE_ACTIVE and refcounts for exclusion.

So add_stripe_bio need to take the lock while adding a bio to the ->toread and ->towrite lists, and ops_run_biodrain() and ops_run_biofill need to take the lock while the move the list from ->to{read,write} to ->{read,written}. But we don't need it anywhere else. e.g. analyse_stripe shouldn't need the lock at all. Any change that could happen during the loop could equally happen after the lock was released so we don't lose by not having the lock.

There is another current user of the lock, but I think that should be discarded as a false optimisation.
We currently try to optimise out extra calls to bitmap_startwrite and bitmap_endwrite when we see back-to-back writes to the one stripe. However I suspect that is extremely unlikely and it just imposes and pointless need for synchronisation in raid5.

We just simply call bitmap_startwrite whenever ->towrite changes from NULL to non-NULL, and call bitmap_endwrite whenever we clear ->written, reguardless what value ->towrite now has.

Quoted 6 lines

Quoted 4 lines

Then I could see what is being added and what is being removed all in the one patch and I can be sure that they balance.

reworked the patch 3-5 to two patches as you suggested, and sent to you. please check.

Thanks. That's looking really good.

However I think we can do better. I've been looking more closely at the code and I think that the only things that we need stripe_lock to protect are ->toread and ->towrite, together with the following bios. e.g. ->toread->bi_next etc.

->read and ->written don't need stripe_lock protection, as they are only manipulated by the handle_stripe state machine which uses STRIPE_ACTIVE and refcounts for exclusion.

So add_stripe_bio need to take the lock while adding a bio to the ->toread and ->towrite lists, and ops_run_biodrain() and ops_run_biofill need to take the lock while the move the list from ->to{read,write} to ->{read,written}.

But we don't need it anywhere else. e.g. analyse_stripe shouldn't need the lock at all. Any change that could happen during the loop could equally happen after the lock was released so we don't lose by not having the lock.

There is another current user of the lock, but I think that should be discarded as a false optimisation.
We currently try to optimise out extra calls to bitmap_startwrite and bitmap_endwrite when we see back-to-back writes to the one stripe. However I suspect that is extremely unlikely and it just imposes and pointless need for synchronisation in raid5.

We just simply call bitmap_startwrite whenever ->towrite changes from NULL to non-NULL, and call bitmap_endwrite whenever we clear ->written, reguardless what value ->towrite now has.

Would you like to experiment with that? If I haven't described it well enough I can write a patch to show what I mean.

Thanks,
NeilBrown

On 2012-07-02 15:39 NeilBrown <[email protected]> Wrote:

Quoted 19 lines · On Mon, 2 Jul 2012 11:16:26 +0800 Shaohua Li <[email protected]> wrote:

Quoted 6 lines

Quoted 4 lines

Then I could see what is being added and what is being removed all in the one patch and I can be sure that they balance.

reworked the patch 3-5 to two patches as you suggested, and sent to you. please check.

Thanks. That's looking really good.

However I think we can do better. I've been looking more closely at the code and I think that the only things that we need stripe_lock to protect are ->toread and ->towrite, together with the following bios. e.g. ->toread->bi_next etc.

->read and ->written don't need stripe_lock protection, as they are only manipulated by the handle_stripe state machine which uses STRIPE_ACTIVE and refcounts for exclusion.

Quoted 4 lines

So add_stripe_bio need to take the lock while adding a bio to the ->toread and ->towrite lists, and ops_run_biodrain() and ops_run_biofill need to take the lock while the move the list from ->to{read,write} to ->{read,written}.

How about xchg()?

Quoted 21 lines

On 2012-07-02 15:39 NeilBrown <[email protected]> Wrote:

Quoted 19 lines · On Mon, 2 Jul 2012 11:16:26 +0800 Shaohua Li <[email protected]> wrote:

Quoted 6 lines

Quoted 4 lines

Then I could see what is being added and what is being removed all in the one patch and I can be sure that they balance.

reworked the patch 3-5 to two patches as you suggested, and sent to you. please check.

Thanks. That's looking really good.

However I think we can do better. I've been looking more closely at the code and I think that the only things that we need stripe_lock to protect are ->toread and ->towrite, together with the following bios. e.g. ->toread->bi_next etc.

->read and ->written don't need stripe_lock protection, as they are only manipulated by the handle_stripe state machine which uses STRIPE_ACTIVE and refcounts for exclusion.

Quoted 6 lines

Quoted 4 lines

So add_stripe_bio need to take the lock while adding a bio to the ->toread and ->towrite lists, and ops_run_biodrain() and ops_run_biofill need to take the lock while the move the list from ->to{read,write} to ->{read,written}.

How about xchg()?

(it would help a little if you made your comments stand out more, and maybe be a bit more verbose).

bio_add_stripe() can insert a new bio into a list that starts are ->towrite or ->toread.
You cannot do that with a simple xchg. You really need a lock.

NeilBrown

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.

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

---
 drivers/md/raid5.c |   73 +++++++++++++++++++++++++++++------------------------
 1 file changed, 40 insertions(+), 33 deletions(-)

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

drivers/md/raid5.c +34 -32

@@ -196,49 +196,56 @@ 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 handle_release_stripe(struct r5conf *conf, struct stripe_head *sh)
 {
-	if (atomic_dec_and_test(&sh->count)) {
-		BUG_ON(!list_empty(&sh->lru));
-		BUG_ON(atomic_read(&conf->active_stripes)==0);
-		if (test_bit(STRIPE_HANDLE, &sh->state)) {
-			if (test_bit(STRIPE_DELAYED, &sh->state) &&
-			    !test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
-				list_add_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);
-			else {
-				clear_bit(STRIPE_DELAYED, &sh->state);
-				clear_bit(STRIPE_BIT_DELAY, &sh->state);
-				list_add_tail(&sh->lru, &conf->handle_list);
-			}
-			md_wakeup_thread(conf->mddev->thread);
-		} else {
-			BUG_ON(stripe_operations_active(sh));
-			if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
-				if (atomic_dec_return(&conf->preread_active_stripes)
-				    < IO_THRESHOLD)
-					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);
-				wake_up(&conf->wait_for_stripe);
-				if (conf->retry_read_aligned)
-					md_wakeup_thread(conf->mddev->thread);
-			}
+	BUG_ON(!list_empty(&sh->lru));
+	BUG_ON(atomic_read(&conf->active_stripes)==0);
+	if (test_bit(STRIPE_HANDLE, &sh->state)) {
+		if (test_bit(STRIPE_DELAYED, &sh->state) &&
+		    !test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
+			list_add_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);
+		else {
+			clear_bit(STRIPE_DELAYED, &sh->state);
+			clear_bit(STRIPE_BIT_DELAY, &sh->state);
+			list_add_tail(&sh->lru, &conf->handle_list);
+		}
+		md_wakeup_thread(conf->mddev->thread);
+	} else {
+		BUG_ON(stripe_operations_active(sh));
+		if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
+			if (atomic_dec_return(&conf->preread_active_stripes)
+			    < IO_THRESHOLD)
+				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);
+			wake_up(&conf->wait_for_stripe);
+			if (conf->retry_read_aligned)
+				md_wakeup_thread(conf->mddev->thread);
 		}
 	}
 }
 
+static void __release_stripe(struct r5conf *conf, struct stripe_head *sh)
+{
+	if (atomic_dec_and_test(&sh->count))
+		handle_release_stripe(conf, sh);
+}
+
 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);

Allow personality providing unplug private data. Next patch will use it.

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

---
 drivers/md/md.c     |   31 +++++++++++++------------------
 drivers/md/md.h     |   20 +++++++++++++++++++-
 drivers/md/raid1.c  |    2 +-
 drivers/md/raid10.c |    2 +-
 drivers/md/raid5.c  |    2 +-
 5 files changed, 35 insertions(+), 22 deletions(-)

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

drivers/md/md.c +13 -18

@@ -498,22 +498,13 @@ void md_flush_request(struct mddev *mdde
 }
 EXPORT_SYMBOL(md_flush_request);
 
-/* Support for plugging.
- * This mirrors the plugging support in request_queue, but does not
- * require having a whole queue or request structures.
- * We allocate an md_plug_cb for each md device and each thread it gets
- * plugged on.  This links tot the private plug_handle structure in the
- * personality data where we keep a count of the number of outstanding
- * plugs so other code can see if a plug is active.
- */
-struct md_plug_cb {
-	struct blk_plug_cb cb;
-	struct mddev *mddev;
-};
 
 static void plugger_unplug(struct blk_plug_cb *cb)
 {
 	struct md_plug_cb *mdcb = container_of(cb, struct md_plug_cb, cb);
+
+	if (mdcb->unplug)
+		mdcb->unplug(mdcb);
 	if (atomic_dec_and_test(&mdcb->mddev->plug_cnt))
 		md_wakeup_thread(mdcb->mddev->thread);
 	kfree(mdcb);

@@ -522,13 +513,14 @@ static void plugger_unplug(struct blk_pl
 /* Check that an unplug wakeup will come shortly.
  * If not, wakeup the md thread immediately
  */
-int mddev_check_plugged(struct mddev *mddev)
+struct md_plug_cb *mddev_check_plugged(struct mddev *mddev,
+	md_unplug_func_t unplug, size_t size)
 {
 	struct blk_plug *plug = current->plug;
 	struct md_plug_cb *mdcb;
 
 	if (!plug)
-		return 0;
+		return NULL;
 
 	list_for_each_entry(mdcb, &plug->cb_list, cb.list) {
 		if (mdcb->cb.callback == plugger_unplug &&

@@ -538,19 +530,22 @@ int mddev_check_plugged(struct mddev *md
 						    struct md_plug_cb,
 						    cb.list))
 				list_move(&mdcb->cb.list, &plug->cb_list);
-			return 1;
+			return mdcb;
 		}
 	}
 	/* Not currently on the callback list */
-	mdcb = kmalloc(sizeof(*mdcb), GFP_ATOMIC);
+	mdcb = kmalloc(sizeof(*mdcb) + size, GFP_ATOMIC);
 	if (!mdcb)
-		return 0;
+		return NULL;
 
 	mdcb->mddev = mddev;
 	mdcb->cb.callback = plugger_unplug;
 	atomic_inc(&mddev->plug_cnt);
 	list_add(&mdcb->cb.list, &plug->cb_list);
-	return 1;
+	mdcb->unplug = unplug;
+	if (size)
+		memset((void *)(mdcb + 1), 0, size);
+	return mdcb;
 }
 EXPORT_SYMBOL_GPL(mddev_check_plugged);
 

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

drivers/md/md.h +19 -1

@@ -630,6 +630,24 @@ extern struct bio *bio_clone_mddev(struc
 				   struct mddev *mddev);
 extern struct bio *bio_alloc_mddev(gfp_t gfp_mask, int nr_iovecs,
 				   struct mddev *mddev);
-extern int mddev_check_plugged(struct mddev *mddev);
+
+/* Support for plugging.
+ * This mirrors the plugging support in request_queue, but does not
+ * require having a whole queue or request structures.
+ * We allocate an md_plug_cb for each md device and each thread it gets
+ * plugged on.  This links tot the private plug_handle structure in the
+ * personality data where we keep a count of the number of outstanding
+ * plugs so other code can see if a plug is active.
+ */
+struct md_plug_cb;
+typedef void (*md_unplug_func_t)(struct md_plug_cb *mdcb);
+struct md_plug_cb {
+	struct blk_plug_cb cb;
+	struct mddev *mddev;
+	md_unplug_func_t unplug;
+};
+
+extern struct md_plug_cb *mddev_check_plugged(struct mddev *mddev,
+	md_unplug_func_t unplug, size_t size);
 extern void md_trim_bio(struct bio *bio, int offset, int size);
 #endif /* _MD_MD_H */

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

drivers/md/raid1.c +1 -1

@@ -1034,7 +1034,7 @@ read_again:
 	 * the bad blocks.  Each set of writes gets it's own r1bio
 	 * with a set of bios attached.
 	 */
-	plugged = mddev_check_plugged(mddev);
+	plugged = !!mddev_check_plugged(mddev, NULL, 0);
 
 	disks = conf->raid_disks * 2;
  retry_write:

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

drivers/md/raid10.c +1 -1

@@ -1239,7 +1239,7 @@ read_again:
 	 * of r10_bios is recored in bio->bi_phys_segments just as with
 	 * the read case.
 	 */
-	plugged = mddev_check_plugged(mddev);
+	plugged = !!mddev_check_plugged(mddev, NULL, 0);
 
 	r10_bio->read_slot = -1; /* make sure repl_bio gets freed */
 	raid10_find_phys(conf, r10_bio);

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

drivers/md/raid5.c +1 -1

@@ -4012,7 +4012,7 @@ static void make_request(struct mddev *m
 	bi->bi_next = NULL;
 	bi->bi_phys_segments = 1;	/* over-loaded to count active stripes */
 
-	plugged = mddev_check_plugged(mddev);
+	plugged = !!mddev_check_plugged(mddev, NULL, 0);
 	for (;logical_sector < last_sector; logical_sector += STRIPE_SECTORS) {
 		DEFINE_WAIT(w);
 		int previous;

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. All the stripes will be released in unplug. The STRIPE_ON_UNPLUG_LIST bit is to protect concurrent access stripe lru.

Quoted 9 lines · On Mon, 25 Jun 2012 15:24:55 +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. All the stripes will be released in unplug. The STRIPE_ON_UNPLUG_LIST bit is to protect concurrent access stripe lru.

I've applied this patch, but I'm afraid I butchered it a bit first :-)

Quoted 4 lines

drivers/md/raid5.c +0 -0

@@ -3984,6 +3985,51 @@ static struct stripe_head *__get_priorit
 	return sh;
 }
 

Review static struct stripe_head *__get_priorit · line 3988

Quoted 1 line

(quoted diff) +1 -0

+#define raid5_unplug_list(mdcb) (struct list_head *)(mdcb + 1)

I really don't like this sort of construct. It is much cleaner (I think) to add to a structure by embedding it in a larger structure, then using "container_of" to map from the inner to the outer structure. So I have changed that.

Review static void make_request(struct mddev *m · line 4168

Quoted 10 lines

(quoted diff) +5 -1

@@ -4114,7 +4161,14 @@ 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);
+			/*
+			 * We must recheck here. schedule() might be called
+			 * above which makes unplug invoked already, so the old
+			 * mdcb is invalid
+			 */

I agree that this is an important check, but as a 'schedule()' can theoretically happen at any time that preempt isn't explicitly disabled, we really need to be even more careful. So I have changed the md code to disable preempt, and require the caller to re-enable preempt after it has used the returned value.

The resulting serious should appear in my for-next shortly. However for easier review I'll include two patches below. The first change mddev_check_plugged to disable preemption.
The second is a diff against your patch which changes it to use an embedded structure and container_of.
I haven't actually tested this yet, so there may be further changes.

Thanks,
NeilBrown

From 04b7dd7d0ad4a21622cad7c10821f914a8d9ccd3 Mon Sep 17 00:00:00 2001
From: NeilBrown <[email protected]>
Date: Mon, 2 Jul 2012 12:14:49 +1000
Subject: [PATCH] md/plug: disable preempt when reported a plug is present.

As 'schedule' will unplug a queue, a plug added by mddev_check_plugged is only valid until the next schedule(). So call preempt_disable before installing the plug, and require the called to call preempt_enable once the value has been used.

Signed-off-by NeilBrown <[email protected]>

drivers/md/md.c +5 -0

index 1369c9d..63ea6d6 100644
@@ -512,6 +512,10 @@ static void plugger_unplug(struct blk_plug_cb *cb)
 
 /* Check that an unplug wakeup will come shortly.
  * If not, wakeup the md thread immediately
+ * Note that the structure returned is only value until
+ * the next schedule(), so preemption is disabled when it
+ * is not NULL, and must be re-enabled after the value
+ * has been used.
  */
 struct md_plug_cb *mddev_check_plugged(struct mddev *mddev,
 				       md_unplug_func_t unplug, size_t size)

@@ -522,6 +526,7 @@ struct md_plug_cb *mddev_check_plugged(struct mddev *mddev,
 	if (!plug)
 		return NULL;
 
+	preempt_disable();
 	list_for_each_entry(mdcb, &plug->cb_list, cb.list) {
 		if (mdcb->cb.callback == plugger_unplug &&
 		    mdcb->mddev == mddev) {

drivers/md/md.c +1 -0

 	/* Not currently on the callback list */
 	if (size < sizeof(*mdcb))
 		size = sizeof(*mdcb);

@@ -540,6 +546,7 @@ struct md_plug_cb *mddev_check_plugged(struct mddev *mddev,
 	if (!mdcb)
 		return NULL;
 
+	preempt_disable();
 	mdcb->mddev = mddev;
 	mdcb->cb.callback = plugger_unplug;
 	atomic_inc(&mddev->plug_cnt);

drivers/md/raid1.c +0 -3

index ebce488..2e19b68 100644
@@ -883,7 +883,6 @@ static void make_request(struct mddev *mddev, struct bio * bio)
 	const unsigned long do_sync = (bio->bi_rw & REQ_SYNC);
 	const unsigned long do_flush_fua = (bio->bi_rw & (REQ_FLUSH | REQ_FUA));
 	struct md_rdev *blocked_rdev;
-	int plugged;
 	int first_clone;
 	int sectors_handled;
 	int max_sectors;

@@ -1034,8 +1033,6 @@ read_again:
 	 * the bad blocks.  Each set of writes gets it's own r1bio
 	 * with a set of bios attached.
 	 */
-	plugged = !!mddev_check_plugged(mddev, NULL, 0);
-
 	disks = conf->raid_disks * 2;
  retry_write:
 	blocked_rdev = NULL;

Quoted 31 lines

From 04b7dd7d0ad4a21622cad7c10821f914a8d9ccd3 Mon Sep 17 00:00:00 2001
From: NeilBrown <[email protected]>
Date: Mon, 2 Jul 2012 12:14:49 +1000
Subject: [PATCH] md/plug: disable preempt when reported a plug is present.

As 'schedule' will unplug a queue, a plug added by mddev_check_plugged is only valid until the next schedule(). So call preempt_disable before installing the plug, and require the called to call preempt_enable once the value has been used.

Signed-off-by NeilBrown <[email protected]>

drivers/md/md.c +4 -0

index 1369c9d..63ea6d6 100644
@@ -512,6 +512,10 @@ static void plugger_unplug(struct blk_plug_cb *cb)
 
 /* Check that an unplug wakeup will come shortly.
  * If not, wakeup the md thread immediately
+ * Note that the structure returned is only value until
+ * the next schedule(), so preemption is disabled when it
+ * is not NULL, and must be re-enabled after the value
+ * has been used.
  */
 struct md_plug_cb *mddev_check_plugged(struct mddev *mddev,
 				       md_unplug_func_t unplug, size_t size)

@@ -522,6 +526,7 @@ struct md_plug_cb *mddev_check_plugged(struct mddev *mddev,
 	if (!plug)
 		return NULL;
 

drivers/md/md.c struct md_plug_cb *mddev_check_plugged(struct mddev *mddev, · line 541

Quoted 9 lines

drivers/md/md.c +2 -0

+	preempt_disable();
 	list_for_each_entry(mdcb, &plug->cb_list, cb.list) {
 		if (mdcb->cb.callback == plugger_unplug &&
 		    mdcb->mddev == mddev) {

@@ -533,6 +538,7 @@ struct md_plug_cb *mddev_check_plugged(struct mddev *mddev,
 			return mdcb;
 		}
 	}
+	preempt_enable();

preempt doesn't do unplug, only yield(schedule) does, so I don't like this, just redoing mddev_check_plugged before checking the return value is fine to me.

drivers/md/md.c struct md_plug_cb *mddev_check_plugged(struct mddev *mddev, · line 544

Quoted 4 lines

drivers/md/md.c +2 -1

-	if (do_sync || !bitmap || !plugged)
+	if (do_sync ||
+	    !mddev_check_plugged(mddev, NULL, 0))
 		md_wakeup_thread(mddev->thread);

Do we really bother to recheck here? just a wakeup.

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

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

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

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

drivers/md/raid5.c +28 -11

@@ -4568,6 +4568,30 @@ static int  retry_aligned_read(struct r5
 	return handled;
 }
 
+#define MAX_STRIPE_BATCH 8
+static int handle_active_stripes(struct r5conf *conf)
+{
+	struct stripe_head *batch[MAX_STRIPE_BATCH], *sh;
+	int i, batch_size = 0;
+
+	while (batch_size < MAX_STRIPE_BATCH &&
+			(sh = __get_priority_stripe(conf)) != NULL)
+		batch[batch_size++] = sh;
+
+	if (batch_size == 0)
+		return batch_size;
+	spin_unlock_irq(&conf->device_lock);
+
+	for (i = 0; i < batch_size; i++)
+		handle_stripe(batch[i]);
+
+	cond_resched();
+
+	spin_lock_irq(&conf->device_lock);
+	for (i = 0; i < batch_size; i++)
+		__release_stripe(conf, batch[i]);
+	return batch_size;
+}
 
 /*
  * This is our raid5 kernel thread.

@@ -4578,7 +4602,6 @@ static int  retry_aligned_read(struct r5
  */
 static void raid5d(struct mddev *mddev)
 {
-	struct stripe_head *sh;
 	struct r5conf *conf = mddev->private;
 	int handled;
 	struct blk_plug plug;

@@ -4592,6 +4615,7 @@ static void raid5d(struct mddev *mddev)
 	spin_lock_irq(&conf->device_lock);
 	while (1) {
 		struct bio *bio;
+		int batch_size;
 
 		if (atomic_read(&mddev->plug_cnt) == 0 &&
 		    !list_empty(&conf->bitmap_list)) {

@@ -4616,21 +4640,16 @@ static void raid5d(struct mddev *mddev)
 			handled++;
 		}
 
-		sh = __get_priority_stripe(conf);
-
-		if (!sh)
+		batch_size = handle_active_stripes(conf);
+		if (!batch_size)
 			break;
-		spin_unlock_irq(&conf->device_lock);
-		
-		handled++;
-		handle_stripe(sh);
-		release_stripe(sh);
-		cond_resched();
+		handled += batch_size;
 
-		if (mddev->flags & ~(1<<MD_CHANGE_PENDING))

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 |  137 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/md/raid5.h |    3 +
 2 files changed, 139 insertions(+), 1 deletion(-)

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

drivers/md/raid5.c +136 -1

@@ -211,6 +211,7 @@ static void handle_release_stripe(struct
 			clear_bit(STRIPE_DELAYED, &sh->state);
 			clear_bit(STRIPE_BIT_DELAY, &sh->state);
 			list_add_tail(&sh->lru, &conf->handle_list);
+			conf->pending_stripes++;
 		}
 		md_wakeup_thread(conf->mddev->thread);
 	} else {

@@ -489,6 +490,10 @@ get_active_stripe(struct r5conf *conf, s
 			} else {
 				if (!test_bit(STRIPE_HANDLE, &sh->state))
 					atomic_inc(&conf->active_stripes);
+				else if (!list_empty(&sh->lru)
+					 && !test_bit(STRIPE_DELAYED, &sh->state)
+					 && !test_bit(STRIPE_BIT_DELAY, &sh->state))
+					conf->pending_stripes--;
 				if (list_empty(&sh->lru) &&
 				    !test_bit(STRIPE_EXPANDING, &sh->state))
 					BUG();

@@ -3670,6 +3675,7 @@ static void raid5_activate_delayed(struc
 			if (!test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
 				atomic_inc(&conf->preread_active_stripes);
 			list_add_tail(&sh->lru, &conf->hold_list);
+			conf->pending_stripes++;
 		}
 	}
 }

@@ -3979,6 +3985,7 @@ static struct stripe_head *__get_priorit
 	} else
 		return NULL;
 
+	conf->pending_stripes--;
 	list_del_init(&sh->lru);
 	atomic_inc(&sh->count);
 	BUG_ON(atomic_read(&sh->count) != 1);

@@ -4593,6 +4600,33 @@ static int handle_active_stripes(struct
 	return batch_size;
 }
 
+static void raid5auxd(struct mddev *mddev)
+{
+	struct r5conf *conf = mddev->private;
+	struct blk_plug plug;
+	int handled;
+
+	pr_debug("+++ raid5auxd active\n");
+
+	blk_start_plug(&plug);
+	handled = 0;
+	spin_lock_irq(&conf->device_lock);
+	while (1) {
+		int batch_size;
+
+		batch_size = handle_active_stripes(conf);
+		if (!batch_size)
+			break;
+		handled += batch_size;
+	}
+	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.
  *

@@ -4615,7 +4649,7 @@ static void raid5d(struct mddev *mddev)
 	spin_lock_irq(&conf->device_lock);
 	while (1) {
 		struct bio *bio;
-		int batch_size;
+		int batch_size, i;
 
 		if (atomic_read(&mddev->plug_cnt) == 0 &&
 		    !list_empty(&conf->bitmap_list)) {

@@ -4645,6 +4679,10 @@ static void raid5d(struct mddev *mddev)
 			break;
 		handled += batch_size;
 
+		for (i = 0; i < conf->aux_thread_num
+		     && i < conf->pending_stripes/MAX_STRIPE_BATCH + 1; i++)
+			md_wakeup_thread(conf->aux_threads[i]);
+
 		if (mddev->flags & ~(1<<MD_CHANGE_PENDING)) {
 			spin_unlock_irq(&conf->device_lock);
 			md_check_recovery(mddev);

@@ -4769,10 +4807,85 @@ 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 = {

@@ -4820,6 +4933,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);

@@ -4914,6 +5028,7 @@ static struct r5conf *setup_conf(struct
 	int raid_disk, memory, max_disks;
 	struct md_rdev *rdev;
 	struct disk_info *disk;
+	int i;
 
 	if (mddev->new_level != 5
 	    && mddev->new_level != 4

@@ -5037,6 +5152,22 @@ static struct r5conf *setup_conf(struct
 		printk(KERN_INFO "md/raid:%s: allocated %dkB\n",
 		       mdname(mddev), memory);
 
+	/* By default, auxthread number equals to disk number */
+	conf->aux_threads = kmalloc(sizeof(struct md_thread *) * max_disks,
+				    GFP_KERNEL);
+	if (!conf->aux_threads)
+		goto abort;
+	for (i = 0; i < max_disks; i++) {
+		char name[10];
+
+		sprintf(name, "aux%d", i);
+		conf->aux_threads[i] = md_register_thread(raid5auxd, mddev, name);
+		if (!conf->aux_threads[i])
+			break;
+	}
+
+	conf->aux_thread_num = i;
+
 	conf->thread = md_register_thread(raid5d, mddev, NULL);
 	if (!conf->thread) {
 		printk(KERN_ERR

@@ -5376,6 +5507,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 +1 -0

@@ -450,6 +450,7 @@ struct r5conf {
 	int			inactive_blocked;	/* release of inactive stripes blocked,
 							 * waiting for 25% to be free
 							 */
+	int			pending_stripes;
 	int			pool_size; /* number of disks in stripeheads in pool */
 	spinlock_t		device_lock;
 	struct disk_info	*disks;

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

Quoted 22 lines · Shaohua Li

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

Can you share a bit more about your test setup? Is this single-threaded throughput? I'm wondering if we can take advantage of keeping the work cpu local.