xfs
[Top] [All Lists]

Re: [PATCH 5/8] xfs: log timestamp updates

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 5/8] xfs: log timestamp updates
From: Ben Myers <bpm@xxxxxxx>
Date: Mon, 5 Mar 2012 17:57:05 -0600
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120229095508.983470279@xxxxxxxxxxxxxxxxxxxxxx>
References: <20120229095347.009884687@xxxxxxxxxxxxxxxxxxxxxx> <20120229095508.983470279@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
On Wed, Feb 29, 2012 at 04:53:52AM -0500, Christoph Hellwig wrote:
> Timestamps on regular files are the last metadata that XFS does not update
> transactionally.  Now that we use the delaylog mode exclusively and made
> the log scode scale extremly well there is no need to bypass that code for
> timestamp updates.  Logging all updates allows to drop a lot of code, and
> will allow for further performance improvements later on.
> 
> Note that this patch drops optimized handling of fdatasync - it will be
> added back in a separate commit.
> 
> Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

...

> Index: xfs/fs/xfs/xfs_trans_inode.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_trans_inode.c 2012-02-13 13:48:02.127012825 -0800
> +++ xfs/fs/xfs/xfs_trans_inode.c      2012-02-19 13:37:57.829955809 -0800
> @@ -95,10 +95,14 @@ xfs_trans_ichgtime(
>       if ((flags & XFS_ICHGTIME_MOD) &&
>           !timespec_equal(&inode->i_mtime, &tv)) {
>               inode->i_mtime = tv;
> +             ip->i_d.di_mtime.t_sec = tv.tv_sec;
> +             ip->i_d.di_mtime.t_nsec = tv.tv_nsec;
>       }
>       if ((flags & XFS_ICHGTIME_CHG) &&
>           !timespec_equal(&inode->i_ctime, &tv)) {
>               inode->i_ctime = tv;
> +             ip->i_d.di_ctime.t_sec = tv.tv_sec;
> +             ip->i_d.di_ctime.t_nsec = tv.tv_nsec;
>       }
>  }

So... you will always want to log the inode after calling
xfs_trans_ichgtime.  FWICS this is done in all cases except for
xfs_qm_scall_trunc_qfile.  Maybe it doesn't matter in that case.

...

>  STATIC void
>  xfs_fs_dirty_inode(
> -     struct inode    *inode,
> -     int             flags)
> -{
> -     barrier();
> -     XFS_I(inode)->i_update_core = 1;
> -}
> -
> -STATIC int
> -xfs_fs_write_inode(
>       struct inode            *inode,
> -     struct writeback_control *wbc)
> +     int                     flags)
>  {
>       struct xfs_inode        *ip = XFS_I(inode);
>       struct xfs_mount        *mp = ip->i_mount;
> -     int                     error = EAGAIN;
> -
> -     trace_xfs_write_inode(ip);
> -
> -     if (XFS_FORCED_SHUTDOWN(mp))
> -             return -XFS_ERROR(EIO);
> -
> -     if (wbc->sync_mode == WB_SYNC_ALL || wbc->for_kupdate) {
> -             /*
> -              * Make sure the inode has made it it into the log.  Instead
> -              * of forcing it all the way to stable storage using a
> -              * synchronous transaction we let the log force inside the
> -              * ->sync_fs call do that for thus, which reduces the number
> -              * of synchronous log forces dramatically.
> -              */
> -             error = xfs_log_dirty_inode(ip, NULL, 0);
> -             if (error)
> -                     goto out;
> -             return 0;
> -     } else {
> -             if (!ip->i_update_core)
> -                     return 0;
> -
> -             /*
> -              * We make this non-blocking if the inode is contended, return
> -              * EAGAIN to indicate to the caller that they did not succeed.
> -              * This prevents the flush path from blocking on inodes inside
> -              * another operation right now, they get caught later by
> -              * xfs_sync.
> -              */
> -             if (!xfs_ilock_nowait(ip, XFS_ILOCK_SHARED))
> -                     goto out;
> +     struct xfs_trans        *tp;
> +     int                     error;
>  
> -             if (xfs_ipincount(ip) || !xfs_iflock_nowait(ip))
> -                     goto out_unlock;
> +     if (flags != I_DIRTY_SYNC)
> +             return;

Ok... so this gets you updates from touch_atime and file_update_time.

> @@ -385,16 +359,6 @@ 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_dirty_inode, 0);
> -

You just put that in there!

Looks good!

Reviewed-by: Ben Myers <bpm@xxxxxxx>

<Prev in Thread] Current Thread [Next in Thread>