On Tue, Apr 02, 2024 at 10:18:31PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> While reviewing the next patch which fixes an ABBA deadlock between the
> AGI and a directory ILOCK, someone asked a question about why we're
> holding the AGI in the first place. The reason for that is to quiesce
> the inode structures for that AG while we do a repair.
>
> I then realized that the xrep_dinode_findmode invokes xchk_iscan_iter,
> which walks the inobts (and hence the AGIs) to find all the inodes.
> This itself is also an ABBA vector, since the damaged inode could be in
> AG 5, which we hold while we scan AG 0 for directories. 5 -> 0 is not
> allowed.
>
> To address this, modify the iscan to allow trylock of the AGI buffer
> using the flags argument to xfs_ialloc_read_agi that the previous patch
> added.
>
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> fs/xfs/scrub/inode_repair.c | 1 +
> fs/xfs/scrub/iscan.c | 36 +++++++++++++++++++++++++++++++++++-
> fs/xfs/scrub/iscan.h | 15 +++++++++++++++
> fs/xfs/scrub/trace.h | 10 ++++++++--
> 4 files changed, 59 insertions(+), 3 deletions(-)
>
>
> diff --git a/fs/xfs/scrub/inode_repair.c b/fs/xfs/scrub/inode_repair.c
> index eab380e95ef40..35da0193c919e 100644
> --- a/fs/xfs/scrub/inode_repair.c
> +++ b/fs/xfs/scrub/inode_repair.c
> @@ -356,6 +356,7 @@ xrep_dinode_find_mode(
> * so there's a real possibility that _iscan_iter can return EBUSY.
> */
> xchk_iscan_start(sc, 5000, 100, &ri->ftype_iscan);
> + xchk_iscan_set_agi_trylock(&ri->ftype_iscan);
> ri->ftype_iscan.skip_ino = sc->sm->sm_ino;
> ri->alleged_ftype = XFS_DIR3_FT_UNKNOWN;
> while ((error = xchk_iscan_iter(&ri->ftype_iscan, &dp)) == 1) {
> diff --git a/fs/xfs/scrub/iscan.c b/fs/xfs/scrub/iscan.c
> index 66ba0fbd059e0..736ce7c9de6a8 100644
> --- a/fs/xfs/scrub/iscan.c
> +++ b/fs/xfs/scrub/iscan.c
> @@ -243,6 +243,40 @@ xchk_iscan_finish(
> mutex_unlock(&iscan->lock);
> }
>
> +/*
> + * Grab the AGI to advance the inode scan. Returns 0 if *agi_bpp is now set,
> + * -ECANCELED if the live scan aborted, -EBUSY if the AGI could not be grabbed,
> + * or the usual negative errno.
> + */
> +STATIC int
> +xchk_iscan_read_agi(
> + struct xchk_iscan *iscan,
> + struct xfs_perag *pag,
> + struct xfs_buf **agi_bpp)
> +{
> + struct xfs_scrub *sc = iscan->sc;
> + unsigned long relax;
> + int ret;
> +
> + if (!xchk_iscan_agi_trylock(iscan))
> + return xfs_ialloc_read_agi(pag, sc->tp, 0, agi_bpp);
> +
> + relax = msecs_to_jiffies(iscan->iget_retry_delay);
> + do {
> + ret = xfs_ialloc_read_agi(pag, sc->tp, XFS_IALLOC_FLAG_TRYLOCK,
> + agi_bpp);
Why is this using xfs_ialloc_read_agi() and not xfs_read_agi()?
How do we get here without the perag AGI state not already
initialised?
i.e. if you just use xfs_read_agi(), all the code that has to plumb
flags into xfs_ialloc_read_agi() goes away and this change because a
lot less intrusive....
> + if (ret != -EAGAIN)
> + return ret;
> + if (!iscan->iget_timeout ||
> + time_is_before_jiffies(iscan->__iget_deadline))
> + return -EBUSY;
> +
> + trace_xchk_iscan_agi_retry_wait(iscan);
> + } while (!schedule_timeout_killable(relax) &&
> + !xchk_iscan_aborted(iscan));
> + return -ECANCELED;
> +}
> +
> /*
> * Advance ino to the next inode that the inobt thinks is allocated, being
> * careful to jump to the next AG if we've reached the right end of this AG's
> @@ -281,7 +315,7 @@ xchk_iscan_advance(
> if (!pag)
> return -ECANCELED;
>
> - ret = xfs_ialloc_read_agi(pag, sc->tp, 0, &agi_bp);
> + ret = xchk_iscan_read_agi(iscan, pag, &agi_bp);
> if (ret)
> goto out_pag;
>
> diff --git a/fs/xfs/scrub/iscan.h b/fs/xfs/scrub/iscan.h
> index 71f657552dfac..c9da8f7721f66 100644
> --- a/fs/xfs/scrub/iscan.h
> +++ b/fs/xfs/scrub/iscan.h
> @@ -59,6 +59,9 @@ struct xchk_iscan {
> /* Set if the scan has been aborted due to some event in the fs. */
> #define XCHK_ISCAN_OPSTATE_ABORTED (1)
>
> +/* Use trylock to acquire the AGI */
> +#define XCHK_ISCAN_OPSTATE_TRYLOCK_AGI (2)
> +
> static inline bool
> xchk_iscan_aborted(const struct xchk_iscan *iscan)
> {
> @@ -71,6 +74,18 @@ xchk_iscan_abort(struct xchk_iscan *iscan)
> set_bit(XCHK_ISCAN_OPSTATE_ABORTED, &iscan->__opstate);
> }
>
> +static inline bool
> +xchk_iscan_agi_trylock(const struct xchk_iscan *iscan)
> +{
> + return test_bit(XCHK_ISCAN_OPSTATE_TRYLOCK_AGI, &iscan->__opstate);
> +}
Function does not actually do any locking, but the name implies it
is actually doing a trylock operation. Perhaps
xchk_iscan_agi_needs_trylock()?
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
