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

From: Dave Chinner <david@fromorbit.com>
To: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 2/2] xfs: Bypass sb alignment checks when custom values are used
Date: Wed, 3 Jun 2020 09:12:35 +1000	[thread overview]
Message-ID: <20200602231235.GJ2040@dread.disaster.area> (raw)
In-Reply-To: <20200602091844.nsi63ixzm6zgxy76@eorzea>

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com