linux-xfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] xfs: fix perag iteration raciness
@ 2021-10-07 12:50 Brian Foster
  2021-10-07 12:50 ` [PATCH 1/3] xfs: fold perag loop iteration logic into helper function Brian Foster
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Brian Foster @ 2021-10-07 12:50 UTC (permalink / raw)
  To: linux-xfs

Hi all,

This adds an iteration function to the perag iteration macros to tweak
the logic and avoid some raciness with growfs when relying on the
presence of xfs_perag structures. This manifests as a couple assert
failures reproduced by xfs/104 (which only seem to reproduce on ppc64le
at the moment; I haven't really dug into why).

This probably could use a couple more patches tacked on: one to fix up
the tag based lookup macro and another to audit the existing callers and
remove any spurious xfs_perag_put() calls. I don't think the former is
currently an issue an practice, but we should probably eliminate the
flaw regardless. I'm just sending the initial fixup on its own as a
first pass..

Brian

Brian Foster (3):
  xfs: fold perag loop iteration logic into helper function
  xfs: rename the next_agno perag iteration variable
  xfs: terminate perag iteration reliably on end agno

 fs/xfs/libxfs/xfs_ag.h | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/3] xfs: fold perag loop iteration logic into helper function
  2021-10-07 12:50 [PATCH 0/3] xfs: fix perag iteration raciness Brian Foster
@ 2021-10-07 12:50 ` 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 ` [PATCH 3/3] xfs: terminate perag iteration reliably on end agno Brian Foster
  2 siblings, 0 replies; 7+ messages in thread
From: Brian Foster @ 2021-10-07 12:50 UTC (permalink / raw)
  To: linux-xfs

Fold the loop iteration logic into a helper in preparation for
further fixups. No functional change in this patch.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_ag.h | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
index 4c6f9045baca..d05a04f3c985 100644
--- a/fs/xfs/libxfs/xfs_ag.h
+++ b/fs/xfs/libxfs/xfs_ag.h
@@ -124,12 +124,23 @@ void xfs_perag_put(struct xfs_perag *pag);
  * 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		*next_agno)
+{
+	struct xfs_mount	*mp = pag->pag_mount;
+
+	*next_agno = pag->pag_agno + 1;
+	xfs_perag_put(pag);
+	pag = xfs_perag_get(mp, *next_agno);
+	return pag;
+}
+
 #define for_each_perag_range(mp, next_agno, end_agno, pag) \
 	for ((pag) = xfs_perag_get((mp), (next_agno)); \
 		(pag) != NULL && (next_agno) <= (end_agno); \
-		(next_agno) = (pag)->pag_agno + 1, \
-		xfs_perag_put(pag), \
-		(pag) = xfs_perag_get((mp), (next_agno)))
+		(pag) = xfs_perag_next((pag), &(next_agno)))
 
 #define for_each_perag_from(mp, next_agno, pag) \
 	for_each_perag_range((mp), (next_agno), (mp)->m_sb.sb_agcount, (pag))
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/3] xfs: rename the next_agno perag iteration variable
  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 ` Brian Foster
  2021-10-07 12:50 ` [PATCH 3/3] xfs: terminate perag iteration reliably on end agno Brian Foster
  2 siblings, 0 replies; 7+ messages in thread
From: Brian Foster @ 2021-10-07 12:50 UTC (permalink / raw)
  To: linux-xfs

Rename the next_agno variable to be consistent across the several
iteration macros and shorten line length.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_ag.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
index d05a04f3c985..d05c9217c3af 100644
--- a/fs/xfs/libxfs/xfs_ag.h
+++ b/fs/xfs/libxfs/xfs_ag.h
@@ -127,23 +127,23 @@ void xfs_perag_put(struct xfs_perag *pag);
 static inline
 struct xfs_perag *xfs_perag_next(
 	struct xfs_perag	*pag,
-	xfs_agnumber_t		*next_agno)
+	xfs_agnumber_t		*agno)
 {
 	struct xfs_mount	*mp = pag->pag_mount;
 
-	*next_agno = pag->pag_agno + 1;
+	*agno = pag->pag_agno + 1;
 	xfs_perag_put(pag);
-	pag = xfs_perag_get(mp, *next_agno);
+	pag = xfs_perag_get(mp, *agno);
 	return pag;
 }
 
-#define for_each_perag_range(mp, next_agno, end_agno, pag) \
-	for ((pag) = xfs_perag_get((mp), (next_agno)); \
-		(pag) != NULL && (next_agno) <= (end_agno); \
-		(pag) = xfs_perag_next((pag), &(next_agno)))
+#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)))
 
-#define for_each_perag_from(mp, next_agno, pag) \
-	for_each_perag_range((mp), (next_agno), (mp)->m_sb.sb_agcount, (pag))
+#define for_each_perag_from(mp, agno, pag) \
+	for_each_perag_range((mp), (agno), (mp)->m_sb.sb_agcount, (pag))
 
 
 #define for_each_perag(mp, agno, pag) \
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/3] xfs: terminate perag iteration reliably on end agno
  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
  2021-10-07 23:02   ` Dave Chinner
  2 siblings, 1 reply; 7+ messages in thread
From: Brian Foster @ 2021-10-07 12:50 UTC (permalink / raw)
  To: linux-xfs

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/3] xfs: terminate perag iteration reliably on end agno
  2021-10-07 12:50 ` [PATCH 3/3] xfs: terminate perag iteration reliably on end agno Brian Foster
@ 2021-10-07 23:02   ` Dave Chinner
  2021-10-08 14:45     ` Brian Foster
  0 siblings, 1 reply; 7+ messages in thread
From: Dave Chinner @ 2021-10-07 23:02 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs

On Thu, Oct 07, 2021 at 08:50:53AM -0400, Brian Foster wrote:
> 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

Yeah, that's a bad assert. It's not valid in the context of grow or
shrink or any of the future advanced per-ag management things we
want to do.

I'm ok with the change being proposed as a expedient bug fix, but
I'll note that the approach taken to fix it is not compatible with
future plans for managing shrink and perag operations. I'll comment
on the patch first, then the rest of the email is commentary about
how xfs_perag_get() is intended to be used...

> 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.

We still really need an iterator for the range iterations so that we
can have a consistent set of behaviours for all iterations and
don't need a special case just for the "mid walk break" where the
code keeps the active reference to the perag for itself...

>   */
>  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;

	*agno = pag->pag_agno + 1;
	xfs_perag_put(pag);
	if (*agno > end_agno)
		return NULL;
	return xfs_perag_get(mp, *agno);

>  }
>  
>  #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))

Isn't this one line the entire bug fix right here? i.e. the
factoring is largely unnecessary, the grow race bug is fixed by just
this one-liner?

----

Anyway, commentary now...

> This occurs because the perag loop in xfs_icount_range() finds and
> attempts to process a perag struct where pag_agno == sb_agcount.

That's not actually a traversal bug. The whole design of the per-ag
infrastructure is that a search for an agno beyond the current
limits of the fs should return NULL, and we use that all over the
place. e.g xfs_reclaim_inodes_count():

       while ((pag = xfs_perag_get_tag(mp, ag, XFS_ICI_RECLAIM_TAG))) {
                ag = pag->pag_agno + 1;
                reclaimable += pag->pag_ici_reclaimable;
                xfs_perag_put(pag);
        }

And xfs_icwalk_get_perag() had exactly the same setup and a mode
that does use tags so absolutely relies on xfs_perag_get() to return
NULL to terminate the walk once at the end of the fs.

So this issue was not introduced by the for_each_perag() iterators;
it has been around for pretty much forever.

This problem is that the growfs process inserts the new perags
before the superblock agcount is updated.

IOWs, the grow process does:

xfs_initialize_perag	>>>> inserts new perags beyond sb_agcount
trans_alloc
xfs_resizefs_init_new_ags
xfs_trans_mod_sb(tp, XFS_TRANS_SB_AGCOUNT, nagcount - oagcount);
			>>>> triggers sb_agcount mod on commit
xfs_trans_commit
			>>>> makes sb_agcount change externally visible

Hence if we have a traversal in progress that terminates on a NULL
AG, it will see these new AGs before sb_agcount is updated. That's
always been the case, the only difference now is that all perag
traversals use NULL as the termination case instead of sb_agcount.
The xfs_icount_range() one is only run from xfs_validate_sb_write()
which means it could run during a grow event via AIL pushing, but
otherwise will not ever be an issue.

FWIW, the underlying reason for NULL terminating traversals is that
on -shrink- perags will go away in the middle of an iteration,
indicating that we've reached the end of the filesystem. THe problem
here is this:

traversal				shrink

end agno = sb_agcount = 32;
for (agno = 0;
     agno < end_agno;
     agno = pag->pag_agno + 1)
.....
					sb_agcount = 16
					removes perags 16-31
.....
    <agno now equals 16>
    <lookup returns NULL>

IOWs, the iterations *need* to terminate on a NULL AG, and they
cannot be dependent on the end agno remaining valid for the entire
duration of the walk.

Similarly, if we are racing with a grow, why would we not want to
find those newly initialised AGs? We may actually need to operate on
them even before the grow has completed (e.g. imagine background
growing by thousands of AGs which might take a couple of minutes on
slow storage, and in the mean time someone runs an admin op that
says "don't allocate anywhere in AGs > 300". We want those newly
initialised perags to be marked as "don't allocate" so that when the
grow finishes and makes them "active" they don't suddenly allow
allocation...

So, really, the direction I'm trying to take these iterators is
to get rid of all dependence on sb_agcount for termination, and
instead rely on NULL lookups and internal per-ag state to determine
if they are seen by iterations.

Along those lines, my current patchset converts all these iterators
from "get/put" semantics to "grab/release" semantics for
active/passive reference counting. Iterators only work with grab
semantics, meaning they take active references only. If the per-ag
does not allow active references (such as it's in the middle of
instantiation via grow, teardown via shrink or being managed by
dynamic instantiation infrastructure) then xfs_perag_grab() will
return NULL rather than an uninitialised/invalid perag.

The grow code now has a two phase intialisation for perags - the
first is to init and insert it as an inactive AG, hence active
reference traversals do not see it. THe second is to mark the AG
active after the superblock has been updated, hence making it
publicly available for active referencing in ongoing operations
only after the grow has completed.

That means the iteration in xfs_icount_range() changes to never see
perags beyond sb_agcount because it can't get active references on
it and so they return NULL.

The next step in this process is to use a tag to indicate the perag
can take active references, hence we can use tag based lookups to
efficiently iterate all the active perags, allowing grow, shrink and
dynamic instantiation to be able to quickly enable/disable active
references without actually changing the perag in the radix tree
index.

We need to keep the perag in the radix tree until all operations on
the perag are done, because things like the buffer cache need the
perag to still be present and findable by xfs_perag_get() even once
active references to the perag are no longer allowed. Dynamic
management of perags needs this, too, because we'll have to wait for
passive references like the buffer cache and busy extents to be
fully purged before we can free the perag...

> 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.

IMO, this just hides the symptom that results from code that isn't
handling unexpected adverse loop termination correctly. The
iterators are going to get more complex in the near future, so we
really need them to have a robust iterator API that does all the
cleanup work correctly, rather than try to hide it all in a
increasingly complex for loop construct.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/3] xfs: terminate perag iteration reliably on end agno
  2021-10-07 23:02   ` Dave Chinner
@ 2021-10-08 14:45     ` Brian Foster
  2021-10-08 21:20       ` Dave Chinner
  0 siblings, 1 reply; 7+ messages in thread
From: Brian Foster @ 2021-10-08 14:45 UTC (permalink / raw)
  To: Dave Chinner; +Cc: linux-xfs

On Fri, Oct 08, 2021 at 10:02:59AM +1100, Dave Chinner wrote:
> On Thu, Oct 07, 2021 at 08:50:53AM -0400, Brian Foster wrote:
> > 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
> 
> Yeah, that's a bad assert. It's not valid in the context of grow or
> shrink or any of the future advanced per-ag management things we
> want to do.
> 

I think it depends on the context. I don't think it's unreasonable to
expect certain paths to not want to process post-eofs perags. I don't
really like the placement of this assert tbh (with it being in a generic
helper) and the sb write verifier is probably not ideal context for the
check in a generic sense, but it does flag unexpected behavior when you
consider the higher level iteration is based on sb_agcount.

> I'm ok with the change being proposed as a expedient bug fix, but
> I'll note that the approach taken to fix it is not compatible with
> future plans for managing shrink and perag operations. I'll comment
> on the patch first, then the rest of the email is commentary about
> how xfs_perag_get() is intended to be used...
> 
> > 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.
> 
> We still really need an iterator for the range iterations so that we
> can have a consistent set of behaviours for all iterations and
> don't need a special case just for the "mid walk break" where the
> code keeps the active reference to the perag for itself...
> 

Ok, but what exactly are you referring to by "an iterator" beyond what
we have here to this point? A walker function with a callback or
something? And why wouldn't we have done that in the first place instead
of introducing the API wart documented above?

> >   */
> >  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;
> 
> 	*agno = pag->pag_agno + 1;
> 	xfs_perag_put(pag);
> 	if (*agno > end_agno)
> 		return NULL;
> 	return xfs_perag_get(mp, *agno);
> 

Will fix.

> >  }
> >  
> >  #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))
> 
> Isn't this one line the entire bug fix right here? i.e. the
> factoring is largely unnecessary, the grow race bug is fixed by just
> this one-liner?
> 

No, the reference count problems can still occur regardless of this
particular change.

...
> 
> > 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.
> 
> IMO, this just hides the symptom that results from code that isn't
> handling unexpected adverse loop termination correctly. The
> iterators are going to get more complex in the near future, so we
> really need them to have a robust iterator API that does all the
> cleanup work correctly, rather than try to hide it all in a
> increasingly complex for loop construct.
> 

Any loop that uses one of the sb_agcount based iteration macros in a
context that can race with growfs and doesn't check pag != NULL post
loop is not handling loop termination correctly. The sb_agcount check
effectively builds in an early termination vector to every such usage,
because we can't guarantee that pag == NULL when the sb_agcount check
causes loop termination.

IOW, the following usage pattern documented in the comment above is not
universally correct/safe:

	for_each_perag(...) {
		/* no early termination */
	}
	/* no perag check because no early termination */

... because for_each_perag() is not implemented correctly to guarantee
that pag == NULL on exit of the loop. This is a simple logic bug with a
simple fix.

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/3] xfs: terminate perag iteration reliably on end agno
  2021-10-08 14:45     ` Brian Foster
@ 2021-10-08 21:20       ` Dave Chinner
  0 siblings, 0 replies; 7+ messages in thread
From: Dave Chinner @ 2021-10-08 21:20 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs

On Fri, Oct 08, 2021 at 10:45:13AM -0400, Brian Foster wrote:
> On Fri, Oct 08, 2021 at 10:02:59AM +1100, Dave Chinner wrote:
> > On Thu, Oct 07, 2021 at 08:50:53AM -0400, Brian Foster wrote:
> > > 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.
> > 
> > We still really need an iterator for the range iterations so that we
> > can have a consistent set of behaviours for all iterations and
> > don't need a special case just for the "mid walk break" where the
> > code keeps the active reference to the perag for itself...
> > 
> 
> Ok, but what exactly are you referring to by "an iterator" beyond what
> we have here to this point? A walker function with a callback or
> something? And why wouldn't we have done that in the first place instead
> of introducing the API wart documented above?

We didn't do it in the first place because we started with the tag
walks that only ever terminate on NULL - that was Darrick's code
used in the inode cache walk functions.. Converting the rest of
agcount based walks was not straight-forward - there were different
cases that needed to be handled and this was the least worst way to
begin the conversion needed for shrink.

Looking back at the history, it was that last conversion for
GETFSMAP that caused the problems sb_agcount based walks in commit
58d43a7e3263 ("xfs: pass perags around in fsmap data dev functions")
where for_each_perag_from() was converted to
for_each_perag_from_range(). There was no termination leak before
this, and races with growfs were simply handled with termination by
NULL lookup return. The bug iwas introduced because we moved away
from termination on NULL lookup by adding a termination via loop
index to support GETFSMAP.

We knew about the API wart and put a plan in place to address it in
future by movign to iterators. That looks something like this but
without the C99 automatic cleanup so external iterator setup and
teardown:

https://lore.kernel.org/linux-xfs/162814685996.2777088.11268635137040103857.stgit@magnolia/

So, yes, we need to fix the bug that was introduced, but asking why
the code wasn't perfect before it was merged doesn't help us make
small steps forwards towards solving the bigger problems we need to
address...

> > >  }
> > >  
> > >  #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))
> > 
> > Isn't this one line the entire bug fix right here? i.e. the
> > factoring is largely unnecessary, the grow race bug is fixed by just
> > this one-liner?
> > 
> 
> No, the reference count problems can still occur regardless of this
> particular change.

Ok, so there are two problems then - one is an off-by one that can
result in perag lookups beyond the EOFS racing with growfs instead
of returning NULL.

The other is loop termination via loop index limits can leak a perag
when terminating the walk on the end index rather than a NULL. I
think these two bug fixes should at least be separate patches. Fix
the off-by-one in one patch, fix the termination issue in another.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-10-08 21:20 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 ` [PATCH 3/3] xfs: terminate perag iteration reliably on end agno Brian Foster
2021-10-07 23:02   ` Dave Chinner
2021-10-08 14:45     ` Brian Foster
2021-10-08 21:20       ` Dave Chinner

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).