Re: [PATCH] xfs: gut error handling in xfs_trans_unreserve_and_mod_sb()

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Nov 21, 2019 at 03:00:23PM +1100, Dave Chinner wrote:
> On Wed, Nov 20, 2019 at 06:38:36PM -0800, Darrick J. Wong wrote:
> > On Thu, Nov 21, 2019 at 11:44:37AM +1100, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Shaokun Zhang reported that XFs was using substantial CPU time in
> > > percpu_count_sum() when running a single threaded benchmark on
> > > a high CPU count (128p) machine from xfs_mod_ifree(). The issue
> > > is that the filesystem is empty when the benchmark runs, so inode
> > > allocation is running with a very low inode free count.
> > > 
> > > With the percpu counter batching, this means comparisons when the
> > > counter is less that 128 * 256 = 32768 use the slow path of adding
> > > up all the counters across the CPUs, and this is expensive on high
> > > CPU count machines.
> > > 
> > > The summing in xfs_mod_ifree() is only used to fire an assert if an
> > > underrun occurs. The error is ignored by the higher level code.
> > > Hence this is really just debug code. Hence we don't need to run it
> > > on production kernels, nor do we need such debug checks to return
> > > error values just to trigger an assert.
> > > 
> > > Further, the error handling in xfs_trans_unreserve_and_mod_sb() is
> > > largely incorrect - Rolling back the changes in the transaction if
> > > only one counter underruns makes all the other counters
> > > incorrect.
> > 
> > Separate change, separate patch...
> 
> Yeah, i can split it up, just wanted to see what people thought
> about the approach...

<nod>

> > >  	if (idelta) {
> > > -		error = xfs_mod_icount(mp, idelta);
> > > -		if (error)
> > > -			goto out_undo_fdblocks;
> > > +		percpu_counter_add_batch(&mp->m_icount, idelta,
> > > +					 XFS_ICOUNT_BATCH);
> > > +		if (idelta < 0)
> > > +			ASSERT(__percpu_counter_compare(&mp->m_icount, 0,
> > > +							XFS_ICOUNT_BATCH) >= 0);
> > >  	}
> > >  
> > >  	if (ifreedelta) {
> > > -		error = xfs_mod_ifree(mp, ifreedelta);
> > > -		if (error)
> > > -			goto out_undo_icount;
> > > +		percpu_counter_add(&mp->m_ifree, ifreedelta);
> > > +		if (ifreedelta < 0)
> > > +			ASSERT(percpu_counter_compare(&mp->m_ifree, 0) >= 0);
> > 
> > Since the whole thing is a debug statement, why not shove everything
> > into a single assert?
> > 
> > ASSERT(ifreedelta >= 0 || percpu_computer_compare() >= 0); ?
> 
> I could, but it still needs to be split over two lines and I find
> unnecessarily complex ASSERT checks hinder understanding. I can look
> at what I wrote at a glance and immediately understand that the
> assert is conditional on the counter being negative, but the single
> line compound assert form requires me to stop, read and think about
> the logic before I can identify that the ifreedelta check is just a
> conditional that reduces the failure scope rather than is a failure
> condition itself.
> 
> I like simple logic with conditional behaviour being obvious via
> pattern matching - it makes my brain hurt less because I'm really
> good at visual pattern matching and I'm really bad at reading
> and writing code.....

Fair enough.  I'm not a paragon of correctness wrt. boolean logic either.
I'm ok if you leave it as is.

> > > -out_undo_frextents:
> > > -	if (rtxdelta)
> > > -		xfs_sb_mod64(&mp->m_sb.sb_frextents, -rtxdelta);
> > > -out_undo_ifree:
> > > +	xfs_sb_mod64(&mp->m_sb.sb_frextents, rtxdelta);
> > 
> > As for these bits... why even bother with a three line helper?  I think
> > this is clearer about what's going on:
> > 
> > 	mp->m_sb.sb_frextents += rtxdelta;
> > 	mp->m_sb.sb_dblocks += tp->t_dblocks_delta;
> > 	...
> > 	ASSERT(!rtxdelta || mp->m_sb.sb_frextents >= 0);
> > 	ASSERT(!tp->t_dblocks_delta || mp->m_sb.sb.dblocks >= 0);
> 
> That required writing more code and adding more logic I'd have to
> think about to write, and then think about again every time I read
> it.

OTOH it's an opportunity to make the asserts more useful, because right
now they just say:

XFS (sda): Assertion failed: counter >= 0, file: xfs_trans.c, line XXX

*Which* counter just tripped the assert?  At least it could say:

XFS (sda): Assertion failed: mp->m_sb.sb_dblocks >= 0, file: xfs_trans.c, line XXX

> > I also wonder if we should be shutting down the fs here if the counts
> > go negative, but <shrug> that would be yet a different patch. :)
> 
> I also thought about that, but all this accounting should have
> already been bounds checked. i.e. We should never get an error here,
> and I don't think I've *ever* seen an assert in this code fire.
> Hence I just went for the dead simple nuke-it-from-orbit patch...

<nod> I have, but only after seriously fubaring some code. :)

--D

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