[PATCH 000 of 2] md: Fixes for md in 2.6.23

[PATCH 000 of 2] md: Fixes for md in 2.6.23

[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

[PATCH 000 of 2] md: Fixes for md in 2.6.23

[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

[PATCH 001 of 2] md: Fix an unsigned compare to allow creation of bitmaps with v1.0 metadata.

[NeilBrown]

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

Signed-off-by: Neil Brown <neilb@suse.de>
Cc: Stable <stable@kernel.org>

### Diffstat output
 ./drivers/md/bitmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff .prev/drivers/md/bitmap.c ./drivers/md/bitmap.c
--- .prev/drivers/md/bitmap.c	2007-10-22 16:55:48.000000000 +1000
+++ ./drivers/md/bitmap.c	2007-10-22 16:55:52.000000000 +1000
@@ -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;

[PATCH 001 of 2] md: Fix an unsigned compare to allow creation of bitmaps with v1.0 metadata.

[NeilBrown]

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

Signed-off-by: Neil Brown <neilb@suse.de>
Cc: Stable <stable@kernel.org>

### Diffstat output
 ./drivers/md/bitmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff .prev/drivers/md/bitmap.c ./drivers/md/bitmap.c
--- .prev/drivers/md/bitmap.c	2007-10-22 16:55:48.000000000 +1000
+++ ./drivers/md/bitmap.c	2007-10-22 16:55:52.000000000 +1000
@@ -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;

[PATCH 002 of 2] md: raid5: fix clearing of biofill operations

[NeilBrown]

From: Dan Williams <dan.j.williams@intel.com>

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 <dan.j.williams@intel.com>
Tested-by: Joël Bertrand <joel.bertrand@systella.fr>
Signed-off-by: Neil Brown <neilb@suse.de>
Cc: Stable <stable@kernel.org>

### Diffstat output
 ./drivers/md/raid5.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff .prev/drivers/md/raid5.c ./drivers/md/raid5.c
--- .prev/drivers/md/raid5.c	2007-10-22 16:55:49.000000000 +1000
+++ ./drivers/md/raid5.c	2007-10-22 16:57:41.000000000 +1000
@@ -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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 002 of 2] md: raid5: fix clearing of biofill operations

[NeilBrown]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]


From: Dan Williams <dan.j.williams@intel.com>

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 <dan.j.williams@intel.com>
Tested-by: Joël Bertrand <joel.bertrand@systella.fr>
Signed-off-by: Neil Brown <neilb@suse.de>
Cc: Stable <stable@kernel.org>

### Diffstat output
 ./drivers/md/raid5.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff .prev/drivers/md/raid5.c ./drivers/md/raid5.c
--- .prev/drivers/md/raid5.c	2007-10-22 16:55:49.000000000 +1000
+++ ./drivers/md/raid5.c	2007-10-22 16:57:41.000000000 +1000
@@ -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;

Re: [stable] [PATCH 000 of 2] md: Fixes for md in 2.6.23

[Greg KH]

On Mon, Oct 22, 2007 at 05:15:27PM +1000, NeilBrown wrote:
> 
> 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

Re: [stable] [PATCH 000 of 2] md: Fixes for md in 2.6.23

[Greg KH]

On Tue, Nov 13, 2007 at 04:22:14PM -0800, Greg KH wrote:
> On Mon, Oct 22, 2007 at 05:15:27PM +1000, NeilBrown wrote:
> > 
> > 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?

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

greg k-h

Re: [stable] [PATCH 000 of 2] md: Fixes for md in 2.6.23

[Dan Williams]

On Nov 13, 2007 5:23 PM, Greg KH <greg@kroah.com> wrote:
> On Tue, Nov 13, 2007 at 04:22:14PM -0800, Greg KH wrote:
> > On Mon, Oct 22, 2007 at 05:15:27PM +1000, NeilBrown wrote:
> > >
> > > 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?
>
> 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.

> greg k-h

Regards,
Dan

Re: [stable] [PATCH 000 of 2] md: Fixes for md in 2.6.23

[Greg KH]

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

On Tue, Nov 13, 2007 at 08:36:05PM -0700, Dan Williams wrote:
> On Nov 13, 2007 5:23 PM, Greg KH <greg@kroah.com> wrote:
> > On Tue, Nov 13, 2007 at 04:22:14PM -0800, Greg KH wrote:
> > > On Mon, Oct 22, 2007 at 05:15:27PM +1000, NeilBrown wrote:
> > > >
> > > > 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?
> >
> > 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.

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

[-- Attachment #2: md-fix-an-unsigned-compare-to-allow-creation-of-bitmaps-with-v1.0-metadata.patch --]
[-- Type: text/x-patch, Size: 1262 bytes --]

From stable-bounces@linux.kernel.org Mon Oct 22 20:45:35 2007
From: akpm@linux-foundation.org
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: torvalds@linux-foundation.org
Cc: neilb@suse.de, akpm@linux-foundation.org, stable@kernel.org
Message-ID: <200710230345.l9N3jB65030288@imap1.linux-foundation.org>


From: NeilBrown <neilb@suse.de>

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 <neilb@suse.de>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/md/bitmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/md/bitmap.c
+++ b/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;

[-- Attachment #3: md-raid5-fix-clearing-of-biofill-operations.patch --]
[-- Type: text/x-patch, Size: 2399 bytes --]

From stable-bounces@linux.kernel.org Mon Oct 22 20:45:44 2007
From: Dan Williams <dan.j.williams@intel.com>
Date: Mon, 22 Oct 2007 20:45:11 -0700
Subject: md: raid5: fix clearing of biofill operations
To: torvalds@linux-foundation.org
Cc: joel.bertrand@systella.fr, neilb@suse.de, akpm@linux-foundation.org, dan.j.williams@intel.com, stable@kernel.org
Message-ID: <200710230345.l9N3jC2M030292@imap1.linux-foundation.org>

From: Dan Williams <dan.j.williams@intel.com>

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 <dan.j.williams@intel.com>
Tested-by: Joel Bertrand <joel.bertrand@systella.fr>
Signed-off-by: Neil Brown <neilb@suse.de>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/md/raid5.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -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;

Re: [stable] [PATCH 000 of 2] md: Fixes for md in 2.6.23

[Greg KH]

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

On Tue, Nov 13, 2007 at 08:36:05PM -0700, Dan Williams wrote:
> On Nov 13, 2007 5:23 PM, Greg KH <greg@kroah.com> wrote:
> > On Tue, Nov 13, 2007 at 04:22:14PM -0800, Greg KH wrote:
> > > On Mon, Oct 22, 2007 at 05:15:27PM +1000, NeilBrown wrote:
> > > >
> > > > 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?
> >
> > 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.

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

[-- Attachment #2: md-fix-an-unsigned-compare-to-allow-creation-of-bitmaps-with-v1.0-metadata.patch --]
[-- Type: text/x-patch, Size: 1263 bytes --]

>From stable-bounces@linux.kernel.org Mon Oct 22 20:45:35 2007
From: akpm@linux-foundation.org
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: torvalds@linux-foundation.org
Cc: neilb@suse.de, akpm@linux-foundation.org, stable@kernel.org
Message-ID: <200710230345.l9N3jB65030288@imap1.linux-foundation.org>


From: NeilBrown <neilb@suse.de>

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 <neilb@suse.de>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/md/bitmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/md/bitmap.c
+++ b/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;

[-- Attachment #3: md-raid5-fix-clearing-of-biofill-operations.patch --]
[-- Type: text/x-patch, Size: 2400 bytes --]

>From stable-bounces@linux.kernel.org Mon Oct 22 20:45:44 2007
From: Dan Williams <dan.j.williams@intel.com>
Date: Mon, 22 Oct 2007 20:45:11 -0700
Subject: md: raid5: fix clearing of biofill operations
To: torvalds@linux-foundation.org
Cc: joel.bertrand@systella.fr, neilb@suse.de, akpm@linux-foundation.org, dan.j.williams@intel.com, stable@kernel.org
Message-ID: <200710230345.l9N3jC2M030292@imap1.linux-foundation.org>

From: Dan Williams <dan.j.williams@intel.com>

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 <dan.j.williams@intel.com>
Tested-by: Joel Bertrand <joel.bertrand@systella.fr>
Signed-off-by: Neil Brown <neilb@suse.de>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/md/raid5.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -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;

Re: [stable] [PATCH 000 of 2] md: Fixes for md in 2.6.23

[Dan Williams]

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

On Nov 13, 2007 8:43 PM, Greg KH <greg@kroah.com> wrote:
> >
> > 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.
>

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?

> thanks,
>
> greg k-h

Thanks,
Dan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: md-raid5-not-raid6-fix-clearing-of-biofill-operations.patch --]
[-- Type: text/x-patch; name=md-raid5-not-raid6-fix-clearing-of-biofill-operations.patch, Size: 1918 bytes --]

raid5: fix clearing of biofill operations

From: Dan Williams <dan.j.williams@intel.com>

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 <dan.j.williams@intel.com>
Tested-by: Joel Bertrand <joel.bertrand@systella.fr>
Signed-off-by: Neil Brown <neilb@suse.de>
---

 drivers/md/raid5.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index f96dea9..3808f52 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -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;

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: raid5-fix-unending-write-sequence.patch --]
[-- Type: text/x-patch; name=raid5-fix-unending-write-sequence.patch, Size: 4194 bytes --]

raid5: fix unending write sequence

From: Dan Williams <dan.j.williams@intel.com>

<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 <dan.j.williams@intel.com>
Tested-by: Joel Bertrand <joel.bertrand@systella.fr>
Cc: stable@kernel.org
---

 drivers/md/raid5.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 3808f52..e86cacb 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -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);

Re: [stable] [PATCH 000 of 2] md: Fixes for md in 2.6.23

[Greg KH]

On Tue, Nov 13, 2007 at 10:36:30PM -0700, Dan Williams wrote:
> On Nov 13, 2007 8:43 PM, Greg KH <greg@kroah.com> wrote:
> > >
> > > 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.
> >
> 
> 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)

Ah, ok, thanks, will do that.

> 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

Re: [stable] [PATCH 000 of 2] md: Fixes for md in 2.6.23

[Neil Brown]

On Tuesday November 13, dan.j.williams@intel.com 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 <neilb@suse.de>

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