| Message ID | 20200715193310.22002-1-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: fix inode allocation block res calculation precedence | expand |
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.
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
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.
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 >
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
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
Looks good as a quick fix:
Reviewed-by: Christoph Hellwig <hch@lst.de>
and I'm all for further cleanups on top.
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))) /*
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(-)