[PATCH] xfs: only SetPageUptodate if all buffers are uptodate

[PATCH] xfs: only SetPageUptodate if all buffers are uptodate

[Ben Myers]

xfs_vm_writepage and xfs_page_convert set a page uptodate if it is determined
that all of the buffer_heads attached to that page are uptodate.

Currently we use the flag variable 'uptodate'.  The flag is initially set = 1
and it is cleared if a !buffer_uptodate buffer is encountered.  In addition we
check that bh == head in order to ensure that all of the buffer_heads have been
checked.  However, it is possible to break out of the buffer_head loop early
having processed only the first buffer.  This leaves uptodate == 1 and bh ==
head, so the uptodate bit can be set on a page even if not all of the buffers
have been checked.  This can lead to data corruption on platforms with > 1
buffer per page.

SGI-PV: 1014173

Signed-off-by: Ben Myers <bpm@sgi.com>
---
 fs/xfs/linux-2.6/xfs_aops.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_aops.c b/fs/xfs/linux-2.6/xfs_aops.c
index 52dbd14..c961f35 100644
--- a/fs/xfs/linux-2.6/xfs_aops.c
+++ b/fs/xfs/linux-2.6/xfs_aops.c
@@ -686,7 +686,7 @@ xfs_convert_page(
 	unsigned long		p_offset;
 	unsigned int		type;
 	int			len, page_dirty;
-	int			count = 0, done = 0, uptodate = 1;
+	int			count = 0, done = 0, uptodate = 0;
  	xfs_off_t		offset = page_offset(page);
 
 	if (page->index != tindex)
@@ -727,8 +727,8 @@ xfs_convert_page(
 	do {
 		if (offset >= end_offset)
 			break;
-		if (!buffer_uptodate(bh))
-			uptodate = 0;
+		if (buffer_uptodate(bh))
+			uptodate++;
 		if (!(PageUptodate(page) || buffer_uptodate(bh))) {
 			done = 1;
 			continue;
@@ -761,7 +761,7 @@ xfs_convert_page(
 		}
 	} while (offset += len, (bh = bh->b_this_page) != head);
 
-	if (uptodate && bh == head)
+	if (uptodate == (PAGE_CACHE_SIZE / len))
 		SetPageUptodate(page);
 
 	if (count) {
@@ -915,7 +915,7 @@ xfs_vm_writepage(
 	__uint64_t              end_offset;
 	pgoff_t                 end_index, last_index;
 	ssize_t			len;
-	int			err, imap_valid = 0, uptodate = 1;
+	int			err, imap_valid = 0, uptodate = 0;
 	int			count = 0;
 	int			nonblocking = 0;
 
@@ -978,8 +978,8 @@ xfs_vm_writepage(
 
 		if (offset >= end_offset)
 			break;
-		if (!buffer_uptodate(bh))
-			uptodate = 0;
+		if (buffer_uptodate(bh))
+			uptodate++;
 
 		/*
 		 * set_page_dirty dirties all buffers in a page, independent
@@ -1047,7 +1047,7 @@ xfs_vm_writepage(
 
 	} while (offset += len, ((bh = bh->b_this_page) != head));
 
-	if (uptodate && bh == head)
+	if (uptodate == (PAGE_CACHE_SIZE / len))
 		SetPageUptodate(page);
 
 	xfs_start_page_writeback(page, 1, count);

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: only SetPageUptodate if all buffers are uptodate

[Christoph Hellwig]

On Tue, Apr 19, 2011 at 06:40:28AM -0500, Ben Myers wrote:
> xfs_vm_writepage and xfs_page_convert set a page uptodate if it is determined
> that all of the buffer_heads attached to that page are uptodate.
> 
> Currently we use the flag variable 'uptodate'.  The flag is initially set = 1
> and it is cleared if a !buffer_uptodate buffer is encountered.  In addition we
> check that bh == head in order to ensure that all of the buffer_heads have been
> checked.  However, it is possible to break out of the buffer_head loop early
> having processed only the first buffer.  This leaves uptodate == 1 and bh ==
> head, so the uptodate bit can be set on a page even if not all of the buffers
> have been checked.  This can lead to data corruption on platforms with > 1
> buffer per page.

Dou have a testcase to reproduce this issue?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: only SetPageUptodate if all buffers are uptodate

[bpm]

Hey Christoph,

On Wed, Apr 20, 2011 at 06:35:21AM -0400, Christoph Hellwig wrote:
> On Tue, Apr 19, 2011 at 06:40:28AM -0500, Ben Myers wrote:
> > xfs_vm_writepage and xfs_page_convert set a page uptodate if it is determined
> > that all of the buffer_heads attached to that page are uptodate.
> > 
> > Currently we use the flag variable 'uptodate'.  The flag is initially set = 1
> > and it is cleared if a !buffer_uptodate buffer is encountered.  In addition we
> > check that bh == head in order to ensure that all of the buffer_heads have been
> > checked.  However, it is possible to break out of the buffer_head loop early
> > having processed only the first buffer.  This leaves uptodate == 1 and bh ==
> > head, so the uptodate bit can be set on a page even if not all of the buffers
> > have been checked.  This can lead to data corruption on platforms with > 1
> > buffer per page.
> 
> Dou have a testcase to reproduce this issue?

Wish I did.  The test case that discovered this only applies to CXFS.  I
would have liked to post a test case for XFS but decided that this has
been on my TODO list for too long already.  Looks to me like it has to
be related to the inode size, so you quit probing buffers after the
first.  Maybe some discussion will ring some bells for somebody.

-Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: only SetPageUptodate if all buffers are uptodate

[Christoph Hellwig]

On Wed, Apr 20, 2011 at 09:57:22AM -0500, bpm@sgi.com wrote:
> Wish I did.  The test case that discovered this only applies to CXFS.  I
> would have liked to post a test case for XFS but decided that this has
> been on my TODO list for too long already.  Looks to me like it has to
> be related to the inode size, so you quit probing buffers after the
> first.  Maybe some discussion will ring some bells for somebody.

It would be really good to have one, but the actual patch looks good
enough that I'd consider putting it in.  I can assumes you ran
xfstests with various small blocksize options for both the test
and scratch device and it didn't show any regressions?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: only SetPageUptodate if all buffers are uptodate

[Dave Chinner]

On Wed, Apr 20, 2011 at 11:36:14AM -0400, Christoph Hellwig wrote:
> On Wed, Apr 20, 2011 at 09:57:22AM -0500, bpm@sgi.com wrote:
> > Wish I did.  The test case that discovered this only applies to CXFS.  I
> > would have liked to post a test case for XFS but decided that this has
> > been on my TODO list for too long already.  Looks to me like it has to
> > be related to the inode size, so you quit probing buffers after the
> > first.  Maybe some discussion will ring some bells for somebody.
> 
> It would be really good to have one, but the actual patch looks good
> enough that I'd consider putting it in.  I can assumes you ran
> xfstests with various small blocksize options for both the test
> and scratch device and it didn't show any regressions?

I've been running this patch for quite some time, but having just
upgraded to the latest xfstests, this patch is causing fsx failures
in tests 075 091 112 127 and 231 on 3.0-rc4 on x86_64 with default
mkfs and mount parameters. fsx passes again with this patch removed
from my test stack....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: only SetPageUptodate if all buffers are uptodate

[Dave Chinner]

On Thu, Jun 23, 2011 at 03:08:19PM +1000, Dave Chinner wrote:
> On Wed, Apr 20, 2011 at 11:36:14AM -0400, Christoph Hellwig wrote:
> > On Wed, Apr 20, 2011 at 09:57:22AM -0500, bpm@sgi.com wrote:
> > > Wish I did.  The test case that discovered this only applies to CXFS.  I
> > > would have liked to post a test case for XFS but decided that this has
> > > been on my TODO list for too long already.  Looks to me like it has to
> > > be related to the inode size, so you quit probing buffers after the
> > > first.  Maybe some discussion will ring some bells for somebody.
> > 
> > It would be really good to have one, but the actual patch looks good
> > enough that I'd consider putting it in.  I can assumes you ran
> > xfstests with various small blocksize options for both the test
> > and scratch device and it didn't show any regressions?
> 
> I've been running this patch for quite some time, but having just
> upgraded to the latest xfstests, this patch is causing fsx failures
> in tests 075 091 112 127 and 231 on 3.0-rc4 on x86_64 with default
> mkfs and mount parameters. fsx passes again with this patch removed
> from my test stack....

Seems I spoke too soon - the fsx failures seems to be intermittent,
and it was just chance that my bisect landed on this patch....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: only SetPageUptodate if all buffers are uptodate

[Dave Chinner]

On Thu, Jun 23, 2011 at 04:09:00PM +1000, Dave Chinner wrote:
> On Thu, Jun 23, 2011 at 03:08:19PM +1000, Dave Chinner wrote:
> > On Wed, Apr 20, 2011 at 11:36:14AM -0400, Christoph Hellwig wrote:
> > > On Wed, Apr 20, 2011 at 09:57:22AM -0500, bpm@sgi.com wrote:
> > > > Wish I did.  The test case that discovered this only applies to CXFS.  I
> > > > would have liked to post a test case for XFS but decided that this has
> > > > been on my TODO list for too long already.  Looks to me like it has to
> > > > be related to the inode size, so you quit probing buffers after the
> > > > first.  Maybe some discussion will ring some bells for somebody.
> > > 
> > > It would be really good to have one, but the actual patch looks good
> > > enough that I'd consider putting it in.  I can assumes you ran
> > > xfstests with various small blocksize options for both the test
> > > and scratch device and it didn't show any regressions?
> > 
> > I've been running this patch for quite some time, but having just
> > upgraded to the latest xfstests, this patch is causing fsx failures
> > in tests 075 091 112 127 and 231 on 3.0-rc4 on x86_64 with default
> > mkfs and mount parameters. fsx passes again with this patch removed
> > from my test stack....
> 
> Seems I spoke too soon - the fsx failures seems to be intermittent,
> and it was just chance that my bisect landed on this patch....

I've reproduced the fsx failure with an unmodified, top-of-tree
Linus 3.0-rc kernel, so it's time to start the mainline bisect dance
again.....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs