| Message ID | 20240206201858.952303-4-kent.overstreet@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | filesystem visibililty ioctls | expand |
On 2/6/24 12:18, Kent Overstreet wrote: > Add a new generic ioctls for querying the filesystem UUID. > > These are lifted versions of the ext4 ioctls, with one change: we're not > using a flexible array member, because UUIDs will never be more than 16 > bytes. > > This patch adds a generic implementation of FS_IOC_GETFSUUID, which > reads from super_block->s_uuid. We're not lifting SETFSUUID from ext4 - > that can be done on offline filesystems by the people who need it, > trying to do it online is just asking for too much trouble. > > Cc: Christian Brauner <brauner@kernel.org> > Cc: Jan Kara <jack@suse.cz> > Cc: Dave Chinner <dchinner@redhat.com> > Cc: "Darrick J. Wong" <djwong@kernel.org> > Cc: Theodore Ts'o <tytso@mit.edu> > Cc: linux-fsdevel@vger.kernel.or > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > --- > fs/ioctl.c | 16 ++++++++++++++++ > include/uapi/linux/fs.h | 17 +++++++++++++++++ > 2 files changed, 33 insertions(+) > > diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h > index 48ad69f7722e..16a6ecadfd8d 100644 > --- a/include/uapi/linux/fs.h > +++ b/include/uapi/linux/fs.h > @@ -64,6 +64,19 @@ struct fstrim_range { > __u64 minlen; > }; > > +/* > + * We include a length field because some filesystems (vfat) have an identifier > + * that we do want to expose as a UUID, but doesn't have the standard length. > + * > + * We use a fixed size buffer beacuse this interface will, by fiat, never because > + * support "UUIDs" longer than 16 bytes; we don't want to force all downstream > + * users to have to deal with that. > + */ > +struct fsuuid2 { > + __u8 len; > + __u8 uuid[16]; > +}; > + > /* extent-same (dedupe) ioctls; these MUST match the btrfs ioctl definitions */ > #define FILE_DEDUPE_RANGE_SAME 0 > #define FILE_DEDUPE_RANGE_DIFFERS 1 > @@ -190,6 +203,9 @@ struct fsxattr { > * (see uapi/linux/blkzoned.h) > */ > > +/* Returns the external filesystem UUID, the same one blkid returns */ > +#define FS_IOC_GETFSUUID _IOR(0x12, 142, struct fsuuid2) > + > #define BMAP_IOCTL 1 /* obsolete - kept for compatibility */ > #define FIBMAP _IO(0x00,1) /* bmap access */ > #define FIGETBSZ _IO(0x00,2) /* get the block size used for bmap */ > @@ -198,6 +214,7 @@ struct fsxattr { > #define FITRIM _IOWR('X', 121, struct fstrim_range) /* Trim */ > #define FICLONE _IOW(0x94, 9, int) > #define FICLONERANGE _IOW(0x94, 13, struct file_clone_range) > + > #define FIDEDUPERANGE _IOWR(0x94, 54, struct file_dedupe_range) Why the additional blank line? (nit) > > #define FSLABEL_MAX 256 /* Max chars for the interface; each fs may differ */
On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote: > Add a new generic ioctls for querying the filesystem UUID. > > These are lifted versions of the ext4 ioctls, with one change: we're not > using a flexible array member, because UUIDs will never be more than 16 > bytes. > > This patch adds a generic implementation of FS_IOC_GETFSUUID, which > reads from super_block->s_uuid. We're not lifting SETFSUUID from ext4 - > that can be done on offline filesystems by the people who need it, > trying to do it online is just asking for too much trouble. > > Cc: Christian Brauner <brauner@kernel.org> > Cc: Jan Kara <jack@suse.cz> > Cc: Dave Chinner <dchinner@redhat.com> > Cc: "Darrick J. Wong" <djwong@kernel.org> > Cc: Theodore Ts'o <tytso@mit.edu> > Cc: linux-fsdevel@vger.kernel.or > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > --- > fs/ioctl.c | 16 ++++++++++++++++ > include/uapi/linux/fs.h | 17 +++++++++++++++++ > 2 files changed, 33 insertions(+) > > diff --git a/fs/ioctl.c b/fs/ioctl.c > index 76cf22ac97d7..046c30294a82 100644 > --- a/fs/ioctl.c > +++ b/fs/ioctl.c > @@ -763,6 +763,19 @@ static int ioctl_fssetxattr(struct file *file, void __user *argp) > return err; > } > > +static int ioctl_getfsuuid(struct file *file, void __user *argp) > +{ > + struct super_block *sb = file_inode(file)->i_sb; > + > + if (!sb->s_uuid_len) > + return -ENOIOCTLCMD; > + > + struct fsuuid2 u = { .len = sb->s_uuid_len, }; > + memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len); > + > + return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0; > +} Can we please keep the declarations separate from the code? I always find this sort of implicit scoping of variables both difficult to read (especially in larger functions) and a landmine waiting to be tripped over. This could easily just be: static int ioctl_getfsuuid(struct file *file, void __user *argp) { struct super_block *sb = file_inode(file)->i_sb; struct fsuuid2 u = { .len = sb->s_uuid_len, }; .... and then it's consistent with all the rest of the code... > diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h > index 48ad69f7722e..16a6ecadfd8d 100644 > --- a/include/uapi/linux/fs.h > +++ b/include/uapi/linux/fs.h > @@ -64,6 +64,19 @@ struct fstrim_range { > __u64 minlen; > }; > > +/* > + * We include a length field because some filesystems (vfat) have an identifier > + * that we do want to expose as a UUID, but doesn't have the standard length. > + * > + * We use a fixed size buffer beacuse this interface will, by fiat, never > + * support "UUIDs" longer than 16 bytes; we don't want to force all downstream > + * users to have to deal with that. > + */ > +struct fsuuid2 { > + __u8 len; > + __u8 uuid[16]; > +}; > + > /* extent-same (dedupe) ioctls; these MUST match the btrfs ioctl definitions */ > #define FILE_DEDUPE_RANGE_SAME 0 > #define FILE_DEDUPE_RANGE_DIFFERS 1 > @@ -190,6 +203,9 @@ struct fsxattr { > * (see uapi/linux/blkzoned.h) > */ > > +/* Returns the external filesystem UUID, the same one blkid returns */ > +#define FS_IOC_GETFSUUID _IOR(0x12, 142, struct fsuuid2) > + Can you add a comment somewhere in the file saying that new VFS ioctls should use the "0x12" namespace in the range 142-255, and mention that BLK ioctls should be kept within the 0x12 {0-141} range? Probably also document this clearly in Documentation/userspace-api/ioctl/ioctl-number.rst, too? -Dave.
On Wed, Feb 07, 2024 at 09:01:05AM +1100, Dave Chinner wrote: > On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote: > > +static int ioctl_getfsuuid(struct file *file, void __user *argp) > > +{ > > + struct super_block *sb = file_inode(file)->i_sb; > > + > > + if (!sb->s_uuid_len) > > + return -ENOIOCTLCMD; > > + > > + struct fsuuid2 u = { .len = sb->s_uuid_len, }; > > + memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len); > > + > > + return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0; > > +} > > Can we please keep the declarations separate from the code? I always > find this sort of implicit scoping of variables both difficult to > read (especially in larger functions) and a landmine waiting to be > tripped over. This could easily just be: > > static int ioctl_getfsuuid(struct file *file, void __user *argp) > { > struct super_block *sb = file_inode(file)->i_sb; > struct fsuuid2 u = { .len = sb->s_uuid_len, }; > > .... > > and then it's consistent with all the rest of the code... The way I'm doing it here is actually what I'm transitioning my own code to - the big reason being that always declaring variables at the tops of functions leads to separating declaration and initialization, and worse it leads people to declaring a variable once and reusing it for multiple things (I've seen that be a source of real bugs too many times). But I won't push that in this patch, we can just keep the style consistent for now. > > +/* Returns the external filesystem UUID, the same one blkid returns */ > > +#define FS_IOC_GETFSUUID _IOR(0x12, 142, struct fsuuid2) > > + > > Can you add a comment somewhere in the file saying that new VFS > ioctls should use the "0x12" namespace in the range 142-255, and > mention that BLK ioctls should be kept within the 0x12 {0-141} > range? Well, if we're going to try to keep the BLK_ and FS_IOC_ ioctls in separate ranges, then FS_IOC_ needs to move to something else becasue otherwise BLK_ won't have a way to expand. So what else - ioctl-number.rst has a bunch of other ranges listed for fs.h, but 0x12 appears to be the only one without conflicts - all the other ranges seem to have originated with other filesystems. So perhaps I will take Darrick's nak (0x15) suggestion after all.
On Tue, Feb 06, 2024 at 05:37:22PM -0500, Kent Overstreet wrote: > On Wed, Feb 07, 2024 at 09:01:05AM +1100, Dave Chinner wrote: > > On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote: > > > +static int ioctl_getfsuuid(struct file *file, void __user *argp) > > > +{ > > > + struct super_block *sb = file_inode(file)->i_sb; > > > + > > > + if (!sb->s_uuid_len) > > > + return -ENOIOCTLCMD; > > > + > > > + struct fsuuid2 u = { .len = sb->s_uuid_len, }; > > > + memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len); > > > + > > > + return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0; > > > +} > > > > Can we please keep the declarations separate from the code? I always > > find this sort of implicit scoping of variables both difficult to > > read (especially in larger functions) and a landmine waiting to be > > tripped over. This could easily just be: > > > > static int ioctl_getfsuuid(struct file *file, void __user *argp) > > { > > struct super_block *sb = file_inode(file)->i_sb; > > struct fsuuid2 u = { .len = sb->s_uuid_len, }; > > > > .... > > > > and then it's consistent with all the rest of the code... > > The way I'm doing it here is actually what I'm transitioning my own code > to - the big reason being that always declaring variables at the tops of > functions leads to separating declaration and initialization, and worse > it leads people to declaring a variable once and reusing it for multiple > things (I've seen that be a source of real bugs too many times). > > But I won't push that in this patch, we can just keep the style > consistent for now. > > > > +/* Returns the external filesystem UUID, the same one blkid returns */ > > > +#define FS_IOC_GETFSUUID _IOR(0x12, 142, struct fsuuid2) > > > + > > > > Can you add a comment somewhere in the file saying that new VFS > > ioctls should use the "0x12" namespace in the range 142-255, and > > mention that BLK ioctls should be kept within the 0x12 {0-141} > > range? > > Well, if we're going to try to keep the BLK_ and FS_IOC_ ioctls in > separate ranges, then FS_IOC_ needs to move to something else becasue > otherwise BLK_ won't have a way to expand. The BLK range can grow downwards towards zero, I think. It starts at 93 and goes to 136, so there's heaps of space for it to grow from 92 to 0.... > So what else - > > ioctl-number.rst has a bunch of other ranges listed for fs.h, but 0x12 > appears to be the only one without conflicts - all the other ranges seem > to have originated with other filesystems. *nod* > So perhaps I will take Darrick's nak (0x15) suggestion after all. That works, too. -Dave.
On Tue, Feb 06, 2024 at 05:37:22PM -0500, Kent Overstreet wrote: > On Wed, Feb 07, 2024 at 09:01:05AM +1100, Dave Chinner wrote: > > On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote: > > > +static int ioctl_getfsuuid(struct file *file, void __user *argp) > > > +{ > > > + struct super_block *sb = file_inode(file)->i_sb; > > > + > > > + if (!sb->s_uuid_len) > > > + return -ENOIOCTLCMD; > > > + > > > + struct fsuuid2 u = { .len = sb->s_uuid_len, }; > > > + memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len); > > > + > > > + return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0; > > > +} > > > > Can we please keep the declarations separate from the code? I always > > find this sort of implicit scoping of variables both difficult to > > read (especially in larger functions) and a landmine waiting to be > > tripped over. This could easily just be: > > > > static int ioctl_getfsuuid(struct file *file, void __user *argp) > > { > > struct super_block *sb = file_inode(file)->i_sb; > > struct fsuuid2 u = { .len = sb->s_uuid_len, }; > > > > .... > > > > and then it's consistent with all the rest of the code... > > The way I'm doing it here is actually what I'm transitioning my own code > to - the big reason being that always declaring variables at the tops of > functions leads to separating declaration and initialization, and worse > it leads people to declaring a variable once and reusing it for multiple > things (I've seen that be a source of real bugs too many times). > I still think this is of questionable value. I know I've mentioned similar concerns to Dave's here on the bcachefs list, but still have not really seen any discussion other than a bit of back and forth on the handful of generally accepted (in the kernel) uses of this sort of thing for limiting scope in loops/branches and such. I was skimming through some more recent bcachefs patches the other day (the journal write pipelining stuff) where I came across one or two medium length functions where this had proliferated, and I found it kind of annoying TBH. It starts to almost look like there are casts all over the place and it's a bit more tedious to filter out logic from the additional/gratuitous syntax, IMO. That's still just my .02, but there was also previous mention of starting/having discussion on this sort of style change. Is that still the plan? If so, before or after proliferating it throughout the bcachefs code? ;) I am curious if there are other folks in kernel land who think this makes enough sense that they'd plan to adopt it. Hm? Brian > But I won't push that in this patch, we can just keep the style > consistent for now. > > > > +/* Returns the external filesystem UUID, the same one blkid returns */ > > > +#define FS_IOC_GETFSUUID _IOR(0x12, 142, struct fsuuid2) > > > + > > > > Can you add a comment somewhere in the file saying that new VFS > > ioctls should use the "0x12" namespace in the range 142-255, and > > mention that BLK ioctls should be kept within the 0x12 {0-141} > > range? > > Well, if we're going to try to keep the BLK_ and FS_IOC_ ioctls in > separate ranges, then FS_IOC_ needs to move to something else becasue > otherwise BLK_ won't have a way to expand. > > So what else - > > ioctl-number.rst has a bunch of other ranges listed for fs.h, but 0x12 > appears to be the only one without conflicts - all the other ranges seem > to have originated with other filesystems. > > So perhaps I will take Darrick's nak (0x15) suggestion after all. >
On Wed, Feb 07, 2024 at 08:05:29AM -0500, Brian Foster wrote: > On Tue, Feb 06, 2024 at 05:37:22PM -0500, Kent Overstreet wrote: > > On Wed, Feb 07, 2024 at 09:01:05AM +1100, Dave Chinner wrote: > > > On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote: > > > > +static int ioctl_getfsuuid(struct file *file, void __user *argp) > > > > +{ > > > > + struct super_block *sb = file_inode(file)->i_sb; > > > > + > > > > + if (!sb->s_uuid_len) > > > > + return -ENOIOCTLCMD; > > > > + > > > > + struct fsuuid2 u = { .len = sb->s_uuid_len, }; > > > > + memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len); > > > > + > > > > + return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0; > > > > +} > > > > > > Can we please keep the declarations separate from the code? I always > > > find this sort of implicit scoping of variables both difficult to > > > read (especially in larger functions) and a landmine waiting to be > > > tripped over. This could easily just be: > > > > > > static int ioctl_getfsuuid(struct file *file, void __user *argp) > > > { > > > struct super_block *sb = file_inode(file)->i_sb; > > > struct fsuuid2 u = { .len = sb->s_uuid_len, }; > > > > > > .... > > > > > > and then it's consistent with all the rest of the code... > > > > The way I'm doing it here is actually what I'm transitioning my own code > > to - the big reason being that always declaring variables at the tops of > > functions leads to separating declaration and initialization, and worse > > it leads people to declaring a variable once and reusing it for multiple > > things (I've seen that be a source of real bugs too many times). > > > > I still think this is of questionable value. I know I've mentioned > similar concerns to Dave's here on the bcachefs list, but still have not > really seen any discussion other than a bit of back and forth on the > handful of generally accepted (in the kernel) uses of this sort of thing > for limiting scope in loops/branches and such. > > I was skimming through some more recent bcachefs patches the other day > (the journal write pipelining stuff) where I came across one or two > medium length functions where this had proliferated, and I found it kind > of annoying TBH. It starts to almost look like there are casts all over > the place and it's a bit more tedious to filter out logic from the > additional/gratuitous syntax, IMO. > > That's still just my .02, but there was also previous mention of > starting/having discussion on this sort of style change. Is that still > the plan? If so, before or after proliferating it throughout the > bcachefs code? ;) I am curious if there are other folks in kernel land > who think this makes enough sense that they'd plan to adopt it. Hm? That was the discussion :) bcachefs is my codebase, so yes, I intend to do it there. I really think this is an instance where you and Dave are used to the way C has historically forced us to do things; our brains get wired to read code a certain way and changes are jarring. But take a step back; if we were used to writing code the way I'm doing it, and you were arguing for putting declarations at the tops of functions, what would the arguments be? I would say you're just breaking up the flow of ideas for no reason; a chain of related statements now includes a declaration that isn't with the actual logic. And bugs due to variable reuse, missed initialization - there's real reasons not to do it that way.
On Thu, Feb 08, 2024 at 04:57:02PM -0500, Kent Overstreet wrote: > On Wed, Feb 07, 2024 at 08:05:29AM -0500, Brian Foster wrote: > > On Tue, Feb 06, 2024 at 05:37:22PM -0500, Kent Overstreet wrote: > > > On Wed, Feb 07, 2024 at 09:01:05AM +1100, Dave Chinner wrote: > > > > On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote: > > > > > +static int ioctl_getfsuuid(struct file *file, void __user *argp) > > > > > +{ > > > > > + struct super_block *sb = file_inode(file)->i_sb; > > > > > + > > > > > + if (!sb->s_uuid_len) > > > > > + return -ENOIOCTLCMD; > > > > > + > > > > > + struct fsuuid2 u = { .len = sb->s_uuid_len, }; > > > > > + memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len); > > > > > + > > > > > + return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0; > > > > > +} > > > > > > > > Can we please keep the declarations separate from the code? I always > > > > find this sort of implicit scoping of variables both difficult to > > > > read (especially in larger functions) and a landmine waiting to be > > > > tripped over. This could easily just be: > > > > > > > > static int ioctl_getfsuuid(struct file *file, void __user *argp) > > > > { > > > > struct super_block *sb = file_inode(file)->i_sb; > > > > struct fsuuid2 u = { .len = sb->s_uuid_len, }; > > > > > > > > .... > > > > > > > > and then it's consistent with all the rest of the code... > > > > > > The way I'm doing it here is actually what I'm transitioning my own code > > > to - the big reason being that always declaring variables at the tops of > > > functions leads to separating declaration and initialization, and worse > > > it leads people to declaring a variable once and reusing it for multiple > > > things (I've seen that be a source of real bugs too many times). > > > > > > > I still think this is of questionable value. I know I've mentioned > > similar concerns to Dave's here on the bcachefs list, but still have not > > really seen any discussion other than a bit of back and forth on the > > handful of generally accepted (in the kernel) uses of this sort of thing > > for limiting scope in loops/branches and such. > > > > I was skimming through some more recent bcachefs patches the other day > > (the journal write pipelining stuff) where I came across one or two > > medium length functions where this had proliferated, and I found it kind > > of annoying TBH. It starts to almost look like there are casts all over > > the place and it's a bit more tedious to filter out logic from the > > additional/gratuitous syntax, IMO. > > > > That's still just my .02, but there was also previous mention of > > starting/having discussion on this sort of style change. Is that still > > the plan? If so, before or after proliferating it throughout the > > bcachefs code? ;) I am curious if there are other folks in kernel land > > who think this makes enough sense that they'd plan to adopt it. Hm? > > That was the discussion :) > > bcachefs is my codebase, so yes, I intend to do it there. I really think > this is an instance where you and Dave are used to the way C has > historically forced us to do things; our brains get wired to read code a > certain way and changes are jarring. > Heh, fair enough. That's certainly your prerogative. I'm certainly not trying to tell you what to do or not with bcachefs. That's at least direct enough that it's clear it's not worth debating too much. ;) > But take a step back; if we were used to writing code the way I'm doing > it, and you were arguing for putting declarations at the tops of > functions, what would the arguments be? > I think my thought process would be similar. I.e., is the proposed benefit of such a change worth the tradeoffs? > I would say you're just breaking up the flow of ideas for no reason; a > chain of related statements now includes a declaration that isn't with > the actual logic. > > And bugs due to variable reuse, missed initialization - there's real > reasons not to do it that way. > And were I in that position, I don't think I would reduce a decision that affects readability/reviewability of my subsystem to a nontrivial degree (for other people, at least) to that single aspect. This would be the answer to the question: "is this worth considering?" Brian
On Mon, Feb 12, 2024 at 07:47:00AM -0500, Brian Foster wrote: > On Thu, Feb 08, 2024 at 04:57:02PM -0500, Kent Overstreet wrote: > > On Wed, Feb 07, 2024 at 08:05:29AM -0500, Brian Foster wrote: > > > On Tue, Feb 06, 2024 at 05:37:22PM -0500, Kent Overstreet wrote: > > > > On Wed, Feb 07, 2024 at 09:01:05AM +1100, Dave Chinner wrote: > > > > > On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote: > > > > > > +static int ioctl_getfsuuid(struct file *file, void __user *argp) > > > > > > +{ > > > > > > + struct super_block *sb = file_inode(file)->i_sb; > > > > > > + > > > > > > + if (!sb->s_uuid_len) > > > > > > + return -ENOIOCTLCMD; > > > > > > + > > > > > > + struct fsuuid2 u = { .len = sb->s_uuid_len, }; > > > > > > + memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len); > > > > > > + > > > > > > + return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0; > > > > > > +} > > > > > > > > > > Can we please keep the declarations separate from the code? I always > > > > > find this sort of implicit scoping of variables both difficult to > > > > > read (especially in larger functions) and a landmine waiting to be > > > > > tripped over. This could easily just be: > > > > > > > > > > static int ioctl_getfsuuid(struct file *file, void __user *argp) > > > > > { > > > > > struct super_block *sb = file_inode(file)->i_sb; > > > > > struct fsuuid2 u = { .len = sb->s_uuid_len, }; > > > > > > > > > > .... > > > > > > > > > > and then it's consistent with all the rest of the code... > > > > > > > > The way I'm doing it here is actually what I'm transitioning my own code > > > > to - the big reason being that always declaring variables at the tops of > > > > functions leads to separating declaration and initialization, and worse > > > > it leads people to declaring a variable once and reusing it for multiple > > > > things (I've seen that be a source of real bugs too many times). > > > > > > > > > > I still think this is of questionable value. I know I've mentioned > > > similar concerns to Dave's here on the bcachefs list, but still have not > > > really seen any discussion other than a bit of back and forth on the > > > handful of generally accepted (in the kernel) uses of this sort of thing > > > for limiting scope in loops/branches and such. > > > > > > I was skimming through some more recent bcachefs patches the other day > > > (the journal write pipelining stuff) where I came across one or two > > > medium length functions where this had proliferated, and I found it kind > > > of annoying TBH. It starts to almost look like there are casts all over > > > the place and it's a bit more tedious to filter out logic from the > > > additional/gratuitous syntax, IMO. > > > > > > That's still just my .02, but there was also previous mention of > > > starting/having discussion on this sort of style change. Is that still > > > the plan? If so, before or after proliferating it throughout the > > > bcachefs code? ;) I am curious if there are other folks in kernel land > > > who think this makes enough sense that they'd plan to adopt it. Hm? > > > > That was the discussion :) > > > > bcachefs is my codebase, so yes, I intend to do it there. I really think > > this is an instance where you and Dave are used to the way C has > > historically forced us to do things; our brains get wired to read code a > > certain way and changes are jarring. > > > > Heh, fair enough. That's certainly your prerogative. I'm certainly not > trying to tell you what to do or not with bcachefs. That's at least > direct enough that it's clear it's not worth debating too much. ;) > > > But take a step back; if we were used to writing code the way I'm doing > > it, and you were arguing for putting declarations at the tops of > > functions, what would the arguments be? > > > > I think my thought process would be similar. I.e., is the proposed > benefit of such a change worth the tradeoffs? > > > I would say you're just breaking up the flow of ideas for no reason; a > > chain of related statements now includes a declaration that isn't with > > the actual logic. > > > > And bugs due to variable reuse, missed initialization - there's real > > reasons not to do it that way. > > > > And were I in that position, I don't think I would reduce a decision > that affects readability/reviewability of my subsystem to a nontrivial > degree (for other people, at least) to that single aspect. This would be > the answer to the question: "is this worth considering?" If you feel this affected by this, how are you going to cope with Rust?
On Mon, Feb 12, 2024 at 08:39:29AM -0500, Kent Overstreet wrote: > On Mon, Feb 12, 2024 at 07:47:00AM -0500, Brian Foster wrote: > > On Thu, Feb 08, 2024 at 04:57:02PM -0500, Kent Overstreet wrote: > > > On Wed, Feb 07, 2024 at 08:05:29AM -0500, Brian Foster wrote: > > > > On Tue, Feb 06, 2024 at 05:37:22PM -0500, Kent Overstreet wrote: > > > > > On Wed, Feb 07, 2024 at 09:01:05AM +1100, Dave Chinner wrote: > > > > > > On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote: > > > > > > > +static int ioctl_getfsuuid(struct file *file, void __user *argp) > > > > > > > +{ > > > > > > > + struct super_block *sb = file_inode(file)->i_sb; > > > > > > > + > > > > > > > + if (!sb->s_uuid_len) > > > > > > > + return -ENOIOCTLCMD; > > > > > > > + > > > > > > > + struct fsuuid2 u = { .len = sb->s_uuid_len, }; > > > > > > > + memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len); > > > > > > > + > > > > > > > + return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0; > > > > > > > +} > > > > > > > > > > > > Can we please keep the declarations separate from the code? I always > > > > > > find this sort of implicit scoping of variables both difficult to > > > > > > read (especially in larger functions) and a landmine waiting to be > > > > > > tripped over. This could easily just be: > > > > > > > > > > > > static int ioctl_getfsuuid(struct file *file, void __user *argp) > > > > > > { > > > > > > struct super_block *sb = file_inode(file)->i_sb; > > > > > > struct fsuuid2 u = { .len = sb->s_uuid_len, }; > > > > > > > > > > > > .... > > > > > > > > > > > > and then it's consistent with all the rest of the code... > > > > > > > > > > The way I'm doing it here is actually what I'm transitioning my own code > > > > > to - the big reason being that always declaring variables at the tops of > > > > > functions leads to separating declaration and initialization, and worse > > > > > it leads people to declaring a variable once and reusing it for multiple > > > > > things (I've seen that be a source of real bugs too many times). > > > > > > > > > > > > > I still think this is of questionable value. I know I've mentioned > > > > similar concerns to Dave's here on the bcachefs list, but still have not > > > > really seen any discussion other than a bit of back and forth on the > > > > handful of generally accepted (in the kernel) uses of this sort of thing > > > > for limiting scope in loops/branches and such. > > > > > > > > I was skimming through some more recent bcachefs patches the other day > > > > (the journal write pipelining stuff) where I came across one or two > > > > medium length functions where this had proliferated, and I found it kind > > > > of annoying TBH. It starts to almost look like there are casts all over > > > > the place and it's a bit more tedious to filter out logic from the > > > > additional/gratuitous syntax, IMO. > > > > > > > > That's still just my .02, but there was also previous mention of > > > > starting/having discussion on this sort of style change. Is that still > > > > the plan? If so, before or after proliferating it throughout the > > > > bcachefs code? ;) I am curious if there are other folks in kernel land > > > > who think this makes enough sense that they'd plan to adopt it. Hm? > > > > > > That was the discussion :) > > > > > > bcachefs is my codebase, so yes, I intend to do it there. I really think > > > this is an instance where you and Dave are used to the way C has > > > historically forced us to do things; our brains get wired to read code a > > > certain way and changes are jarring. > > > > > > > Heh, fair enough. That's certainly your prerogative. I'm certainly not > > trying to tell you what to do or not with bcachefs. That's at least > > direct enough that it's clear it's not worth debating too much. ;) > > > > > But take a step back; if we were used to writing code the way I'm doing > > > it, and you were arguing for putting declarations at the tops of > > > functions, what would the arguments be? > > > > > > > I think my thought process would be similar. I.e., is the proposed > > benefit of such a change worth the tradeoffs? > > > > > I would say you're just breaking up the flow of ideas for no reason; a > > > chain of related statements now includes a declaration that isn't with > > > the actual logic. > > > > > > And bugs due to variable reuse, missed initialization - there's real > > > reasons not to do it that way. > > > > > > > And were I in that position, I don't think I would reduce a decision > > that affects readability/reviewability of my subsystem to a nontrivial > > degree (for other people, at least) to that single aspect. This would be > > the answer to the question: "is this worth considering?" > > If you feel this affected by this, how are you going to cope with Rust? > Well I'm still a Rust newbie, but I've been exposed to some of the basic syntax and semantics and it hasn't been a problem yet. I'll keep my fingers crossed, I guess. Brian
diff --git a/fs/ioctl.c b/fs/ioctl.c index 76cf22ac97d7..046c30294a82 100644 --- a/fs/ioctl.c +++ b/fs/ioctl.c @@ -763,6 +763,19 @@ static int ioctl_fssetxattr(struct file *file, void __user *argp) return err; } +static int ioctl_getfsuuid(struct file *file, void __user *argp) +{ + struct super_block *sb = file_inode(file)->i_sb; + + if (!sb->s_uuid_len) + return -ENOIOCTLCMD; + + struct fsuuid2 u = { .len = sb->s_uuid_len, }; + memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len); + + return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0; +} + /* * do_vfs_ioctl() is not for drivers and not intended to be EXPORT_SYMBOL()'d. * It's just a simple helper for sys_ioctl and compat_sys_ioctl. @@ -845,6 +858,9 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd, case FS_IOC_FSSETXATTR: return ioctl_fssetxattr(filp, argp); + case FS_IOC_GETFSUUID: + return ioctl_getfsuuid(filp, argp); + default: if (S_ISREG(inode->i_mode)) return file_ioctl(filp, cmd, argp); diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h index 48ad69f7722e..16a6ecadfd8d 100644 --- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h @@ -64,6 +64,19 @@ struct fstrim_range { __u64 minlen; }; +/* + * We include a length field because some filesystems (vfat) have an identifier + * that we do want to expose as a UUID, but doesn't have the standard length. + * + * We use a fixed size buffer beacuse this interface will, by fiat, never + * support "UUIDs" longer than 16 bytes; we don't want to force all downstream + * users to have to deal with that. + */ +struct fsuuid2 { + __u8 len; + __u8 uuid[16]; +}; + /* extent-same (dedupe) ioctls; these MUST match the btrfs ioctl definitions */ #define FILE_DEDUPE_RANGE_SAME 0 #define FILE_DEDUPE_RANGE_DIFFERS 1 @@ -190,6 +203,9 @@ struct fsxattr { * (see uapi/linux/blkzoned.h) */ +/* Returns the external filesystem UUID, the same one blkid returns */ +#define FS_IOC_GETFSUUID _IOR(0x12, 142, struct fsuuid2) + #define BMAP_IOCTL 1 /* obsolete - kept for compatibility */ #define FIBMAP _IO(0x00,1) /* bmap access */ #define FIGETBSZ _IO(0x00,2) /* get the block size used for bmap */ @@ -198,6 +214,7 @@ struct fsxattr { #define FITRIM _IOWR('X', 121, struct fstrim_range) /* Trim */ #define FICLONE _IOW(0x94, 9, int) #define FICLONERANGE _IOW(0x94, 13, struct file_clone_range) + #define FIDEDUPERANGE _IOWR(0x94, 54, struct file_dedupe_range) #define FSLABEL_MAX 256 /* Max chars for the interface; each fs may differ */
Add a new generic ioctls for querying the filesystem UUID. These are lifted versions of the ext4 ioctls, with one change: we're not using a flexible array member, because UUIDs will never be more than 16 bytes. This patch adds a generic implementation of FS_IOC_GETFSUUID, which reads from super_block->s_uuid. We're not lifting SETFSUUID from ext4 - that can be done on offline filesystems by the people who need it, trying to do it online is just asking for too much trouble. Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Dave Chinner <dchinner@redhat.com> Cc: "Darrick J. Wong" <djwong@kernel.org> Cc: Theodore Ts'o <tytso@mit.edu> Cc: linux-fsdevel@vger.kernel.or Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> --- fs/ioctl.c | 16 ++++++++++++++++ include/uapi/linux/fs.h | 17 +++++++++++++++++ 2 files changed, 33 insertions(+)