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

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

[Jianpeng Ma]

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 <majianpeng@gmail.com>
---
 drivers/md/raid5.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index a56aa5b..554ca9e 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -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
-- 
1.7.9.5

Re: [PATCH 3/3] raid5: Fix to_read/to_write judgement.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2685 bytes --]

On Sat, 22 Sep 2012 10:22:58 +0800 "Jianpeng Ma" <majianpeng@gmail.com> 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 <majianpeng@gmail.com>
> ---
>  drivers/md/raid5.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index a56aa5b..554ca9e 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -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


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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]