Re: [PATCH 2/2] xfs: log all dirty inodes in xfs_fs_sync_fs

Sean Thomas Caron
2011-12-18T20:03:08Z

Thanks, Christoph. I'll try to get a kernel built tomorrow that
incorporates these patches and give it some testing.

Best,

-Sean

Quoting Christoph Hellwig <hch@infradead.org>:

> Since Linux 2.6.36 the writeback code has introduces various measures for
> live lock prevention during sync(). Unfortunately some of these are
> actively harmful for the XFS model, where the inode gets marked dirty for
> metadata from the data I/O handler.
>
> The older_than_this checks that are now more strictly enforced since
>
> writeback: avoid livelocking WB_SYNC_ALL writeback
>
> by only calling into __writeback_inodes_sb and thus only sampling the
> current cut off time once. But on a slow enough devices the previous
> asynchronous sync pass might not have fully completed yet, and thus XFS
> might mark metadata dirty only after that sampling of the cut off time for
> the blocking pass already happened. I have not myself reproduced this
> myself on a real system, but by introducing artificial delay into the
> XFS I/O completion workqueues it can be reproduced easily.
>
> Fix this by iterating over all XFS inodes in ->sync_fs and log all that
> are dirty. This might log inode that only got redirtied after the
> previous pass, but given how cheap delayed logging of inodes is it
> isn't a major concern for performance.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Index: xfs/fs/xfs/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_sync.c 2011-12-14 05:33:06.436599621 -0800
> +++ xfs/fs/xfs/xfs_sync.c 2011-12-14 05:38:49.084743337 -0800
> @@ -336,6 +336,29 @@ xfs_sync_fsdata(
> return error;
> }
>
> +int
> +xfs_log_inode(
> + struct xfs_inode *ip,
> + struct xfs_perag *pag,
> + int flags)
> +{
> + struct xfs_mount *mp = ip->i_mount;
> + struct xfs_trans *tp;
> + int error;
> +
> + tp = xfs_trans_alloc(mp, XFS_TRANS_FSYNC_TS);
> + error = xfs_trans_reserve(tp, 0, XFS_FSYNC_TS_LOG_RES(mp), 0, 0, 0);
> + if (error) {
> + xfs_trans_cancel(tp, 0);
> + return error;
> + }
> +
> + xfs_ilock(ip, XFS_ILOCK_EXCL);
> + xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> + return xfs_trans_commit(tp, 0);
> +}
> +
> /*
> * When remounting a filesystem read-only or freezing the
> filesystem, we have
> * two phases to execute. This first phase is syncing the data before we
> @@ -359,6 +382,16 @@ xfs_quiesce_data(
> {
> int error, error2 = 0;
>
> + /*
> + * Log all pending size and timestamp updates. The vfs writeback
> + * code is supposed to do this, but due to its overagressive
> + * livelock detection it will skip inodes where appending writes
> + * were written out in the first non-blocking sync phase if their
> + * completion took long enough that it happened after taking the
> + * timestamp for the cut-off in the blocking phase.
> + */
> + xfs_inode_ag_iterator(mp, xfs_log_inode, 0);
> +
> xfs_qm_sync(mp, SYNC_TRYLOCK);
> xfs_qm_sync(mp, SYNC_WAIT);
>
> Index: xfs/fs/xfs/xfs_sync.h
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_sync.h 2011-10-17 00:28:57.255149593 -0700
> +++ xfs/fs/xfs/xfs_sync.h 2011-12-14 05:39:23.187891918 -0800
> @@ -34,6 +34,8 @@ void xfs_quiesce_attr(struct xfs_mount *
>
> void xfs_flush_inodes(struct xfs_inode *ip);
>
> +int xfs_log_inode(struct xfs_inode *ip, struct xfs_perag *pag, int flags);
> +
> int xfs_reclaim_inodes(struct xfs_mount *mp, int mode);
> int xfs_reclaim_inodes_count(struct xfs_mount *mp);
> void xfs_reclaim_inodes_nr(struct xfs_mount *mp, int nr_to_scan);
> Index: xfs/fs/xfs/xfs_super.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_super.c 2011-12-14 05:33:07.193262189 -0800
> +++ xfs/fs/xfs/xfs_super.c 2011-12-14 05:38:56.108038623 -0800
> @@ -869,27 +869,6 @@ xfs_fs_dirty_inode(
> }
>
> STATIC int
> -xfs_log_inode(
> - struct xfs_inode *ip)
> -{
> - struct xfs_mount *mp = ip->i_mount;
> - struct xfs_trans *tp;
> - int error;
> -
> - tp = xfs_trans_alloc(mp, XFS_TRANS_FSYNC_TS);
> - error = xfs_trans_reserve(tp, 0, XFS_FSYNC_TS_LOG_RES(mp), 0, 0, 0);
> - if (error) {
> - xfs_trans_cancel(tp, 0);
> - return error;
> - }
> -
> - xfs_ilock(ip, XFS_ILOCK_EXCL);
> - xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> - xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> - return xfs_trans_commit(tp, 0);
> -}
> -
> -STATIC int
> xfs_fs_write_inode(
> struct inode *inode,
> struct writeback_control *wbc)
> @@ -913,7 +892,7 @@ xfs_fs_write_inode(
> * ->sync_fs call do that for thus, which reduces the number
> * of synchronous log forces dramatically.
> */
> - error = xfs_log_inode(ip);
> + error = xfs_log_inode(ip, NULL, 0);
> if (error)
> goto out;
> return 0;