| Message ID | 170086925774.2768713.17299783083709212096.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: prevent livelocks in xchk_iget | expand |
On Fri, Nov 24, 2023 at 03:46:54PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > When scrub is trying to iget an inode, ensure that it won't end up > deadlocked on a cycle in the inode btree by using an empty transaction > to store all the buffers. My only concern here is how I'm suppsed to know when to use the _safe version or not. Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Fri, Nov 24, 2023 at 08:57:57PM -0800, Christoph Hellwig wrote: > On Fri, Nov 24, 2023 at 03:46:54PM -0800, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > When scrub is trying to iget an inode, ensure that it won't end up > > deadlocked on a cycle in the inode btree by using an empty transaction > > to store all the buffers. > > My only concern here is how I'm suppsed to know when to use the _safe > version or not. For xchk_iget_safe, I'll amend the comment to read: /* * Safe version of (untrusted) xchk_iget that uses an empty transaction to * avoid deadlocking on loops in the inobt. This should only be used in a * scrub or repair setup routine, and only prior to grabbing a transaction. */ and add a comment for xchk_iget that reads: /* * Grab the inode at @inum. The caller must have created a scrub transaction * so that we can confirm the inumber by walking the inobt and not deadlock on * a loop in the inobt. */ > Otherwise looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> Thanks! --D
diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c index de24532fe0830..23944fcc1a6ca 100644 --- a/fs/xfs/scrub/common.c +++ b/fs/xfs/scrub/common.c @@ -733,6 +733,8 @@ xchk_iget( xfs_ino_t inum, struct xfs_inode **ipp) { + ASSERT(sc->tp != NULL); + return xfs_iget(sc->mp, sc->tp, inum, XFS_IGET_UNTRUSTED, 0, ipp); } @@ -882,8 +884,8 @@ xchk_iget_for_scrubbing( if (!xfs_verify_ino(sc->mp, sc->sm->sm_ino)) return -ENOENT; - /* Try a regular untrusted iget. */ - error = xchk_iget(sc, sc->sm->sm_ino, &ip); + /* Try a safe untrusted iget. */ + error = xchk_iget_safe(sc, sc->sm->sm_ino, &ip); if (!error) return xchk_install_handle_inode(sc, ip); if (error == -ENOENT) diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h index cabdc0e16838c..a39dbe6be1e59 100644 --- a/fs/xfs/scrub/common.h +++ b/fs/xfs/scrub/common.h @@ -157,6 +157,25 @@ int xchk_iget_agi(struct xfs_scrub *sc, xfs_ino_t inum, void xchk_irele(struct xfs_scrub *sc, struct xfs_inode *ip); int xchk_install_handle_inode(struct xfs_scrub *sc, struct xfs_inode *ip); +/* + * Safe version of (untrusted) xchk_iget that uses an empty transaction to + * avoid deadlocking on loops in the inobt. + */ +static inline int +xchk_iget_safe(struct xfs_scrub *sc, xfs_ino_t inum, struct xfs_inode **ipp) +{ + int error; + + ASSERT(sc->tp == NULL); + + error = xchk_trans_alloc(sc, 0); + if (error) + return error; + error = xchk_iget(sc, inum, ipp); + xchk_trans_cancel(sc); + return error; +} + /* * Don't bother cross-referencing if we already found corruption or cross * referencing discrepancies. diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c index 889f556bc98f6..b7a93380a1ab0 100644 --- a/fs/xfs/scrub/inode.c +++ b/fs/xfs/scrub/inode.c @@ -95,8 +95,8 @@ xchk_setup_inode( if (!xfs_verify_ino(sc->mp, sc->sm->sm_ino)) return -ENOENT; - /* Try a regular untrusted iget. */ - error = xchk_iget(sc, sc->sm->sm_ino, &ip); + /* Try a safe untrusted iget. */ + error = xchk_iget_safe(sc, sc->sm->sm_ino, &ip); if (!error) return xchk_install_handle_iscrub(sc, ip); if (error == -ENOENT)