Hi Christoph,
Just few minor suggestions to maintain modularity:
On Wed, Sep 14, 2011 at 7:38 PM, Christoph Hellwig <hch@xxxxxxxxxxxxx> 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@xxxxxx>
> Reported-by: Amit Sahrawat <amit.sahrawat83@xxxxxxxxx>
> Tested-by: Amit Sahrawat <amit.sahrawat83@xxxxxxxxx>
>
> 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@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
>
Regards,
Amit Sahrawat
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
