On Sat, Mar 18, 2023 at 2:39 AM Darrick J. Wong <djwong@xxxxxxxxxx> wrote:
>
> On Thu, Mar 16, 2023 at 10:09:56AM +0200, Amir Goldstein wrote:
> > On Thu, Mar 16, 2023 at 1:13 AM Catherine Hoang
> > <catherine.hoang@xxxxxxxxxx> wrote:
> > >
> > > > On Mar 13, 2023, at 10:50 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> > > >
> > > > On Tue, Mar 14, 2023 at 6:27 AM Catherine Hoang
> > > > <catherine.hoang@xxxxxxxxxx> wrote:
> > > >>
> > > >> Add a new ioctl to set the uuid of a mounted filesystem.
> > > >>
> > > >> Signed-off-by: Catherine Hoang <catherine.hoang@xxxxxxxxxx>
> > > >> ---
> > > >> fs/xfs/libxfs/xfs_fs.h | 1 +
> > > >> fs/xfs/xfs_ioctl.c | 107 +++++++++++++++++++++++++++++++++++++++++
> > > >> fs/xfs/xfs_log.c | 19 ++++++++
> > > >> fs/xfs/xfs_log.h | 2 +
> > > >> 4 files changed, 129 insertions(+)
> > > >>
> > > >> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > > >> index 1cfd5bc6520a..a350966cce99 100644
> > > >> --- a/fs/xfs/libxfs/xfs_fs.h
> > > >> +++ b/fs/xfs/libxfs/xfs_fs.h
> > > >> @@ -831,6 +831,7 @@ struct xfs_scrub_metadata {
> > > >> #define XFS_IOC_FSGEOMETRY _IOR ('X', 126, struct xfs_fsop_geom)
> > > >> #define XFS_IOC_BULKSTAT _IOR ('X', 127, struct xfs_bulkstat_req)
> > > >> #define XFS_IOC_INUMBERS _IOR ('X', 128, struct xfs_inumbers_req)
> > > >> +#define XFS_IOC_SETFSUUID _IOR ('X', 129, uuid_t)
> > > >
> > > > Should be _IOW.
> > >
> > > Ok, will fix that.
> > > >
> > > > Would you consider defining that as FS_IOC_SETFSUUID in fs.h,
> > > > so that other fs could implement it later on, instead of hoisting it later?
> > > >
> > > > It would be easy to add support for FS_IOC_SETFSUUID to ext4
> > > > by generalizing ext4_ioctl_setuuid().
> > > >
> > > > Alternatively, we could hoist EXT4_IOC_SETFSUUID and struct fsuuid
> > > > to fs.h and use that ioctl also for xfs.
> > >
> > > I actually did try to hoist the ext4 ioctls previously, but we weren’t able to come
> > > to a consensus on the implementation.
> > >
> > > https://lore.kernel.org/linux-xfs/20221118211408.72796-2-catherine.hoang@xxxxxxxxxx/
> > >
> > > I would prefer to keep this defined as an xfs specific ioctl to avoid all of the
> > > fsdevel bikeshedding.
> >
> > For the greater good, please do try to have this bikeshedding, before giving up.
> > The discussion you pointed to wasn't so far from consensus IMO except
> > fsdevel was not CCed.
>
> Why? fsdevel bikeshedding is a pointless waste of time. Jeremy ran
[+ linux-api]
I do not think that it was a waste of time at all...
> four rounds of proposing the new api on linux-api, linux-fsdevel, and
> linux-ext4. Matthew Wilcox and I sent in our comments, including adding
> some flexibility for shorter or longer uuids, so he updated the proposal
> and it got merged:
>
> https://lore.kernel.org/linux-api/?q=Bongio
>
> The instant Catherine started talking about using this new API, Dave
> came in and said no, flex arrays for uuids are stupid, and told
> Catherine she ought to "fix" the landmines by changing the structure
> definition:
>
> https://lore.kernel.org/linux-xfs/20221121211437.GK3600936@xxxxxxxxxxxxxxxxxxx/
>
> Never mind that changing the struct size causes the output of _IOR to
> change, which means a new ioctl command number, which is effectively a
> new interface. I think we'll just put new ioctls in xfs_fs_staging.h,
> merge the code, let people kick the tires for a few months, and only
> then make it permanent.
>
What you perceive as a waste of time, I perceive as a healthy process.
This is what I see when reading the threads:
- Serious and responsible API discussion with several important review
inputs incorporated.
- New API was added as "staging" in ext4 only
- Less than 1 year later, another fs wants to use the new API
- Dave (who may have missed the original API discussion?) points out a problem
- In my understanding, in the original discussion there was a consensus
that uuid size is limited to 16 bytes [1] so the fact that fsu_uuid[]
ended up as a variable array is just a human mistake?
[1] https://lore.kernel.org/linux-api/YthI9qp+VeNbFQP3@xxxxxxxxxxxxxxxxxxxx/
So we had a design discussion, we had a staging API and we found a problem
in the staging API. This is what I call a healthy process.
When that happens it is not too late to fix the API problem.
and the fix is simple - increase the size of the struct to maximal uuid size
and change the ioctl numbers.
Yes, ext4 tools and kernel driver have to pay the penalty of backward compat
support for the V1 API, but as I showed in the sketch patch, that will be
quite easy to do and that is the price that one has to pay for being a
pioneer ;)
So for a workplan, Catherine can use the extended fsuuid struct and add it to
staging in xfs as you proposed, with the intention of hoisting it to
fs.h later on.
And that plan should be published to linux-api and linux-fsdevel.
Assuming that ext4 developers are fine with this plan, they could already
adjust ext4 and e2fsprogs to the V2 API before being hoisted, because the
adjustments are pretty simple.
Thanks,
Amir.
