Re: [RFC PATCH] xfs: fix per-cpu CIL structure aggregation racing with dying cpus

[Darrick J. Wong]

On Wed, Aug 23, 2023 at 09:28:16AM +1000, Dave Chinner wrote:
> On Tue, Aug 22, 2023 at 11:30:16AM -0700, Darrick J. Wong wrote:
> > On Mon, Aug 21, 2023 at 03:17:52PM +1000, Dave Chinner wrote:
> > > On Fri, Aug 04, 2023 at 03:38:54PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> > > > 
> > > > In commit 7c8ade2121200 ("xfs: implement percpu cil space used
> > > > calculation"), the XFS committed (log) item list code was converted to
> > > > use per-cpu lists and space tracking to reduce cpu contention when
> > > > multiple threads are modifying different parts of the filesystem and
> > > > hence end up contending on the log structures during transaction commit.
> > > > Each CPU tracks its own commit items and space usage, and these do not
> > > > have to be merged into the main CIL until either someone wants to push
> > > > the CIL items, or we run over a soft threshold and switch to slower (but
> > > > more accurate) accounting with atomics.
> > > > 
> > > > Unfortunately, the for_each_cpu iteration suffers from the same race
> > > > with cpu dying problem that was identified in commit 8b57b11cca88f
> > > > ("pcpcntrs: fix dying cpu summation race") -- CPUs are removed from
> > > > cpu_online_mask before the CPUHP_XFS_DEAD callback gets called.  As a
> > > > result, both CIL percpu structure aggregation functions fail to collect
> > > > the items and accounted space usage at the correct point in time.
> > > > 
> > > > If we're lucky, the items that are collected from the online cpus exceed
> > > > the space given to those cpus, and the log immediately shuts down in
> > > > xlog_cil_insert_items due to the (apparent) log reservation overrun.
> > > > This happens periodically with generic/650, which exercises cpu hotplug
> > > > vs. the filesystem code.
> > > > 
> > > > Applying the same sort of fix from 8b57b11cca88f to the CIL code seems
> > > > to make the generic/650 problem go away, but I've been told that tglx
> > > > was not happy when he saw:
> > > > 
> > > > "...the only thing we actually need to care about is that
> > > > percpu_counter_sum() iterates dying CPUs. That's trivial to do, and when
> > > > there are no CPUs dying, it has no addition overhead except for a
> > > > cpumask_or() operation."
> > > > 
> > > > I have no idea what the /correct/ solution is, but I've been holding on
> > > > to this patch for 3 months.  In that time, 8b57b11cca88 hasn't been
> > > > reverted and cpu_dying_mask hasn't been removed, so I'm sending this and
> > > > we'll see what happens.
> > > > 
> > > > So, how /do/ we perform periodic aggregation of per-cpu data safely?
> > > > Move the xlog_cil_pcp_dead call to the dying section, where at least the
> > > > other cpus will all be stopped?  And have it dump its items into any
> > > > online cpu's data structure?
> > > 
> > > I suspect that we have to stop using for_each_*cpu() and hotplug
> > > notifiers altogether for this code.
> > > 
> > > That is, we simply create our own bitmap for nr_possible_cpus in the
> > > CIL checkpoint context we allocate for each checkpoint (i.e. the
> > > struct xfs_cil_ctx). When we store something on that CPU for that
> > > CIL context, we set the corresponding bit for that CPU. Then when we
> > > are aggregating the checkpoint, we simply walk all the cpus with the
> > > "has items" bit set and grab everything.
> > > 
> > > This gets rid of the need for hotplug notifiers completely - we just
> > > don't care if the CPU is online or offline when we sweep the CIL
> > > context for items at push time - if the bit is set then there's
> > > stuff to sweep...
> > 
> > Oooh, good idea.  Something like this, then?  Lightly tested via
> > generic/650 for five minutes...
> 
> Not quite what I was saying - you put the cpu mask in the struct
> xfs_cil, which means it can potentially be accessed from both commit
> and aggregation at the same time for different checkpoints.
> 
> What I was suggesting is that it gets placed in the struct
> xfs_cil_ctx which is guaranteed to be private to

private to...?

> > 
> > --
> > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> > Subject: [PATCH] xfs: fix per-cpu CIL structure aggregation racing with dying cpus
> > 
> > In commit 7c8ade2121200 ("xfs: implement percpu cil space used
> > calculation"), the XFS committed (log) item list code was converted to
> > use per-cpu lists and space tracking to reduce cpu contention when
> > multiple threads are modifying different parts of the filesystem and
> > hence end up contending on the log structures during transaction commit.
> > Each CPU tracks its own commit items and space usage, and these do not
> > have to be merged into the main CIL until either someone wants to push
> > the CIL items, or we run over a soft threshold and switch to slower (but
> > more accurate) accounting with atomics.
> > 
> > Unfortunately, the for_each_cpu iteration suffers from the same race
> > with cpu dying problem that was identified in commit 8b57b11cca88f
> > ("pcpcntrs: fix dying cpu summation race") -- CPUs are removed from
> > cpu_online_mask before the CPUHP_XFS_DEAD callback gets called.  As a
> > result, both CIL percpu structure aggregation functions fail to collect
> > the items and accounted space usage at the correct point in time.
> > 
> > If we're lucky, the items that are collected from the online cpus exceed
> > the space given to those cpus, and the log immediately shuts down in
> > xlog_cil_insert_items due to the (apparent) log reservation overrun.
> > This happens periodically with generic/650, which exercises cpu hotplug
> > vs. the filesystem code:
> > 
> > smpboot: CPU 3 is now offline
> > XFS (sda3): ctx ticket reservation ran out. Need to up reservation
> > XFS (sda3): ticket reservation summary:
> > XFS (sda3):   unit res    = 9268 bytes
> > XFS (sda3):   current res = -40 bytes
> > XFS (sda3):   original count  = 1
> > XFS (sda3):   remaining count = 1
> > XFS (sda3): Filesystem has been shut down due to log error (0x2).
> > 
> > Applying the same sort of fix from 8b57b11cca88f to the CIL code seems
> > to make the generic/650 problem go away, but I've been told that tglx
> > was not happy when he saw:
> > 
> > "...the only thing we actually need to care about is that
> > percpu_counter_sum() iterates dying CPUs. That's trivial to do, and when
> > there are no CPUs dying, it has no addition overhead except for a
> > cpumask_or() operation."
> > 
> > The CPU hotplug code is rather complex and difficult to understand and I
> > don't want to try to understand the cpu hotplug locking well enough to
> > use cpu_dying mask.  Furthermore, there's a performance improvement that
> > could be had here.  Attach a private cpu mask to the CIL structure so
> > that we can track exactly which cpus have accessed the percpu data at
> > all.  It doesn't matter if the cpu has since gone offline; log item
> > aggregation will still find the items.  Better yet, we skip cpus that
> > have not recently logged anything.
> > 
> > Link: https://lore.kernel.org/linux-xfs/ZOLzgBOuyWHapOyZ@xxxxxxxxxxxxxxxxxxx/T/
> > Link: https://lore.kernel.org/lkml/877cuj1mt1.ffs@tglx/
> > Link: https://lore.kernel.org/lkml/20230414162755.281993820@xxxxxxxxxxxxx/
> > Cc: tglx@xxxxxxxxxxxxx
> > Cc: peterz@xxxxxxxxxxxxx
> > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > ---
> >  fs/xfs/xfs_log_cil.c  |   58 ++++++++++++++++++++-----------------------------
> >  fs/xfs/xfs_log_priv.h |   15 ++++++-------
> >  fs/xfs/xfs_super.c    |    1 -
> >  3 files changed, 31 insertions(+), 43 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> > index eccbfb99e894..d91589a959ee 100644
> > --- a/fs/xfs/xfs_log_cil.c
> > +++ b/fs/xfs/xfs_log_cil.c
> > @@ -124,7 +124,7 @@ xlog_cil_push_pcp_aggregate(
> >  	struct xlog_cil_pcp	*cilpcp;
> >  	int			cpu;
> >  
> > -	for_each_online_cpu(cpu) {
> > +	for_each_cpu(cpu, &cil->xc_pcpmask) {
> >  		cilpcp = per_cpu_ptr(cil->xc_pcp, cpu);
> >  
> >  		ctx->ticket->t_curr_res += cilpcp->space_reserved;
> > @@ -165,7 +165,14 @@ xlog_cil_insert_pcp_aggregate(
> >  	if (!test_and_clear_bit(XLOG_CIL_PCP_SPACE, &cil->xc_flags))
> >  		return;
> >  
> > -	for_each_online_cpu(cpu) {
> > +	/*
> > +	 * We don't hold xc_push_lock here, so we can race with other cpus
> > +	 * setting xc_pcpmask.  However, we've atomically cleared PCP_SPACE
> > +	 * which forces other threads to add to the global space used count.
> > +	 * xc_pcpmask is a superset of cilpcp structures that could have a
> > +	 * nonzero space_used.
> > +	 */
> > +	for_each_cpu(cpu, &cil->xc_pcpmask) {
> >  		int	old, prev;
> 
> I don't think the xc_push_lock means anything here; if we use atomic
> bit ops to set the pcpmask it certainly doesn't. AFAICT, there
> aren't any new races here by adding the private pcp mask. If we do
> race, then the push aggregation will still correctly aggregate any
> count that was missed on the global total...

Agreed.

> >  		cilpcp = per_cpu_ptr(cil->xc_pcp, cpu);
> > @@ -186,6 +193,7 @@ xlog_cil_ctx_switch(
> >  	xlog_cil_set_iclog_hdr_count(cil);
> >  	set_bit(XLOG_CIL_EMPTY, &cil->xc_flags);
> >  	set_bit(XLOG_CIL_PCP_SPACE, &cil->xc_flags);
> > +	cpumask_clear(&cil->xc_pcpmask);
> >  	ctx->sequence = ++cil->xc_current_sequence;
> >  	ctx->cil = cil;
> >  	cil->xc_ctx = ctx;
> > @@ -554,6 +562,7 @@ xlog_cil_insert_items(
> >  	int			iovhdr_res = 0, split_res = 0, ctx_res = 0;
> >  	int			space_used;
> >  	int			order;
> > +	unsigned int		cpu_nr;
> >  	struct xlog_cil_pcp	*cilpcp;
> >  
> >  	ASSERT(tp);
> > @@ -579,6 +588,18 @@ xlog_cil_insert_items(
> >  	 */
> >  	cilpcp = get_cpu_ptr(cil->xc_pcp);
> >  
> > +	/*
> > +	 * Add this cpu to the mask of cpus that could have touched xc_pcp.
> > +	 * The push aggregation function is called with the push lock held,
> > +	 * so take it here if we have to update the mask.
> > +	 */
> > +	cpu_nr = current_cpu();
> > +	if (!cpumask_test_cpu(cpu_nr, &cil->xc_pcpmask)) {
> > +		spin_lock(&cil->xc_push_lock);
> > +		cpumask_set_cpu(cpu_nr, &cil->xc_pcpmask);
> > +		spin_unlock(&cil->xc_push_lock);
> > +	}
> 
> I don't think this needs a spin lock - an atomic test-and-set would
> be just fine here (i.e. cpumask_test_and_set_cpu()) and so we only
> need a single atomic operation here instead of two. i.e. make it a
> test-test-and-set operation similar to clearing XLOG_CIL_EMPTY in
> this fast path:
> 
> 	if (!cpumask_test_cpu(cpu, &cil->xc_pcpmask))
> 		cpumask_test_and_set_cpu(cpu_nr, &cil->xc_pcpmask);
> 
> We also don't have to care about memory barriers, etc w.r.t the
> push aggregate function because this is all done under the
> cil->xc_ctx_lock held shared.

Oh, hah.  I missed that cpumask_test_and_set_cpu is atomic, so we don't
need the spinlock here at all.  Good catch!

> Also, I think it would be better to put all this insert code
> explicitly under a single defined preempt disabled per-cpu section,
> rather than using current_cpu() inside the preempt disabled section
> without commenting on why it can be used like this safely. i.e.:
> 
>         /*
> 	 * Grab the cpu before we start any accounting.  That
> 	 * ensures that we are running with pre-emption disabled and
> 	 * so we can't be scheduled away between split sample/update
> 	 * operations that are done without outside locking to
> 	 * serialise them.
>          */
> 	cpu = get_cpu();
> 	cilpcp = this_cpu_ptr(cil->xc_pcp);
> 
> 	/* Tell the future push it has work to do on this CPU. */
> 	if (!cpumask_test_cpu(cpu, &cil->xc_pcpmask))
> 		cpumask_test_and_set_cpu(cpu, &cil->xc_pcpmask);
> 	....
> 	<done with cilpcp>
> 	put_cpu(cpu);

Much cleaner, thanks for the suggestion.

> .....
> 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 09638e8fb4ee..ef7775657ce3 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -2313,7 +2313,6 @@ xfs_cpu_dead(
> >  	list_for_each_entry_safe(mp, n, &xfs_mount_list, m_mount_list) {
> >  		spin_unlock(&xfs_mount_list_lock);
> >  		xfs_inodegc_cpu_dead(mp, cpu);
> > -		xlog_cil_pcp_dead(mp->m_log, cpu);
> >  		spin_lock(&xfs_mount_list_lock);
> >  	}
> >  	spin_unlock(&xfs_mount_list_lock);
> 
> I wonder if we can do something similar for the inodegc code, and
> once again get rid of the need for hotpulg notifiers in XFS?

I would imagine so, if you don't mind bloating up xfs_mount.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@xxxxxxxxxxxxx