[PATCH 0/2] buffer caches fixes for Linux 3.5

[PATCH 0/2] buffer caches fixes for Linux 3.5

[Christoph Hellwig]

Two fixes for problems created or made worse by the on-stack plugging
changes.

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

[PATCH 1/2] xfs: prevent recursion in xfs_buf_iorequest

[Christoph Hellwig]

[-- Attachment #1: xfs-fix-io-error-recursion --]
[-- Type: text/plain, Size: 1191 bytes --]

If the b_iodone handler is run in calling context in xfs_buf_iorequest we
can run into a recursion where xfs_buf_iodone_callbacks keeps calling back
into xfs_buf_iorequest because an I/O error happened, which keeps calling
back into xfs_buf_iorequest.  This chain will usually not take long
because the filesystem gets shut down because of log I/O errors, but even
over a short time it can cause stack overflows if run on the same context.

As a short term workaround make sure we always call the iodone handler in
workqueue context.

Signed-off-by: Christoph Hellwig <hch@lst.de>

---
 fs/xfs/xfs_buf.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: xfs/fs/xfs/xfs_buf.c
===================================================================
--- xfs.orig/fs/xfs/xfs_buf.c	2012-06-26 23:20:22.369740722 +0200
+++ xfs/fs/xfs/xfs_buf.c	2012-06-26 23:20:23.686407379 +0200
@@ -1243,7 +1243,7 @@ xfs_buf_iorequest(
 	 */
 	atomic_set(&bp->b_io_remaining, 1);
 	_xfs_buf_ioapply(bp);
-	_xfs_buf_ioend(bp, 0);
+	_xfs_buf_ioend(bp, 1);
 
 	xfs_buf_rele(bp);
 }

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

[PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks

[Christoph Hellwig]

[-- Attachment #1: xfs-kill-xfs_bdstrat_cb-1 --]
[-- Type: text/plain, Size: 2845 bytes --]

xfs_bdstrat_cb only adds a check for a shutdown filesystem over xfs_buf_iorequest,
but xfs_buf_iodone_callbacks just checked for a shut down filesystem a little
earlier.  In addition the shutdown handling in xfs_bdstrat_cb is not very suitable
for this caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>

---
 fs/xfs/xfs_buf.c      |   51 +++++++++++++++++++++-----------------------------
 fs/xfs/xfs_buf_item.c |    2 -
 2 files changed, 23 insertions(+), 30 deletions(-)

Index: xfs/fs/xfs/xfs_buf_item.c
===================================================================
--- xfs.orig/fs/xfs/xfs_buf_item.c	2012-06-26 23:20:22.326407387 +0200
+++ xfs/fs/xfs/xfs_buf_item.c	2012-06-26 23:20:28.766407349 +0200
@@ -954,7 +954,7 @@ xfs_buf_iodone_callbacks(
 
 		if (!XFS_BUF_ISSTALE(bp)) {
 			bp->b_flags |= XBF_WRITE | XBF_ASYNC | XBF_DONE;
-			xfs_bdstrat_cb(bp);
+			xfs_buf_iorequest(bp);
 		} else {
 			xfs_buf_relse(bp);
 		}
Index: xfs/fs/xfs/xfs_buf.c
===================================================================
--- xfs.orig/fs/xfs/xfs_buf.c	2012-06-26 23:20:23.686407379 +0200
+++ xfs/fs/xfs/xfs_buf.c	2012-06-26 23:21:40.286406924 +0200
@@ -989,27 +989,6 @@ xfs_buf_ioerror_alert(
 		(__uint64_t)XFS_BUF_ADDR(bp), func, bp->b_error, bp->b_length);
 }
 
-int
-xfs_bwrite(
-	struct xfs_buf		*bp)
-{
-	int			error;
-
-	ASSERT(xfs_buf_islocked(bp));
-
-	bp->b_flags |= XBF_WRITE;
-	bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q);
-
-	xfs_bdstrat_cb(bp);
-
-	error = xfs_buf_iowait(bp);
-	if (error) {
-		xfs_force_shutdown(bp->b_target->bt_mount,
-				   SHUTDOWN_META_IO_ERROR);
-	}
-	return error;
-}
-
 /*
  * Called when we want to stop a buffer from getting written or read.
  * We attach the EIO error, muck with its flags, and call xfs_buf_ioend
@@ -1079,14 +1058,7 @@ xfs_bioerror_relse(
 	return EIO;
 }
 
-
-/*
- * All xfs metadata buffers except log state machine buffers
- * get this attached as their b_bdstrat callback function.
- * This is so that we can catch a buffer
- * after prematurely unpinning it to forcibly shutdown the filesystem.
- */
-int
+STATIC int
 xfs_bdstrat_cb(
 	struct xfs_buf	*bp)
 {
@@ -1107,6 +1079,27 @@ xfs_bdstrat_cb(
 	return 0;
 }
 
+int
+xfs_bwrite(
+	struct xfs_buf		*bp)
+{
+	int			error;
+
+	ASSERT(xfs_buf_islocked(bp));
+
+	bp->b_flags |= XBF_WRITE;
+	bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q);
+
+	xfs_bdstrat_cb(bp);
+
+	error = xfs_buf_iowait(bp);
+	if (error) {
+		xfs_force_shutdown(bp->b_target->bt_mount,
+				   SHUTDOWN_META_IO_ERROR);
+	}
+	return error;
+}
+
 /*
  * Wrapper around bdstrat so that we can stop data from going to disk in case
  * we are shutting down the filesystem.  Typically user data goes thru this

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

Re: [PATCH 1/2] xfs: prevent recursion in xfs_buf_iorequest

[Dave Chinner]

On Mon, Jul 02, 2012 at 06:00:04AM -0400, Christoph Hellwig wrote:
> If the b_iodone handler is run in calling context in xfs_buf_iorequest we
> can run into a recursion where xfs_buf_iodone_callbacks keeps calling back
> into xfs_buf_iorequest because an I/O error happened, which keeps calling
> back into xfs_buf_iorequest.  This chain will usually not take long
> because the filesystem gets shut down because of log I/O errors, but even
> over a short time it can cause stack overflows if run on the same context.
> 
> As a short term workaround make sure we always call the iodone handler in
> workqueue context.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks fine - final completion from io submission is a rare case so
this shouldn't cause issues....

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks

[Dave Chinner]

On Mon, Jul 02, 2012 at 06:00:05AM -0400, Christoph Hellwig wrote:
> xfs_bdstrat_cb only adds a check for a shutdown filesystem over xfs_buf_iorequest,
> but xfs_buf_iodone_callbacks just checked for a shut down filesystem a little
> earlier.  In addition the shutdown handling in xfs_bdstrat_cb is not very suitable
> for this caller.

Makes sense - I have a patch locally that does this as part of a
xfs_buf_iorequest(bp, blkno, length) conversion to get rid of
XFS_BUF_SETADDR()....

> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks

[Christoph Hellwig]

On Tue, Jul 03, 2012 at 10:28:57AM +1000, Dave Chinner wrote:
> On Mon, Jul 02, 2012 at 06:00:05AM -0400, Christoph Hellwig wrote:
> > xfs_bdstrat_cb only adds a check for a shutdown filesystem over xfs_buf_iorequest,
> > but xfs_buf_iodone_callbacks just checked for a shut down filesystem a little
> > earlier.  In addition the shutdown handling in xfs_bdstrat_cb is not very suitable
> > for this caller.
> 
> Makes sense - I have a patch locally that does this as part of a
> xfs_buf_iorequest(bp, blkno, length) conversion to get rid of
> XFS_BUF_SETADDR()....

Can you send these out ASAP?  I have another big batch of buffer error
handling changes for 3.6 I'm working on right now.

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

Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks

[Dave Chinner]

On Tue, Jul 03, 2012 at 12:05:31PM -0400, Christoph Hellwig wrote:
> On Tue, Jul 03, 2012 at 10:28:57AM +1000, Dave Chinner wrote:
> > On Mon, Jul 02, 2012 at 06:00:05AM -0400, Christoph Hellwig wrote:
> > > xfs_bdstrat_cb only adds a check for a shutdown filesystem over xfs_buf_iorequest,
> > > but xfs_buf_iodone_callbacks just checked for a shut down filesystem a little
> > > earlier.  In addition the shutdown handling in xfs_bdstrat_cb is not very suitable
> > > for this caller.
> > 
> > Makes sense - I have a patch locally that does this as part of a
> > xfs_buf_iorequest(bp, blkno, length) conversion to get rid of
> > XFS_BUF_SETADDR()....
> 
> Can you send these out ASAP?

It's not working yet - I found an issue with logging and writeback
of uncached buffers through the AIL (i.e. the superblock). This only
works by good fortune right now and requires uncached buffers to
carry their block number internally, so I need to rethink and rework
the patch.

I think that I will make the superblock buffer a cached buffer once
again, but keep the current shortcut to look it up. Doing so will
enable me to keep the distinction I created between cached and
uncached buffers. It will also solve the AIL flush waiting problem
we currently also have with it on freeze/unmount.

> I have another big batch of buffer error
> handling changes for 3.6 I'm working on right now.

How much does it change? I'm also trying to get all the read verify
callback infrastructure changes done for 3.6, and i suspect these may
step on each other. I've just about got those patches done - testing
and bug fixing is happening at the moment....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks

[Christoph Hellwig]

On Wed, Jul 04, 2012 at 09:29:23AM +1000, Dave Chinner wrote:
> It's not working yet - I found an issue with logging and writeback
> of uncached buffers through the AIL (i.e. the superblock). This only
> works by good fortune right now and requires uncached buffers to
> carry their block number internally, so I need to rethink and rework
> the patch.

What is the problem with uncached buffers?  I'd hate having to move
the superblock buffer away from the uncached scheme.

> How much does it change? I'm also trying to get all the read verify
> callback infrastructure changes done for 3.6, and i suspect these may
> step on each other. I've just about got those patches done - testing
> and bug fixing is happening at the moment....

Basically a lot of impact around all callers of xfsbdstrat (which is
going away) and some impact in xfs_buf.c around the higher-level
read/write code.  I'm not entirely done with the plane I have so
other things might get in the way and make it more complicated in
the end.

Another thing it depends on is to only start the sync work item later
during mount so that the re-read of the superblock after recovery can
use normal buffer cache interfaces instead of xfsbdstrat.  I have a
minimal version of that in my tree but was waiting for your planned
large changes in that area.

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

Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks

[Dave Chinner]

On Wed, Jul 04, 2012 at 01:57:23AM -0400, Christoph Hellwig wrote:
> On Wed, Jul 04, 2012 at 09:29:23AM +1000, Dave Chinner wrote:
> > It's not working yet - I found an issue with logging and writeback
> > of uncached buffers through the AIL (i.e. the superblock). This only
> > works by good fortune right now and requires uncached buffers to
> > carry their block number internally, so I need to rethink and rework
> > the patch.
> 
> What is the problem with uncached buffers?  I'd hate having to move
> the superblock buffer away from the uncached scheme.

I'm not exactly sure yet - I last looked at it before I went on
holidays last week. I was getting random ASSERT failures, always on
the superblock buffer and always due ot trying to IO to
XFS_BUF_DADDR_NULL because it was an uncached buffer. The path to it
was always through the AIL flushing, so it was using
xfs_buf_iorequest(bp) and getting the null blkno rather than
xfs_buf_uncached_iorequest(bp, blkno, len) which supplies the blkno.
Like I said, I need to rework is so that either
xfs_buf_iorequest(bp) works with uncached buffers, or we don't use
uncached buffers in transactions.

IMO, using uncached buffers in transactions is dangerous because
concurrent transactions can't find buffers uncached buffers at the
same address and so such buffers need higher level synchronisation
interfaces (as the superblock has).

I'm happy to leave the superblock as uncached and work around it in
some way, but it just seems wrong to me to have one buffer behaves
differently to all the cached and other uncached buffers in the
system. It doesn't sit well in my mind to do that when the problem
simply goes away if we make the superblock a cached buffer again....

> > How much does it change? I'm also trying to get all the read verify
> > callback infrastructure changes done for 3.6, and i suspect these may
> > step on each other. I've just about got those patches done - testing
> > and bug fixing is happening at the moment....
> 
> Basically a lot of impact around all callers of xfsbdstrat (which is
> going away) and some impact in xfs_buf.c around the higher-level
> read/write code.  I'm not entirely done with the plane I have so
> other things might get in the way and make it more complicated in
> the end.
> 
> Another thing it depends on is to only start the sync work item later
> during mount so that the re-read of the superblock after recovery can
> use normal buffer cache interfaces instead of xfsbdstrat.

Well, we've already got conflicts there, because all of the uncached
IO changes I've made remove xfsbdstrat() from those paths....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks

[Christoph Hellwig]

On Wed, Jul 04, 2012 at 06:32:34PM +1000, Dave Chinner wrote:
> IMO, using uncached buffers in transactions is dangerous because
> concurrent transactions can't find buffers uncached buffers at the
> same address and so such buffers need higher level synchronisation
> interfaces (as the superblock has).
> 
> I'm happy to leave the superblock as uncached and work around it in
> some way, but it just seems wrong to me to have one buffer behaves
> differently to all the cached and other uncached buffers in the
> system. It doesn't sit well in my mind to do that when the problem
> simply goes away if we make the superblock a cached buffer again....

The superblock buffer always has been more special than others, remember
the old FS_MANAGED flag before we went for uncached buffers?

If you want to move it back to normal buffers how do you want to look
it up, given that IIRC we read it before setting up the perag buffers.

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

Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks

[Ben Myers]

Hey Christoph,

On Mon, Jul 02, 2012 at 06:00:05AM -0400, Christoph Hellwig wrote:
> xfs_bdstrat_cb only adds a check for a shutdown filesystem over xfs_buf_iorequest,
> but xfs_buf_iodone_callbacks just checked for a shut down filesystem a little
> earlier.  In addition the shutdown handling in xfs_bdstrat_cb is not very suitable
> for this caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> ---
>  fs/xfs/xfs_buf.c      |   51 +++++++++++++++++++++-----------------------------
>  fs/xfs/xfs_buf_item.c |    2 -
>  2 files changed, 23 insertions(+), 30 deletions(-)
> 
> Index: xfs/fs/xfs/xfs_buf_item.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_buf_item.c	2012-06-26 23:20:22.326407387 +0200
> +++ xfs/fs/xfs/xfs_buf_item.c	2012-06-26 23:20:28.766407349 +0200
> @@ -954,7 +954,7 @@ xfs_buf_iodone_callbacks(
>  
>  		if (!XFS_BUF_ISSTALE(bp)) {
>  			bp->b_flags |= XBF_WRITE | XBF_ASYNC | XBF_DONE;
> -			xfs_bdstrat_cb(bp);
> +			xfs_buf_iorequest(bp);
>  		} else {
>  			xfs_buf_relse(bp);
>  		}
> Index: xfs/fs/xfs/xfs_buf.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_buf.c	2012-06-26 23:20:23.686407379 +0200
> +++ xfs/fs/xfs/xfs_buf.c	2012-06-26 23:21:40.286406924 +0200
> @@ -989,27 +989,6 @@ xfs_buf_ioerror_alert(
>  		(__uint64_t)XFS_BUF_ADDR(bp), func, bp->b_error, bp->b_length);
>  }
>  
> -int
> -xfs_bwrite(
> -	struct xfs_buf		*bp)
> -{
> -	int			error;
> -
> -	ASSERT(xfs_buf_islocked(bp));
> -
> -	bp->b_flags |= XBF_WRITE;
> -	bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q);
> -
> -	xfs_bdstrat_cb(bp);
> -
> -	error = xfs_buf_iowait(bp);
> -	if (error) {
> -		xfs_force_shutdown(bp->b_target->bt_mount,
> -				   SHUTDOWN_META_IO_ERROR);
> -	}
> -	return error;
> -}
> -
>  /*
>   * Called when we want to stop a buffer from getting written or read.
>   * We attach the EIO error, muck with its flags, and call xfs_buf_ioend
> @@ -1079,14 +1058,7 @@ xfs_bioerror_relse(
>  	return EIO;
>  }
>  
> -
> -/*
> - * All xfs metadata buffers except log state machine buffers
> - * get this attached as their b_bdstrat callback function.
> - * This is so that we can catch a buffer
> - * after prematurely unpinning it to forcibly shutdown the filesystem.
> - */
> -int
> +STATIC int

xfs/fs/xfs/xfs_buf.c:1062: error: static declaration of ‘xfs_bdstrat_cb’ follows non-static declaration
xfs/fs/xfs/xfs_buf.h:183: error: previous declaration of ‘xfs_bdstrat_cb’ was here

CONFIG_XFS_DEBUG was not set.

I'm happy to have caught that before pulling this in.  Most of the time I am
testing with CONFIG_XFS_DEBUG=y.  I'll fix it up.

Regards,
	Ben

>  xfs_bdstrat_cb(
>  	struct xfs_buf	*bp)
>  {
> @@ -1107,6 +1079,27 @@ xfs_bdstrat_cb(
>  	return 0;
>  }
>  
> +int
> +xfs_bwrite(
> +	struct xfs_buf		*bp)
> +{
> +	int			error;
> +
> +	ASSERT(xfs_buf_islocked(bp));
> +
> +	bp->b_flags |= XBF_WRITE;
> +	bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q);
> +
> +	xfs_bdstrat_cb(bp);
> +
> +	error = xfs_buf_iowait(bp);
> +	if (error) {
> +		xfs_force_shutdown(bp->b_target->bt_mount,
> +				   SHUTDOWN_META_IO_ERROR);
> +	}
> +	return error;
> +}
> +
>  /*
>   * Wrapper around bdstrat so that we can stop data from going to disk in case
>   * we are shutting down the filesystem.  Typically user data goes thru this
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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

Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks

[Christoph Hellwig]

On Thu, Jul 12, 2012 at 06:04:21PM -0500, Ben Myers wrote:
> xfs/fs/xfs/xfs_buf.c:1062: error: static declaration of ???xfs_bdstrat_cb??? follows non-static declaration
> xfs/fs/xfs/xfs_buf.h:183: error: previous declaration of ???xfs_bdstrat_cb??? was here
> 
> CONFIG_XFS_DEBUG was not set.
> 
> I'm happy to have caught that before pulling this in.  Most of the time I am
> testing with CONFIG_XFS_DEBUG=y.  I'll fix it up.

The one in the header should just go away, that's why I moved
xfs_bwrite.

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

[PATCH 2/2 v2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks

[Christoph Hellwig]

xfs_bdstrat_cb only adds a check for a shutdown filesystem over
xfs_buf_iorequest, but xfs_buf_iodone_callbacks just checked for a shut down
filesystem a little earlier.  In addition the shutdown handling in
xfs_bdstrat_cb is not very suitable for this caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>

---
 fs/xfs/xfs_buf.c      |   51 +++++++++++++++++++++-----------------------------
 fs/xfs/xfs_buf.h      |    1 
 fs/xfs/xfs_buf_item.c |    2 -
 3 files changed, 23 insertions(+), 31 deletions(-)

Index: xfs/fs/xfs/xfs_buf_item.c
===================================================================
--- xfs.orig/fs/xfs/xfs_buf_item.c	2012-07-13 08:11:17.019997085 +0200
+++ xfs/fs/xfs/xfs_buf_item.c	2012-07-13 08:11:19.699997069 +0200
@@ -1101,7 +1101,7 @@ xfs_buf_iodone_callbacks(
 
 		if (!XFS_BUF_ISSTALE(bp)) {
 			bp->b_flags |= XBF_WRITE | XBF_ASYNC | XBF_DONE;
-			xfs_bdstrat_cb(bp);
+			xfs_buf_iorequest(bp);
 		} else {
 			xfs_buf_relse(bp);
 		}
Index: xfs/fs/xfs/xfs_buf.c
===================================================================
--- xfs.orig/fs/xfs/xfs_buf.c	2012-07-13 08:11:19.346663738 +0200
+++ xfs/fs/xfs/xfs_buf.c	2012-07-13 08:11:19.699997069 +0200
@@ -1049,27 +1049,6 @@ xfs_buf_ioerror_alert(
 		(__uint64_t)XFS_BUF_ADDR(bp), func, bp->b_error, bp->b_length);
 }
 
-int
-xfs_bwrite(
-	struct xfs_buf		*bp)
-{
-	int			error;
-
-	ASSERT(xfs_buf_islocked(bp));
-
-	bp->b_flags |= XBF_WRITE;
-	bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q);
-
-	xfs_bdstrat_cb(bp);
-
-	error = xfs_buf_iowait(bp);
-	if (error) {
-		xfs_force_shutdown(bp->b_target->bt_mount,
-				   SHUTDOWN_META_IO_ERROR);
-	}
-	return error;
-}
-
 /*
  * Called when we want to stop a buffer from getting written or read.
  * We attach the EIO error, muck with its flags, and call xfs_buf_ioend
@@ -1139,14 +1118,7 @@ xfs_bioerror_relse(
 	return EIO;
 }
 
-
-/*
- * All xfs metadata buffers except log state machine buffers
- * get this attached as their b_bdstrat callback function.
- * This is so that we can catch a buffer
- * after prematurely unpinning it to forcibly shutdown the filesystem.
- */
-int
+STATIC int
 xfs_bdstrat_cb(
 	struct xfs_buf	*bp)
 {
@@ -1167,6 +1139,27 @@ xfs_bdstrat_cb(
 	return 0;
 }
 
+int
+xfs_bwrite(
+	struct xfs_buf		*bp)
+{
+	int			error;
+
+	ASSERT(xfs_buf_islocked(bp));
+
+	bp->b_flags |= XBF_WRITE;
+	bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q);
+
+	xfs_bdstrat_cb(bp);
+
+	error = xfs_buf_iowait(bp);
+	if (error) {
+		xfs_force_shutdown(bp->b_target->bt_mount,
+				   SHUTDOWN_META_IO_ERROR);
+	}
+	return error;
+}
+
 /*
  * Wrapper around bdstrat so that we can stop data from going to disk in case
  * we are shutting down the filesystem.  Typically user data goes thru this
Index: xfs/fs/xfs/xfs_buf.h
===================================================================
--- xfs.orig/fs/xfs/xfs_buf.h	2012-07-02 12:11:56.429113153 +0200
+++ xfs/fs/xfs/xfs_buf.h	2012-07-13 08:11:36.246663636 +0200
@@ -250,7 +250,6 @@ extern void xfs_buf_unlock(xfs_buf_t *);
 extern int xfs_bwrite(struct xfs_buf *bp);
 
 extern void xfsbdstrat(struct xfs_mount *, struct xfs_buf *);
-extern int xfs_bdstrat_cb(struct xfs_buf *);
 
 extern void xfs_buf_ioend(xfs_buf_t *,	int);
 extern void xfs_buf_ioerror(xfs_buf_t *, int);

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

Re: [PATCH 2/2 v2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks

[Ben Myers]

On Fri, Jul 13, 2012 at 02:24:10AM -0400, Christoph Hellwig wrote:
> xfs_bdstrat_cb only adds a check for a shutdown filesystem over
> xfs_buf_iorequest, but xfs_buf_iodone_callbacks just checked for a shut down
> filesystem a little earlier.  In addition the shutdown handling in
> xfs_bdstrat_cb is not very suitable for this caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Thanks.

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

Re: [PATCH 2/2 v2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks

[Ben Myers]

On Fri, Jul 13, 2012 at 02:24:10AM -0400, Christoph Hellwig wrote:
> xfs_bdstrat_cb only adds a check for a shutdown filesystem over
> xfs_buf_iorequest, but xfs_buf_iodone_callbacks just checked for a shut down
> filesystem a little earlier.  In addition the shutdown handling in
> xfs_bdstrat_cb is not very suitable for this caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

This new version looks good.

Reviewed-by: Ben Myers <bpm@sgi.com>

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