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".
Funny thing is I thought about "let's initialize to -1" and gave up because it
seemed ugly :)
>
> 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.
Sure, I'm not opposed to fix things automatically, I just thought it wasn't an
acceptable solution, but looks like I'm wrong.
>
> 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.
I haven't been following this development. So, you meant geometry mount options
or mount options as a whole?
> That way users who have issues like this
> don't need to unmount the filesystem to fix it,
Sure, but it doesn't fix those filesystems which were already unmounted. But,
fixing it automatically during mount seem to cover this part.
cheers.
--
Carlos
