On Mon, Feb 15, 2010 at 04:44:48AM -0500, Christoph Hellwig wrote:
> Currently the fsync file operation is divided into a low-level routine doing
> all the work and one that implements the Linux file operation and does minimal
> argument wrapping. This is a leftover from the days of the vnode operations
> layer and can be removed to simplify the code a bit, as well as preparing for
> the implementation of an optimized fdatasync which needs to look at the
> Linux inode state.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
Looks good, one minor thing:
>
> Index: xfs/fs/xfs/linux-2.6/xfs_file.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_file.c 2010-02-15 10:18:58.640023657
> +0100
> +++ xfs/fs/xfs/linux-2.6/xfs_file.c 2010-02-15 10:28:07.311260422 +0100
> @@ -35,6 +35,7 @@
> #include "xfs_dir2_sf.h"
> #include "xfs_dinode.h"
> #include "xfs_inode.h"
> +#include "xfs_inode_item.h"
> #include "xfs_bmap.h"
> #include "xfs_error.h"
> #include "xfs_rw.h"
> @@ -96,6 +97,120 @@ xfs_iozero(
> return (-status);
> }
>
> +/*
> + * We ignore the datasync flag here because a datasync is effectively
> + * identical to an fsync. That is, datasync implies that we need to write
> + * only the metadata needed to be able to access the data that is written
> + * if we crash after the call completes. Hence if we are writing beyond
> + * EOF we have to log the inode size change as well, which makes it a
> + * full fsync. If we don't write beyond EOF, the inode core will be
> + * clean in memory and so we don't need to log the inode, just like
> + * fsync.
> + */
> +STATIC int
> +xfs_file_fsync(
> + struct file *file,
> + struct dentry *dentry,
> + int datasync)
> +{
> + struct xfs_inode *ip = XFS_I(dentry->d_inode);
> + struct xfs_trans *tp;
> + int error = 0;
> + int log_flushed = 0;
> +
> + xfs_itrace_entry(ip);
> +
> + if (XFS_FORCED_SHUTDOWN(ip->i_mount))
> + return -XFS_ERROR(EIO);
> +
> + xfs_iflags_clear(ip, XFS_ITRUNCATED);
> +
> + /*
> + * We always need to make sure that the required inode state is safe on
> + * disk. The inode might be clean but we still might need to force the
> + * log because of committed transactions that haven't hit the disk yet.
> + * Likewise, there could be unflushed non-transactional changes to the
> + * inode core that have to go to disk and this requires us to issue
> + * a synchronous transaction to capture these changes correctly.
> + *
> + * This code relies on the assumption that if the i_update_core field
> + * of the inode is clear and the inode is unpinned then it is clean
> + * and no action is required.
> + */
> + xfs_ilock(ip, XFS_ILOCK_SHARED);
> +
> + if (ip->i_update_core) {
> + /*
> + * Kick off a transaction to log the inode core to get the
> + * updates. The sync transaction will also force the log.
> + */
> + xfs_iunlock(ip, XFS_ILOCK_SHARED);
> + tp = xfs_trans_alloc(ip->i_mount, XFS_TRANS_FSYNC_TS);
> + error = xfs_trans_reserve(tp, 0,
> + XFS_FSYNC_TS_LOG_RES(ip->i_mount), 0, 0, 0);
> + if (error) {
> + xfs_trans_cancel(tp, 0);
> + return -error;
> + }
> + xfs_ilock(ip, XFS_ILOCK_EXCL);
> +
> + /*
> + * Note - it's possible that we might have pushed ourselves out
> + * of the way during trans_reserve which would flush the inode.
> + * But there's no guarantee that the inode buffer has actually
> + * gone out yet (it's delwri). Plus the buffer could be pinned
> + * anyway if it's part of an inode in another recent
> + * transaction. So we play it safe and fire off the
> + * transaction anyway.
> + */
> + xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> + xfs_trans_ihold(tp, ip);
> + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> + xfs_trans_set_sync(tp);
> + error = _xfs_trans_commit(tp, 0, &log_flushed);
> +
> + xfs_iunlock(ip, XFS_ILOCK_EXCL);
> + } else {
> + /*
> + * Timestamps/size haven't changed since last inode flush or
> + * inode transaction commit. That means either nothing got
> + * written or a transaction committed which caught the updates.
> + * If the latter happened and the transaction hasn't hit the
> + * disk yet, the inode will be still be pinned. If it is,
> + * force the log.
> + */
> + xfs_iunlock(ip, XFS_ILOCK_SHARED);
> + if (xfs_ipincount(ip)) {
> + if (ip->i_itemp->ili_last_lsn) {
> + error = _xfs_log_force_lsn(ip->i_mount,
> + ip->i_itemp->ili_last_lsn,
> + XFS_LOG_SYNC, &log_flushed);
> + } else {
> + error = _xfs_log_force(ip->i_mount,
> + XFS_LOG_SYNC, &log_flushed);
> + }
> + }
To be technically correct, the ilock should be held over the
pincount check and log force, as is done in xfs_iunpin_wait().
That way we can guarantee the inode was correctly forced and not
unpinned between the unlock/check/log force being issued. I know
this is just a copy of the existing fsync code, but I think that
the existing code is wrong, too. ;)
Also, if the inode is pinned while we have it locked, then
ip->i_itemp->ili_last_lsn is guaranteed to be set as it is updated
in IOP_COMMITTING() which is called during transaction commit.
As it is, ili_last_lsn is never reset to zero after a transaction,
so i think the _xfs_log_force() branch will never be executed,
either.
Other than that, the change looks ok.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
|