xfs: manage inode DONTCACHE status at irele time

Right now, there are statements scattered all over the online fsck
codebase about how we can't use XFS_IGET_DONTCACHE because of concerns
about scrub's unusual practice of releasing inodes with transactions
held.

However, iget is the wrong place to handle this -- the DONTCACHE state
doesn't matter at all until we try to *release* the inode, and here we
get things wrong in multiple ways:

First, if we /do/ have a transaction, we must NOT drop the inode,
because the inode could have dirty pages, dropping the inode will
trigger writeback, and writeback can trigger a nested transaction.

Second, if the inode already had an active reference and the DONTCACHE
flag set, the icache hit when scrub grabs another ref will not clear
DONTCACHE.  This is sort of by design, since DONTCACHE is now used to
initiate cache drops so that sysadmins can change a file's access mode
between pagecache and DAX.

Third, if we do actually have the last active reference to the inode, we
can set DONTCACHE to avoid polluting the cache.  This is the /one/ case
where we actually want that flag.

Create an xchk_irele helper to encode all that logic and switch the
online fsck code to use it.  Since this now means that nearly all
scrubbers use the same xfs_iget flags, we can wrap them too.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Dave Chinner <dchinner@redhat.com>

diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
index 813ded9..9af653a 100644 (file)
--- a/fs/xfs/scrub/common.c
+++ b/fs/xfs/scrub/common.c
@@ -718,6 +718,16 @@ xchk_checkpoint_log(
        return 0;
 }
 
+/* Verify that an inode is allocated ondisk, then return its cached inode. */
+int
+xchk_iget(
+       struct xfs_scrub        *sc,
+       xfs_ino_t               inum,
+       struct xfs_inode        **ipp)
+{
+       return xfs_iget(sc->mp, sc->tp, inum, XFS_IGET_UNTRUSTED, 0, ipp);
+}
+
 /*
  * Given an inode and the scrub control structure, grab either the
  * inode referenced in the control structure or the inode passed in.
@@ -743,8 +753,7 @@ xchk_get_inode(
        /* Look up the inode, see if the generation number matches. */
        if (xfs_internal_inum(mp, sc->sm->sm_ino))
                return -ENOENT;
-       error = xfs_iget(mp, NULL, sc->sm->sm_ino,
-                       XFS_IGET_UNTRUSTED | XFS_IGET_DONTCACHE, 0, &ip);
+       error = xchk_iget(sc, sc->sm->sm_ino, &ip);
        switch (error) {
        case -ENOENT:
                /* Inode doesn't exist, just bail out. */
@@ -768,7 +777,7 @@ xchk_get_inode(
                pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, sc->sm->sm_ino));
                if (pag) {
                        error = xfs_imap(pag, sc->tp, sc->sm->sm_ino, &imap,
-                                       XFS_IGET_UNTRUSTED | XFS_IGET_DONTCACHE);
+                                       XFS_IGET_UNTRUSTED);
                        xfs_perag_put(pag);
                        if (error)
                                return -ENOENT;
@@ -783,7 +792,7 @@ xchk_get_inode(
                return error;
        }
        if (VFS_I(ip)->i_generation != sc->sm->sm_gen) {
-               xfs_irele(ip);
+               xchk_irele(sc, ip);
                return -ENOENT;
        }
 
@@ -791,6 +800,41 @@ xchk_get_inode(
        return 0;
 }
 
+/* Release an inode, possibly dropping it in the process. */
+void
+xchk_irele(
+       struct xfs_scrub        *sc,
+       struct xfs_inode        *ip)
+{
+       if (current->journal_info != NULL) {
+               ASSERT(current->journal_info == sc->tp);
+
+               /*
+                * If we are in a transaction, we /cannot/ drop the inode
+                * ourselves, because the VFS will trigger writeback, which
+                * can require a transaction.  Clear DONTCACHE to force the
+                * inode to the LRU, where someone else can take care of
+                * dropping it.
+                *
+                * Note that when we grabbed our reference to the inode, it
+                * could have had an active ref and DONTCACHE set if a sysadmin
+                * is trying to coerce a change in file access mode.  icache
+                * hits do not clear DONTCACHE, so we must do it here.
+                */
+               spin_lock(&VFS_I(ip)->i_lock);
+               VFS_I(ip)->i_state &= ~I_DONTCACHE;
+               spin_unlock(&VFS_I(ip)->i_lock);
+       } else if (atomic_read(&VFS_I(ip)->i_count) == 1) {
+               /*
+                * If this is the last reference to the inode and the caller
+                * permits it, set DONTCACHE to avoid thrashing.
+                */
+               d_mark_dontcache(VFS_I(ip));
+       }
+
+       xfs_irele(ip);
+}
+
 /* Set us up to scrub a file's contents. */
 int
 xchk_setup_inode_contents(

diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
index 544f86f..7e9e8b7 100644 (file)
--- a/fs/xfs/scrub/common.h
+++ b/fs/xfs/scrub/common.h
@@ -137,6 +137,9 @@ int xchk_get_inode(struct xfs_scrub *sc);
 int xchk_setup_inode_contents(struct xfs_scrub *sc, unsigned int resblks);
 void xchk_buffer_recheck(struct xfs_scrub *sc, struct xfs_buf *bp);
 
+int xchk_iget(struct xfs_scrub *sc, xfs_ino_t inum, struct xfs_inode **ipp);
+void xchk_irele(struct xfs_scrub *sc, struct xfs_inode *ip);
+
 /*
  * Don't bother cross-referencing if we already found corruption or cross
  * referencing discrepancies.

diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
index 6404201..0b49178 100644 (file)
--- a/fs/xfs/scrub/dir.c
+++ b/fs/xfs/scrub/dir.c
@@ -117,21 +117,15 @@ xchk_dir_actor(
        }
 
        /*
-        * Grab the inode pointed to by the dirent.  We release the
-        * inode before we cancel the scrub transaction.  Since we're
-        * don't know a priori that releasing the inode won't trigger
-        * eofblocks cleanup (which allocates what would be a nested
-        * transaction), we can't use DONTCACHE here because DONTCACHE
-        * inodes can trigger immediate inactive cleanup of the inode.
-        * Use UNTRUSTED here to check the allocation status of the inode in
-        * the inode btrees.
+        * Grab the inode pointed to by the dirent.  We release the inode
+        * before we cancel the scrub transaction.
         *
         * If _iget returns -EINVAL or -ENOENT then the child inode number is
         * garbage and the directory is corrupt.  If the _iget returns
         * -EFSCORRUPTED or -EFSBADCRC then the child is corrupt which is a
         *  cross referencing error.  Any other error is an operational error.
         */
-       error = xfs_iget(mp, sc->tp, ino, XFS_IGET_UNTRUSTED, 0, &ip);
+       error = xchk_iget(sc, ino, &ip);
        if (error == -EINVAL || error == -ENOENT) {
                error = -EFSCORRUPTED;
                xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error);
@@ -141,7 +135,7 @@ xchk_dir_actor(
                goto out;
 
        xchk_dir_check_ftype(sc, offset, ip, name->type);
-       xfs_irele(ip);
+       xchk_irele(sc, ip);
 out:
        if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
                return -ECANCELED;

diff --git a/fs/xfs/scrub/parent.c b/fs/xfs/scrub/parent.c
index b6c8f6d..58d5dfb 100644 (file)
--- a/fs/xfs/scrub/parent.c
+++ b/fs/xfs/scrub/parent.c
@@ -127,20 +127,15 @@ xchk_parent_validate(
        expected_nlink = VFS_I(sc->ip)->i_nlink == 0 ? 0 : 1;
 
        /*
-        * Grab this parent inode.  We release the inode before we
-        * cancel the scrub transaction.  Since we're don't know a
-        * priori that releasing the inode won't trigger eofblocks
-        * cleanup (which allocates what would be a nested transaction)
-        * if the parent pointer erroneously points to a file, we
-        * can't use DONTCACHE here because DONTCACHE inodes can trigger
-        * immediate inactive cleanup of the inode.
+        * Grab the parent directory inode.  This must be released before we
+        * cancel the scrub transaction.
         *
         * If _iget returns -EINVAL or -ENOENT then the parent inode number is
         * garbage and the directory is corrupt.  If the _iget returns
         * -EFSCORRUPTED or -EFSBADCRC then the parent is corrupt which is a
         *  cross referencing error.  Any other error is an operational error.
         */
-       error = xfs_iget(mp, sc->tp, parent_ino, XFS_IGET_UNTRUSTED, 0, &dp);
+       error = xchk_iget(sc, parent_ino, &dp);
        if (error == -EINVAL || error == -ENOENT) {
                error = -EFSCORRUPTED;
                xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error);
@@ -176,7 +171,7 @@ xchk_parent_validate(
 out_unlock:
        xfs_iunlock(dp, lock_mode);
 out_rele:
-       xfs_irele(dp);
+       xchk_irele(sc, dp);
        return error;
 }
 

diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
index 787a909..03ec455 100644 (file)
--- a/fs/xfs/scrub/scrub.c
+++ b/fs/xfs/scrub/scrub.c
@@ -181,7 +181,7 @@ xchk_teardown(
                        xfs_iunlock(sc->ip, sc->ilock_flags);
                if (sc->ip != ip_in &&
                    !xfs_internal_inum(sc->mp, sc->ip->i_ino))
-                       xfs_irele(sc->ip);
+                       xchk_irele(sc, sc->ip);
                sc->ip = NULL;
        }
        if (sc->sm->sm_flags & XFS_SCRUB_IFLAG_REPAIR)