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

[Dave Chinner <david@fromorbit.com>]

On Sun, Dec 18, 2011 at 10:50:15AM -0500, Christoph Hellwig wrote:
> 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);
> +}

This will do a transaction on the inode, clean or dirty. That's an
awful lot of overhead for the few inodes (out of perhaps millions in
memory) that actually need it. with the ->dirty_inode callback from
the VFS, we know the only inodes that need logging are those with
i_update_core set....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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