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

[Dave Chinner <david@fromorbit.com>]

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

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