Re: [PATCH 1/2] xfs: improve FSGROWFSRT precondition checking

["Darrick J. Wong" <djwong@kernel.org>]

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@kernel.org>
> > 
> > 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@kernel.org>
> > ---
> >  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@fromorbit.com