raid5: remove unnecessary bitmap write optimization

PATCH Superseded Shaohua Li 2 participants 2 messages Jul 3, 2012 – Jul 4, 2012 MD RAID

Share & tools

Shaohua Li

author

2012-07-03 07:51 UTC #0 lore raw

PATCH 3/3 raid5: remove unnecessary bitmap write optimization

Neil pointed out the bitmap write optimization in handle_stripe_clean_event() is unnecessary, because the chance one stripe gets written twice in the mean time is rare. We can always do a bitmap_startwrite when a write request is added to a stripe and bitmap_endwrite after write request is done. Delete the optimization. With it, we can delete some cases of stripe_lock.

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

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

drivers/md/raid5.c +8 ~~-20~~

Index: linux/drivers/md/raid5.c
===================================================================
@@ -2350,7 +2350,7 @@ static int add_stripe_bio(struct stripe_
 	spin_lock_irq(&sh->stripe_lock);
 	if (forwrite) {
 		bip = &sh->dev[dd_idx].towrite;
-		if (*bip == NULL && sh->dev[dd_idx].written == NULL)
+		if (*bip == NULL)
 			firstwrite = 1;
 	} else
 		bip = &sh->dev[dd_idx].toread;

@@ -2427,7 +2427,6 @@ handle_failed_stripe(struct r5conf *conf
 	int i;
 	for (i = disks; i--; ) {
 		struct bio *bi;
-		int bitmap_end = 0;
 
 		if (test_bit(R5_ReadError, &sh->dev[i].flags)) {
 			struct md_rdev *rdev;

@@ -2451,10 +2450,9 @@ handle_failed_stripe(struct r5conf *conf
 		/* fail all writes first */
 		bi = sh->dev[i].towrite;
 		sh->dev[i].towrite = NULL;
-		if (bi) {
+		if (bi)
 			s->to_write--;
-			bitmap_end = 1;
-		}
+		spin_unlock_irq(&sh->stripe_lock);
 
 		if (test_and_clear_bit(R5_Overlap, &sh->dev[i].flags))
 			wake_up(&conf->wait_for_overlap);

@@ -2473,7 +2471,6 @@ handle_failed_stripe(struct r5conf *conf
 		/* and fail all 'written' */
 		bi = sh->dev[i].written;
 		sh->dev[i].written = NULL;
-		if (bi) bitmap_end = 1;
 		while (bi && bi->bi_sector <
 		       sh->dev[i].sector + STRIPE_SECTORS) {
 			struct bio *bi2 = r5_next_bio(bi, sh->dev[i].sector);

@@ -2509,10 +2506,8 @@ handle_failed_stripe(struct r5conf *conf
 				bi = nextbi;
 			}
 		}
-		spin_unlock_irq(&sh->stripe_lock);
-		if (bitmap_end)
-			bitmap_endwrite(conf->mddev->bitmap, sh->sector,
-					STRIPE_SECTORS, 0, 0);
+		bitmap_endwrite(conf->mddev->bitmap, sh->sector,
+				STRIPE_SECTORS, 0, 0);
 		/* If we were in the middle of a write the parity block might
 		 * still be locked - so just clear all R5_LOCKED flags
 		 */

@@ -2713,9 +2708,7 @@ static void handle_stripe_clean_event(st
 				test_bit(R5_UPTODATE, &dev->flags)) {
 				/* We can return any write requests */
 				struct bio *wbi, *wbi2;
-				int bitmap_end = 0;
 				pr_debug("Return write for disc %d\n", i);
-				spin_lock_irq(&sh->stripe_lock);
 				wbi = dev->written;
 				dev->written = NULL;
 				while (wbi && wbi->bi_sector <

@@ -2728,15 +2721,10 @@ static void handle_stripe_clean_event(st
 					}
 					wbi = wbi2;
 				}
-				if (dev->towrite == NULL)
-					bitmap_end = 1;
-				spin_unlock_irq(&sh->stripe_lock);
-				if (bitmap_end)
-					bitmap_endwrite(conf->mddev->bitmap,
-							sh->sector,
-							STRIPE_SECTORS,
+				bitmap_endwrite(conf->mddev->bitmap, sh->sector,
+						STRIPE_SECTORS,
 					 !test_bit(STRIPE_DEGRADED, &sh->state),
-							0);
+						0);
 			}
 		}

NeilBrown

2012-07-04 03:17 UTC #1 lore raw

Quoted 56 lines · On Tue, 3 Jul 2012 15:51:11 +0800 Shaohua Li <[email protected]> wrote:

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

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

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

drivers/md/raid5.c +3 -6

@@ -2350,7 +2350,7 @@ static int add_stripe_bio(struct stripe_
 	spin_lock_irq(&sh->stripe_lock);
 	if (forwrite) {
 		bip = &sh->dev[dd_idx].towrite;
-		if (*bip == NULL && sh->dev[dd_idx].written == NULL)
+		if (*bip == NULL)
 			firstwrite = 1;
 	} else
 		bip = &sh->dev[dd_idx].toread;

@@ -2427,7 +2427,6 @@ handle_failed_stripe(struct r5conf *conf
 	int i;
 	for (i = disks; i--; ) {
 		struct bio *bi;
-		int bitmap_end = 0;
 
 		if (test_bit(R5_ReadError, &sh->dev[i].flags)) {
 			struct md_rdev *rdev;

@@ -2451,10 +2450,9 @@ handle_failed_stripe(struct r5conf *conf
 		/* fail all writes first */
 		bi = sh->dev[i].towrite;
 		sh->dev[i].towrite = NULL;
-		if (bi) {
+		if (bi)
 			s->to_write--;
-			bitmap_end = 1;
-		}
+		spin_unlock_irq(&sh->stripe_lock);
 
 		if (test_and_clear_bit(R5_Overlap, &sh->dev[i].flags))
 			wake_up(&conf->wait_for_overlap);

@@ -2473,7 +2471,6 @@ handle_failed_stripe(struct r5conf *conf
 		/* and fail all 'written' */
 		bi = sh->dev[i].written;
 		sh->dev[i].written = NULL;
-		if (bi) bitmap_end = 1;
 		while (bi && bi->bi_sector <
 		       sh->dev[i].sector + STRIPE_SECTORS) {
 			struct bio *bi2 = r5_next_bio(bi, sh->dev[i].sector);

@@ -2509,10 +2506,8 @@ handle_failed_stripe(struct r5conf *conf
 				bi = nextbi;
 			}

drivers/md/raid5.c handle_failed_stripe(struct r5conf *conf · line 2513

Quoted 10 lines

drivers/md/raid5.c +2 -4

 		}
-		spin_unlock_irq(&sh->stripe_lock);
-		if (bitmap_end)
-			bitmap_endwrite(conf->mddev->bitmap, sh->sector,
-					STRIPE_SECTORS, 0, 0);
+		bitmap_endwrite(conf->mddev->bitmap, sh->sector,
+				STRIPE_SECTORS, 0, 0);
 		/* If we were in the middle of a write the parity block might
 		 * still be locked - so just clear all R5_LOCKED flags
 		 */

Thanks.
However this section - handle_failed_stripe - isn't correct.

bitmap_startwrite and bitmap_endwrite increment and decrement a counter and so must be balanced.
We are now counting once for each list that is on either ->towrite or ->written. We bitmap_startwrite when we set ->towrite, we then move that to ->written. Then when we remove from ->written we bitmap_endwrite. In the handle_failed_stripe case we may remove 0, 1, or 2 lists. So we need to call bitmap_endwrite 0, 1, or 2 times.
So if towrite was not NULL, we want to call bitmap_endwrite
then if written was not NULL we want to call it again.

Also I think I'd prefer it if this patch were before "add a per-stripe lock". It is best to first get rid of use of device_lock first, then change some of the remaining ones to stripe_lock. Changing some to stripe_lock, then discarding them seems messy.

Thanks,
NeilBrown

Quoted 29 lines

drivers/md/raid5.c +3 ~~-10~~

@@ -2713,9 +2708,7 @@ static void handle_stripe_clean_event(st
 				test_bit(R5_UPTODATE, &dev->flags)) {
 				/* We can return any write requests */
 				struct bio *wbi, *wbi2;
-				int bitmap_end = 0;
 				pr_debug("Return write for disc %d\n", i);
-				spin_lock_irq(&sh->stripe_lock);
 				wbi = dev->written;
 				dev->written = NULL;
 				while (wbi && wbi->bi_sector <

@@ -2728,15 +2721,10 @@ static void handle_stripe_clean_event(st
 					}
 					wbi = wbi2;
 				}
-				if (dev->towrite == NULL)
-					bitmap_end = 1;
-				spin_unlock_irq(&sh->stripe_lock);
-				if (bitmap_end)
-					bitmap_endwrite(conf->mddev->bitmap,
-							sh->sector,
-							STRIPE_SECTORS,
+				bitmap_endwrite(conf->mddev->bitmap, sh->sector,
+						STRIPE_SECTORS,
 					 !test_bit(STRIPE_DEGRADED, &sh->state),
-							0);
+						0);
 			}
 		}