On Thu, Feb 22, 2018 at 07:29:57AM -0800, Christoph Hellwig wrote:
> Use the VFS dirty inode tracking for lazytime inodes only, and just
> log them in ->dirty_inode.
This is a lot cleaner than what I was looking at doing. :)
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/xfs_iops.c | 5 +++++
> fs/xfs/xfs_super.c | 23 +++++++++++++++++++++++
> fs/xfs/xfs_trans_inode.c | 8 ++++++++
> 3 files changed, 36 insertions(+)
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 56475fcd76f2..0946a3baae6a 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -46,6 +46,7 @@
> #include <linux/security.h>
> #include <linux/iomap.h>
> #include <linux/slab.h>
> +#include <linux/iversion.h>
>
> /*
> * Directories have different lock order w.r.t. mmap_sem compared to regular
> @@ -1057,6 +1058,10 @@ xfs_vn_update_time(
>
> trace_xfs_update_time(ip);
>
> + if ((inode->i_sb->s_flags & SB_LAZYTIME) &&
> + !((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false)))
> + return generic_update_time(inode, now, flags);
So if we've incremented iversion here on a lazytime update (hence
making it not lazy), we won't do the iversion update in
xfs_trans_log_inode() because the query bit has been cleared here.
Hence we won't set XFS_ILOG_CORE in the transaction and the iversion
update will not be logged.
Maybe:
int log_flags = XFS_ILOG_TIMESTAMP;
...
if (inode->i_sb->s_flags & SB_LAZYTIME) {
if (!(flags & S_VERSION) ||
!inode_maybe_inc_iversion(inode, false)))
return generic_update_time(inode, now, flags);
/* Capture the iversion update that just occurred */
log_flags |= XFS_ILOG_CORE;
}
.....
xfs_trans_log_inode(tp, ip, log_flags);
.....
> diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
> index fddacf9575df..769943fcc1f2 100644
> --- a/fs/xfs/xfs_trans_inode.c
> +++ b/fs/xfs/xfs_trans_inode.c
> @@ -98,9 +98,17 @@ xfs_trans_log_inode(
> xfs_inode_t *ip,
> uint flags)
> {
> + struct inode *inode = VFS_I(ip);
> +
> ASSERT(ip->i_itemp != NULL);
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>
> + if (inode->i_state & (I_DIRTY_TIME | I_DIRTY_TIME_EXPIRED)) {
> + spin_lock(&inode->i_lock);
> + inode->i_state &= ~(I_DIRTY_TIME | I_DIRTY_TIME_EXPIRED);
> + spin_unlock(&inode->i_lock);
> + }
I suspect this is racy w.r.t other code that sets/clears the
I_DIRTY_TIME fields. Shouldn't both the check and clear be under the
spin lock?
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
