Re: [PATCH v1 3/4] xfs: add XFS_IOC_SETFSUUID ioctl

On Sat, Mar 18, 2023 at 2:39 AM Darrick J. Wong [off-list ref] wrote:

On Thu, Mar 16, 2023 at 10:09:56AM +0200, Amir Goldstein wrote:

quoted

On Thu, Mar 16, 2023 at 1:13 AM Catherine Hoang
[off-list ref] wrote:

quoted

quoted

On Mar 13, 2023, at 10:50 PM, Amir Goldstein [off-list ref] wrote:

On Tue, Mar 14, 2023 at 6:27 AM Catherine Hoang
[off-list ref] wrote:

quoted

Add a new ioctl to set the uuid of a mounted filesystem.

Signed-off-by: Catherine Hoang <redacted>
---
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.

quoted

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@oracle.com/ (local)

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@dread.disaster.area/ (local)

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@casper.infradead.org/ (local)

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.