From cbab28f4c0719c956fa7b83613a3591c361126c7 Mon Sep 17 00:00:00 2001 From: "Darrick J. Wong" Date: Tue, 11 Apr 2023 19:00:19 -0700 Subject: [PATCH 1/3] xfs: remove xchk_parent_count_parent_dentries This helper is now trivial, so get rid of it. Signed-off-by: Darrick J. Wong Reviewed-by: Dave Chinner --- fs/xfs/scrub/parent.c | 42 +++++++++++++----------------------------- 1 file changed, 13 insertions(+), 29 deletions(-) diff --git a/fs/xfs/scrub/parent.c b/fs/xfs/scrub/parent.c index af351c4ee6ec..bbf6492c8e8e 100644 --- a/fs/xfs/scrub/parent.c +++ b/fs/xfs/scrub/parent.c @@ -63,30 +63,6 @@ xchk_parent_actor( return 0; } -/* Count the number of dentries in the parent dir that point to this inode. */ -STATIC int -xchk_parent_count_parent_dentries( - struct xfs_scrub *sc, - struct xfs_inode *parent, - xfs_nlink_t *nlink) -{ - struct xchk_parent_ctx spc = { - .sc = sc, - .nlink = 0, - }; - uint lock_mode; - int error = 0; - - lock_mode = xfs_ilock_data_map_shared(parent); - error = xchk_dir_walk(sc, parent, xchk_parent_actor, &spc); - xfs_iunlock(parent, lock_mode); - if (error) - return error; - - *nlink = spc.nlink; - return error; -} - /* * Given the inode number of the alleged parent of the inode being * scrubbed, try to validate that the parent has exactly one directory @@ -98,10 +74,14 @@ xchk_parent_validate( xfs_ino_t dnum, bool *try_again) { + struct xchk_parent_ctx spc = { + .sc = sc, + .nlink = 0, + }; struct xfs_mount *mp = sc->mp; struct xfs_inode *dp = NULL; xfs_nlink_t expected_nlink; - xfs_nlink_t nlink; + uint lock_mode; int error = 0; *try_again = false; @@ -156,11 +136,13 @@ xchk_parent_validate( * the child inodes. */ if (xfs_ilock_nowait(dp, XFS_IOLOCK_SHARED)) { - error = xchk_parent_count_parent_dentries(sc, dp, &nlink); + 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 (nlink != expected_nlink) + if (spc.nlink != expected_nlink) xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, 0); goto out_unlock; } @@ -178,7 +160,9 @@ xchk_parent_validate( goto out_rele; /* Go looking for our dentry. */ - error = xchk_parent_count_parent_dentries(sc, dp, &nlink); + 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; @@ -213,7 +197,7 @@ xchk_parent_validate( * '..' didn't change, so check that there was only one entry * for us in the parent. */ - if (nlink != expected_nlink) + if (spc.nlink != expected_nlink) xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, 0); return error; From b049962c0f6eb6fb17e8294721f948285a44a672 Mon Sep 17 00:00:00 2001 From: "Darrick J. Wong" Date: Tue, 11 Apr 2023 19:00:19 -0700 Subject: [PATCH 2/3] 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 Reviewed-by: Dave Chinner --- fs/xfs/scrub/parent.c | 102 ++++++++++-------------------------------- 1 file changed, 24 insertions(+), 78 deletions(-) diff --git a/fs/xfs/scrub/parent.c b/fs/xfs/scrub/parent.c index bbf6492c8e8e..50dc423041ee 100644 --- 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; + if (!xfs_ilock_nowait(dp, XFS_IOLOCK_SHARED)) { + *try_again = true; + goto out_rele; } - /* - * 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) - 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) - xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, 0); - goto out; - } - do { - error = xchk_parent_validate(sc, dnum, &try_again); + /* 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; + } + + error = xchk_parent_validate(sc, parent_ino, &try_again); if (error) goto out; } while (try_again && ++tries < 20); From 0916056eba4fd816f8042a3960597c316ea10256 Mon Sep 17 00:00:00 2001 From: "Darrick J. Wong" Date: Tue, 11 Apr 2023 19:00:20 -0700 Subject: [PATCH 3/3] xfs: fix parent pointer scrub racing with subdirectory reparenting Jan Kara pointed out that rename() doesn't lock a subdirectory that is being moved from one parent to another, even though the move requires an update to the subdirectory's dotdot entry. This means that it's *not* sufficient to hold a directory's IOLOCK to stabilize the dotdot entry. We must hold the ILOCK of both the child and the alleged parent, and there's no use in holding the parent's IOLOCK. With that in mind, we can get rid of all the messy code that tries to grab the parent's IOLOCK, which means we don't need to let go of the ILOCK of the directory whose parent we are checking. We still have to use nonblocking mode to take the ILOCK of the alleged parent, so the revalidation loop has to stay. However, we can remove the retry counter, since threads aren't supposed to hold the ILOCK for long periods of time. Remove the inverted ilock helper from the common code since nobody uses it. Remove the entire source of -EDEADLOCK-based "retry harder" scrub executions. Link: https://lore.kernel.org/linux-xfs/20230117123735.un7wbamlbdihninm@quack3/ Signed-off-by: Darrick J. Wong Reviewed-by: Dave Chinner --- fs/xfs/scrub/common.c | 22 -------- fs/xfs/scrub/common.h | 1 - fs/xfs/scrub/parent.c | 118 ++++++++++++++++++++---------------------- 3 files changed, 57 insertions(+), 84 deletions(-) diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c index dcfe66044d4a..813ded91661b 100644 --- a/fs/xfs/scrub/common.c +++ b/fs/xfs/scrub/common.c @@ -962,28 +962,6 @@ xchk_metadata_inode_forks( return 0; } -/* - * Try to lock an inode in violation of the usual locking order rules. For - * example, trying to get the IOLOCK while in transaction context, or just - * plain breaking AG-order or inode-order inode locking rules. Either way, - * the only way to avoid an ABBA deadlock is to use trylock and back off if - * we can't. - */ -int -xchk_ilock_inverted( - struct xfs_inode *ip, - uint lock_mode) -{ - int i; - - for (i = 0; i < 20; i++) { - if (xfs_ilock_nowait(ip, lock_mode)) - return 0; - delay(1); - } - return -EDEADLOCK; -} - /* Pause background reaping of resources. */ void xchk_stop_reaping( diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h index 83b1a392930a..544f86ff8d1d 100644 --- a/fs/xfs/scrub/common.h +++ b/fs/xfs/scrub/common.h @@ -148,7 +148,6 @@ static inline bool xchk_skip_xref(struct xfs_scrub_metadata *sm) } int xchk_metadata_inode_forks(struct xfs_scrub *sc); -int xchk_ilock_inverted(struct xfs_inode *ip, uint lock_mode); void xchk_stop_reaping(struct xfs_scrub *sc); void xchk_start_reaping(struct xfs_scrub *sc); diff --git a/fs/xfs/scrub/parent.c b/fs/xfs/scrub/parent.c index 50dc423041ee..b6c8f6dccc8f 100644 --- a/fs/xfs/scrub/parent.c +++ b/fs/xfs/scrub/parent.c @@ -64,15 +64,37 @@ xchk_parent_actor( } /* - * Given the inode number of the alleged parent of the inode being - * scrubbed, try to validate that the parent has exactly one directory - * entry pointing back to the inode being scrubbed. + * Try to lock a parent directory for checking dirents. Returns the inode + * flags for the locks we now hold, or zero if we failed. + */ +STATIC unsigned int +xchk_parent_ilock_dir( + struct xfs_inode *dp) +{ + if (!xfs_ilock_nowait(dp, XFS_ILOCK_SHARED)) + return 0; + + if (!xfs_need_iread_extents(&dp->i_df)) + return XFS_ILOCK_SHARED; + + xfs_iunlock(dp, XFS_ILOCK_SHARED); + + if (!xfs_ilock_nowait(dp, XFS_ILOCK_EXCL)) + return 0; + + return XFS_ILOCK_EXCL; +} + +/* + * Given the inode number of the alleged parent of the inode being scrubbed, + * try to validate that the parent has exactly one directory entry pointing + * back to the inode being scrubbed. Returns -EAGAIN if we need to revalidate + * the dotdot entry. */ STATIC int xchk_parent_validate( struct xfs_scrub *sc, - xfs_ino_t parent_ino, - bool *try_again) + xfs_ino_t parent_ino) { struct xchk_parent_ctx spc = { .sc = sc, @@ -81,23 +103,21 @@ xchk_parent_validate( struct xfs_mount *mp = sc->mp; struct xfs_inode *dp = NULL; xfs_nlink_t expected_nlink; - uint lock_mode; + unsigned int lock_mode; int error = 0; - *try_again = false; - /* 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; + return 0; } /* '..' must not point to ourselves. */ if (sc->ip->i_ino == parent_ino) { xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, 0); - goto out; + return 0; } /* @@ -124,41 +144,39 @@ xchk_parent_validate( if (error == -EINVAL || error == -ENOENT) { error = -EFSCORRUPTED; xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error); - goto out; + return error; } if (!xchk_fblock_xref_process_error(sc, XFS_DATA_FORK, 0, &error)) - goto out; + return error; if (dp == sc->ip || !S_ISDIR(VFS_I(dp)->i_mode)) { xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, 0); goto out_rele; } - /* - * We prefer to keep the inode locked while we lock and search - * its alleged parent for a forward reference. If we can grab - * the iolock, validate the pointers and we're done. We must - * use nowait here to avoid an ABBA deadlock on the parent and - * the child inodes. - */ - if (!xfs_ilock_nowait(dp, XFS_IOLOCK_SHARED)) { - *try_again = true; + lock_mode = xchk_parent_ilock_dir(dp); + if (!lock_mode) { + xfs_iunlock(sc->ip, XFS_ILOCK_EXCL); + xfs_ilock(sc->ip, XFS_ILOCK_EXCL); + error = -EAGAIN; goto out_rele; } - lock_mode = xfs_ilock_data_map_shared(dp); + /* Look for a directory entry in the parent pointing to the child. */ 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; + /* + * Ensure that the parent has as many links to the child as the child + * thinks it has to the parent. + */ if (spc.nlink != expected_nlink) xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, 0); out_unlock: - xfs_iunlock(dp, XFS_IOLOCK_SHARED); + xfs_iunlock(dp, lock_mode); out_rele: xfs_irele(dp); -out: return error; } @@ -169,8 +187,6 @@ xchk_parent( { struct xfs_mount *mp = sc->mp; xfs_ino_t parent_ino; - bool try_again; - int tries = 0; int error = 0; /* @@ -183,49 +199,29 @@ xchk_parent( /* We're not a special inode, are we? */ if (!xfs_verify_dir_ino(mp, sc->ip->i_ino)) { xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, 0); - goto out; + return 0; } - /* - * The VFS grabs a read or write lock via i_rwsem before it reads - * or writes to a directory. If we've gotten this far we've - * already obtained IOLOCK_EXCL, which (since 4.10) is the same as - * getting a write lock on i_rwsem. Therefore, it is safe for us - * to drop the ILOCK here in order to do directory lookups. - */ - sc->ilock_flags &= ~(XFS_ILOCK_EXCL | XFS_MMAPLOCK_EXCL); - xfs_iunlock(sc->ip, XFS_ILOCK_EXCL | XFS_MMAPLOCK_EXCL); - do { + if (xchk_should_terminate(sc, &error)) + break; + /* Look up '..' */ - error = xfs_dir_lookup(sc->tp, sc->ip, &xfs_name_dotdot, - &parent_ino, NULL); + error = xchk_dir_lookup(sc, sc->ip, &xfs_name_dotdot, + &parent_ino); if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error)) - goto out; + return error; if (!xfs_verify_dir_ino(mp, parent_ino)) { xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, 0); - goto out; + return 0; } - error = xchk_parent_validate(sc, parent_ino, &try_again); - if (error) - goto out; - } while (try_again && ++tries < 20); + /* + * Check that the dotdot entry points to a parent directory + * containing a dirent pointing to this subdirectory. + */ + error = xchk_parent_validate(sc, parent_ino); + } while (error == -EAGAIN); - /* - * We gave it our best shot but failed, so mark this scrub - * incomplete. Userspace can decide if it wants to try again. - */ - if (try_again && tries == 20) - xchk_set_incomplete(sc); -out: - /* - * If we failed to lock the parent inode even after a retry, just mark - * this scrub incomplete and return. - */ - if ((sc->flags & XCHK_TRY_HARDER) && error == -EDEADLOCK) { - error = 0; - xchk_set_incomplete(sc); - } return error; }