Re: [PATCH 2/2] xfs: Bypass sb alignment checks when custom values are used

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Jun 03, 2020 at 09:12:35AM +1000, Dave Chinner wrote:
> On Tue, Jun 02, 2020 at 11:18:44AM +0200, Carlos Maiolino wrote:
> > Hi Dave.
> > 
> > On Tue, Jun 02, 2020 at 07:21:15AM +1000, Dave Chinner wrote:
> > > On Mon, Jun 01, 2020 at 04:01:53PM +0200, Carlos Maiolino wrote:
> > > > index 4df87546bd40..72dae95a5e4a 100644
> > > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > > @@ -360,19 +360,27 @@ xfs_validate_sb_common(
> > > >  		}
> > > >  	}
> > > >  
> > > > -	if (sbp->sb_unit) {
> > > > -		if (!xfs_sb_version_hasdalign(sbp) ||
> > > > -		    sbp->sb_unit > sbp->sb_width ||
> > > > -		    (sbp->sb_width % sbp->sb_unit) != 0) {
> > > > -			xfs_notice(mp, "SB stripe unit sanity check failed");
> > > > +	/*
> > > > +	 * Ignore superblock alignment checks if sunit/swidth mount options
> > > > +	 * were used or alignment turned off.
> > > > +	 * The custom alignment validation will happen later on xfs_mountfs()
> > > > +	 */
> > > > +	if (!(mp->m_flags & XFS_MOUNT_ALIGN) &&
> > > > +	    !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
> > > 
> > > mp->m_dalign tells us at this point if a user specified sunit as a
> > > mount option.  That's how xfs_fc_validate_params() determines the user
> > > specified a custom sunit, so there is no need for a new mount flag
> > > here to indicate that mp->m_dalign was set by the user....
> > 
> > At a first glance, I thought about it too, but, there is nothing preventing an
> > user to mount a filesystem passing sunit=0,swidth=0. So, this means we can't
> > really rely on the m_dalign/m_swidth values to check an user passed in (or not)
> > alignment values. Unless we first deny users to pass 0 values into it.
> 
> Sure we can. We do this sort of "was the mount option set" detection
> with m_logbufs and m_logbsize by initialising them to -1. Hence if
> they are set by mount options, they'll have a valid, in-range value
> instead of "-1".
> 
> That said, if you want users passing in sunit=0,swidth=0 to
> correctly override existing on-disk values (i.e. effectively mean -o
> noalign), then you are going to need to modify
> xfs_update_alignment() and xfs_validate_new_dalign() to handle
> mp->m_dalign == 0 as a valid value instead of "sunit/swidth mount
> option not set, use existing superblock values".....
> 
> IOWs, there are deeper changes needed here than just setting a new
> flag to say "mount option was set" for it to function correctly and
> consistently as you intend. This is why I think we should just fix
> this situation automatically, and not require the user to manually
> override the bad values.
> 
> Thinking bigger picture, I'd like to see the mount options
> deprecated and new xfs_admin options added to change the values on a
> live, mounted filesystem. That way users who have issues like this
> don't need to unmount the filesystem to fix it, not to mention users
> who reshape online raid arrays and grow the filesystem can also
> change the filesystem geometry without needing to take the
> filesystem offline...

TBH I've always wondered, why not let root obtain the fs geometry,
modify whatever features it wants, and then push it back to the fs to
validate and update as necessary?  In theory you could even use this for
online filesystem upgrades, should we ever again allow users to do
that...

--D

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