Re: [RFC-PATCH] xfs: do not update sunit/swidth in the superblock to match those provided during mount

From: Alex Lyakas <alex@zadara.com>
To: Dave Chinner <david@fromorbit.com>
Cc: Christoph Hellwig <hch@infradead.org>,
	Eric Sandeen <sandeen@sandeen.net>,
	Brian Foster <bfoster@redhat.com>,
	linux-xfs@vger.kernel.org
Subject: Re: [RFC-PATCH] xfs: do not update sunit/swidth in the superblock to match those provided during mount
Date: Sun, 1 Dec 2019 11:00:32 +0200	[thread overview]
Message-ID: <CAOcd+r21Ur=jxvJgUdXs+dQj37EnC=ZWP8F45sLesQFJ_GCejg@mail.gmail.com> (raw)
In-Reply-To: <20191130202853.GA2695@dread.disaster.area>

Hi Dave,

Thank you for your response.

On Sat, Nov 30, 2019 at 10:28 PM Dave Chinner <david@fromorbit.com> wrote:
>
> On Wed, Nov 27, 2019 at 06:19:29AM -0800, Christoph Hellwig wrote:
> > Can we all take a little step back and think about the implications
> > of the original patch from Alex?  Because I think there is very little.
> > And updated sunit/swidth is just a little performance optimization,
> > and anyone who really cares about changing that after the fact can
> > trivially add those to fstab.
> >
> > So I think something like his original patch plus a message during
> > mount that the new values are not persisted should be perfectly fine.
>
> Well, the original purpose of the mount options was to persist a new
> sunit/swidth to the superblock...
>
> Let's ignore the fact that it was a result of a CXFS client mount
> bug trashing the existing sunit/swidth values, and instead focus on
> the fact we've been telling people for years that you "only need to
> set these once after a RAID reshape" and so we have a lot of users
> out there expecting it to persist the new values...
>
> I don't think we can just redefine the documented and expected
> behaviour of a mount option like this.
>
> With that in mind, the xfs(5) man page explicitly states this:
>
>         The sunit and swidth parameters specified must be compatible
>         with the existing filesystem alignment characteristics.  In
>         general,  that  means  the  only  valid changes to sunit are
>         increasing it by a power-of-2 multiple. Valid swidth values
>         are any integer multiple of a valid sunit value.
>
> Note the comment about changes to sunit? What is being done here -
> halving the sunit from 64 to 32 blocks is invalid, documented as
> invalid, but the kernel does not enforce this. We should fix the
> kernel code to enforce the alignment rules that the mount option
> is documented to require.
>
> If we want to change the alignment characteristics after mkfs, then
> use su=1,sw=1 as the initial values, then the first mount can use
> the options to change it to whatever is present after mkfs has run.

If I understand your response correctly:
- some sunit/swidth changes during mount are legal and some aren't
- the legal changes should be persisted in the superblock

What about the repair? Even if user performs a legal change, it still
breaks the repairability of the file system.

For now, we made a local change to not persist sunit/swidth updates in
the superblock. Because we must have a working repair, and our kernel
(4.14 stable) allows any sunit/swidth changes.

We can definitely adhere to the recommended behavior of setting
sunit/swidth=1 during mkfs, provided the repair still works after
mounting with different sunit/swidth.

Thanks,
Alex.

>
> Filesystems on storage that has dynamically changeable geometry
> probably shouldn't be using fixed physical alignment in the first
> place, though...
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com