On Wed, Jul 14, 2021 at 10:58:50AM +1000, Dave Chinner wrote: > On Mon, Jul 12, 2021 at 03:07:25PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@xxxxxxxxxx> > > > > Improve the checking at the start of a realtime grow operation so that > > we avoid accidentally set a new extent size that is too large and avoid > > adding an rt volume to a filesystem with rmap or reflink because we > > don't support rt rmap or reflink yet. > > > > While we're at it, separate the checks so that we're only testing one > > aspect at a time. > > > > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx> > > --- > > fs/xfs/xfs_rtalloc.c | 20 ++++++++++++++++---- > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c > > index 4e7be6b4ca8e..8920bce4fb0a 100644 > > --- a/fs/xfs/xfs_rtalloc.c > > +++ b/fs/xfs/xfs_rtalloc.c > > @@ -928,11 +928,23 @@ xfs_growfs_rt( > > */ > > if (!capable(CAP_SYS_ADMIN)) > > return -EPERM; > > - if (mp->m_rtdev_targp == NULL || mp->m_rbmip == NULL || > > - (nrblocks = in->newblocks) <= sbp->sb_rblocks || > > - (sbp->sb_rblocks && (in->extsize != sbp->sb_rextsize))) > > + if (mp->m_rtdev_targp == NULL || !mp->m_rbmip || !mp->m_rsumip) > > return -EINVAL; > > Shouldn't this use XFS_IS_REALTIME_MOUNT() so it always fails if > CONFIG_XFS_RT=n? xfs_rtalloc.c isn't even linked into the binary if CONFIG_XFS_RT=n. > i.e. if we have to check mp->m_rbmip and mp->m_rsumip to determine > if this mount is realtime enabled, then doesn't > XFS_IS_REALTIME_MOUNT() need to be fixed? TBH I think technically we could actually drop the m_rbmip/m_rsumip checks since the mount will fail if those files cannot be iget'd. That said, given how poorly tested realtime is, I figured it doesn't hurt to double-check for this infrequent operation. > > > - if ((error = xfs_sb_validate_fsb_count(sbp, nrblocks))) > > + if (in->newblocks <= sbp->sb_rblocks) > > + return -EINVAL; > > + if (xfs_sb_version_hasrealtime(&mp->m_sb) && > > + in->extsize != sbp->sb_rextsize) > > + return -EINVAL; > > xfs_sb_version_hasrealtime() checks "sbp->sb_rblocks > 0", it's not > an actual version flag check. I think this makes much more sense > being open coded rather than masquerading as a feature check.... Ok, I'll change it back. > > > + if (XFS_FSB_TO_B(mp, in->extsize) > XFS_MAX_RTEXTSIZE || > > + XFS_FSB_TO_B(mp, in->extsize) < XFS_MIN_RTEXTSIZE) > > + return -EINVAL; > > + if (xfs_sb_version_hasrmapbt(&mp->m_sb) || > > + xfs_sb_version_hasreflink(&mp->m_sb)) > > + return -EOPNOTSUPP; > > + > > + nrblocks = in->newblocks; > > + error = xfs_sb_validate_fsb_count(sbp, nrblocks); > > + if (error) > > return error; > > Otherwise looks like a reasonable set of additional checks. Cool! Thanks for the review. --D > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx
