raid5: Fix to_read/to_write judgement.

PATCH In review Jianpeng Ma 2 participants 2 messages Sep 22, 2012 – Sep 25, 2012 MD RAID

Share & tools

Jianpeng Ma

author

2012-09-22 02:22 UTC #0 lore raw

PATCH 3/3 raid5: Fix to_read/to_write judgement.

In func analyse_stripe, it added to_read/to_write by sh->dev[i]->toread/towrite.
If stripe failed, in func handle_failed_stripe it decreased to_read/to_write also by sh->dev[i]->toread/towrite.
But between func analyse_stripe and handle_failed_stripe, toread/towrite can change.So the to_read/to_write maybe less zero.
For example, if to_write was less zero,it may introduce a bug in func async_xor:
'BUG_ON(src_cnt <= 1);'.
So after handle_failed_stripe,it should use 'to_read/to_write > 0' instead of judging 'to_read/to_write'.

Signed-off-by Jianpeng Ma <[email protected]>

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

drivers/md/raid5.c +4 -4

index a56aa5b..554ca9e 100644
@@ -2641,7 +2641,7 @@ static int fetch_block(struct stripe_head *sh, struct stripe_head_state *s,
 	     (s->failed >= 2 && fdev[1]->toread) ||
 	     (sh->raid_conf->level <= 5 && s->failed && fdev[0]->towrite &&
 	      !test_bit(R5_OVERWRITE, &fdev[0]->flags)) ||
-	     (sh->raid_conf->level == 6 && s->failed && s->to_write))) {
+	     (sh->raid_conf->level == 6 && s->failed && s->to_write > 0))) {
 		/* we would like to get this block, possibly by computing it,
 		 * otherwise read it if the backing disk is insync
 		 */

@@ -3471,8 +3471,8 @@ static void handle_stripe(struct stripe_head *sh)
 	 * parity, or to satisfy requests
 	 * or to load a block that is being partially written.
 	 */
-	if (s.to_read || s.non_overwrite
-	    || (conf->level == 6 && s.to_write && s.failed)
+	if (s.to_read > 0 || s.non_overwrite
+	    || (conf->level == 6 && s.to_write > 0 && s.failed)
 	    || (s.syncing && (s.uptodate + s.compute < disks))
 	    || s.replacing
 	    || s.expanding)

@@ -3519,7 +3519,7 @@ static void handle_stripe(struct stripe_head *sh)
 	 * 2/ A 'check' operation is in flight, as it may clobber the parity
 	 *    block.
 	 */
-	if (s.to_write && !sh->reconstruct_state && !sh->check_state)
+	if (s.to_write > 0 && !sh->reconstruct_state && !sh->check_state)
 		handle_stripe_dirtying(conf, sh, &s, disks);
 
 	/* maybe we need to check and possibly fix the parity for this stripe

Signature · 1 line

1.7.9.5

NeilBrown

2012-09-25 07:12 UTC #1 lore raw

Quoted 41 lines · On Sat, 22 Sep 2012 10:22:58 +0800 "Jianpeng Ma" <[email protected]> wrote:

In func analyse_stripe, it added to_read/to_write by sh->dev[i]->toread/towrite.
If stripe failed, in func handle_failed_stripe it decreased to_read/to_write also by sh->dev[i]->toread/towrite.
But between func analyse_stripe and handle_failed_stripe, toread/towrite can change.So the to_read/to_write maybe less zero.
For example, if to_write was less zero,it may introduce a bug in func async_xor:
'BUG_ON(src_cnt <= 1);'.
So after handle_failed_stripe,it should use 'to_read/to_write > 0' instead of judging 'to_read/to_write'.
Signed-off-by Jianpeng Ma <[email protected]>
---
 drivers/md/raid5.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/md/raid5.c +3 -3
index a56aa5b..554ca9e 100644
@@ -2641,7 +2641,7 @@ static int fetch_block(struct stripe_head *sh, struct stripe_head_state *s,
 	     (s->failed >= 2 && fdev[1]->toread) ||
 	     (sh->raid_conf->level <= 5 && s->failed && fdev[0]->towrite &&
 	      !test_bit(R5_OVERWRITE, &fdev[0]->flags)) ||
-	     (sh->raid_conf->level == 6 && s->failed && s->to_write))) {
+	     (sh->raid_conf->level == 6 && s->failed && s->to_write > 0))) {
 		/* we would like to get this block, possibly by computing it,
 		 * otherwise read it if the backing disk is insync
 		 */
@@ -3471,8 +3471,8 @@ static void handle_stripe(struct stripe_head *sh)
 	 * parity, or to satisfy requests
 	 * or to load a block that is being partially written.
 	 */
-	if (s.to_read || s.non_overwrite
-	    || (conf->level == 6 && s.to_write && s.failed)
+	if (s.to_read > 0 || s.non_overwrite
+	    || (conf->level == 6 && s.to_write > 0 && s.failed)
 	    || (s.syncing && (s.uptodate + s.compute < disks))
 	    || s.replacing
 	    || s.expanding)

drivers/md/raid5.c static void handle_stripe(struct stripe_head *sh) · line 3525

Quoted 8 lines

drivers/md/raid5.c +1 -1

@@ -3519,7 +3519,7 @@ static void handle_stripe(struct stripe_head *sh)
 	 * 2/ A 'check' operation is in flight, as it may clobber the parity
 	 *    block.
 	 */
-	if (s.to_write && !sh->reconstruct_state && !sh->check_state)
+	if (s.to_write > 0 && !sh->reconstruct_state && !sh->check_state)
 		handle_stripe_dirtying(conf, sh, &s, disks);
 
 	/* maybe we need to check and possibly fix the parity for this stripe

Thanks.
However I would prefer to fix this by not decrementing the counters in handle_failed_stripe. There is no harm in the counters being a little bigger than they should be.
I have queued a patch to do this.

Thanks,
NeilBrown