[PATCH] md: fix raid5 'repair' operations

[PATCH] md: fix raid5 'repair' operations

[Dan Williams]

commit bd2ab67030e9116f1e4aae1289220255412b37fd "md: close a livelock
window in handle_parity_checks5" introduced a bug in handling 'repair'
operations.  After a repair operation completes we clear the state bits
tracking this operation.  However, they are cleared too early and this
results in the code deciding to re-run the parity check operation.  Since
we have done the repair in memory the second check does not find a mismatch
and thus does not do a writeback.

Test results:
$ echo repair > /sys/block/md0/md/sync_action
$ cat /sys/block/md0/md/mismatch_cnt
51072
$ echo repair > /sys/block/md0/md/sync_action
$ cat /sys/block/md0/md/mismatch_cnt
0

Cc: <stable@kernel.org>
Reported-by: George Spelvin <linux@horizon.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---

 drivers/md/raid5.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)


diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 087eee0..da3390d 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2400,16 +2400,6 @@ static void handle_parity_checks5(raid5_conf_t *conf, struct stripe_head *sh,
 			canceled_check = 1; /* STRIPE_INSYNC is not set */
 	}
 
-	/* check if we can clear a parity disk reconstruct */
-	if (test_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.complete) &&
-		test_bit(STRIPE_OP_MOD_REPAIR_PD, &sh->ops.pending)) {
-
-		clear_bit(STRIPE_OP_MOD_REPAIR_PD, &sh->ops.pending);
-		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.complete);
-		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.ack);
-		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.pending);
-	}
-
 	/* start a new check operation if there are no failures, the stripe is
 	 * not insync, and a repair is not in flight
 	 */
@@ -2424,6 +2414,17 @@ static void handle_parity_checks5(raid5_conf_t *conf, struct stripe_head *sh,
 		}
 	}
 
+	/* check if we can clear a parity disk reconstruct */
+	if (test_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.complete) &&
+		test_bit(STRIPE_OP_MOD_REPAIR_PD, &sh->ops.pending)) {
+
+		clear_bit(STRIPE_OP_MOD_REPAIR_PD, &sh->ops.pending);
+		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.complete);
+		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.ack);
+		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.pending);
+	}
+
+
 	/* Wait for check parity and compute block operations to complete
 	 * before write-back.  If a failure occurred while the check operation
 	 * was in flight we need to cycle this stripe through handle_stripe

Re: [PATCH] md: fix raid5 'repair' operations

[Neil Brown]

On Thursday May 1, dan.j.williams@intel.com wrote:
> commit bd2ab67030e9116f1e4aae1289220255412b37fd "md: close a livelock
> window in handle_parity_checks5" introduced a bug in handling 'repair'
> operations.  After a repair operation completes we clear the state bits
> tracking this operation.  However, they are cleared too early and this
> results in the code deciding to re-run the parity check operation.  Since
> we have done the repair in memory the second check does not find a mismatch
> and thus does not do a writeback.

yes....
I must admit that I find that code fairly hard to make sense of, but I
can see how it was failing before and how this fixes it, and testing
confirms that, so I suspect it is right.

I cannot help feeling that there must be some way to simplify all
those .pending and .complete bits and make it somewhat clearer, but I
haven't been able to figure out how :-(

So: Acked-by: NeilBrown <neilb@suse.de>

I'm heading for a weekend, but feel free to send this to akpm.

Thanks,
NeilBrown


> 
> Test results:
> $ echo repair > /sys/block/md0/md/sync_action
> $ cat /sys/block/md0/md/mismatch_cnt
> 51072
> $ echo repair > /sys/block/md0/md/sync_action
> $ cat /sys/block/md0/md/mismatch_cnt
> 0
> 
> Cc: <stable@kernel.org>
> Reported-by: George Spelvin <linux@horizon.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> 
>  drivers/md/raid5.c |   21 +++++++++++----------
>  1 files changed, 11 insertions(+), 10 deletions(-)
> 
> 
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 087eee0..da3390d 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -2400,16 +2400,6 @@ static void handle_parity_checks5(raid5_conf_t *conf, struct stripe_head *sh,
>  			canceled_check = 1; /* STRIPE_INSYNC is not set */
>  	}
>  
> -	/* check if we can clear a parity disk reconstruct */
> -	if (test_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.complete) &&
> -		test_bit(STRIPE_OP_MOD_REPAIR_PD, &sh->ops.pending)) {
> -
> -		clear_bit(STRIPE_OP_MOD_REPAIR_PD, &sh->ops.pending);
> -		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.complete);
> -		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.ack);
> -		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.pending);
> -	}
> -
>  	/* start a new check operation if there are no failures, the stripe is
>  	 * not insync, and a repair is not in flight
>  	 */
> @@ -2424,6 +2414,17 @@ static void handle_parity_checks5(raid5_conf_t *conf, struct stripe_head *sh,
>  		}
>  	}
>  
> +	/* check if we can clear a parity disk reconstruct */
> +	if (test_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.complete) &&
> +		test_bit(STRIPE_OP_MOD_REPAIR_PD, &sh->ops.pending)) {
> +
> +		clear_bit(STRIPE_OP_MOD_REPAIR_PD, &sh->ops.pending);
> +		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.complete);
> +		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.ack);
> +		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.pending);
> +	}
> +
> +
>  	/* Wait for check parity and compute block operations to complete
>  	 * before write-back.  If a failure occurred while the check operation
>  	 * was in flight we need to cycle this stripe through handle_stripe

Re: [PATCH] md: fix raid5 'repair' operations

[Michael Tokarev]

Neil Brown wrote:
> On Thursday May 1, dan.j.williams@intel.com wrote:
>> commit bd2ab67030e9116f1e4aae1289220255412b37fd "md: close a livelock
>> window in handle_parity_checks5" introduced a bug in handling 'repair'
>> operations.  After a repair operation completes we clear the state bits
>> tracking this operation.  However, they are cleared too early and this
>> results in the code deciding to re-run the parity check operation.  Since
>> we have done the repair in memory the second check does not find a mismatch
>> and thus does not do a writeback.
> 
> yes....
> I must admit that I find that code fairly hard to make sense of, but I
> can see how it was failing before and how this fixes it, and testing
> confirms that, so I suspect it is right.
> 
> I cannot help feeling that there must be some way to simplify all
> those .pending and .complete bits and make it somewhat clearer, but I
> haven't been able to figure out how :-(
> 
> So: Acked-by: NeilBrown <neilb@suse.de>
> 
> I'm heading for a weekend, but feel free to send this to akpm.

Hmm.  Should this be sent to stable- as well?  I were just biten by
this very bug here, and after applying the patch and rebooting the
problem went away...  2.6.25.0 here.

/mjt

Re: [PATCH] md: fix raid5 'repair' operations

[George Spelvin]

Just to confirm, after 2 hours repair + 2 hours check, it seems to have
worked.  And the e2fsck didn't complain.

So if you think it's good, please submit for 2.6.25.2.

Re: [PATCH] md: fix raid5 'repair' operations

[Dan Williams]

On Fri, May 2, 2008 at 12:26 AM, Neil Brown <neilb@suse.de> wrote:
> On Thursday May 1, dan.j.williams@intel.com wrote:
>  > commit bd2ab67030e9116f1e4aae1289220255412b37fd "md: close a livelock
>  > window in handle_parity_checks5" introduced a bug in handling 'repair'
>  > operations.  After a repair operation completes we clear the state bits
>  > tracking this operation.  However, they are cleared too early and this
>  > results in the code deciding to re-run the parity check operation.  Since
>  > we have done the repair in memory the second check does not find a mismatch
>  > and thus does not do a writeback.
>
>  yes....
>  I must admit that I find that code fairly hard to make sense of, but I
>  can see how it was failing before and how this fixes it, and testing
>  confirms that, so I suspect it is right.
>
>  I cannot help feeling that there must be some way to simplify all
>  those .pending and .complete bits and make it somewhat clearer, but I
>  haven't been able to figure out how :-(
>

Agreed, the current scheme is not easily readable, and has proven
tricky to manipulate.  I will spend some cycles looking at this...

>  So: Acked-by: NeilBrown <neilb@suse.de>
>
>  I'm heading for a weekend, but feel free to send this to akpm.
>
>  Thanks,
>  NeilBrown
>

Thanks,
Dan

Re: [PATCH] md: fix raid5 'repair' operations

[Dan Williams]

On Fri, May 2, 2008 at 9:24 AM, George Spelvin <linux@horizon.com> wrote:
> Just to confirm, after 2 hours repair + 2 hours check, it seems to have
>  worked.  And the e2fsck didn't complain.
>
>  So if you think it's good, please submit for 2.6.25.2.

Thanks for reporting this and confirming the fix,
Dan

[PATCH] md: fix raid5 'repair' operations

[Dan Williams]

commit bd2ab67030e9116f1e4aae1289220255412b37fd "md: close a livelock
window in handle_parity_checks5" introduced a bug in handling 'repair'
operations.  After a repair operation completes we clear the state bits
tracking this operation.  However, they are cleared too early and this
results in the code deciding to re-run the parity check operation.  Since
we have done the repair in memory the second check does not find a mismatch
and thus does not do a writeback.

Test results:
$ echo repair > /sys/block/md0/md/sync_action
$ cat /sys/block/md0/md/mismatch_cnt
51072
$ echo repair > /sys/block/md0/md/sync_action
$ cat /sys/block/md0/md/mismatch_cnt
0

(also fix incorrect indentation)

Cc: <stable@kernel.org>
Tested-by: George Spelvin <linux@horizon.com>
Acked-by: NeilBrown <neilb@suse.de>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---

 drivers/md/raid5.c |   25 +++++++++++++------------
 1 files changed, 13 insertions(+), 12 deletions(-)


diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 087eee0..ee0ea91 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2369,8 +2369,8 @@ static void handle_parity_checks5(raid5_conf_t *conf, struct stripe_head *sh,
 
 	/* complete a check operation */
 	if (test_and_clear_bit(STRIPE_OP_CHECK, &sh->ops.complete)) {
-	    clear_bit(STRIPE_OP_CHECK, &sh->ops.ack);
-	    clear_bit(STRIPE_OP_CHECK, &sh->ops.pending);
+		clear_bit(STRIPE_OP_CHECK, &sh->ops.ack);
+		clear_bit(STRIPE_OP_CHECK, &sh->ops.pending);
 		if (s->failed == 0) {
 			if (sh->ops.zero_sum_result == 0)
 				/* parity is correct (on disc,
@@ -2400,16 +2400,6 @@ static void handle_parity_checks5(raid5_conf_t *conf, struct stripe_head *sh,
 			canceled_check = 1; /* STRIPE_INSYNC is not set */
 	}
 
-	/* check if we can clear a parity disk reconstruct */
-	if (test_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.complete) &&
-		test_bit(STRIPE_OP_MOD_REPAIR_PD, &sh->ops.pending)) {
-
-		clear_bit(STRIPE_OP_MOD_REPAIR_PD, &sh->ops.pending);
-		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.complete);
-		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.ack);
-		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.pending);
-	}
-
 	/* start a new check operation if there are no failures, the stripe is
 	 * not insync, and a repair is not in flight
 	 */
@@ -2424,6 +2414,17 @@ static void handle_parity_checks5(raid5_conf_t *conf, struct stripe_head *sh,
 		}
 	}
 
+	/* check if we can clear a parity disk reconstruct */
+	if (test_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.complete) &&
+	    test_bit(STRIPE_OP_MOD_REPAIR_PD, &sh->ops.pending)) {
+
+		clear_bit(STRIPE_OP_MOD_REPAIR_PD, &sh->ops.pending);
+		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.complete);
+		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.ack);
+		clear_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.pending);
+	}
+
+
 	/* Wait for check parity and compute block operations to complete
 	 * before write-back.  If a failure occurred while the check operation
 	 * was in flight we need to cycle this stripe through handle_stripe