Re: [PATCH v6 6/6] fs: introduce file_getattr and file_setattr syscalls

["Pali Rohár" <pali@kernel.org>]

On Tuesday 01 July 2025 12:08:57 Darrick J. Wong wrote:
> On Tue, Jul 01, 2025 at 08:54:57PM +0200, Pali Rohár wrote:
> > On Tuesday 01 July 2025 11:43:17 Darrick J. Wong wrote:
> > > On Mon, Jun 30, 2025 at 06:20:16PM +0200, Andrey Albershteyn wrote:
> > > > diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> > > > index 0098b0ce8ccb..0784f2033ba4 100644
> > > > --- a/include/uapi/linux/fs.h
> > > > +++ b/include/uapi/linux/fs.h
> > > > @@ -148,6 +148,24 @@ struct fsxattr {
> > > >  	unsigned char	fsx_pad[8];
> > > >  };
> > > >  
> > > > +/*
> > > > + * Variable size structure for file_[sg]et_attr().
> > > > + *
> > > > + * Note. This is alternative to the structure 'struct fileattr'/'struct fsxattr'.
> > > > + * As this structure is passed to/from userspace with its size, this can
> > > > + * be versioned based on the size.
> > > > + */
> > > > +struct fsx_fileattr {
> > > > +	__u32	fsx_xflags;	/* xflags field value (get/set) */
> > > 
> > > Should this to be __u64 from the start?  Seeing as (a) this struct is
> > > not already a multiple of 8 bytes and (b) it's likely that we'll have to
> > > add a u64 field at some point.  That would also address brauner's
> > > comment about padding.
> > 
> > Hello!
> > 
> > As I have already mentioned, after this syscall API/ABI is finished, I'm
> > planning to prepare patches for changing just selected fields / flags by
> > introducing a new mask field, and support for additional flags used by
> > existing filesystems (like windows flags).
> > 
> > My idea is extending this structure for a new "u32 fsx_xflags_mask"
> > and new "u32 fsx_xflags2" + "u32 fsx_xflags2_mask". (field names are
> > just examples).
> > 
> > So in case you are extending the structure now, please consider if it
> > makes sense to add all members, so we do not have to define 2 or 3
> > structure versions in near feature.
> > 
> > Your idea of __u64 for fsx_xflags means that it will already cover the
> > "u32 fsx_xflags2" field.
> 
> Ah, ok, so that work *is* still coming. :)

Yes. I'm just waiting until this patch series is accepted.

In past I have already sent RFC patches to the list which modifies the
existing ioctl interface. So you can look at it if you want :-)

> Are you still planning to add masks for xflags bits that are clearable
> and settable?  i.e.
> 
> 	__u64	fa_xflags;		/* state */
> 	...
> 	<end of V0 structure>
> 
> 	__u64	fa_xflags_mask;		/* bits for setattr to examine */
> 	__u64	fa_xflags_clearable;	/* clearable bits */
> 	__u64	fa_xflags_settable;	/* settable bits */
> 
> I think it's easier just to define u64 in the V0 structure and then add
> the three new fields in V1.  What do you think?

I wanted the interface which would allow to atomically change specified
bit/flag without the need for get-modify-set. And I think that this
would not work as the fa_xflags requires the state.

My idea is following:

  __u64 fa_xflags;
  ...
  <end of V0 structure>
  __u64 fa_xflags_mask;

The fa_xflags_mask will specify which bits from the fa_xflags and from
other fa_* fields in V0 struct are going to be changed.

> --D
> 
> > > --D
> > > 
> > > > +	__u32	fsx_extsize;	/* extsize field value (get/set)*/
> > > > +	__u32	fsx_nextents;	/* nextents field value (get)   */
> > > > +	__u32	fsx_projid;	/* project identifier (get/set) */
> > > > +	__u32	fsx_cowextsize;	/* CoW extsize field value (get/set) */
> > > > +};
> > > > +
> > > > +#define FSX_FILEATTR_SIZE_VER0 20
> > > > +#define FSX_FILEATTR_SIZE_LATEST FSX_FILEATTR_SIZE_VER0
> > > > +
> > > >  /*
> > > >   * Flags for the fsx_xflags field
> > > >   */
> > > > diff --git a/scripts/syscall.tbl b/scripts/syscall.tbl
> > > > index 580b4e246aec..d1ae5e92c615 100644
> > > > --- a/scripts/syscall.tbl
> > > > +++ b/scripts/syscall.tbl
> > > > @@ -408,3 +408,5 @@
> > > >  465	common	listxattrat			sys_listxattrat
> > > >  466	common	removexattrat			sys_removexattrat
> > > >  467	common	open_tree_attr			sys_open_tree_attr
> > > > +468	common	file_getattr			sys_file_getattr
> > > > +469	common	file_setattr			sys_file_setattr
> > > > 
> > > > -- 
> > > > 2.47.2
> > > > 
> > > > 
> >