From: Brian Foster <bfoster@redhat.com>
To: linux-xfs@vger.kernel.org
Subject: [PATCH 3/3] xfs: terminate perag iteration reliably on end agno
Date: Thu, 7 Oct 2021 08:50:53 -0400 [thread overview]
Message-ID: <20211007125053.1096868-4-bfoster@redhat.com> (raw)
In-Reply-To: <20211007125053.1096868-1-bfoster@redhat.com>
The for_each_perag*() set of macros are hacky in that some (i.e. those
based on sb_agcount) rely on the assumption that perag iteration
terminates naturally with a NULL perag at the specified end agno. Others
allow for the final AG to have a valid perag and require the calling
function to clean up any potential leftover xfs_perag reference on
termination of the loop.
Aside from providing a subtly inconsistent interface, the former variant
is racy with a potential growfs in progress because growfs can create
discoverable post-eofs perags before the final superblock update that
completes the grow operation and increases sb_agcount. This leads to
unexpected assert failures (reproduced by xfs/104) such as the following
in the superblock buffer write verifier path:
XFS: Assertion failed: agno < mp->m_sb.sb_agcount, file: fs/xfs/libxfs/xfs_types.c, line: 22
This occurs because the perag loop in xfs_icount_range() finds and
attempts to process a perag struct where pag_agno == sb_agcount.
The following assert failure occasionally triggers during the xfs_perag
free path on unmount, presumably because one of the many
for_each_perag() loops in the code that is expected to terminate with a
NULL pag raced with a growfs and actually terminated with a non-NULL
reference to post-eofs (at the time) perag.
XFS: Assertion failed: atomic_read(&pag->pag_ref) == 0, file: fs/xfs/libxfs/xfs_ag.c, line: 195
Rework the lower level perag iteration logic to explicitly terminate
on the specified end agno, not implicitly rely on pag == NULL as a
termination clause and thus avoid these problems. As of this change,
the remaining post-loop xfs_perag_put() checks that exist purely to
cover the natural termination case (i.e., not mid-loop breaks) are
spurious (yet harmless) and can be removed.
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/libxfs/xfs_ag.h | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
index d05c9217c3af..edcdd4fbc225 100644
--- a/fs/xfs/libxfs/xfs_ag.h
+++ b/fs/xfs/libxfs/xfs_ag.h
@@ -116,34 +116,30 @@ void xfs_perag_put(struct xfs_perag *pag);
/*
* Perag iteration APIs
- *
- * XXX: for_each_perag_range() usage really needs an iterator to clean up when
- * we terminate at end_agno because we may have taken a reference to the perag
- * beyond end_agno. Right now callers have to be careful to catch and clean that
- * up themselves. This is not necessary for the callers of for_each_perag() and
- * for_each_perag_from() because they terminate at sb_agcount where there are
- * no perag structures in tree beyond end_agno.
*/
static inline
struct xfs_perag *xfs_perag_next(
struct xfs_perag *pag,
- xfs_agnumber_t *agno)
+ xfs_agnumber_t *agno,
+ xfs_agnumber_t end_agno)
{
struct xfs_mount *mp = pag->pag_mount;
*agno = pag->pag_agno + 1;
xfs_perag_put(pag);
- pag = xfs_perag_get(mp, *agno);
+ pag = NULL;
+ if (*agno <= end_agno)
+ pag = xfs_perag_get(mp, *agno);
return pag;
}
#define for_each_perag_range(mp, agno, end_agno, pag) \
for ((pag) = xfs_perag_get((mp), (agno)); \
- (pag) != NULL && (agno) <= (end_agno); \
- (pag) = xfs_perag_next((pag), &(agno)))
+ (pag) != NULL; \
+ (pag) = xfs_perag_next((pag), &(agno), (end_agno)))
#define for_each_perag_from(mp, agno, pag) \
- for_each_perag_range((mp), (agno), (mp)->m_sb.sb_agcount, (pag))
+ for_each_perag_range((mp), (agno), (mp)->m_sb.sb_agcount - 1, (pag))
#define for_each_perag(mp, agno, pag) \
--
2.31.1
next prev parent reply other threads:[~2021-10-07 12:51 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-07 12:50 [PATCH 0/3] xfs: fix perag iteration raciness Brian Foster
2021-10-07 12:50 ` [PATCH 1/3] xfs: fold perag loop iteration logic into helper function Brian Foster
2021-10-07 12:50 ` [PATCH 2/3] xfs: rename the next_agno perag iteration variable Brian Foster
2021-10-07 12:50 ` Brian Foster [this message]
2021-10-07 23:02 ` [PATCH 3/3] xfs: terminate perag iteration reliably on end agno Dave Chinner
2021-10-08 14:45 ` Brian Foster
2021-10-08 21:20 ` Dave Chinner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211007125053.1096868-4-bfoster@redhat.com \
--to=bfoster@redhat.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).