xfs: simplify xchk_parent_validate

This function is unnecessarily long because it contains code to
revalidate a dotdot entry after cycling locks to try to confirm a
subdirectory parent pointer.  Shorten the codebase by making the
parent's lookup call do double duty as the revalidation code.

This weakeans the efficacy of this scrub function temporarily, but the
next patch will resolve this as part of fixing an unhandled race that is
the result of the VFS rename locking model not working the way Darrick
thought it did.

Rename this stupid 'dnum' variable too.

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

diff --git a/fs/xfs/scrub/parent.c b/fs/xfs/scrub/parent.c
index bbf6492..50dc423 100644 (file)
--- a/fs/xfs/scrub/parent.c
+++ b/fs/xfs/scrub/parent.c
@@ -71,7 +71,7 @@ xchk_parent_actor(
 STATIC int
 xchk_parent_validate(
        struct xfs_scrub        *sc,
-       xfs_ino_t               dnum,
+       xfs_ino_t               parent_ino,
        bool                    *try_again)
 {
        struct xchk_parent_ctx  spc = {
@@ -86,11 +86,16 @@ xchk_parent_validate(
 
        *try_again = false;
 
-       if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
+       /* Is this the root dir?  Then '..' must point to itself. */
+       if (sc->ip == mp->m_rootip) {
+               if (sc->ip->i_ino != mp->m_sb.sb_rootino ||
+                   sc->ip->i_ino != parent_ino)
+                       xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, 0);
                goto out;
+       }
 
        /* '..' must not point to ourselves. */
-       if (sc->ip->i_ino == dnum) {
+       if (sc->ip->i_ino == parent_ino) {
                xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, 0);
                goto out;
        }
@@ -115,7 +120,7 @@ xchk_parent_validate(
         * -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, dnum, XFS_IGET_UNTRUSTED, 0, &dp);
+       error = xfs_iget(mp, sc->tp, parent_ino, XFS_IGET_UNTRUSTED, 0, &dp);
        if (error == -EINVAL || error == -ENOENT) {
                error = -EFSCORRUPTED;
                xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error);
@@ -135,71 +140,19 @@ xchk_parent_validate(
         * use nowait here to avoid an ABBA deadlock on the parent and
         * the child inodes.
         */
-       if (xfs_ilock_nowait(dp, XFS_IOLOCK_SHARED)) {
-               lock_mode = xfs_ilock_data_map_shared(dp);
-               error = xchk_dir_walk(sc, dp, xchk_parent_actor, &spc);
-               xfs_iunlock(dp, lock_mode);
-               if (!xchk_fblock_xref_process_error(sc, XFS_DATA_FORK, 0,
-                               &error))
-                       goto out_unlock;
-               if (spc.nlink != expected_nlink)
-                       xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, 0);
-               goto out_unlock;
-       }
-
-       /*
-        * The game changes if we get here.  We failed to lock the parent,
-        * so we're going to try to verify both pointers while only holding
-        * one lock so as to avoid deadlocking with something that's actually
-        * trying to traverse down the directory tree.
-        */
-       xfs_iunlock(sc->ip, sc->ilock_flags);
-       sc->ilock_flags = 0;
-       error = xchk_ilock_inverted(dp, XFS_IOLOCK_SHARED);
-       if (error)
+       if (!xfs_ilock_nowait(dp, XFS_IOLOCK_SHARED)) {
+               *try_again = true;
                goto out_rele;
+       }
 
-       /* Go looking for our dentry. */
        lock_mode = xfs_ilock_data_map_shared(dp);
        error = xchk_dir_walk(sc, dp, xchk_parent_actor, &spc);
        xfs_iunlock(dp, lock_mode);
        if (!xchk_fblock_xref_process_error(sc, XFS_DATA_FORK, 0, &error))
                goto out_unlock;
 
-       /* Drop the parent lock, relock this inode. */
-       xfs_iunlock(dp, XFS_IOLOCK_SHARED);
-       error = xchk_ilock_inverted(sc->ip, XFS_IOLOCK_EXCL);
-       if (error)
-               goto out_rele;
-       sc->ilock_flags = XFS_IOLOCK_EXCL;
-
-       /*
-        * If we're an unlinked directory, the parent /won't/ have a link
-        * to us.  Otherwise, it should have one link.  We have to re-set
-        * it here because we dropped the lock on sc->ip.
-        */
-       expected_nlink = VFS_I(sc->ip)->i_nlink == 0 ? 0 : 1;
-
-       /* Look up '..' to see if the inode changed. */
-       error = xfs_dir_lookup(sc->tp, sc->ip, &xfs_name_dotdot, &dnum, NULL);
-       if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error))
-               goto out_rele;
-
-       /* Drat, parent changed.  Try again! */
-       if (dnum != dp->i_ino) {
-               xfs_irele(dp);
-               *try_again = true;
-               return 0;
-       }
-       xfs_irele(dp);
-
-       /*
-        * '..' didn't change, so check that there was only one entry
-        * for us in the parent.
-        */
        if (spc.nlink != expected_nlink)
                xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, 0);
-       return error;
 
 out_unlock:
        xfs_iunlock(dp, XFS_IOLOCK_SHARED);
@@ -215,7 +168,7 @@ xchk_parent(
        struct xfs_scrub        *sc)
 {
        struct xfs_mount        *mp = sc->mp;
-       xfs_ino_t               dnum;
+       xfs_ino_t               parent_ino;
        bool                    try_again;
        int                     tries = 0;
        int                     error = 0;
@@ -243,25 +196,18 @@ xchk_parent(
        sc->ilock_flags &= ~(XFS_ILOCK_EXCL | XFS_MMAPLOCK_EXCL);
        xfs_iunlock(sc->ip, XFS_ILOCK_EXCL | XFS_MMAPLOCK_EXCL);
 
-       /* Look up '..' */
-       error = xfs_dir_lookup(sc->tp, sc->ip, &xfs_name_dotdot, &dnum, NULL);
-       if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error))
-               goto out;
-       if (!xfs_verify_dir_ino(mp, dnum)) {
-               xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, 0);
-               goto out;
-       }
-
-       /* Is this the root dir?  Then '..' must point to itself. */
-       if (sc->ip == mp->m_rootip) {
-               if (sc->ip->i_ino != mp->m_sb.sb_rootino ||
-                   sc->ip->i_ino != dnum)
+       do {
+               /* Look up '..' */
+               error = xfs_dir_lookup(sc->tp, sc->ip, &xfs_name_dotdot,
+                               &parent_ino, NULL);
+               if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error))
+                       goto out;
+               if (!xfs_verify_dir_ino(mp, parent_ino)) {
                        xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, 0);
-               goto out;
-       }
+                       goto out;
+               }
 
-       do {
-               error = xchk_parent_validate(sc, dnum, &try_again);
+               error = xchk_parent_validate(sc, parent_ino, &try_again);
                if (error)
                        goto out;
        } while (try_again && ++tries < 20);