[PATCH] xfs: fix buffer flushing during unmount

[PATCH] xfs: fix buffer flushing during unmount

[Christoph Hellwig]

The code to flush buffers in the umount code is a bit iffy: we first flush
all delwri buffers out, but then might be able to queue up a new one when
logging the sb counts.  On a normal shutdown that one would get flushed
out when doing the synchronous superblock write in xfs_unmountfs_writesb,
but we skip that one if the filesystem has been shut down.

Fix this by moving the delwri list flushing until just before unmounting
the log, and while we're at it also remove the superflous delwri list
and buffer lru flusing for the rt and log device that can never have
cached or delwri buffers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reported-by: Amit Sahrawat <amit.sahrawat83@gmail.com>
Tested-by: Amit Sahrawat <amit.sahrawat83@gmail.com>

Index: xfs/fs/xfs/xfs_buf.h
===================================================================
--- xfs.orig/fs/xfs/xfs_buf.h	2011-09-13 10:55:20.744090516 -0400
+++ xfs/fs/xfs/xfs_buf.h	2011-09-13 10:55:52.461091480 -0400
@@ -318,7 +318,6 @@ extern struct list_head *xfs_get_buftarg
 #define xfs_getsize_buftarg(buftarg)	block_size((buftarg)->bt_bdev)
 #define xfs_readonly_buftarg(buftarg)	bdev_read_only((buftarg)->bt_bdev)
 
-#define xfs_binval(buftarg)		xfs_flush_buftarg(buftarg, 1)
 #define XFS_bflush(buftarg)		xfs_flush_buftarg(buftarg, 1)
 
 #endif	/* __XFS_BUF_H__ */
Index: xfs/fs/xfs/xfs_mount.c
===================================================================
--- xfs.orig/fs/xfs/xfs_mount.c	2011-09-13 10:55:20.748087866 -0400
+++ xfs/fs/xfs/xfs_mount.c	2011-09-13 10:56:19.108088343 -0400
@@ -44,9 +44,6 @@
 #include "xfs_trace.h"
 
 
-STATIC void	xfs_unmountfs_wait(xfs_mount_t *);
-
-
 #ifdef HAVE_PERCPU_SB
 STATIC void	xfs_icsb_balance_counter(xfs_mount_t *, xfs_sb_field_t,
 						int);
@@ -1496,11 +1493,6 @@ xfs_unmountfs(
 	 */
 	xfs_log_force(mp, XFS_LOG_SYNC);
 
-	xfs_binval(mp->m_ddev_targp);
-	if (mp->m_rtdev_targp) {
-		xfs_binval(mp->m_rtdev_targp);
-	}
-
 	/*
 	 * Unreserve any blocks we have so that when we unmount we don't account
 	 * the reserved free space as used. This is really only necessary for
@@ -1526,7 +1518,16 @@ xfs_unmountfs(
 		xfs_warn(mp, "Unable to update superblock counters. "
 				"Freespace may not be correct on next mount.");
 	xfs_unmountfs_writesb(mp);
-	xfs_unmountfs_wait(mp); 		/* wait for async bufs */
+
+	/*
+	 * Make sure all buffers have been flushed and completed before
+	 * unmounting the log.
+	 */
+	error = xfs_flush_buftarg(mp->m_ddev_targp, 1);
+	if (error)
+		xfs_warn(mp, "%d busy buffers during unmount.", error);
+	xfs_wait_buftarg(mp->m_ddev_targp);
+
 	xfs_log_unmount_write(mp);
 	xfs_log_unmount(mp);
 	xfs_uuid_unmount(mp);
@@ -1537,16 +1538,6 @@ xfs_unmountfs(
 	xfs_free_perag(mp);
 }
 
-STATIC void
-xfs_unmountfs_wait(xfs_mount_t *mp)
-{
-	if (mp->m_logdev_targp != mp->m_ddev_targp)
-		xfs_wait_buftarg(mp->m_logdev_targp);
-	if (mp->m_rtdev_targp)
-		xfs_wait_buftarg(mp->m_rtdev_targp);
-	xfs_wait_buftarg(mp->m_ddev_targp);
-}
-
 int
 xfs_fs_writable(xfs_mount_t *mp)
 {

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

Re: [PATCH] xfs: fix buffer flushing during unmount

[Alex Elder]

On Wed, 2011-09-14 at 10:08 -0400, Christoph Hellwig wrote:
> The code to flush buffers in the umount code is a bit iffy: we first flush
> all delwri buffers out, but then might be able to queue up a new one when
> logging the sb counts.  On a normal shutdown that one would get flushed
> out when doing the synchronous superblock write in xfs_unmountfs_writesb,
> but we skip that one if the filesystem has been shut down.
> 
> Fix this by moving the delwri list flushing until just before unmounting
> the log, and while we're at it also remove the superflous delwri list
> and buffer lru flusing for the rt and log device that can never have
> cached or delwri buffers.

What prevents blocks on a realtime device from being cached?

I haven't looked exhaustively, but it looks like the summary
and bitmap data are operated on without caching, but the
realtime data itself is treated no different from "normal"
data blocks.

Please correct me if I'm wrong about that.

I do agree about the log device not having delwri cached
buffers.  Still, it might be good to have this change
done in a different commit from the one that changes
the order of things in xfs_unmountfs().

					-Alex


> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Amit Sahrawat <amit.sahrawat83@gmail.com>
> Tested-by: Amit Sahrawat <amit.sahrawat83@gmail.com>


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

Re: [PATCH] xfs: fix buffer flushing during unmount

[Christoph Hellwig]

On Mon, Sep 19, 2011 at 12:33:20PM -0500, Alex Elder wrote:
> On Wed, 2011-09-14 at 10:08 -0400, Christoph Hellwig wrote:
> > The code to flush buffers in the umount code is a bit iffy: we first flush
> > all delwri buffers out, but then might be able to queue up a new one when
> > logging the sb counts.  On a normal shutdown that one would get flushed
> > out when doing the synchronous superblock write in xfs_unmountfs_writesb,
> > but we skip that one if the filesystem has been shut down.
> > 
> > Fix this by moving the delwri list flushing until just before unmounting
> > the log, and while we're at it also remove the superflous delwri list
> > and buffer lru flusing for the rt and log device that can never have
> > cached or delwri buffers.
> 
> What prevents blocks on a realtime device from being cached?

Note that cached here means using buffers that end up in the rbtree
and/or on the lru.  We simply never do that for the rt and log device,
as that would mean complicating the code for no reason - the only
time we ever use buffer on the rt device is for the size check during
mount and the _uncached variant where we free the buffer after the
refcount hits zero is just fine for that, and xfs_zero_remaining_bytes,
which actually operates on a regular file and thus must not use the
rbtree or the lru (and should eventually stop using buffer routines at
all).  All actual metadata which the buffer cache is used for resides
on the main device. Log buffers stay alive from mount to unmount so
we're fine with that interface as well.

> 
> I haven't looked exhaustively, but it looks like the summary
> and bitmap data are operated on without caching, but the
> realtime data itself is treated no different from "normal"
> data blocks.

The buffer cache is never used for actual file data.

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

Re: [PATCH] xfs: fix buffer flushing during unmount

[Alex Elder]

On Wed, 2011-09-28 at 12:34 -0400, Christoph Hellwig wrote:
> On Mon, Sep 19, 2011 at 12:33:20PM -0500, Alex Elder wrote:
> > On Wed, 2011-09-14 at 10:08 -0400, Christoph Hellwig wrote:
> > > The code to flush buffers in the umount code is a bit iffy: we first flush
> > > all delwri buffers out, but then might be able to queue up a new one when
> > > logging the sb counts.  On a normal shutdown that one would get flushed
> > > out when doing the synchronous superblock write in xfs_unmountfs_writesb,
> > > but we skip that one if the filesystem has been shut down.
> > > 
> > > Fix this by moving the delwri list flushing until just before unmounting
> > > the log, and while we're at it also remove the superflous delwri list
> > > and buffer lru flusing for the rt and log device that can never have
> > > cached or delwri buffers.
> > 
> > What prevents blocks on a realtime device from being cached?
> 
> Note that cached here means using buffers that end up in the rbtree
> and/or on the lru.  We simply never do that for the rt and log device,
> as that would mean complicating the code for no reason - the only
> time we ever use buffer on the rt device is for the size check during
> mount and the _uncached variant where we free the buffer after the
> refcount hits zero is just fine for that, and xfs_zero_remaining_bytes,
> which actually operates on a regular file and thus must not use the
> rbtree or the lru (and should eventually stop using buffer routines at
> all).  All actual metadata which the buffer cache is used for resides
> on the main device. Log buffers stay alive from mount to unmount so
> we're fine with that interface as well.

And now that you've said that I see that there are in fact
no calls to xfs_buf_get() that specify anything other than
the data device.

> > 
> > I haven't looked exhaustively, but it looks like the summary
> > and bitmap data are operated on without caching, but the
> > realtime data itself is treated no different from "normal"
> > data blocks.
> 
> The buffer cache is never used for actual file data.

OK, I get it.  Your original patch looks good.

Reviewed-by: Alex Elder <aelder@sgi.com>



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

Re: [PATCH] xfs: fix buffer flushing during unmount

[Amit Sahrawat]

Hi Christoph,
Just few minor suggestions to maintain modularity:

On Wed, Sep 14, 2011 at 7:38 PM, Christoph Hellwig <hch@infradead.org> wrote:
> The code to flush buffers in the umount code is a bit iffy: we first flush
> all delwri buffers out, but then might be able to queue up a new one when
> logging the sb counts.  On a normal shutdown that one would get flushed
> out when doing the synchronous superblock write in xfs_unmountfs_writesb,
> but we skip that one if the filesystem has been shut down.
>
> Fix this by moving the delwri list flushing until just before unmounting
> the log, and while we're at it also remove the superflous delwri list
> and buffer lru flusing for the rt and log device that can never have
> cached or delwri buffers.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Amit Sahrawat <amit.sahrawat83@gmail.com>
> Tested-by: Amit Sahrawat <amit.sahrawat83@gmail.com>
>
> Index: xfs/fs/xfs/xfs_buf.h
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_buf.h   2011-09-13 10:55:20.744090516 -0400
> +++ xfs/fs/xfs/xfs_buf.h        2011-09-13 10:55:52.461091480 -0400
> @@ -318,7 +318,6 @@ extern struct list_head *xfs_get_buftarg
>  #define xfs_getsize_buftarg(buftarg)   block_size((buftarg)->bt_bdev)
>  #define xfs_readonly_buftarg(buftarg)  bdev_read_only((buftarg)->bt_bdev)
>
> -#define xfs_binval(buftarg)            xfs_flush_buftarg(buftarg, 1)
>  #define XFS_bflush(buftarg)            xfs_flush_buftarg(buftarg, 1)
>
>  #endif /* __XFS_BUF_H__ */
> Index: xfs/fs/xfs/xfs_mount.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_mount.c 2011-09-13 10:55:20.748087866 -0400
> +++ xfs/fs/xfs/xfs_mount.c      2011-09-13 10:56:19.108088343 -0400
> @@ -44,9 +44,6 @@
>  #include "xfs_trace.h"
>
>
> -STATIC void    xfs_unmountfs_wait(xfs_mount_t *);
> -
> -
>  #ifdef HAVE_PERCPU_SB
>  STATIC void    xfs_icsb_balance_counter(xfs_mount_t *, xfs_sb_field_t,
>                                                int);
> @@ -1496,11 +1493,6 @@ xfs_unmountfs(
>         */
>        xfs_log_force(mp, XFS_LOG_SYNC);
>
> -       xfs_binval(mp->m_ddev_targp);
> -       if (mp->m_rtdev_targp) {
> -               xfs_binval(mp->m_rtdev_targp);
> -       }
> -
This is OK.
>        /*
>         * Unreserve any blocks we have so that when we unmount we don't account
>         * the reserved free space as used. This is really only necessary for
> @@ -1526,7 +1518,16 @@ xfs_unmountfs(
>                xfs_warn(mp, "Unable to update superblock counters. "
>                                "Freespace may not be correct on next mount.");
>        xfs_unmountfs_writesb(mp);
> -       xfs_unmountfs_wait(mp);                 /* wait for async bufs */
> +
> +       /*
> +        * Make sure all buffers have been flushed and completed before
> +        * unmounting the log.
> +        */
> +       error = xfs_flush_buftarg(mp->m_ddev_targp, 1);
> +       if (error)
> +               xfs_warn(mp, "%d busy buffers during unmount.", error);
> +       xfs_wait_buftarg(mp->m_ddev_targp);
> +
instead of removing xfs_unmountfs_wait() altogether, how about keeping
the function and modifying the contents with above changes?
flushing/waiting at this point looks a little out of order.
>        xfs_log_unmount_write(mp);
>        xfs_log_unmount(mp);
>        xfs_uuid_unmount(mp);
> @@ -1537,16 +1538,6 @@ xfs_unmountfs(
>        xfs_free_perag(mp);
>  }
>
> -STATIC void
> -xfs_unmountfs_wait(xfs_mount_t *mp)
> -{
> -       if (mp->m_logdev_targp != mp->m_ddev_targp)
> -               xfs_wait_buftarg(mp->m_logdev_targp);
> -       if (mp->m_rtdev_targp)
> -               xfs_wait_buftarg(mp->m_rtdev_targp);
> -       xfs_wait_buftarg(mp->m_ddev_targp);
> -}
> -
I mean the above should be like this:
STATIC void
xfs_unmountfs_wait(xfs_mount_t *mp)
{
 +
 +       /*
 +        * Make sure all buffers have been flushed and completed before
 +        * unmounting the log.
 +        */
 +       error = xfs_flush_buftarg(mp->m_ddev_targp, 1);
 +       if (error)
 +               xfs_warn(mp, "%d busy buffers during unmount.", error);
 +       xfs_wait_buftarg(mp->m_ddev_targp);
 +
}

>  int
>  xfs_fs_writable(xfs_mount_t *mp)
>  {
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
>

Regards,
Amit Sahrawat

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

Re: [PATCH] xfs: fix buffer flushing during unmount

[Christoph Hellwig]

I really don't see what a trivial wrapper would buy us there.

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