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

Pali Rohár
2025-07-01T19:17:49Z

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
> > > >
> > > >
> >