| Message ID | 20230314042109.82161-4-catherine.hoang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | setting uuid of online filesystems | expand |
On Tue, Mar 14, 2023 at 6:27 AM Catherine Hoang <catherine.hoang@oracle.com> wrote: > > Add a new ioctl to set the uuid of a mounted filesystem. > > Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com> > --- > 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. 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. Using an extensible struct with flags for that ioctl may turn out to be useful, for example, to verify that the new uuid is unique, despite the fact that xfs was mounted with -onouuid (useful IMO) or to explicitly request a restore of old uuid that would fail if new_uuid != meta uuid. Thanks, Amir.
> On Mar 13, 2023, at 10:50 PM, Amir Goldstein <amir73il@gmail.com> wrote: > > On Tue, Mar 14, 2023 at 6:27 AM Catherine Hoang > <catherine.hoang@oracle.com> wrote: >> >> Add a new ioctl to set the uuid of a mounted filesystem. >> >> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com> >> --- >> 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@oracle.com/ I would prefer to keep this defined as an xfs specific ioctl to avoid all of the fsdevel bikeshedding. > > Using an extensible struct with flags for that ioctl may turn out to be useful, > for example, to verify that the new uuid is unique, despite the fact > that xfs was > mounted with -onouuid (useful IMO) or to explicitly request a restore of old > uuid that would fail if new_uuid != meta uuid. I think using a struct is probably a good idea, I can add that in the next version. > > Thanks, > Amir.
On Thu, Mar 16, 2023 at 1:13 AM Catherine Hoang <catherine.hoang@oracle.com> wrote: > > > On Mar 13, 2023, at 10:50 PM, Amir Goldstein <amir73il@gmail.com> wrote: > > > > On Tue, Mar 14, 2023 at 6:27 AM Catherine Hoang > > <catherine.hoang@oracle.com> wrote: > >> > >> Add a new ioctl to set the uuid of a mounted filesystem. > >> > >> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com> > >> --- > >> 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@oracle.com/ > > 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. > > > > Using an extensible struct with flags for that ioctl may turn out to be useful, > > for example, to verify that the new uuid is unique, despite the fact > > that xfs was > > mounted with -onouuid (useful IMO) or to explicitly request a restore of old > > uuid that would fail if new_uuid != meta uuid. > > I think using a struct is probably a good idea, I can add that in the next version. Well, if you agree about a struct and agree about flags then the only thing left is Dave's concern about variable size arrays in ioctl and that could be addressed in a way that is compatible with ext4. See untested patch below. Thanks, Amir. diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h index b7b56871029c..143a4735486e 100644 --- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h @@ -215,6 +215,17 @@ struct fsxattr { #define FS_IOC_FSSETXATTR _IOW('X', 32, struct fsxattr) #define FS_IOC_GETFSLABEL _IOR(0x94, 49, char[FSLABEL_MAX]) #define FS_IOC_SETFSLABEL _IOW(0x94, 50, char[FSLABEL_MAX]) +#define FS_IOC_GETFSUUID _IOR('v', 44, struct fsuuid) +#define FS_IOC_SETFSUUID _IOW('v', 44, struct fsuuid) + +/* + * Structure for FS_IOC_GETFSUUID/FS_IOC_SETFSUUID + */ +struct fsuuid { + __u32 fsu_len; /* for backward compat has to be 16 */ + __u32 fsu_flags; + __u8 fsu_uuid[16]; +}; diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 140e1eb300d1..c4ded5d5e421 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -722,8 +722,8 @@ enum { #define EXT4_IOC_GETSTATE _IOW('f', 41, __u32) #define EXT4_IOC_GET_ES_CACHE _IOWR('f', 42, struct fiemap) #define EXT4_IOC_CHECKPOINT _IOW('f', 43, __u32) -#define EXT4_IOC_GETFSUUID _IOR('f', 44, struct fsuuid) -#define EXT4_IOC_SETFSUUID _IOW('f', 44, struct fsuuid) +#define EXT4_IOC_GETFSUUID _IOR('f', 44, struct fsuuid_bdr) +#define EXT4_IOC_SETFSUUID _IOW('f', 44, struct fsuuid_hdr) #define EXT4_IOC_SHUTDOWN _IOR ('X', 125, __u32) @@ -756,7 +756,7 @@ enum { /* * Structure for EXT4_IOC_GETFSUUID/EXT4_IOC_SETFSUUID */ -struct fsuuid { +struct fsuuid_hdr { __u32 fsu_len; __u32 fsu_flags; __u8 fsu_uuid[]; diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c index 8067ccda34e4..fc744231ad24 100644 --- a/fs/ext4/ioctl.c +++ b/fs/ext4/ioctl.c @@ -1149,7 +1149,7 @@ static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi, struct fsuuid fsuuid; __u8 uuid[UUID_SIZE]; - if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid))) + if (copy_from_user(&fsuuid, ufsuuid, offsetof(fsuuid, fsu_uuid))) return -EFAULT; if (fsuuid.fsu_len == 0) { @@ -1168,7 +1168,7 @@ static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi, unlock_buffer(sbi->s_sbh); fsuuid.fsu_len = UUID_SIZE; - if (copy_to_user(ufsuuid, &fsuuid, sizeof(fsuuid)) || + if (copy_to_user(ufsuuid, &fsuuid, offsetof(fsuuid, fsu_uuid)) || copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE)) return -EFAULT; return 0; @@ -1194,7 +1194,7 @@ static int ext4_ioctl_setuuid(struct file *filp, || ext4_has_feature_stable_inodes(sb)) return -EOPNOTSUPP; - if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid))) + if (copy_from_user(&fsuuid, ufsuuid, offsetof(fsuuid, fsu_uuid))) return -EFAULT; if (fsuuid.fsu_len != UUID_SIZE || fsuuid.fsu_flags != 0) @@ -1596,8 +1596,10 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) return ext4_ioctl_setlabel(filp, (const void __user *)arg); + case FS_IOC_GETFSUUID: case EXT4_IOC_GETFSUUID: return ext4_ioctl_getuuid(EXT4_SB(sb), (void __user *)arg); + case FS_IOC_SETFSUUID: case EXT4_IOC_SETFSUUID: return ext4_ioctl_setuuid(filp, (const void __user *)arg); default:
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@oracle.com> wrote: > > > > > On Mar 13, 2023, at 10:50 PM, Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > On Tue, Mar 14, 2023 at 6:27 AM Catherine Hoang > > > <catherine.hoang@oracle.com> wrote: > > >> > > >> Add a new ioctl to set the uuid of a mounted filesystem. > > >> > > >> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com> > > >> --- > > >> 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@oracle.com/ > > > > 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 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/ 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. Though really, that's the least of the problems, especially since Dave had a pretty good list of questions elsewhere in this thread. --D > > > > > > Using an extensible struct with flags for that ioctl may turn out to be useful, > > > for example, to verify that the new uuid is unique, despite the fact > > > that xfs was > > > mounted with -onouuid (useful IMO) or to explicitly request a restore of old > > > uuid that would fail if new_uuid != meta uuid. > > > > I think using a struct is probably a good idea, I can add that in the next version. > > Well, if you agree about a struct and agree about flags then the only thing > left is Dave's concern about variable size arrays in ioctl and that could be > addressed in a way that is compatible with ext4. > > See untested patch below. > > Thanks, > Amir. > > diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h > index b7b56871029c..143a4735486e 100644 > --- a/include/uapi/linux/fs.h > +++ b/include/uapi/linux/fs.h > @@ -215,6 +215,17 @@ struct fsxattr { > #define FS_IOC_FSSETXATTR _IOW('X', 32, struct fsxattr) > #define FS_IOC_GETFSLABEL _IOR(0x94, 49, char[FSLABEL_MAX]) > #define FS_IOC_SETFSLABEL _IOW(0x94, 50, char[FSLABEL_MAX]) > +#define FS_IOC_GETFSUUID _IOR('v', 44, struct fsuuid) > +#define FS_IOC_SETFSUUID _IOW('v', 44, struct fsuuid) > + > +/* > + * Structure for FS_IOC_GETFSUUID/FS_IOC_SETFSUUID > + */ > +struct fsuuid { > + __u32 fsu_len; /* for backward compat has to be 16 */ > + __u32 fsu_flags; > + __u8 fsu_uuid[16]; Um, these two ioctls have different namespace /and/ different structure sizes. This is literally defining a new interface. > +}; > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 140e1eb300d1..c4ded5d5e421 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -722,8 +722,8 @@ enum { > #define EXT4_IOC_GETSTATE _IOW('f', 41, __u32) > #define EXT4_IOC_GET_ES_CACHE _IOWR('f', 42, struct fiemap) > #define EXT4_IOC_CHECKPOINT _IOW('f', 43, __u32) > -#define EXT4_IOC_GETFSUUID _IOR('f', 44, struct fsuuid) > -#define EXT4_IOC_SETFSUUID _IOW('f', 44, struct fsuuid) > +#define EXT4_IOC_GETFSUUID _IOR('f', 44, struct fsuuid_bdr) > +#define EXT4_IOC_SETFSUUID _IOW('f', 44, struct fsuuid_hdr) > > #define EXT4_IOC_SHUTDOWN _IOR ('X', 125, __u32) > > @@ -756,7 +756,7 @@ enum { > /* > * Structure for EXT4_IOC_GETFSUUID/EXT4_IOC_SETFSUUID > */ > -struct fsuuid { > +struct fsuuid_hdr { > __u32 fsu_len; > __u32 fsu_flags; > __u8 fsu_uuid[]; > > diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c > index 8067ccda34e4..fc744231ad24 100644 > --- a/fs/ext4/ioctl.c > +++ b/fs/ext4/ioctl.c > @@ -1149,7 +1149,7 @@ static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi, > struct fsuuid fsuuid; > __u8 uuid[UUID_SIZE]; > > - if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid))) > + if (copy_from_user(&fsuuid, ufsuuid, offsetof(fsuuid, fsu_uuid))) > return -EFAULT; > > if (fsuuid.fsu_len == 0) { > @@ -1168,7 +1168,7 @@ static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi, > unlock_buffer(sbi->s_sbh); > > fsuuid.fsu_len = UUID_SIZE; > - if (copy_to_user(ufsuuid, &fsuuid, sizeof(fsuuid)) || > + if (copy_to_user(ufsuuid, &fsuuid, offsetof(fsuuid, fsu_uuid)) || > copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE)) > return -EFAULT; > return 0; > @@ -1194,7 +1194,7 @@ static int ext4_ioctl_setuuid(struct file *filp, > || ext4_has_feature_stable_inodes(sb)) > return -EOPNOTSUPP; > > - if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid))) > + if (copy_from_user(&fsuuid, ufsuuid, offsetof(fsuuid, fsu_uuid))) > return -EFAULT; > > if (fsuuid.fsu_len != UUID_SIZE || fsuuid.fsu_flags != 0) > @@ -1596,8 +1596,10 @@ static long __ext4_ioctl(struct file *filp, > unsigned int cmd, unsigned long arg) > return ext4_ioctl_setlabel(filp, > (const void __user *)arg); > > + case FS_IOC_GETFSUUID: > case EXT4_IOC_GETFSUUID: > return ext4_ioctl_getuuid(EXT4_SB(sb), (void __user *)arg); > + case FS_IOC_SETFSUUID: > case EXT4_IOC_SETFSUUID: > return ext4_ioctl_setuuid(filp, (const void __user *)arg); > default:
On Sat, Mar 18, 2023 at 2:39 AM Darrick J. Wong <djwong@kernel.org> 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@oracle.com> wrote: > > > > > > > On Mar 13, 2023, at 10:50 PM, Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > > > On Tue, Mar 14, 2023 at 6:27 AM Catherine Hoang > > > > <catherine.hoang@oracle.com> wrote: > > > >> > > > >> Add a new ioctl to set the uuid of a mounted filesystem. > > > >> > > > >> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com> > > > >> --- > > > >> 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@oracle.com/ > > > > > > 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/ > > 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/ 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.
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) /* XFS_IOC_GETFSUUID ---------- deprecated 140 */ diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 55bb01173cde..f0699a7169e4 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -38,6 +38,7 @@ #include "xfs_reflink.h" #include "xfs_ioctl.h" #include "xfs_xattr.h" +#include "xfs_log.h" #include <linux/mount.h> #include <linux/namei.h> @@ -1861,6 +1862,109 @@ xfs_fs_eofblocks_from_user( return 0; } +static int +xfs_ioc_setfsuuid( + struct file *filp, + struct xfs_mount *mp, + uuid_t __user *uuid) +{ + uuid_t old_uuid; + uuid_t new_uuid; + uuid_t *forget_uuid = NULL; + int error; + + if (!capable(CAP_SYS_ADMIN)) + return -EPERM; + + if (!xfs_sb_is_v5(&mp->m_sb)) + return -EOPNOTSUPP; + + if (copy_from_user(&new_uuid, uuid, sizeof(uuid_t))) + return -EFAULT; + if (uuid_is_null(&new_uuid)) + return -EINVAL; + + /* Check that the uuid is unique and save a slot in the uuid table. */ + if (!(xfs_has_nouuid(mp))) { + error = xfs_uuid_remember(&new_uuid); + if (error) + return error; + forget_uuid = &new_uuid; + } + + error = xfs_internal_freeze(mp); + if (error) + goto out_drop_uuid; + + spin_lock(&mp->m_sb_lock); + uuid_copy(&old_uuid, &mp->m_sb.sb_uuid); + + /* + * On a v5 filesystem, every metadata object has a uuid stamped into + * the header. The particular uuid used is either sb_uuid or + * sb_meta_uuid, depending on whether the meta_uuid feature is set. + * + * If the meta_uuid feature is set: + * - The user visible uuid is set in sb_uuid + * - The uuid used for metadata blocks is set in sb_meta_uuid + * - If new_uuid == sb_meta_uuid, then we'll deactivate the feature + * and set sb_uuid to the new uuid + * + * If the meta_uuid feature is not set: + * - The user visible uuid is set in sb_uuid + * - The uuid used for meta blocks should match sb_uuid + * - If new_uuid != sb_uuid, we need to copy sb_uuid to sb_meta_uuid, + * set the meta_uuid feature bit, and set sb_uuid to the new uuid + */ + if (xfs_has_metauuid(mp) && + uuid_equal(&new_uuid, &mp->m_sb.sb_meta_uuid)) { + mp->m_sb.sb_features_incompat &= ~XFS_SB_FEAT_INCOMPAT_META_UUID; + mp->m_features &= ~XFS_FEAT_META_UUID; + } else if (!xfs_has_metauuid(mp) && + !uuid_equal(&new_uuid, &mp->m_sb.sb_uuid)) { + uuid_copy(&mp->m_sb.sb_meta_uuid, &mp->m_sb.sb_uuid); + mp->m_sb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_META_UUID; + mp->m_features |= XFS_FEAT_META_UUID; + } + + uuid_copy(&mp->m_sb.sb_uuid, &new_uuid); + spin_unlock(&mp->m_sb_lock); + + xlog_iclog_update_uuid(mp); + + xfs_buf_lock(mp->m_sb_bp); + xfs_buf_hold(mp->m_sb_bp); + + xfs_sb_to_disk(mp->m_sb_bp->b_addr, &mp->m_sb); + error = xfs_bwrite(mp->m_sb_bp); + xfs_buf_relse(mp->m_sb_bp); + if (error) + goto out_drop_freeze; + + /* Update incore state and prepare to drop the old uuid. */ + uuid_copy(&mp->m_super->s_uuid, &new_uuid); + if (!(xfs_has_nouuid(mp))) + forget_uuid = &old_uuid; + + /* + * Update the secondary supers, being aware that growfs also updates + * backup supers so we need to lock against that. + */ + mutex_lock(&mp->m_growlock); + error = xfs_update_secondary_sbs(mp); + mutex_unlock(&mp->m_growlock); + + invalidate_bdev(mp->m_ddev_targp->bt_bdev); + xfs_log_clean(mp); + +out_drop_freeze: + xfs_internal_unfreeze(mp); +out_drop_uuid: + if (forget_uuid) + xfs_uuid_forget(forget_uuid); + return error; +} + /* * These long-unused ioctls were removed from the official ioctl API in 5.17, * but retain these definitions so that we can log warnings about them. @@ -2149,6 +2253,9 @@ xfs_file_ioctl( return error; } + case XFS_IOC_SETFSUUID: + return xfs_ioc_setfsuuid(filp, mp, arg); + default: return -ENOTTY; } diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index fc61cc024023..d79b6065ee9c 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -3921,3 +3921,22 @@ xlog_drop_incompat_feat( { up_read(&log->l_incompat_users); } + +/* + * Cycle all the iclog buffers and update the uuid. + */ +void +xlog_iclog_update_uuid( + struct xfs_mount *mp) +{ + int i; + struct xlog *log = mp->m_log; + struct xlog_in_core *iclog = log->l_iclog; + xlog_rec_header_t *head; + + for (i = 0; i < log->l_iclog_bufs; i++) { + head = &iclog->ic_header; + memcpy(&head->h_fs_uuid, &mp->m_sb.sb_uuid, sizeof(uuid_t)); + iclog = iclog->ic_next; + } +} diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h index 2728886c2963..6b607619163e 100644 --- a/fs/xfs/xfs_log.h +++ b/fs/xfs/xfs_log.h @@ -163,4 +163,6 @@ void xlog_use_incompat_feat(struct xlog *log); void xlog_drop_incompat_feat(struct xlog *log); int xfs_attr_use_log_assist(struct xfs_mount *mp); +void xlog_iclog_update_uuid(struct xfs_mount *mp); + #endif /* __XFS_LOG_H__ */
Add a new ioctl to set the uuid of a mounted filesystem. Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com> --- 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(+)