* [PATCH 0/2] buffer caches fixes for Linux 3.5 @ 2012-07-02 10:00 Christoph Hellwig 2012-07-02 10:00 ` [PATCH 1/2] xfs: prevent recursion in xfs_buf_iorequest Christoph Hellwig 2012-07-02 10:00 ` [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks Christoph Hellwig 0 siblings, 2 replies; 15+ messages in thread From: Christoph Hellwig @ 2012-07-02 10:00 UTC (permalink / raw) To: xfs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] xfs: prevent recursion in xfs_buf_iorequest 2012-07-02 10:00 [PATCH 0/2] buffer caches fixes for Linux 3.5 Christoph Hellwig @ 2012-07-02 10:00 ` Christoph Hellwig 2012-07-03 0:27 ` Dave Chinner 2012-07-02 10:00 ` [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks Christoph Hellwig 1 sibling, 1 reply; 15+ messages in thread From: Christoph Hellwig @ 2012-07-02 10:00 UTC (permalink / raw) To: xfs [-- 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] xfs: prevent recursion in xfs_buf_iorequest 2012-07-02 10:00 ` [PATCH 1/2] xfs: prevent recursion in xfs_buf_iorequest Christoph Hellwig @ 2012-07-03 0:27 ` Dave Chinner 0 siblings, 0 replies; 15+ messages in thread From: Dave Chinner @ 2012-07-03 0:27 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks 2012-07-02 10:00 [PATCH 0/2] buffer caches fixes for Linux 3.5 Christoph Hellwig 2012-07-02 10:00 ` [PATCH 1/2] xfs: prevent recursion in xfs_buf_iorequest Christoph Hellwig @ 2012-07-02 10:00 ` Christoph Hellwig 2012-07-03 0:28 ` Dave Chinner 2012-07-12 23:04 ` Ben Myers 1 sibling, 2 replies; 15+ messages in thread From: Christoph Hellwig @ 2012-07-02 10:00 UTC (permalink / raw) To: xfs [-- 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks 2012-07-02 10:00 ` [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks Christoph Hellwig @ 2012-07-03 0:28 ` Dave Chinner 2012-07-03 16:05 ` Christoph Hellwig 2012-07-12 23:04 ` Ben Myers 1 sibling, 1 reply; 15+ messages in thread From: Dave Chinner @ 2012-07-03 0:28 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks 2012-07-03 0:28 ` Dave Chinner @ 2012-07-03 16:05 ` Christoph Hellwig 2012-07-03 23:29 ` Dave Chinner 0 siblings, 1 reply; 15+ messages in thread From: Christoph Hellwig @ 2012-07-03 16:05 UTC (permalink / raw) To: Dave Chinner; +Cc: Christoph Hellwig, xfs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks 2012-07-03 16:05 ` Christoph Hellwig @ 2012-07-03 23:29 ` Dave Chinner 2012-07-04 5:57 ` Christoph Hellwig 0 siblings, 1 reply; 15+ messages in thread From: Dave Chinner @ 2012-07-03 23:29 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks 2012-07-03 23:29 ` Dave Chinner @ 2012-07-04 5:57 ` Christoph Hellwig 2012-07-04 8:32 ` Dave Chinner 0 siblings, 1 reply; 15+ messages in thread From: Christoph Hellwig @ 2012-07-04 5:57 UTC (permalink / raw) To: Dave Chinner; +Cc: Christoph Hellwig, xfs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks 2012-07-04 5:57 ` Christoph Hellwig @ 2012-07-04 8:32 ` Dave Chinner 2012-07-04 15:16 ` Christoph Hellwig 0 siblings, 1 reply; 15+ messages in thread From: Dave Chinner @ 2012-07-04 8:32 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks 2012-07-04 8:32 ` Dave Chinner @ 2012-07-04 15:16 ` Christoph Hellwig 0 siblings, 0 replies; 15+ messages in thread From: Christoph Hellwig @ 2012-07-04 15:16 UTC (permalink / raw) To: Dave Chinner; +Cc: Christoph Hellwig, xfs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks 2012-07-02 10:00 ` [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks Christoph Hellwig 2012-07-03 0:28 ` Dave Chinner @ 2012-07-12 23:04 ` Ben Myers 2012-07-13 6:16 ` Christoph Hellwig 1 sibling, 1 reply; 15+ messages in thread From: Ben Myers @ 2012-07-12 23:04 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks 2012-07-12 23:04 ` Ben Myers @ 2012-07-13 6:16 ` Christoph Hellwig 2012-07-13 6:24 ` [PATCH 2/2 v2] " Christoph Hellwig 0 siblings, 1 reply; 15+ messages in thread From: Christoph Hellwig @ 2012-07-13 6:16 UTC (permalink / raw) To: Ben Myers; +Cc: Christoph Hellwig, xfs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2 v2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks 2012-07-13 6:16 ` Christoph Hellwig @ 2012-07-13 6:24 ` Christoph Hellwig 2012-07-13 16:59 ` Ben Myers 2012-07-13 19:12 ` Ben Myers 0 siblings, 2 replies; 15+ messages in thread From: Christoph Hellwig @ 2012-07-13 6:24 UTC (permalink / raw) To: Ben Myers; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2 v2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks 2012-07-13 6:24 ` [PATCH 2/2 v2] " Christoph Hellwig @ 2012-07-13 16:59 ` Ben Myers 2012-07-13 19:12 ` Ben Myers 1 sibling, 0 replies; 15+ messages in thread From: Ben Myers @ 2012-07-13 16:59 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2 v2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks 2012-07-13 6:24 ` [PATCH 2/2 v2] " Christoph Hellwig 2012-07-13 16:59 ` Ben Myers @ 2012-07-13 19:12 ` Ben Myers 1 sibling, 0 replies; 15+ messages in thread From: Ben Myers @ 2012-07-13 19:12 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2012-07-13 19:07 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-07-02 10:00 [PATCH 0/2] buffer caches fixes for Linux 3.5 Christoph Hellwig 2012-07-02 10:00 ` [PATCH 1/2] xfs: prevent recursion in xfs_buf_iorequest Christoph Hellwig 2012-07-03 0:27 ` Dave Chinner 2012-07-02 10:00 ` [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks Christoph Hellwig 2012-07-03 0:28 ` Dave Chinner 2012-07-03 16:05 ` Christoph Hellwig 2012-07-03 23:29 ` Dave Chinner 2012-07-04 5:57 ` Christoph Hellwig 2012-07-04 8:32 ` Dave Chinner 2012-07-04 15:16 ` Christoph Hellwig 2012-07-12 23:04 ` Ben Myers 2012-07-13 6:16 ` Christoph Hellwig 2012-07-13 6:24 ` [PATCH 2/2 v2] " Christoph Hellwig 2012-07-13 16:59 ` Ben Myers 2012-07-13 19:12 ` Ben Myers
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.