Skip to contentSkip to conversation

md: Fix data corruption when a degraded raid5 array is reshaped.

PATCH In review NeilBrown 2 participants 4 messages Jan 3, 2008 – Jan 4, 2008 1 message not in the archive MD RAID

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): 20080104094516.19216.patches@notabene

NeilBrown

author
#1 lore raw

PATCH 1/1 md: Fix data corruption when a degraded raid5 array is reshaped.

This patch fixes a fairly serious bug in md/raid5 in 2.6.23 and 24-rc. It would be great if it cold get into 23.13 and 24.final. Thanks.
NeilBrown

### Comments for Changeset

We currently do not wait for the block from the missing device to be computed from parity before copying data to the new stripe layout.

The change in the raid6 code is not techincally needed as we don't delay data block recovery in the same way for raid6 yet. But making the change now is safer long-term.

This bug exists in 2.6.23 and 2.6.24-rc

### Diffstat output

 ./drivers/md/raid5.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/md/raid5.c +2 -2
diff .prev/drivers/md/raid5.c ./drivers/md/raid5.c
@@ -2865,7 +2865,7 @@ static void handle_stripe5(struct stripe
 		md_done_sync(conf->mddev, STRIPE_SECTORS, 1);
 	}
 
-	if (s.expanding && s.locked == 0)
+	if (s.expanding && s.locked == 0 && s.req_compute == 0)
 		handle_stripe_expansion(conf, sh, NULL);
 
 	if (sh->ops.count)
@@ -3067,7 +3067,7 @@ static void handle_stripe6(struct stripe
 		md_done_sync(conf->mddev, STRIPE_SECTORS, 1);
 	}
 
-	if (s.expanding && s.locked == 0)
+	if (s.expanding && s.locked == 0 && s.req_compute == 0)
 		handle_stripe_expansion(conf, sh, &r6s);
 
 	spin_unlock(&sh->lock);
Acked-by on v1 1/1
#2 lore raw
Quoted 15 lines · NeilBrown

This patch fixes a fairly serious bug in md/raid5 in 2.6.23 and 24-rc. It would be great if it cold get into 23.13 and 24.final. Thanks.
NeilBrown

### Comments for Changeset

We currently do not wait for the block from the missing device to be computed from parity before copying data to the new stripe layout.

The change in the raid6 code is not techincally needed as we don't delay data block recovery in the same way for raid6 yet. But making the change now is safer long-term.

Quoted 6 lines

This bug exists in 2.6.23 and 2.6.24-rc

#3 lore raw
Quoted 17 lines · Dan J
Quoted 1 line · NeilBrown

This patch fixes a fairly serious bug in md/raid5 in 2.6.23 and

24-rc.

Quoted 14 lines

It would be great if it cold get into 23.13 and 24.final. Thanks.
NeilBrown

### Comments for Changeset

We currently do not wait for the block from the missing device to be computed from parity before copying data to the new stripe layout.

The change in the raid6 code is not techincally needed as we don't delay data block recovery in the same way for raid6 yet. But making the change now is safer long-term.

Quoted 8 lines
Quoted 6 lines

This bug exists in 2.6.23 and 2.6.24-rc

On closer look the safer test is:

	!test_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.pending).

The 'req_compute' field only indicates that a 'compute_block' operation was requested during this pass through handle_stripe so that we can issue a linked chain of asynchronous operations.

---

From: Neil Brown <[email protected]>

md: Fix data corruption when a degraded raid5 array is reshaped.

We currently do not wait for the block from the missing device to be computed from parity before copying data to the new stripe layout.

The change in the raid6 code is not techincally needed as we don't delay data block recovery in the same way for raid6 yet. But making the change now is safer long-term.

This bug exists in 2.6.23 and 2.6.24-rc

---

 drivers/md/raid5.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
drivers/md/raid5.c +4 -2
index a5aad8c..e8c8157 100644
@@ -2865,7 +2865,8 @@ static void handle_stripe5(struct stripe_head *sh)
 		md_done_sync(conf->mddev, STRIPE_SECTORS, 1);
 	}
 
-	if (s.expanding && s.locked == 0)
+	if (s.expanding && s.locked == 0 &&
+	    !test_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.pending))
 		handle_stripe_expansion(conf, sh, NULL);
 
 	if (sh->ops.count)
@@ -3067,7 +3068,8 @@ static void handle_stripe6(struct stripe_head *sh, struct page *tmp_page)
 		md_done_sync(conf->mddev, STRIPE_SECTORS, 1);
 	}
 
-	if (s.expanding && s.locked == 0)
+	if (s.expanding && s.locked == 0 &&
+	    !test_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.pending))
 		handle_stripe_expansion(conf, sh, &r6s);
 
 	spin_unlock(&sh->lock);
author Acked-by
#4 lore raw
Quoted 5 lines · On Thursday January 3, [email protected] wrote:

On closer look the safer test is:

	!test_bit(STRIPE_OP_COMPUTE_BLK, &sh->ops.pending).
Quoted 7 lines

The 'req_compute' field only indicates that a 'compute_block' operation was requested during this pass through handle_stripe so that we can issue a linked chain of asynchronous operations.

---

From: Neil Brown <[email protected]>

Technically that should probably be
From: Dan Williams <[email protected]>

now, and then I add

because I completely agree with your improvement.

We should keep an eye out for then Andrew commits this and make sure the right patch goes in...

Thanks,
NeilBrown