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

[Dave Chinner <david@fromorbit.com>]

On Wed, Nov 20, 2019 at 08:50:03PM -0800, Darrick J. Wong wrote:
> 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:
> > > > -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

Ok, that's a decent reason to make the code a bit more complex. I'll
see what I can do....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com