review: fix remount vs barrier options

review: fix remount vs barrier options

[Nathan Scott]

Hi,

Jeremy has had me scratching my head for a few days trying to
figure out how the SCSI traces he's looking at can still show
signs of write barriers being issued to the device, despite a
"remount,nobarrier" having been done.

It finally clicked that we are not clearing the buffer flag
from a previously written log buffer, even though we'll no
longer set a new flag into a buffer (due to the mount flag
being cleared), so we _can_ still issue barrier writes when
remounted without barriers.

This was made more complicated by the way a freshly mounted
filesystem with 8 log buffers wouldn't show up the problem,
since we have to slowly cycle through the "clear" log buffers
before we see the bug.  This seems like the simplest fix...

(Hmmm, actually, I wonder if this will also resolve the quota
log I/O problem that was reported the other day too).

-- 
Nathan


Index: xfs-linux/xfs_log.c
===================================================================
--- xfs-linux.orig/xfs_log.c	2006-07-21 08:55:24.520992250 +1000
+++ xfs-linux/xfs_log.c	2006-07-21 09:47:08.429216000 +1000
@@ -1471,6 +1471,8 @@ xlog_sync(xlog_t		*log,
 	 */
 	if (log->l_mp->m_flags & XFS_MOUNT_BARRIER)
 		XFS_BUF_ORDERED(bp);
+	else
+		XFS_BUF_UNORDERED(bp);
 
 	ASSERT(XFS_BUF_ADDR(bp) <= log->l_logBBsize-1);
 	ASSERT(XFS_BUF_ADDR(bp) + BTOBB(count) <= log->l_logBBsize);
@@ -1503,6 +1505,8 @@ xlog_sync(xlog_t		*log,
 		XFS_BUF_ASYNC(bp);
 		if (log->l_mp->m_flags & XFS_MOUNT_BARRIER)
 			XFS_BUF_ORDERED(bp);
+		else
+			XFS_BUF_UNORDERED(bp);
 		dptr = XFS_BUF_PTR(bp);
 		/*
 		 * Bump the cycle numbers at the start of each block

Re: review: fix remount vs barrier options

[Timothy Shimmin]

Nathan Scott wrote:
> Hi,
> 
> Jeremy has had me scratching my head for a few days trying to
> figure out how the SCSI traces he's looking at can still show
> signs of write barriers being issued to the device, despite a
> "remount,nobarrier" having been done.
> 
> It finally clicked that we are not clearing the buffer flag
> from a previously written log buffer, even though we'll no
> longer set a new flag into a buffer (due to the mount flag
> being cleared), so we _can_ still issue barrier writes when
> remounted without barriers.
> 
> This was made more complicated by the way a freshly mounted
> filesystem with 8 log buffers wouldn't show up the problem,
> since we have to slowly cycle through the "clear" log buffers
> before we see the bug.  This seems like the simplest fix...
> 
> (Hmmm, actually, I wonder if this will also resolve the quota
> log I/O problem that was reported the other day too).
> 
Patch looks good to me.

--Tim

Re: review: fix remount vs barrier options

[Christoph Hellwig]

> It finally clicked that we are not clearing the buffer flag
> from a previously written log buffer, even though we'll no
> longer set a new flag into a buffer (due to the mount flag
> being cleared), so we _can_ still issue barrier writes when
> remounted without barriers.

That's true.  Ooops.

> This was made more complicated by the way a freshly mounted
> filesystem with 8 log buffers wouldn't show up the problem,
> since we have to slowly cycle through the "clear" log buffers
> before we see the bug.  This seems like the simplest fix...
> 
> (Hmmm, actually, I wonder if this will also resolve the quota
> log I/O problem that was reported the other day too).

Shouldn't we make sure we clear all flags when reusing a log buffer?
Relying on clearing individual flags seems rather fragile to me.

Re: review: fix remount vs barrier options

[Nathan Scott]

On Sun, Jul 23, 2006 at 08:06:50PM +0100, Christoph Hellwig wrote:
> > It finally clicked that we are not clearing the buffer flag
> > from a previously written log buffer, even though we'll no
> > longer set a new flag into a buffer (due to the mount flag
> > being cleared), so we _can_ still issue barrier writes when
> > remounted without barriers.
> 
> That's true.  Ooops.
> 
> > This was made more complicated by the way a freshly mounted
> > filesystem with 8 log buffers wouldn't show up the problem,
> > since we have to slowly cycle through the "clear" log buffers
> > before we see the bug.  This seems like the simplest fix...
> > 
> > (Hmmm, actually, I wonder if this will also resolve the quota
> > log I/O problem that was reported the other day too).
> 
> Shouldn't we make sure we clear all flags when reusing a log buffer?
> Relying on clearing individual flags seems rather fragile to me.

*nod* - good idea.  I'll rework xlog_sync, and resend later.

thanks.

-- 
Nathan

Re: review: fix remount vs barrier options

[Nathan Scott]

On Mon, Jul 24, 2006 at 10:01:48AM +1000, Nathan Scott wrote:
> On Sun, Jul 23, 2006 at 08:06:50PM +0100, Christoph Hellwig wrote:
> > Shouldn't we make sure we clear all flags when reusing a log buffer?
> > Relying on clearing individual flags seems rather fragile to me.
> 
> *nod* - good idea.  I'll rework xlog_sync, and resend later.

After looking more, I'm less convinced.  There's some flags we wont
want to touch - the "internal" flags like PAGE_CACHE, etc (that one
is obviously not relevent here, but still, at some point a flag may
be introduced that we accidentally break by clearing all flags).

There is a ZEROFLAGS macro, I've added ORDERED to that and used it
instead.  I also fixed the double barrier issue for the split log
write case - here's an updated patch...

cheers.

-- 
Nathan


Index: xfs-linux/xfs_log.c
===================================================================
--- xfs-linux.orig/xfs_log.c	2006-07-21 08:55:24.520992250 +1000
+++ xfs-linux/xfs_log.c	2006-07-24 11:13:22.743144500 +1000
@@ -1444,7 +1444,7 @@ xlog_sync(xlog_t		*log,
 	ops = iclog->ic_header.h_num_logops;
 	INT_SET(iclog->ic_header.h_num_logops, ARCH_CONVERT, ops);
 
-	bp	    = iclog->ic_bp;
+	bp = iclog->ic_bp;
 	ASSERT(XFS_BUF_FSPRIVATE2(bp, unsigned long) == (unsigned long)1);
 	XFS_BUF_SET_FSPRIVATE2(bp, (unsigned long)2);
 	XFS_BUF_SET_ADDR(bp, BLOCK_LSN(INT_GET(iclog->ic_header.h_lsn, ARCH_CONVERT)));
@@ -1461,15 +1461,14 @@ xlog_sync(xlog_t		*log,
 	}
 	XFS_BUF_SET_PTR(bp, (xfs_caddr_t) &(iclog->ic_header), count);
 	XFS_BUF_SET_FSPRIVATE(bp, iclog);	/* save for later */
+	XFS_BUF_ZEROFLAGS(bp);
 	XFS_BUF_BUSY(bp);
 	XFS_BUF_ASYNC(bp);
 	/*
 	 * Do an ordered write for the log block.
-	 *
-	 * It may not be needed to flush the first split block in the log wrap
-	 * case, but do it anyways to be safe -AK
+	 * Its unnecessary to flush the first split block in the log wrap case.
 	 */
-	if (log->l_mp->m_flags & XFS_MOUNT_BARRIER)
+	if (!split && (log->l_mp->m_flags & XFS_MOUNT_BARRIER))
 		XFS_BUF_ORDERED(bp);
 
 	ASSERT(XFS_BUF_ADDR(bp) <= log->l_logBBsize-1);
@@ -1491,7 +1490,7 @@ xlog_sync(xlog_t		*log,
 		return error;
 	}
 	if (split) {
-		bp		= iclog->ic_log->l_xbuf;
+		bp = iclog->ic_log->l_xbuf;
 		ASSERT(XFS_BUF_FSPRIVATE2(bp, unsigned long) ==
 							(unsigned long)1);
 		XFS_BUF_SET_FSPRIVATE2(bp, (unsigned long)2);
@@ -1499,6 +1498,7 @@ xlog_sync(xlog_t		*log,
 		XFS_BUF_SET_PTR(bp, (xfs_caddr_t)((__psint_t)&(iclog->ic_header)+
 					    (__psint_t)count), split);
 		XFS_BUF_SET_FSPRIVATE(bp, iclog);
+		XFS_BUF_ZEROFLAGS(bp);
 		XFS_BUF_BUSY(bp);
 		XFS_BUF_ASYNC(bp);
 		if (log->l_mp->m_flags & XFS_MOUNT_BARRIER)
Index: xfs-linux/linux-2.6/xfs_buf.h
===================================================================
--- xfs-linux.orig/linux-2.6/xfs_buf.h	2006-07-24 11:04:17.203179750 +1000
+++ xfs-linux/linux-2.6/xfs_buf.h	2006-07-24 11:09:29.652577250 +1000
@@ -247,8 +247,8 @@ extern void xfs_buf_trace(xfs_buf_t *, c
 #define BUF_BUSY		XBF_DONT_BLOCK
 
 #define XFS_BUF_BFLAGS(bp)	((bp)->b_flags)
-#define XFS_BUF_ZEROFLAGS(bp)	\
-	((bp)->b_flags &= ~(XBF_READ|XBF_WRITE|XBF_ASYNC|XBF_DELWRI))
+#define XFS_BUF_ZEROFLAGS(bp)	((bp)->b_flags &= \
+		~(XBF_READ|XBF_WRITE|XBF_ASYNC|XBF_DELWRI|XBF_ORDERED))
 
 #define XFS_BUF_STALE(bp)	((bp)->b_flags |= XFS_B_STALE)
 #define XFS_BUF_UNSTALE(bp)	((bp)->b_flags &= ~XFS_B_STALE)

Re: review: fix remount vs barrier options

[Christoph Hellwig]

On Mon, Jul 24, 2006 at 11:27:37AM +1000, Nathan Scott wrote:
> On Mon, Jul 24, 2006 at 10:01:48AM +1000, Nathan Scott wrote:
> > On Sun, Jul 23, 2006 at 08:06:50PM +0100, Christoph Hellwig wrote:
> > > Shouldn't we make sure we clear all flags when reusing a log buffer?
> > > Relying on clearing individual flags seems rather fragile to me.
> > 
> > *nod* - good idea.  I'll rework xlog_sync, and resend later.
> 
> After looking more, I'm less convinced.  There's some flags we wont
> want to touch - the "internal" flags like PAGE_CACHE, etc (that one
> is obviously not relevent here, but still, at some point a flag may
> be introduced that we accidentally break by clearing all flags).
> 
> There is a ZEROFLAGS macro, I've added ORDERED to that and used it
> instead.  I also fixed the double barrier issue for the split log
> write case - here's an updated patch...

The flag clearing changes look good.  But why is it okay to skip the
ordered flag on the first block?  We want to make sure all previous I/O
is finished before even doing the first log block write, don't we?

Re: review: fix remount vs barrier options

[Nathan Scott]

Hi Christoph,

On Tue, Jul 25, 2006 at 10:44:38AM +0100, Christoph Hellwig wrote:
> ...
> The flag clearing changes look good.  But why is it okay to skip the
> ordered flag on the first block?  We want to make sure all previous I/O
> is finished before even doing the first log block write, don't we?

No, thats not necessary - Tim and I looked into this at length - the
way the split log write happens, we keep a count in the iclog struct
for this write (its initially 2); once the first write completes (and
this is guaranteed to be before the final write by the final write's
barrier), we atomically decrement that counter.  Once it reaches zero
(i.e. only one buffer completion was remaining, which is also the non-
split log write case) only then will we do any callback processing,
metadata unpinning, etc, etc.

So, its a safe and valid optimisation - otherwise we do an extra write
with barrier whenever we wrap around the physical log end.

Thanks for the reviews!

cheers.

-- 
Nathan