Re: [PATCH] xfs: fix inode allocation block res calculation precedence

[Brian Foster <bfoster@redhat.com>]

On Thu, Jul 16, 2020 at 12:02:09PM +1000, Dave Chinner wrote:
> On Wed, Jul 15, 2020 at 06:47:59PM -0700, Darrick J. Wong wrote:
> > On Thu, Jul 16, 2020 at 08:29:35AM +1000, Dave Chinner wrote:
> > > On Wed, Jul 15, 2020 at 03:33:10PM -0400, Brian Foster wrote:
> > > > The block reservation calculation for inode allocation is supposed
> > > > to consist of the blocks required for the inode chunk plus
> > > > (maxlevels-1) of the inode btree multiplied by the number of inode
> > > > btrees in the fs (2 when finobt is enabled, 1 otherwise).
> > > > 
> > > > Instead, the macro returns (ialloc_blocks + 2) due to a precedence
> > > > error in the calculation logic. This leads to block reservation
> > > > overruns via generic/531 on small block filesystems with finobt
> > > > enabled. Add braces to fix the calculation and reserve the
> > > > appropriate number of blocks.
> > > > 
> > > > Fixes: 9d43b180af67 ("xfs: update inode allocation/free transaction reservations for finobt")
> > > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > > ---
> > > >  fs/xfs/libxfs/xfs_trans_space.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_trans_space.h b/fs/xfs/libxfs/xfs_trans_space.h
> > > > index 88221c7a04cc..c6df01a2a158 100644
> > > > --- a/fs/xfs/libxfs/xfs_trans_space.h
> > > > +++ b/fs/xfs/libxfs/xfs_trans_space.h
> > > > @@ -57,7 +57,7 @@
> > > >  	XFS_DAREMOVE_SPACE_RES(mp, XFS_DATA_FORK)
> > > >  #define	XFS_IALLOC_SPACE_RES(mp)	\
> > > >  	(M_IGEO(mp)->ialloc_blks + \
> > > > -	 (xfs_sb_version_hasfinobt(&mp->m_sb) ? 2 : 1 * \
> > > > +	 ((xfs_sb_version_hasfinobt(&mp->m_sb) ? 2 : 1) * \
> > > >  	  (M_IGEO(mp)->inobt_maxlevels - 1)))
> > > 
> > > Ugh. THese macros really need rewriting as static inline functions.
> > > This would not have happened if it were written as:
> > > 
> > > static inline int
> > > xfs_ialloc_space_res(struct xfs_mount *mp)
> > > {
> > > 	int	res = M_IGEO(mp)->ialloc_blks;
> > > 
> > > 	res += M_IGEO(mp)->inobt_maxlevels - 1;
> > > 	if (xfs_sb_version_hasfinobt(&mp->m_sb))
> > > 		res += M_IGEO(mp)->inobt_maxlevels - 1;
> > > 	return res;
> > > }
> > > 
> > > Next question: why is this even a macro that is calculated on demand
> > > instead of a read-only constant held in inode geometry calculated
> > > at mount time? Then it doesn't even need to be an inline function
> > > and can just be rolled into xfs_ialloc_setup_geometry()....
> > 
> > Yeah, I hate those macros too.  Fixing all that sounds like a <cough>
> > cleanup series for someone, but in the meantime this is easy enough to
> > backport to stable kernels.
> 
> Well, I'm not suggesting that we have to fix all of them at once.
> Just converting this specific one to a IGEO variable is probably
> only 20 lines of code, which is still an "easy to backport" fix.
> 
> i.e. XFS_IALLOC_SPACE_RES() is used in just 7 places in the code,
> 4 of them are in that same header file, so it's a simple, standalone
> patch that fixes the bug by addressing the underlying cause of
> the problem (i.e. nasty macro!).
> 

I agree that the inline is nicer than the macro, but a transaction
reservation value seems misplaced to me in the IGEO. Perhaps having
something analogous to struct xfs_trans_resv might be more appropriate.

Regardless, I agree with Darrick on the backporting situation. The
original patch needs to be backportable to however many upstream stable
releases back to v3.16 and similarly for distro kernels. Either patch
might not be complex overall, but for somebody who might be processing
hundreds of backports across various subsystems refactoring things as
such in the same patch is clearly not equivalent to a one line change to
an otherwise unchanged line since the original commit. I'll post a patch
on top of this one to rework into an inline if people view that as
preferable to the macro.

Brian

> Historically speaking , we have cleaned up stuff like this to fix
> the bug, not done a one liner and then left fixing the root cause to
> some larger chunk of future work. The "one-liner" approach is
> largely a recent invention. I look at this sort of thing as being
> similar to cleaning up typedefs: we remove typedefs as we change
> surrounding code, thereby slowly remove them over time. We could
> just remove them all as one big patchset, but we don't do that
> because of all the outstanding work it would cause conflicts in.
> 
> Perhaps we've lost sight of the fact that doing things in little
> chunks on demand actually results in a lot of good cleanup change
> over time. We really don't have to do cleanups as one huge chunk of
> work all at once....
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>