xfs: implement ->dirty_inode to fix timestamp handling

This is picking up on Felix's repost of Dave's patch to implement a
.dirty_inode method.  We really need this notification because
the VFS keeps writing directly into the inode structure instead
of going through methods to update this state.  In addition to
the long-known atime issue we now also have a caller in VM code
that updates c/mtime that way for shared writeable mmaps.  And
I found another one that no one has noticed in practice in the FIFO
code.

So implement ->dirty_inode to set i_update_core whenever the
inode gets externally dirtied, and switch the c/mtime handling to
the same scheme we already use for atime (always picking up
the value from the Linux inode).

Note that this patch also removes the xfs_synchronize_atime call
in xfs_reclaim it was superflous as we already synchronize the time
when writing the inode via the log (xfs_inode_item_format) or the
normal buffers (xfs_iflush_int).

In addition also remove the I_CLEAR check before copying the Linux
timestamps - now that we always have the Linux inode available
we can always use the timestamps in it.

Also switch to just using file_update_time for regular reads/writes -
that will get us all optimization done to it for free and make
sure we notice early when it breaks.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Felix Blyakher <felixb@sgi.com>
Reviewed-by: Alex Elder <aelder@sgi.com>
Signed-off-by: Alex Elder <aelder@sgi.com>

diff --git a/fs/xfs/linux-2.6/xfs_aops.c b/fs/xfs/linux-2.6/xfs_aops.c
index d5e5559e31db3774ba6f76e350302009ccf831eb..40d2226060d177b72fa27e44962202687da0aaae 100644 (file)
--- a/fs/xfs/linux-2.6/xfs_aops.c
+++ b/fs/xfs/linux-2.6/xfs_aops.c
@@ -215,7 +215,6 @@ xfs_setfilesize(
 
        if (ip->i_d.di_size < isize) {
                ip->i_d.di_size = isize;
-               ip->i_update_core = 1;
                xfs_mark_inode_dirty_sync(ip);
        }
 

diff --git a/fs/xfs/linux-2.6/xfs_iops.c b/fs/xfs/linux-2.6/xfs_iops.c
index 626b474b2cdde38cb16d839f6d7a814ae4d875da..cdf7114d41fc32615007253f05cc14743b2e2b63 100644 (file)
--- a/fs/xfs/linux-2.6/xfs_iops.c
+++ b/fs/xfs/linux-2.6/xfs_iops.c
@@ -57,19 +57,22 @@
 #include <linux/fiemap.h>
 
 /*
- * Bring the atime in the XFS inode uptodate.
- * Used before logging the inode to disk or when the Linux inode goes away.
+ * Bring the timestamps in the XFS inode uptodate.
+ *
+ * Used before writing the inode to disk.
  */
 void
-xfs_synchronize_atime(
+xfs_synchronize_times(
        xfs_inode_t     *ip)
 {
        struct inode    *inode = VFS_I(ip);
 
-       if (!(inode->i_state & I_CLEAR)) {
-               ip->i_d.di_atime.t_sec = (__int32_t)inode->i_atime.tv_sec;
-               ip->i_d.di_atime.t_nsec = (__int32_t)inode->i_atime.tv_nsec;
-       }
+       ip->i_d.di_atime.t_sec = (__int32_t)inode->i_atime.tv_sec;
+       ip->i_d.di_atime.t_nsec = (__int32_t)inode->i_atime.tv_nsec;
+       ip->i_d.di_ctime.t_sec = (__int32_t)inode->i_ctime.tv_sec;
+       ip->i_d.di_ctime.t_nsec = (__int32_t)inode->i_ctime.tv_nsec;
+       ip->i_d.di_mtime.t_sec = (__int32_t)inode->i_mtime.tv_sec;
+       ip->i_d.di_mtime.t_nsec = (__int32_t)inode->i_mtime.tv_nsec;
 }
 
 /*
@@ -106,32 +109,20 @@ xfs_ichgtime(
        if ((flags & XFS_ICHGTIME_MOD) &&
            !timespec_equal(&inode->i_mtime, &tv)) {
                inode->i_mtime = tv;
-               ip->i_d.di_mtime.t_sec = (__int32_t)tv.tv_sec;
-               ip->i_d.di_mtime.t_nsec = (__int32_t)tv.tv_nsec;
                sync_it = 1;
        }
        if ((flags & XFS_ICHGTIME_CHG) &&
            !timespec_equal(&inode->i_ctime, &tv)) {
                inode->i_ctime = tv;
-               ip->i_d.di_ctime.t_sec = (__int32_t)tv.tv_sec;
-               ip->i_d.di_ctime.t_nsec = (__int32_t)tv.tv_nsec;
                sync_it = 1;
        }
 
        /*
-        * We update the i_update_core field _after_ changing
-        * the timestamps in order to coordinate properly with
-        * xfs_iflush() so that we don't lose timestamp updates.
-        * This keeps us from having to hold the inode lock
-        * while doing this.  We use the SYNCHRONIZE macro to
-        * ensure that the compiler does not reorder the update
-        * of i_update_core above the timestamp updates above.
+        * Update complete - now make sure everyone knows that the inode
+        * is dirty.
         */
-       if (sync_it) {
-               SYNCHRONIZE();
-               ip->i_update_core = 1;
+       if (sync_it)
                xfs_mark_inode_dirty_sync(ip);
-       }
 }
 
 /*
@@ -514,10 +505,8 @@ xfs_vn_getattr(
        stat->gid = ip->i_d.di_gid;
        stat->ino = ip->i_ino;
        stat->atime = inode->i_atime;
-       stat->mtime.tv_sec = ip->i_d.di_mtime.t_sec;
-       stat->mtime.tv_nsec = ip->i_d.di_mtime.t_nsec;
-       stat->ctime.tv_sec = ip->i_d.di_ctime.t_sec;
-       stat->ctime.tv_nsec = ip->i_d.di_ctime.t_nsec;
+       stat->mtime = inode->i_mtime;
+       stat->ctime = inode->i_ctime;
        stat->blocks =
                XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks);
 

diff --git a/fs/xfs/linux-2.6/xfs_lrw.c b/fs/xfs/linux-2.6/xfs_lrw.c
index 49e4a6aea73c3c85f7d388d56d4371867c930178..072050f8d34634d59e4d200dfe14218bcf0e75ad 100644 (file)
--- a/fs/xfs/linux-2.6/xfs_lrw.c
+++ b/fs/xfs/linux-2.6/xfs_lrw.c
@@ -667,7 +667,7 @@ start:
                xip->i_new_size = new_size;
 
        if (likely(!(ioflags & IO_INVIS)))
-               xfs_ichgtime(xip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+               file_update_time(file);
 
        /*
         * If the offset is beyond the size of the file, we have a couple

diff --git a/fs/xfs/linux-2.6/xfs_super.c b/fs/xfs/linux-2.6/xfs_super.c
index 5d7c60ac77b4967d262fbca08037993109c9c39b..1ea65b6e5ab667a2362b4122d651a9523b12af12 100644 (file)
--- a/fs/xfs/linux-2.6/xfs_super.c
+++ b/fs/xfs/linux-2.6/xfs_super.c
@@ -976,6 +976,28 @@ xfs_fs_inode_init_once(
        mrlock_init(&ip->i_iolock, MRLOCK_BARRIER, "xfsio", ip->i_ino);
 }
 
+/*
+ * Dirty the XFS inode when mark_inode_dirty_sync() is called so that
+ * we catch unlogged VFS level updates to the inode. Care must be taken
+ * here - the transaction code calls mark_inode_dirty_sync() to mark the
+ * VFS inode dirty in a transaction and clears the i_update_core field;
+ * it must clear the field after calling mark_inode_dirty_sync() to
+ * correctly indicate that the dirty state has been propagated into the
+ * inode log item.
+ *
+ * We need the barrier() to maintain correct ordering between unlogged
+ * updates and the transaction commit code that clears the i_update_core
+ * field. This requires all updates to be completed before marking the
+ * inode dirty.
+ */
+STATIC void
+xfs_fs_dirty_inode(
+       struct inode    *inode)
+{
+       barrier();
+       XFS_I(inode)->i_update_core = 1;
+}
+
 /*
  * Attempt to flush the inode, this will actually fail
  * if the inode is pinned, but we dirty the inode again
@@ -1539,6 +1561,7 @@ xfs_fs_get_sb(
 static struct super_operations xfs_super_operations = {
        .alloc_inode            = xfs_fs_alloc_inode,
        .destroy_inode          = xfs_fs_destroy_inode,
+       .dirty_inode            = xfs_fs_dirty_inode,
        .write_inode            = xfs_fs_write_inode,
        .clear_inode            = xfs_fs_clear_inode,
        .put_super              = xfs_fs_put_super,

diff --git a/fs/xfs/xfs_dfrag.c b/fs/xfs/xfs_dfrag.c
index 7465f9ee125fcc2b80194043e9c141799a05ffde..ab89a7e94a0fd4b861114f656b99e5fcf0355a14 100644 (file)
--- a/fs/xfs/xfs_dfrag.c
+++ b/fs/xfs/xfs_dfrag.c
@@ -206,10 +206,10 @@ xfs_swap_extents(
         * process that the file was not changed out from
         * under it.
         */
-       if ((sbp->bs_ctime.tv_sec != ip->i_d.di_ctime.t_sec) ||
-           (sbp->bs_ctime.tv_nsec != ip->i_d.di_ctime.t_nsec) ||
-           (sbp->bs_mtime.tv_sec != ip->i_d.di_mtime.t_sec) ||
-           (sbp->bs_mtime.tv_nsec != ip->i_d.di_mtime.t_nsec)) {
+       if ((sbp->bs_ctime.tv_sec != VFS_I(ip)->i_ctime.tv_sec) ||
+           (sbp->bs_ctime.tv_nsec != VFS_I(ip)->i_ctime.tv_nsec) ||
+           (sbp->bs_mtime.tv_sec != VFS_I(ip)->i_mtime.tv_sec) ||
+           (sbp->bs_mtime.tv_nsec != VFS_I(ip)->i_mtime.tv_nsec)) {
                error = XFS_ERROR(EBUSY);
                goto out_unlock;
        }

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index c1dc7ef5a1d853ea935fd954ad0cf4863e701cd4..b92a4fa2a0a12eda161c2ff897bd820f5cbde1e8 100644 (file)
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3068,9 +3068,9 @@ xfs_iflush_int(
        SYNCHRONIZE();
 
        /*
-        * Make sure to get the latest atime from the Linux inode.
+        * Make sure to get the latest timestamps from the Linux inode.
         */
-       xfs_synchronize_atime(ip);
+       xfs_synchronize_times(ip);
 
        if (XFS_TEST_ERROR(be16_to_cpu(dip->di_magic) != XFS_DINODE_MAGIC,
                               mp, XFS_ERRTAG_IFLUSH_1, XFS_RANDOM_IFLUSH_1)) {

diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 0b38b9a869ece2136bf798a5329d2a38b8bf98ea..41555de1d1dbd4e6befcd64840712de5beff8235 100644 (file)
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -504,7 +504,7 @@ void                xfs_ichgtime(xfs_inode_t *, int);
 void           xfs_lock_inodes(xfs_inode_t **, int, uint);
 void           xfs_lock_two_inodes(xfs_inode_t *, xfs_inode_t *, uint);
 
-void           xfs_synchronize_atime(xfs_inode_t *);
+void           xfs_synchronize_times(xfs_inode_t *);
 void           xfs_mark_inode_dirty_sync(xfs_inode_t *);
 
 #if defined(XFS_INODE_TRACE)

diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 47d5b663c37e44a2b3528413edba37ec43eba880..9794b876d6ff65e34336e390a9becca46fa8e330 100644 (file)
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -231,6 +231,15 @@ xfs_inode_item_format(
        vecp++;
        nvecs        = 1;
 
+       /*
+        * Make sure the linux inode is dirty. We do this before
+        * clearing i_update_core as the VFS will call back into
+        * XFS here and set i_update_core, so we need to dirty the
+        * inode first so that the ordering of i_update_core and
+        * unlogged modifications still works as described below.
+        */
+       xfs_mark_inode_dirty_sync(ip);
+
        /*
         * Clear i_update_core if the timestamps (or any other
         * non-transactional modification) need flushing/logging
@@ -263,14 +272,9 @@ xfs_inode_item_format(
        }
 
        /*
-        * Make sure to get the latest atime from the Linux inode.
+        * Make sure to get the latest timestamps from the Linux inode.
         */
-       xfs_synchronize_atime(ip);
-
-       /*
-        * make sure the linux inode is dirty
-        */
-       xfs_mark_inode_dirty_sync(ip);
+       xfs_synchronize_times(ip);
 
        vecp->i_addr = (xfs_caddr_t)&ip->i_d;
        vecp->i_len  = sizeof(struct xfs_icdinode);

diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index b68f9107e26cce1e3eb7f4b08f272d493a9d8b06..62efab2f38392e845bf82fc8ba48825f3c016727 100644 (file)
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -59,6 +59,7 @@ xfs_bulkstat_one_iget(
 {
        xfs_icdinode_t  *dic;   /* dinode core info pointer */
        xfs_inode_t     *ip;            /* incore inode pointer */
+       struct inode    *inode;
        int             error;
 
        error = xfs_iget(mp, NULL, ino,
@@ -72,6 +73,7 @@ xfs_bulkstat_one_iget(
        ASSERT(ip->i_imap.im_blkno != 0);
 
        dic = &ip->i_d;
+       inode = VFS_I(ip);
 
        /* xfs_iget returns the following without needing
         * further change.
@@ -83,16 +85,19 @@ xfs_bulkstat_one_iget(
        buf->bs_uid = dic->di_uid;
        buf->bs_gid = dic->di_gid;
        buf->bs_size = dic->di_size;
+
        /*
-        * We are reading the atime from the Linux inode because the
-        * dinode might not be uptodate.
+        * We need to read the timestamps from the Linux inode because
+        * the VFS keeps writing directly into the inode structure instead
+        * of telling us about the updates.
         */
-       buf->bs_atime.tv_sec = VFS_I(ip)->i_atime.tv_sec;
-       buf->bs_atime.tv_nsec = VFS_I(ip)->i_atime.tv_nsec;
-       buf->bs_mtime.tv_sec = dic->di_mtime.t_sec;
-       buf->bs_mtime.tv_nsec = dic->di_mtime.t_nsec;
-       buf->bs_ctime.tv_sec = dic->di_ctime.t_sec;
-       buf->bs_ctime.tv_nsec = dic->di_ctime.t_nsec;
+       buf->bs_atime.tv_sec = inode->i_atime.tv_sec;
+       buf->bs_atime.tv_nsec = inode->i_atime.tv_nsec;
+       buf->bs_mtime.tv_sec = inode->i_mtime.tv_sec;
+       buf->bs_mtime.tv_nsec = inode->i_mtime.tv_nsec;
+       buf->bs_ctime.tv_sec = inode->i_ctime.tv_sec;
+       buf->bs_ctime.tv_nsec = inode->i_ctime.tv_nsec;
+
        buf->bs_xflags = xfs_ip2xflags(ip);
        buf->bs_extsize = dic->di_extsize << mp->m_sb.sb_blocklog;
        buf->bs_extents = dic->di_nextents;

diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index a434f287962d18588e0711110b8b758818b9ceee..b572f7e840e0b82a372225581d87c85c104dfec6 100644 (file)
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -2475,12 +2475,6 @@ xfs_reclaim(
 
        ASSERT(XFS_FORCED_SHUTDOWN(ip->i_mount) || ip->i_delayed_blks == 0);
 
-       /*
-        * Make sure the atime in the XFS inode is correct before freeing the
-        * Linux inode.
-        */
-       xfs_synchronize_atime(ip);
-
        /*
         * If we have nothing to flush with this inode then complete the
         * teardown now, otherwise break the link between the xfs inode and the