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

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

[Brian Foster]

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)))
 
 /*
-- 
2.21.3

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

[Dave Chinner]

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Darrick J. Wong]

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.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

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

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

[Dave Chinner]

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

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

[Brian Foster]

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
> 

[PATCH 2/2] xfs: replace ialloc space res macro with inline helper

[Brian Foster]

Rewrite the macro as a static inline helper to clean up the logic
and have one less macro.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_trans_space.h | 24 ++++++++++++++++--------
 fs/xfs/xfs_inode.c              |  4 ++--
 fs/xfs/xfs_symlink.c            |  2 +-
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_trans_space.h b/fs/xfs/libxfs/xfs_trans_space.h
index c6df01a2a158..d08dfc8795c3 100644
--- a/fs/xfs/libxfs/xfs_trans_space.h
+++ b/fs/xfs/libxfs/xfs_trans_space.h
@@ -55,10 +55,18 @@
 	 XFS_DIRENTER_MAX_SPLIT(mp,nl))
 #define	XFS_DIRREMOVE_SPACE_RES(mp)	\
 	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) * \
-	  (M_IGEO(mp)->inobt_maxlevels - 1)))
+
+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;
+}
 
 /*
  * Space reservation values for various transactions.
@@ -71,7 +79,7 @@
 #define	XFS_ATTRSET_SPACE_RES(mp, v)	\
 	(XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK) + XFS_B_TO_FSB(mp, v))
 #define	XFS_CREATE_SPACE_RES(mp,nl)	\
-	(XFS_IALLOC_SPACE_RES(mp) + XFS_DIRENTER_SPACE_RES(mp,nl))
+	(xfs_ialloc_space_res(mp) + XFS_DIRENTER_SPACE_RES(mp,nl))
 #define	XFS_DIOSTRAT_SPACE_RES(mp, v)	\
 	(XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK) + (v))
 #define	XFS_GROWFS_SPACE_RES(mp)	\
@@ -81,18 +89,18 @@
 #define	XFS_LINK_SPACE_RES(mp,nl)	\
 	XFS_DIRENTER_SPACE_RES(mp,nl)
 #define	XFS_MKDIR_SPACE_RES(mp,nl)	\
-	(XFS_IALLOC_SPACE_RES(mp) + XFS_DIRENTER_SPACE_RES(mp,nl))
+	(xfs_ialloc_space_res(mp) + XFS_DIRENTER_SPACE_RES(mp,nl))
 #define	XFS_QM_DQALLOC_SPACE_RES(mp)	\
 	(XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK) + \
 	 XFS_DQUOT_CLUSTER_SIZE_FSB)
 #define	XFS_QM_QINOCREATE_SPACE_RES(mp)	\
-	XFS_IALLOC_SPACE_RES(mp)
+	xfs_ialloc_space_res(mp)
 #define	XFS_REMOVE_SPACE_RES(mp)	\
 	XFS_DIRREMOVE_SPACE_RES(mp)
 #define	XFS_RENAME_SPACE_RES(mp,nl)	\
 	(XFS_DIRREMOVE_SPACE_RES(mp) + XFS_DIRENTER_SPACE_RES(mp,nl))
 #define	XFS_SYMLINK_SPACE_RES(mp,nl,b)	\
-	(XFS_IALLOC_SPACE_RES(mp) + XFS_DIRENTER_SPACE_RES(mp,nl) + (b))
+	(xfs_ialloc_space_res(mp) + XFS_DIRENTER_SPACE_RES(mp,nl) + (b))
 #define XFS_IFREE_SPACE_RES(mp)		\
 	(xfs_sb_version_hasfinobt(&mp->m_sb) ? \
 			M_IGEO(mp)->inobt_maxlevels : 0)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 5c07bf491d9f..3420bc595e1b 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1195,7 +1195,7 @@ xfs_create(
 	unlock_dp_on_error = false;
 
 	error = xfs_dir_createname(tp, dp, name, ip->i_ino,
-					resblks - XFS_IALLOC_SPACE_RES(mp));
+					resblks - xfs_ialloc_space_res(mp));
 	if (error) {
 		ASSERT(error != -ENOSPC);
 		goto out_trans_cancel;
@@ -1290,7 +1290,7 @@ xfs_create_tmpfile(
 	if (error)
 		return error;
 
-	resblks = XFS_IALLOC_SPACE_RES(mp);
+	resblks = xfs_ialloc_space_res(mp);
 	tres = &M_RES(mp)->tr_create_tmpfile;
 
 	error = xfs_trans_alloc(mp, tres, resblks, 0, 0, &tp);
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index 8e88a7ca387e..b0a58cdd4c78 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -243,7 +243,7 @@ xfs_symlink(
 	 */
 	xfs_qm_vop_create_dqattach(tp, ip, udqp, gdqp, pdqp);
 
-	resblks -= XFS_IALLOC_SPACE_RES(mp);
+	resblks -= xfs_ialloc_space_res(mp);
 	/*
 	 * If the symlink will fit into the inode, write it inline.
 	 */
-- 
2.21.3

Re: [PATCH 2/2] xfs: replace ialloc space res macro with inline helper

[Dave Chinner]

On Thu, Jul 16, 2020 at 08:18:49AM -0400, Brian Foster wrote:
> Rewrite the macro as a static inline helper to clean up the logic
> and have one less macro.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_trans_space.h | 24 ++++++++++++++++--------
>  fs/xfs/xfs_inode.c              |  4 ++--
>  fs/xfs/xfs_symlink.c            |  2 +-
>  3 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_trans_space.h b/fs/xfs/libxfs/xfs_trans_space.h
> index c6df01a2a158..d08dfc8795c3 100644
> --- a/fs/xfs/libxfs/xfs_trans_space.h
> +++ b/fs/xfs/libxfs/xfs_trans_space.h
> @@ -55,10 +55,18 @@
>  	 XFS_DIRENTER_MAX_SPLIT(mp,nl))
>  #define	XFS_DIRREMOVE_SPACE_RES(mp)	\
>  	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) * \
> -	  (M_IGEO(mp)->inobt_maxlevels - 1)))
> +
> +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;
> +}

This misses the point I made. i.e. that the space reservation is
constant and never changes, yet we calculate it -twice- per inode
create. That means we can be calculating it hundreds of thousands of
times a second instead of just reading a variable that is likely hot
in cache.

IOWs, if we are going to improve this code, it should to be moved to
a pre-calculated, read-only, per-mount variable so the repeated
calculation goes away entirely.

Then the macro/function goes away entirely an is replaced simply
by mp->m_ialloc_space_res or M_IGEO(mp)->alloc_space_res....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: replace ialloc space res macro with inline helper

[Brian Foster]

On Fri, Jul 17, 2020 at 08:01:56AM +1000, Dave Chinner wrote:
> On Thu, Jul 16, 2020 at 08:18:49AM -0400, Brian Foster wrote:
> > Rewrite the macro as a static inline helper to clean up the logic
> > and have one less macro.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_trans_space.h | 24 ++++++++++++++++--------
> >  fs/xfs/xfs_inode.c              |  4 ++--
> >  fs/xfs/xfs_symlink.c            |  2 +-
> >  3 files changed, 19 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_trans_space.h b/fs/xfs/libxfs/xfs_trans_space.h
> > index c6df01a2a158..d08dfc8795c3 100644
> > --- a/fs/xfs/libxfs/xfs_trans_space.h
> > +++ b/fs/xfs/libxfs/xfs_trans_space.h
> > @@ -55,10 +55,18 @@
> >  	 XFS_DIRENTER_MAX_SPLIT(mp,nl))
> >  #define	XFS_DIRREMOVE_SPACE_RES(mp)	\
> >  	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) * \
> > -	  (M_IGEO(mp)->inobt_maxlevels - 1)))
> > +
> > +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;
> > +}
> 
> This misses the point I made. i.e. that the space reservation is
> constant and never changes, yet we calculate it -twice- per inode
> create. That means we can be calculating it hundreds of thousands of
> times a second instead of just reading a variable that is likely hot
> in cache.
> 

Partly.. I mentioned in my earlier reply that the geometry structure
doesn't seem like the right place to stuff transaction reservation
related values. An alternative I mentioned would be a new (or update to
the existing) structure that similarly precalculates log reservations,
but I wasn't going to go do that without some discussion/feedback on the
approach. Replacing the macro seemed broadly acceptable, so I sent that
patch as an incremental improvement and to keep the bug fix isolated.

Also, while stuffing a new value in a pre-existing structure might lend
itself to a one-off patch, I'm not sure that creating a new data
structure does lest it fail to justify the existence of the structure
itself. Therefore, it might be better to create a small series to
convert over several values to start such a structure and perhaps do the
rest over time to reduce the churn..

> IOWs, if we are going to improve this code, it should to be moved to
> a pre-calculated, read-only, per-mount variable so the repeated
> calculation goes away entirely.
> 

I figured replacing the macro was an incremental improvement independent
from how precalculated transaction block reservations might be
structured. TBH, I don't mind the macros as much as others seem to, so
feel free to defer or discard this patch altogether..

> Then the macro/function goes away entirely an is replaced simply
> by mp->m_ialloc_space_res or M_IGEO(mp)->alloc_space_res....
> 

Feel free to comment on my feedback on this in the previous reply..

Brian

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

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

[Eric Sandeen]

On 7/16/20 5:18 AM, Brian Foster wrote:
> On Thu, Jul 16, 2020 at 12:02:09PM +1000, Dave Chinner wrote:

...

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

For whatever my opinion is worth these days, it seems like doing
a survey to see how many of these reservations are static would be a
good first step, and then decide where they should all go if they should
move. I agree that IGEO might be a little odd, depending on what other
static reservation types there are and what they're associated with.

I see both sides of the discussion re: how fixes like this move forward
and what's easily backportable but in this case (and maybe I'm missing
context) it seems like a wider survey would be wise before deciding to
move this one value to IGEO in particular.

-Eric

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

[Darrick J. Wong]

On Fri, Jul 17, 2020 at 10:16:02AM -0700, Eric Sandeen wrote:
> On 7/16/20 5:18 AM, Brian Foster wrote:
> > On Thu, Jul 16, 2020 at 12:02:09PM +1000, Dave Chinner wrote:
> 
> ...
> 
> >> 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.
> 
> For whatever my opinion is worth these days, it seems like doing
> a survey to see how many of these reservations are static would be a
> good first step, and then decide where they should all go if they should
> move. I agree that IGEO might be a little odd, depending on what other
> static reservation types there are and what they're associated with.
> 
> I see both sides of the discussion re: how fixes like this move forward
> and what's easily backportable but in this case (and maybe I'm missing
> context) it seems like a wider survey would be wise before deciding to
> move this one value to IGEO in particular.

Agreed.  AFAICT the first patch is a bug fix for broken functionality,
so I will put it in the 5.9 branch update next week.

--D

> -Eric

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

[Christoph Hellwig]

Looks good as a quick fix:

Reviewed-by: Christoph Hellwig <hch@lst.de>

and I'm all for further cleanups on top.