Skip to contentSkip to conversation

md: Fixes for md in 2.6.23

PATCH In review NeilBrown 3 participants 10 messages Oct 22, 2007 – Nov 15, 2007 MD RAID

lore
Share & tools
lore

NeilBrown

author
#0 lore raw

PATCH 1/1 md: Fixes for md in 2.6.23

It appears that a couple of bugs slipped in to md for 2.6.23. These two patches fix them and are appropriate for 2.6.23.y as well as 2.6.24-rcX

Thanks,
NeilBrown

[PATCH 001 of 2] md: Fix an unsigned compare to allow creation of bitmaps with v1.0 metadata. [PATCH 002 of 2] md: raid5: fix clearing of biofill operations

author
#1 lore raw

PATCH 1/1 md: Fix an unsigned compare to allow creation of bitmaps with v1.0 metadata.

As page->index is unsigned, this all becomes an unsigned comparison, which
almost always returns an error.

Signed-off-by Neil Brown <[email protected]>
Cc Stable <[email protected]>

### Diffstat output

 ./drivers/md/bitmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/md/bitmap.c +1 -1
diff .prev/drivers/md/bitmap.c ./drivers/md/bitmap.c
@@ -274,7 +274,7 @@ static int write_sb_page(struct bitmap *
 			if (bitmap->offset < 0) {
 				/* DATA  BITMAP METADATA  */
 				if (bitmap->offset
-				    + page->index * (PAGE_SIZE/512)
+				    + (long)(page->index * (PAGE_SIZE/512))
 				    + size/512 > 0)
 					/* bitmap runs in to metadata */
 					return -EINVAL;
author
#2 lore raw

PATCH 1/1 md: raid5: fix clearing of biofill operations

In reply to NeilBrown

From: Dan Williams <[email protected]>

ops_complete_biofill() runs outside of spin_lock(&sh->lock) and clears the 'pending' and 'ack' bits. Since the test_and_ack_op() macro only checks against 'complete' it can get an inconsistent snapshot of pending work.

Move the clearing of these bits to handle_stripe5(), under the lock.

Signed-off-by Dan Williams <[email protected]>
Tested-by Joël Bertrand <[email protected]>
Signed-off-by Neil Brown <[email protected]>
Cc Stable <[email protected]>

### Diffstat output

 ./drivers/md/raid5.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)
drivers/md/raid5.c +14 -3
diff .prev/drivers/md/raid5.c ./drivers/md/raid5.c
@@ -665,7 +665,12 @@ static unsigned long get_stripe_work(str
 		ack++;
 
 	sh->ops.count -= ack;
-	BUG_ON(sh->ops.count < 0);
+	if (unlikely(sh->ops.count < 0)) {
+		printk(KERN_ERR "pending: %#lx ops.pending: %#lx ops.ack: %#lx "
+			"ops.complete: %#lx\n", pending, sh->ops.pending,
+			sh->ops.ack, sh->ops.complete);
+		BUG();
+	}
 
 	return pending;
 }

@@ -842,8 +847,7 @@ static void ops_complete_biofill(void *s
 			}
 		}
 	}
-	clear_bit(STRIPE_OP_BIOFILL, &sh->ops.ack);
-	clear_bit(STRIPE_OP_BIOFILL, &sh->ops.pending);
+	set_bit(STRIPE_OP_BIOFILL, &sh->ops.complete);
 
 	return_io(return_bi);
 

@@ -3130,6 +3134,13 @@ static void handle_stripe5(struct stripe
 	s.expanded = test_bit(STRIPE_EXPAND_READY, &sh->state);
 	/* Now to look around and see what can be done */
 
+	/* clean-up completed biofill operations */
+	if (test_bit(STRIPE_OP_BIOFILL, &sh->ops.complete)) {
+		clear_bit(STRIPE_OP_BIOFILL, &sh->ops.pending);
+		clear_bit(STRIPE_OP_BIOFILL, &sh->ops.ack);
+		clear_bit(STRIPE_OP_BIOFILL, &sh->ops.complete);
+	}
+
 	rcu_read_lock();
 	for (i=disks; i--; ) {
 		mdk_rdev_t *rdev;

-
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
In reply to NeilBrown
Quoted 10 lines · NeilBrown

It appears that a couple of bugs slipped in to md for 2.6.23. These two patches fix them and are appropriate for 2.6.23.y as well as 2.6.24-rcX

Thanks,
NeilBrown

[PATCH 001 of 2] md: Fix an unsigned compare to allow creation of bitmaps with v1.0 metadata. [PATCH 002 of 2] md: raid5: fix clearing of biofill operations

I don't see these patches in 2.6.24-rcX, are they there under some other subject?

thanks,

greg k-h

#4 lore raw
Quoted 6 lines · Greg KH
Quoted 5 lines · NeilBrown

It appears that a couple of bugs slipped in to md for 2.6.23. These two patches fix them and are appropriate for 2.6.23.y as well as 2.6.24-rcX

Quoted 8 lines
Quoted 5 lines

Thanks,
NeilBrown

[PATCH 001 of 2] md: Fix an unsigned compare to allow creation of bitmaps with v1.0 metadata. [PATCH 002 of 2] md: raid5: fix clearing of biofill operations

I don't see these patches in 2.6.24-rcX, are they there under some other subject?

Oh nevermind, I found them, sorry for the noise...

greg k-h

#5 lore raw
Quoted 10 lines · Greg KH
Quoted 9 lines · Greg KH
Quoted 8 lines · NeilBrown

It appears that a couple of bugs slipped in to md for 2.6.23. These two patches fix them and are appropriate for 2.6.23.y as well as 2.6.24-rcX

Thanks,
NeilBrown

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

[PATCH 001 of 2] md: Fix an unsigned compare to allow creation of bitmaps with v1.0 metadata. [PATCH 002 of 2] md: raid5: fix clearing of biofill operations

I don't see these patches in 2.6.24-rcX, are they there under some other subject?

Oh nevermind, I found them, sorry for the noise...

Careful, it looks like you cherry picked commit 4ae3f847 "md: raid5:
fix clearing of biofill operations" which ended up misapplied in Linus' tree, You should either also pick up def6ae26 "md: fix misapplied patch in raid5.c" or I can resend the original "raid5: fix clearing of biofill operations."

The other patch for -stable "raid5: fix unending write sequence" is currently in -mm.

Quoted 1 line

greg k-h

Regards,
Dan

Tested-by Joel Bertrand
#6 lore raw
Quoted 20 lines · Dan Williams
Quoted 10 lines · Greg KH
Quoted 9 lines · Greg KH
Quoted 8 lines · NeilBrown

It appears that a couple of bugs slipped in to md for 2.6.23. These two patches fix them and are appropriate for 2.6.23.y as well as 2.6.24-rcX

Thanks,
NeilBrown

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

[PATCH 001 of 2] md: Fix an unsigned compare to allow creation of bitmaps with v1.0 metadata. [PATCH 002 of 2] md: raid5: fix clearing of biofill operations

I don't see these patches in 2.6.24-rcX, are they there under some other subject?

Oh nevermind, I found them, sorry for the noise...

Quoted 8 lines

Careful, it looks like you cherry picked commit 4ae3f847 "md: raid5:
fix clearing of biofill operations" which ended up misapplied in Linus' tree, You should either also pick up def6ae26 "md: fix misapplied patch in raid5.c" or I can resend the original "raid5: fix clearing of biofill operations."

The other patch for -stable "raid5: fix unending write sequence" is currently in -mm.

Hm, I've attached the two patches that I have right now in the -stable tree so far (still have over 100 patches to go, so I might not have gotten to them yet if you have sent them). These were sent to me by Andrew on their way to Linus. if I should drop either one, or add another one, please let me know.

thanks,

greg k-h

Quoted 1 line

From [email protected] Mon Oct 22 20:45:35 2007

From: [email protected]
Date: Mon, 22 Oct 2007 20:45:11 -0700
Subject: md: fix an unsigned compare to allow creation of bitmaps with v1.0 metadata
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Message-ID: <[email protected]>

From: NeilBrown <[email protected]>

patch 85bfb4da8cad483a4e550ec89060d05a4daf895b in mainline.

As page->index is unsigned, this all becomes an unsigned comparison, which
almost always returns an error.

Signed-off-by Neil Brown <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
Signed-off-by Linus Torvalds <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 drivers/md/bitmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/md/bitmap.c +1 -1
@@ -274,7 +274,7 @@ static int write_sb_page(struct bitmap *
 			if (bitmap->offset < 0) {
 				/* DATA  BITMAP METADATA  */
 				if (bitmap->offset
-				    + page->index * (PAGE_SIZE/512)
+				    + (long)(page->index * (PAGE_SIZE/512))
 				    + size/512 > 0)
 					/* bitmap runs in to metadata */
 					return -EINVAL;
Quoted 1 line

From [email protected] Mon Oct 22 20:45:44 2007

From: Dan Williams <[email protected]>
Date: Mon, 22 Oct 2007 20:45:11 -0700
Subject: md: raid5: fix clearing of biofill operations
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Message-ID: <[email protected]>

From: Dan Williams <[email protected]>

patch 4ae3f847e49e3787eca91bced31f8fd328d50496 in mainline.

ops_complete_biofill() runs outside of spin_lock(&sh->lock) and clears the 'pending' and 'ack' bits. Since the test_and_ack_op() macro only checks against 'complete' it can get an inconsistent snapshot of pending work.

Move the clearing of these bits to handle_stripe5(), under the lock.

Signed-off-by Dan Williams <[email protected]>
Tested-by Joel Bertrand <[email protected]>
Signed-off-by Neil Brown <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
Signed-off-by Linus Torvalds <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 drivers/md/raid5.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)
drivers/md/raid5.c +14 -3
@@ -377,7 +377,12 @@ static unsigned long get_stripe_work(str
 		ack++;
 
 	sh->ops.count -= ack;
-	BUG_ON(sh->ops.count < 0);
+	if (unlikely(sh->ops.count < 0)) {
+		printk(KERN_ERR "pending: %#lx ops.pending: %#lx ops.ack: %#lx "
+			"ops.complete: %#lx\n", pending, sh->ops.pending,
+			sh->ops.ack, sh->ops.complete);
+		BUG();
+	}
 
 	return pending;
 }

@@ -551,8 +556,7 @@ static void ops_complete_biofill(void *s
 			}
 		}
 	}
-	clear_bit(STRIPE_OP_BIOFILL, &sh->ops.ack);
-	clear_bit(STRIPE_OP_BIOFILL, &sh->ops.pending);
+	set_bit(STRIPE_OP_BIOFILL, &sh->ops.complete);
 
 	return_io(return_bi);
 

@@ -2903,6 +2907,13 @@ static void handle_stripe6(struct stripe
 	s.expanded = test_bit(STRIPE_EXPAND_READY, &sh->state);
 	/* Now to look around and see what can be done */
 
+	/* clean-up completed biofill operations */
+	if (test_bit(STRIPE_OP_BIOFILL, &sh->ops.complete)) {
+		clear_bit(STRIPE_OP_BIOFILL, &sh->ops.pending);
+		clear_bit(STRIPE_OP_BIOFILL, &sh->ops.ack);
+		clear_bit(STRIPE_OP_BIOFILL, &sh->ops.complete);
+	}
+
 	rcu_read_lock();
 	for (i=disks; i--; ) {
 		mdk_rdev_t *rdev;
Tested-by Joel Bertrand Tested-by Joel Bertrand
#7 lore raw
Quoted 7 lines · Greg KH
Quoted 7 lines

Careful, it looks like you cherry picked commit 4ae3f847 "md: raid5:
fix clearing of biofill operations" which ended up misapplied in Linus' tree, You should either also pick up def6ae26 "md: fix misapplied patch in raid5.c" or I can resend the original "raid5: fix clearing of biofill operations."

Quoted 9 lines
Quoted 2 lines

The other patch for -stable "raid5: fix unending write sequence" is currently in -mm.

Hm, I've attached the two patches that I have right now in the -stable tree so far (still have over 100 patches to go, so I might not have gotten to them yet if you have sent them). These were sent to me by Andrew on their way to Linus. if I should drop either one, or add another one, please let me know.

Drop md-raid5-fix-clearing-of-biofill-operations.patch and replace it with the attached md-raid5-not-raid6-fix-clearing-of-biofill-operations.patch (the original sent to Neil).

The critical difference is that the replacement patch touches handle_stripe5, not handle_stripe6. Diffing the patches shows the changes for hunk #3:

-@@ -2903,6 +2907,13 @@ static void handle_stripe6(struct stripe +@@ -2630,6 +2634,13 @@ static void handle_stripe5(struct stripe_head *sh)

raid5-fix-unending-write-sequence.patch is in -mm and I believe is waiting on an Acked-by from Neil?

Quoted 3 lines

thanks,

greg k-h

Thanks,
Dan

raid5: fix clearing of biofill operations

From: Dan Williams <[email protected]>

ops_complete_biofill() runs outside of spin_lock(&sh->lock) and clears the 'pending' and 'ack' bits. Since the test_and_ack_op() macro only checks against 'complete' it can get an inconsistent snapshot of pending work.

Move the clearing of these bits to handle_stripe5(), under the lock.

Signed-off-by Dan Williams <[email protected]>
Tested-by Joel Bertrand <[email protected]>
Signed-off-by Neil Brown <[email protected]>

---

 drivers/md/raid5.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)
drivers/md/raid5.c +14 -3
index f96dea9..3808f52 100644
@@ -377,7 +377,12 @@ static unsigned long get_stripe_work(struct stripe_head *sh)
 		ack++;
 
 	sh->ops.count -= ack;
-	BUG_ON(sh->ops.count < 0);
+	if (unlikely(sh->ops.count < 0)) {
+		printk(KERN_ERR "pending: %#lx ops.pending: %#lx ops.ack: %#lx "
+			"ops.complete: %#lx\n", pending, sh->ops.pending,
+			sh->ops.ack, sh->ops.complete);
+		BUG();
+	}
 
 	return pending;
 }

@@ -551,8 +556,7 @@ static void ops_complete_biofill(void *stripe_head_ref)
 			}
 		}
 	}
-	clear_bit(STRIPE_OP_BIOFILL, &sh->ops.ack);
-	clear_bit(STRIPE_OP_BIOFILL, &sh->ops.pending);
+	set_bit(STRIPE_OP_BIOFILL, &sh->ops.complete);
 
 	return_io(return_bi);
 

@@ -2630,6 +2634,13 @@ static void handle_stripe5(struct stripe_head *sh)
 	s.expanded = test_bit(STRIPE_EXPAND_READY, &sh->state);
 	/* Now to look around and see what can be done */
 
+	/* clean-up completed biofill operations */
+	if (test_bit(STRIPE_OP_BIOFILL, &sh->ops.complete)) {
+		clear_bit(STRIPE_OP_BIOFILL, &sh->ops.pending);
+		clear_bit(STRIPE_OP_BIOFILL, &sh->ops.ack);
+		clear_bit(STRIPE_OP_BIOFILL, &sh->ops.complete);
+	}
+
 	rcu_read_lock();
 	for (i=disks; i--; ) {
 		mdk_rdev_t *rdev;

raid5: fix unending write sequence

From: Dan Williams <[email protected]>

<debug output from Joel's system>
handling stripe 7629696, state=0x14 cnt=1, pd_idx=2 ops=0:0:0
check 5: state 0x6 toread 0000000000000000 read 0000000000000000 write fffff800ffcffcc0 written 0000000000000000 check 4: state 0x6 toread 0000000000000000 read 0000000000000000 write fffff800fdd4e360 written 0000000000000000 check 3: state 0x1 toread 0000000000000000 read 0000000000000000 write 0000000000000000 written 0000000000000000 check 2: state 0x1 toread 0000000000000000 read 0000000000000000 write 0000000000000000 written 0000000000000000 check 1: state 0x6 toread 0000000000000000 read 0000000000000000 write fffff800ff517e40 written 0000000000000000 check 0: state 0x6 toread 0000000000000000 read 0000000000000000 write fffff800fd4cae60 written 0000000000000000 locked=4 uptodate=2 to_read=0 to_write=4 failed=0 failed_num=0
for sector 7629696, rmw=0 rcw=0
</debug>

These blocks were prepared to be written out, but were never handled in ops_run_biodrain(), so they remain locked forever. The operations flags are all clear which means handle_stripe() thinks nothing else needs to be done.

This state suggests that the STRIPE_OP_PREXOR bit was sampled 'set' when it should not have been. This patch cleans up cases where the code looks at sh->ops.pending when it should be looking at the consistent stack-based snapshot of the operations flags.

Report from Joel:
Resync done. Patch fix this bug.

Signed-off-by Dan Williams <[email protected]>
Tested-by Joel Bertrand <[email protected]>
Cc [email protected]

---

 drivers/md/raid5.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)
drivers/md/raid5.c +9 -7
index 3808f52..e86cacb 100644
@@ -689,7 +689,8 @@ ops_run_prexor(struct stripe_head *sh, struct dma_async_tx_descriptor *tx)
 }
 
 static struct dma_async_tx_descriptor *
-ops_run_biodrain(struct stripe_head *sh, struct dma_async_tx_descriptor *tx)
+ops_run_biodrain(struct stripe_head *sh, struct dma_async_tx_descriptor *tx,
+		 unsigned long pending)
 {
 	int disks = sh->disks;
 	int pd_idx = sh->pd_idx, i;

@@ -697,7 +698,7 @@ ops_run_biodrain(struct stripe_head *sh, struct dma_async_tx_descriptor *tx)
 	/* check if prexor is active which means only process blocks
 	 * that are part of a read-modify-write (Wantprexor)
 	 */
-	int prexor = test_bit(STRIPE_OP_PREXOR, &sh->ops.pending);
+	int prexor = test_bit(STRIPE_OP_PREXOR, &pending);
 
 	pr_debug("%s: stripe %llu\n", __FUNCTION__,
 		(unsigned long long)sh->sector);

@@ -774,7 +775,8 @@ static void ops_complete_write(void *stripe_head_ref)
 }
 
 static void
-ops_run_postxor(struct stripe_head *sh, struct dma_async_tx_descriptor *tx)
+ops_run_postxor(struct stripe_head *sh, struct dma_async_tx_descriptor *tx,
+		unsigned long pending)
 {
 	/* kernel stack size limits the total number of disks */
 	int disks = sh->disks;

@@ -782,7 +784,7 @@ ops_run_postxor(struct stripe_head *sh, struct dma_async_tx_descriptor *tx)
 
 	int count = 0, pd_idx = sh->pd_idx, i;
 	struct page *xor_dest;
-	int prexor = test_bit(STRIPE_OP_PREXOR, &sh->ops.pending);
+	int prexor = test_bit(STRIPE_OP_PREXOR, &pending);
 	unsigned long flags;
 	dma_async_tx_callback callback;
 

@@ -809,7 +811,7 @@ ops_run_postxor(struct stripe_head *sh, struct dma_async_tx_descriptor *tx)
 	}
 
 	/* check whether this postxor is part of a write */
-	callback = test_bit(STRIPE_OP_BIODRAIN, &sh->ops.pending) ?
+	callback = test_bit(STRIPE_OP_BIODRAIN, &pending) ?
 		ops_complete_write : ops_complete_postxor;
 
 	/* 1/ if we prexor'd then the dest is reused as a source

@@ -897,12 +899,12 @@ static void raid5_run_ops(struct stripe_head *sh, unsigned long pending)
 		tx = ops_run_prexor(sh, tx);
 
 	if (test_bit(STRIPE_OP_BIODRAIN, &pending)) {
-		tx = ops_run_biodrain(sh, tx);
+		tx = ops_run_biodrain(sh, tx, pending);
 		overlap_clear++;
 	}
 
 	if (test_bit(STRIPE_OP_POSTXOR, &pending))
-		ops_run_postxor(sh, tx);
+		ops_run_postxor(sh, tx, pending);
 
 	if (test_bit(STRIPE_OP_CHECK, &pending))
 		ops_run_check(sh);
#8 lore raw
Quoted 23 lines · Dan Williams
Quoted 7 lines · Greg KH
Quoted 7 lines

Careful, it looks like you cherry picked commit 4ae3f847 "md: raid5:
fix clearing of biofill operations" which ended up misapplied in Linus' tree, You should either also pick up def6ae26 "md: fix misapplied patch in raid5.c" or I can resend the original "raid5: fix clearing of biofill operations."

Quoted 9 lines
Quoted 2 lines

The other patch for -stable "raid5: fix unending write sequence" is currently in -mm.

Hm, I've attached the two patches that I have right now in the -stable tree so far (still have over 100 patches to go, so I might not have gotten to them yet if you have sent them). These were sent to me by Andrew on their way to Linus. if I should drop either one, or add another one, please let me know.

Drop md-raid5-fix-clearing-of-biofill-operations.patch and replace it with the attached md-raid5-not-raid6-fix-clearing-of-biofill-operations.patch (the original sent to Neil).

Quoted 6 lines

The critical difference is that the replacement patch touches handle_stripe5, not handle_stripe6. Diffing the patches shows the changes for hunk #3:

-@@ -2903,6 +2907,13 @@ static void handle_stripe6(struct stripe +@@ -2630,6 +2634,13 @@ static void handle_stripe5(struct stripe_head *sh)

Ah, ok, thanks, will do that.

Quoted 2 lines

raid5-fix-unending-write-sequence.patch is in -mm and I believe is waiting on an Acked-by from Neil?

I don't see it in Linus's tree yet, so I can't apply it to -stable...

thanks,

greg k-h

author Acked-by
#9 lore raw
In reply to Dan Williams
Quoted 4 lines · On Tuesday November 13, [email protected] wrote:

raid5-fix-unending-write-sequence.patch is in -mm and I believe is waiting on an Acked-by from Neil?

It seems to have just been sent on to Linus, so it probably will go in without:

Acked-by NeilBrown <[email protected]>

I'm beginning to think that I really should sit down and make sure I understand exactly how those STRIPE_OP_ flags are uses. They generally make sense but there seem to be a number of corner cases where they aren't quite handled properly.. Maybe they are all found now, or maybe....

NeilBrown