This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 fs/ioctl.c               | 8 ++++++++
 include/linux/fileattr.h | 4 ++--
 include/uapi/linux/fs.h  | 2 ++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 638a36be31c1..9f3609b50779 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -480,6 +480,10 @@ void fileattr_fill_xflags(struct fileattr *fa, u32 xflags)
 		fa->flags |= FS_DAX_FL;
 	if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
 		fa->flags |= FS_PROJINHERIT_FL;
+	if (fa->fsx_xflags & FS_XFLAG_COMPRESSED)
+		fa->flags |= FS_COMPR_FL;
+	if (fa->fsx_xflags & FS_XFLAG_ENCRYPTED)
+		fa->flags |= FS_ENCRYPT_FL;
 }
 EXPORT_SYMBOL(fileattr_fill_xflags);
 
@@ -496,6 +500,8 @@ void fileattr_fill_flags(struct fileattr *fa, u32 flags)
 	memset(fa, 0, sizeof(*fa));
 	fa->flags_valid = true;
 	fa->flags = flags;
+	if (fa->flags & FS_COMPR_FL)
+		fa->fsx_xflags |= FS_XFLAG_COMPRESSED;
 	if (fa->flags & FS_SYNC_FL)
 		fa->fsx_xflags |= FS_XFLAG_SYNC;
 	if (fa->flags & FS_IMMUTABLE_FL)
@@ -506,6 +512,8 @@ void fileattr_fill_flags(struct fileattr *fa, u32 flags)
 		fa->fsx_xflags |= FS_XFLAG_NODUMP;
 	if (fa->flags & FS_NOATIME_FL)
 		fa->fsx_xflags |= FS_XFLAG_NOATIME;
+	if (fa->flags & FS_ENCRYPT_FL)
+		fa->fsx_xflags |= FS_XFLAG_ENCRYPTED;
 	if (fa->flags & FS_DAX_FL)
 		fa->fsx_xflags |= FS_XFLAG_DAX;
 	if (fa->flags & FS_PROJINHERIT_FL)
diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
index 47c05a9851d0..c297e6151703 100644
--- a/include/linux/fileattr.h
+++ b/include/linux/fileattr.h
@@ -7,12 +7,12 @@
 #define FS_COMMON_FL \
 	(FS_SYNC_FL | FS_IMMUTABLE_FL | FS_APPEND_FL | \
 	 FS_NODUMP_FL |	FS_NOATIME_FL | FS_DAX_FL | \
-	 FS_PROJINHERIT_FL)
+	 FS_PROJINHERIT_FL | FS_COMPR_FL | FS_ENCRYPT_FL)
 
 #define FS_XFLAG_COMMON \
 	(FS_XFLAG_SYNC | FS_XFLAG_IMMUTABLE | FS_XFLAG_APPEND | \
 	 FS_XFLAG_NODUMP | FS_XFLAG_NOATIME | FS_XFLAG_DAX | \
-	 FS_XFLAG_PROJINHERIT)
+	 FS_XFLAG_PROJINHERIT | FS_XFLAG_COMPRESSED | FS_XFLAG_ENCRYPTED)
 
 /*
  * Merged interface for miscellaneous file attributes.  'flags' originates from
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 2bbe00cf1248..367bc5289c47 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -167,6 +167,8 @@ struct fsxattr {
 #define FS_XFLAG_FILESTREAM	0x00004000	/* use filestream allocator */
 #define FS_XFLAG_DAX		0x00008000	/* use DAX for IO */
 #define FS_XFLAG_COWEXTSIZE	0x00010000	/* CoW extent size allocator hint */
+#define FS_XFLAG_COMPRESSED	0x00020000	/* compressed file */
+#define FS_XFLAG_ENCRYPTED	0x00040000	/* encrypted file */
 #define FS_XFLAG_HASATTR	0x80000000	/* no DIFLAG for this	*/
 
 /* the read-only stuff doesn't really belong here, but any other place is
-- 
2.20.1

On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
which use that flag?  In the fscrypt case it's very intentional that
FS_ENCRYPT_FL can be gotten via FS_IOC_GETFLAGS but not set via FS_IOC_SETFLAGS.
A simple toggle of the flag can't work, as it doesn't provide the needed
information.  Instead there is a separate ioctl (FS_IOC_SET_ENCRYPTION_POLICY)
for enabling encryption which takes additional parameters and only works on
empty directories.

- Eric

On Sun, Feb 16, 2025 at 7:34 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
>
> Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> which use that flag?

As far as I can tell, after fileattr_fill_xflags() call in
ioctl_fssetxattr(), the call
to ext4_fileattr_set() should behave exactly the same if it came some
FS_IOC_FSSETXATTR or from FS_IOC_SETFLAGS.
IOW, EXT4_FL_USER_MODIFIABLE mask will still apply.

However, unlike the legacy API, we now have an opportunity to deal with
EXT4_FL_USER_MODIFIABLE better than this:
        /*
         * chattr(1) grabs flags via GETFLAGS, modifies the result and
         * passes that to SETFLAGS. So we cannot easily make SETFLAGS
         * more restrictive than just silently masking off visible but
         * not settable flags as we always did.
         */

if we have the xflags_mask in the new API (not only the xflags) then
chattr(1) can set EXT4_FL_USER_MODIFIABLE in xflags_mask
ext4_fileattr_set() can verify that
(xflags_mask & ~EXT4_FL_USER_MODIFIABLE == 0).

However, Pali, this is an important point that your RFC did not follow -
AFAICT, the current kernel code of ext4_fileattr_set() and xfs_fileattr_set()
(and other fs) does not return any error for unknown xflags, it just
ignores them.

This is why a new ioctl pair FS_IOC_[GS]ETFSXATTR2 is needed IMO
before adding support to ANY new xflags, whether they are mapped to
existing flags like in this patch or are completely new xflags.

Thanks,
Amir.

On Sunday 16 February 2025 21:17:55 Amir Goldstein wrote:
> On Sun, Feb 16, 2025 at 7:34 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> > >
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> >
> > Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> > this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> > which use that flag?
> 
> As far as I can tell, after fileattr_fill_xflags() call in
> ioctl_fssetxattr(), the call
> to ext4_fileattr_set() should behave exactly the same if it came some
> FS_IOC_FSSETXATTR or from FS_IOC_SETFLAGS.
> IOW, EXT4_FL_USER_MODIFIABLE mask will still apply.
> 
> However, unlike the legacy API, we now have an opportunity to deal with
> EXT4_FL_USER_MODIFIABLE better than this:
>         /*
>          * chattr(1) grabs flags via GETFLAGS, modifies the result and
>          * passes that to SETFLAGS. So we cannot easily make SETFLAGS
>          * more restrictive than just silently masking off visible but
>          * not settable flags as we always did.
>          */
> 
> if we have the xflags_mask in the new API (not only the xflags) then
> chattr(1) can set EXT4_FL_USER_MODIFIABLE in xflags_mask
> ext4_fileattr_set() can verify that
> (xflags_mask & ~EXT4_FL_USER_MODIFIABLE == 0).
> 
> However, Pali, this is an important point that your RFC did not follow -
> AFAICT, the current kernel code of ext4_fileattr_set() and xfs_fileattr_set()
> (and other fs) does not return any error for unknown xflags, it just
> ignores them.
> 
> This is why a new ioctl pair FS_IOC_[GS]ETFSXATTR2 is needed IMO
> before adding support to ANY new xflags, whether they are mapped to
> existing flags like in this patch or are completely new xflags.
> 
> Thanks,
> Amir.

But xflags_mask is available in this new API. It is available if the
FS_XFLAG_HASEXTFIELDS flag is set. So I think that the ext4 improvement
mentioned above can be included into this new API.

Or I'm missing something?

On Sun, Feb 16, 2025 at 9:24 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Sunday 16 February 2025 21:17:55 Amir Goldstein wrote:
> > On Sun, Feb 16, 2025 at 7:34 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > >
> > > On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > > > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> > > >
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > >
> > > Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> > > this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> > > which use that flag?
> >
> > As far as I can tell, after fileattr_fill_xflags() call in
> > ioctl_fssetxattr(), the call
> > to ext4_fileattr_set() should behave exactly the same if it came some
> > FS_IOC_FSSETXATTR or from FS_IOC_SETFLAGS.
> > IOW, EXT4_FL_USER_MODIFIABLE mask will still apply.
> >
> > However, unlike the legacy API, we now have an opportunity to deal with
> > EXT4_FL_USER_MODIFIABLE better than this:
> >         /*
> >          * chattr(1) grabs flags via GETFLAGS, modifies the result and
> >          * passes that to SETFLAGS. So we cannot easily make SETFLAGS
> >          * more restrictive than just silently masking off visible but
> >          * not settable flags as we always did.
> >          */
> >
> > if we have the xflags_mask in the new API (not only the xflags) then
> > chattr(1) can set EXT4_FL_USER_MODIFIABLE in xflags_mask
> > ext4_fileattr_set() can verify that
> > (xflags_mask & ~EXT4_FL_USER_MODIFIABLE == 0).
> >
> > However, Pali, this is an important point that your RFC did not follow -
> > AFAICT, the current kernel code of ext4_fileattr_set() and xfs_fileattr_set()
> > (and other fs) does not return any error for unknown xflags, it just
> > ignores them.
> >
> > This is why a new ioctl pair FS_IOC_[GS]ETFSXATTR2 is needed IMO
> > before adding support to ANY new xflags, whether they are mapped to
> > existing flags like in this patch or are completely new xflags.
> >
> > Thanks,
> > Amir.
>
> But xflags_mask is available in this new API. It is available if the
> FS_XFLAG_HASEXTFIELDS flag is set. So I think that the ext4 improvement
> mentioned above can be included into this new API.
>
> Or I'm missing something?

Yes, you are missing something very fundamental to backward compat API -
You cannot change the existing kernels.

You should ask yourself one question:
What happens if I execute the old ioctl FS_IOC_FSSETXATTR
on an existing old kernel with the new extended flags?

The answer, to the best of my code emulation abilities is that
old kernel will ignore the new xflags including FS_XFLAG_HASEXTFIELDS
and this is suboptimal, because it would be better for the new chattr tool
to get -EINVAL when trying to set new xflags and mask on an old kernel.

It is true that the new chattr can call the old FS_IOC_FSGETXATTR
ioctl and see that it has no FS_XFLAG_HASEXTFIELDS,
so I agree that a new ioctl is not absolutely necessary,
but I still believe that it is a better API design.

Would love to hear what other fs developers prefer.

Thanks,
Amir.

On Sun, Feb 16, 2025 at 09:43:02PM +0100, Amir Goldstein wrote:
> On Sun, Feb 16, 2025 at 9:24 PM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Sunday 16 February 2025 21:17:55 Amir Goldstein wrote:
> > > On Sun, Feb 16, 2025 at 7:34 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > >
> > > > On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > > > > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> > > > >
> > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > >
> > > > Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> > > > this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> > > > which use that flag?
> > >
> > > As far as I can tell, after fileattr_fill_xflags() call in
> > > ioctl_fssetxattr(), the call
> > > to ext4_fileattr_set() should behave exactly the same if it came some
> > > FS_IOC_FSSETXATTR or from FS_IOC_SETFLAGS.
> > > IOW, EXT4_FL_USER_MODIFIABLE mask will still apply.
> > >
> > > However, unlike the legacy API, we now have an opportunity to deal with
> > > EXT4_FL_USER_MODIFIABLE better than this:
> > >         /*
> > >          * chattr(1) grabs flags via GETFLAGS, modifies the result and
> > >          * passes that to SETFLAGS. So we cannot easily make SETFLAGS
> > >          * more restrictive than just silently masking off visible but
> > >          * not settable flags as we always did.
> > >          */
> > >
> > > if we have the xflags_mask in the new API (not only the xflags) then
> > > chattr(1) can set EXT4_FL_USER_MODIFIABLE in xflags_mask
> > > ext4_fileattr_set() can verify that
> > > (xflags_mask & ~EXT4_FL_USER_MODIFIABLE == 0).
> > >
> > > However, Pali, this is an important point that your RFC did not follow -
> > > AFAICT, the current kernel code of ext4_fileattr_set() and xfs_fileattr_set()
> > > (and other fs) does not return any error for unknown xflags, it just
> > > ignores them.
> > >
> > > This is why a new ioctl pair FS_IOC_[GS]ETFSXATTR2 is needed IMO
> > > before adding support to ANY new xflags, whether they are mapped to
> > > existing flags like in this patch or are completely new xflags.
> > >
> > > Thanks,
> > > Amir.
> >
> > But xflags_mask is available in this new API. It is available if the
> > FS_XFLAG_HASEXTFIELDS flag is set. So I think that the ext4 improvement
> > mentioned above can be included into this new API.
> >
> > Or I'm missing something?
> 
> Yes, you are missing something very fundamental to backward compat API -
> You cannot change the existing kernels.
> 
> You should ask yourself one question:
> What happens if I execute the old ioctl FS_IOC_FSSETXATTR
> on an existing old kernel with the new extended flags?

It should ignore all the things it does not know about.

But the correct usage of FS_IOC_FSSETXATTR is to call
FS_IOC_FSGETXATTR to initialise the structure with all the current
inode state. In this situation, the fsx.fsx_xflags field needs to
return a flag that says "FS_XFLAGS_HAS_WIN_ATTRS" and now userspace
knows that it can set/clear the MS windows attr flags.  Hence if the
filesystem supports windows attributes, we can require them to
-support the entire set-.

i.e. if an attempt to set a win attr that the filesystem cannot
implement (e.g. compression) then it returns an error (-EINVAL),
otherwise it will store the attr and perform whatever operation it
requires.

IMO, the whole implementation in the patchset is wrong - there is no
need for a new xflags2 field, and there is no need for whacky field
masks or anything like that. All it needs is a single bit to
indicate if the windows attributes are supported, and they are all
implemented as normal FS_XFLAG fields in the fsx_xflags field.

> The answer, to the best of my code emulation abilities is that
> old kernel will ignore the new xflags including FS_XFLAG_HASEXTFIELDS
> and this is suboptimal, because it would be better for the new chattr tool
> to get -EINVAL when trying to set new xflags and mask on an old kernel.

What new chattr tool? I would expect that xfs_io -c "chattr ..."
will be extended to support all these new fields because that is the
historical tool we use for FS_IOC_FS{GS}ETXATTR regression test
support in fstests. I would also expect that the e2fsprogs chattr(1)
program to grow support for the new FS_XFLAGS bits as well...

> It is true that the new chattr can call the old FS_IOC_FSGETXATTR
> ioctl and see that it has no FS_XFLAG_HASEXTFIELDS,
> so I agree that a new ioctl is not absolutely necessary,
> but I still believe that it is a better API design.

This is how FS{GS}ETXATTR interface has always worked. You *must*
do a GET before a SET so that the fsx structure has been correctly
initialised so the SET operation makes only the exact change being
asked for.

This makes the -API pair- backwards compatible by allowing struct
fsxattr feature bits to be reported in the GET operation. If the
feature bit is set, then those optional fields can be set. If the
feature bit is not set by the GET operation, then if you try to use
it on a SET operation you'll either get an error or it will be
silently ignored.

> Would love to hear what other fs developers prefer.

Do not overcomplicate the problem. Use the interface as intended:
GET then SET, and GET returns feature bits in the xflags field to
indicate what is valid in the overall struct fsxattr. It's trivial
to probe for native fs support of windows attributes like this. It
also allows filesystems to be converted one at a time to support the
windows attributes (e.g. as they have on-disk format support for
them added). Applications like Samba can then switch behaviours
based on what they probe from the underlying filesystem...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

On Tue, Feb 18, 2025 at 2:33 AM Dave Chinner <david@fromorbit.com> wrote:
>
> On Sun, Feb 16, 2025 at 09:43:02PM +0100, Amir Goldstein wrote:
> > On Sun, Feb 16, 2025 at 9:24 PM Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Sunday 16 February 2025 21:17:55 Amir Goldstein wrote:
> > > > On Sun, Feb 16, 2025 at 7:34 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > >
> > > > > On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > > > > > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> > > > > >
> > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > >
> > > > > Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> > > > > this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> > > > > which use that flag?
> > > >
> > > > As far as I can tell, after fileattr_fill_xflags() call in
> > > > ioctl_fssetxattr(), the call
> > > > to ext4_fileattr_set() should behave exactly the same if it came some
> > > > FS_IOC_FSSETXATTR or from FS_IOC_SETFLAGS.
> > > > IOW, EXT4_FL_USER_MODIFIABLE mask will still apply.
> > > >
> > > > However, unlike the legacy API, we now have an opportunity to deal with
> > > > EXT4_FL_USER_MODIFIABLE better than this:
> > > >         /*
> > > >          * chattr(1) grabs flags via GETFLAGS, modifies the result and
> > > >          * passes that to SETFLAGS. So we cannot easily make SETFLAGS
> > > >          * more restrictive than just silently masking off visible but
> > > >          * not settable flags as we always did.
> > > >          */
> > > >
> > > > if we have the xflags_mask in the new API (not only the xflags) then
> > > > chattr(1) can set EXT4_FL_USER_MODIFIABLE in xflags_mask
> > > > ext4_fileattr_set() can verify that
> > > > (xflags_mask & ~EXT4_FL_USER_MODIFIABLE == 0).
> > > >
> > > > However, Pali, this is an important point that your RFC did not follow -
> > > > AFAICT, the current kernel code of ext4_fileattr_set() and xfs_fileattr_set()
> > > > (and other fs) does not return any error for unknown xflags, it just
> > > > ignores them.
> > > >
> > > > This is why a new ioctl pair FS_IOC_[GS]ETFSXATTR2 is needed IMO
> > > > before adding support to ANY new xflags, whether they are mapped to
> > > > existing flags like in this patch or are completely new xflags.
> > > >
> > > > Thanks,
> > > > Amir.
> > >
> > > But xflags_mask is available in this new API. It is available if the
> > > FS_XFLAG_HASEXTFIELDS flag is set. So I think that the ext4 improvement
> > > mentioned above can be included into this new API.
> > >
> > > Or I'm missing something?
> >
> > Yes, you are missing something very fundamental to backward compat API -
> > You cannot change the existing kernels.
> >
> > You should ask yourself one question:
> > What happens if I execute the old ioctl FS_IOC_FSSETXATTR
> > on an existing old kernel with the new extended flags?
>
> It should ignore all the things it does not know about.
>

I don't know about "should" but it certainly does, that's why I was
wondering if a new fresh ioctl was in order before extending to new flags.

> But the correct usage of FS_IOC_FSSETXATTR is to call
> FS_IOC_FSGETXATTR to initialise the structure with all the current
> inode state.

Yeh, this is how the API is being used by exiting xfs_io chattr.
It does not mean that this is a good API.

> In this situation, the fsx.fsx_xflags field needs to
> return a flag that says "FS_XFLAGS_HAS_WIN_ATTRS" and now userspace
> knows that it can set/clear the MS windows attr flags.  Hence if the
> filesystem supports windows attributes, we can require them to
> -support the entire set-.
>
> i.e. if an attempt to set a win attr that the filesystem cannot
> implement (e.g. compression) then it returns an error (-EINVAL),
> otherwise it will store the attr and perform whatever operation it
> requires.
>

I prefer not to limit the discussion to new "win" attributes,
especially when discussing COMPR/ENCRYPT flags which are existing
flags that are also part of the win attributes.

To put it another way, suppose Pali did not come forward with a patch set
to add win attribute control to smb, but to add ENCRYPT support to xfs.
What would have been your prefered way to set the ENCRYPT flag in xfs?
via FS_IOC_SETFLAGS or by extending FS_IOC_FSSETXATTR?
and if the latter, then how would xfs_io chattr be expected to know if
setting the ENCRYPT flag is supported or not?

> IMO, the whole implementation in the patchset is wrong - there is no
> need for a new xflags2 field,

xflags2 is needed to add more bits. I am not following.

> and there is no need for whacky field
> masks or anything like that. All it needs is a single bit to
> indicate if the windows attributes are supported, and they are all
> implemented as normal FS_XFLAG fields in the fsx_xflags field.
>

Sorry, I did not understand your vision about the API.
On the one hand, you are saying that there is no need for xflags2.
On the other hand, that new flags should be treated differently than
old flags (FS_XFLAGS_HAS_WIN_ATTRS).
Either I did not understand what you mean or the documentation of
what you are proposing sounds terribly confusing to me.

> > The answer, to the best of my code emulation abilities is that
> > old kernel will ignore the new xflags including FS_XFLAG_HASEXTFIELDS
> > and this is suboptimal, because it would be better for the new chattr tool
> > to get -EINVAL when trying to set new xflags and mask on an old kernel.
>
> What new chattr tool? I would expect that xfs_io -c "chattr ..."
> will be extended to support all these new fields because that is the
> historical tool we use for FS_IOC_FS{GS}ETXATTR regression test
> support in fstests. I would also expect that the e2fsprogs chattr(1)
> program to grow support for the new FS_XFLAGS bits as well...
>

That's exactly what I meant by "new chattr tool".
when user executes chattr +i or xfs_io -c "chattr +i"
user does not care which ioctl is used and it is best if both
utils will support the entire set of modern flags with the new ioctls
so that eventually (after old fs are deprecated) the old ioctl may also
be deprecated.

> > It is true that the new chattr can call the old FS_IOC_FSGETXATTR
> > ioctl and see that it has no FS_XFLAG_HASEXTFIELDS,
> > so I agree that a new ioctl is not absolutely necessary,
> > but I still believe that it is a better API design.
>
> This is how FS{GS}ETXATTR interface has always worked. You *must*
> do a GET before a SET so that the fsx structure has been correctly
> initialised so the SET operation makes only the exact change being
> asked for.
>
> This makes the -API pair- backwards compatible by allowing struct
> fsxattr feature bits to be reported in the GET operation. If the
> feature bit is set, then those optional fields can be set. If the
> feature bit is not set by the GET operation, then if you try to use
> it on a SET operation you'll either get an error or it will be
> silently ignored.
>

Yes, I know. I still think that this is a poor API design pattern.
Imagine that people will be interested to include the fsxattr
in rsync or such (it has been mentioned in the context of this effort)
The current API is pretty useless for backup/restore and for
copying supported attributes across filesystems.

BTW, the internal ->fileattr_[gs]et() vfs API was created so that
overlayfs could copy flags between filesystems on copy-up,
but right now it only copies common flags.

Adding a support mask to the API will allow smarter copy of
supported attributes between filesystems that have a more
specific common set of supported flags.

> > Would love to hear what other fs developers prefer.
>
> Do not overcomplicate the problem. Use the interface as intended:
> GET then SET, and GET returns feature bits in the xflags field to
> indicate what is valid in the overall struct fsxattr. It's trivial
> to probe for native fs support of windows attributes like this. It
> also allows filesystems to be converted one at a time to support the
> windows attributes (e.g. as they have on-disk format support for
> them added). Applications like Samba can then switch behaviours
> based on what they probe from the underlying filesystem...
>

OK, so we have two opinions.
Debating at design time is much better than after API is released...

I'd still like to hear from other stakeholders before we perpetuate
the existing design pattern which requires apps to GET before SET
and treat new (WIN) flags differently than old flags.

Thanks,
Amir.

On Tue, Feb 18, 2025 at 10:13:46AM +0100, Amir Goldstein wrote:
> On Tue, Feb 18, 2025 at 2:33 AM Dave Chinner <david@fromorbit.com> wrote:
> > > Yes, you are missing something very fundamental to backward compat API -
> > > You cannot change the existing kernels.
> > >
> > > You should ask yourself one question:
> > > What happens if I execute the old ioctl FS_IOC_FSSETXATTR
> > > on an existing old kernel with the new extended flags?
> >
> > It should ignore all the things it does not know about.
> 
> I don't know about "should" but it certainly does, that's why I was
> wondering if a new fresh ioctl was in order before extending to new flags.
> 
> > But the correct usage of FS_IOC_FSSETXATTR is to call
> > FS_IOC_FSGETXATTR to initialise the structure with all the current
> > inode state.
> 
> Yeh, this is how the API is being used by exiting xfs_io chattr.
> It does not mean that this is a good API.
> 
> > In this situation, the fsx.fsx_xflags field needs to
> > return a flag that says "FS_XFLAGS_HAS_WIN_ATTRS" and now userspace
> > knows that it can set/clear the MS windows attr flags.  Hence if the
> > filesystem supports windows attributes, we can require them to
> > -support the entire set-.
> >
> > i.e. if an attempt to set a win attr that the filesystem cannot
> > implement (e.g. compression) then it returns an error (-EINVAL),
> > otherwise it will store the attr and perform whatever operation it
> > requires.
> 
> I prefer not to limit the discussion to new "win" attributes,
> especially when discussing COMPR/ENCRYPT flags which are existing
> flags that are also part of the win attributes.

Not sure I understand why you think I don't know this, and why it
is a problem in any way?

> To put it another way, suppose Pali did not come forward with a patch set
> to add win attribute control to smb, but to add ENCRYPT support to xfs.
> What would have been your prefered way to set the ENCRYPT flag in xfs?

<sigh>

We don't have encryption on XFS yet, so we'd completely ignore it.
It would never be set in the GET op, and it would be ignored on the
SET op.

> via FS_IOC_SETFLAGS or by extending FS_IOC_FSSETXATTR?
> and if the latter, then how would xfs_io chattr be expected to know if
> setting the ENCRYPT flag is supported or not?

chattr is not the interface for enabling encryption!

FS_IOC_FSGETXATTR returns various status information, and a subset
of that information can be used for changing inode state with
the FS_IOC_FSSETXATTR command.

The reason SET ignores stuff it can't set is because it expects that
GET->SET will result in flags being set that it can't actually
change, and so it ignores flags that cannot be set....

> > IMO, the whole implementation in the patchset is wrong - there is no
> > need for a new xflags2 field,
> 
> xflags2 is needed to add more bits. I am not following.

We've got a 13 or 14 free flag bits still remaining in fsx_xflags
before we need another flags field.

> > and there is no need for whacky field
> > masks or anything like that. All it needs is a single bit to
> > indicate if the windows attributes are supported, and they are all
> > implemented as normal FS_XFLAG fields in the fsx_xflags field.
> >
> 
> Sorry, I did not understand your vision about the API.
> On the one hand, you are saying that there is no need for xflags2.

Because we have enough spare bits for all the new flags, yes?

> On the other hand, that new flags should be treated differently than
> old flags (FS_XFLAGS_HAS_WIN_ATTRS).

Yes, new flags can have different behaviour. If we tell userspace
that we support windows attributes, we can define whatever behaviour
we want when setting -those new flag fields-.

> Either I did not understand what you mean or the documentation of
> what you are proposing sounds terribly confusing to me.

Misunderstanding, I think.

> > > The answer, to the best of my code emulation abilities is that
> > > old kernel will ignore the new xflags including FS_XFLAG_HASEXTFIELDS
> > > and this is suboptimal, because it would be better for the new chattr tool
> > > to get -EINVAL when trying to set new xflags and mask on an old kernel.
> >
> > What new chattr tool? I would expect that xfs_io -c "chattr ..."
> > will be extended to support all these new fields because that is the
> > historical tool we use for FS_IOC_FS{GS}ETXATTR regression test
> > support in fstests. I would also expect that the e2fsprogs chattr(1)
> > program to grow support for the new FS_XFLAGS bits as well...
> >
> 
> That's exactly what I meant by "new chattr tool".

"new chattr tool" implies a new implementation/binary, not modifying
the existing tools.

> when user executes chattr +i or xfs_io -c "chattr +i"
> user does not care which ioctl is used and it is best if both
> utils will support the entire set of modern flags with the new ioctls
> so that eventually (after old fs are deprecated) the old ioctl may also
> be deprecated.

We will never be able to deprecate/remove deprecate the existing
ioctls.


> > > It is true that the new chattr can call the old FS_IOC_FSGETXATTR
> > > ioctl and see that it has no FS_XFLAG_HASEXTFIELDS,
> > > so I agree that a new ioctl is not absolutely necessary,
> > > but I still believe that it is a better API design.
> >
> > This is how FS{GS}ETXATTR interface has always worked. You *must*
> > do a GET before a SET so that the fsx structure has been correctly
> > initialised so the SET operation makes only the exact change being
> > asked for.
> >
> > This makes the -API pair- backwards compatible by allowing struct
> > fsxattr feature bits to be reported in the GET operation. If the
> > feature bit is set, then those optional fields can be set. If the
> > feature bit is not set by the GET operation, then if you try to use
> > it on a SET operation you'll either get an error or it will be
> > silently ignored.
> >
> 
> Yes, I know. I still think that this is a poor API design pattern.
> Imagine that people will be interested to include the fsxattr
> in rsync or such (it has been mentioned in the context of this effort)
> The current API is pretty useless for backup/restore and for
> copying supported attributes across filesystems.

xfsrestore has been using this interface for backup/restore
for about 25 years now. It only uses the SET function, because the
dump format stores all the flags from the dump side GET operation
natively.

i.e. xfsdump returns all the FS_XFLAGS that it supports in bulkstat
output so that xfsdump can avoid needing to call GET on every inode
it is going to back up.

So, yeah, we've been using this get/set xattr interface successfully
for backup for a long, long time.

> BTW, the internal ->fileattr_[gs]et() vfs API was created so that
> overlayfs could copy flags between filesystems on copy-up,
> but right now it only copies common flags.

That's an implementation problem, not an API issue.

> Adding a support mask to the API will allow smarter copy of
> supported attributes between filesystems that have a more
> specific common set of supported flags.

I don't think such a static mask belongs in the GET/SET interface. If
overlay wants to know what features a filesytem supports so it can
find commonality, then the feature mask it should be calculated
once at mount time and not on every single operation...

> I'd still like to hear from other stakeholders before we perpetuate
> the existing design pattern which requires apps to GET before SET
> and treat new (WIN) flags differently than old flags.

I just don't see why we need -yet another- inode attribute userspace
API just to support a few new feature flags...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

On Tuesday 18 February 2025 10:13:46 Amir Goldstein wrote:
> On Tue, Feb 18, 2025 at 2:33 AM Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Sun, Feb 16, 2025 at 09:43:02PM +0100, Amir Goldstein wrote:
> > > On Sun, Feb 16, 2025 at 9:24 PM Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Sunday 16 February 2025 21:17:55 Amir Goldstein wrote:
> > > > > On Sun, Feb 16, 2025 at 7:34 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > > >
> > > > > > On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > > > > > > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> > > > > > >
> > > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > >
> > > > > > Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> > > > > > this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> > > > > > which use that flag?
> > > > >
> > > > > As far as I can tell, after fileattr_fill_xflags() call in
> > > > > ioctl_fssetxattr(), the call
> > > > > to ext4_fileattr_set() should behave exactly the same if it came some
> > > > > FS_IOC_FSSETXATTR or from FS_IOC_SETFLAGS.
> > > > > IOW, EXT4_FL_USER_MODIFIABLE mask will still apply.
> > > > >
> > > > > However, unlike the legacy API, we now have an opportunity to deal with
> > > > > EXT4_FL_USER_MODIFIABLE better than this:
> > > > >         /*
> > > > >          * chattr(1) grabs flags via GETFLAGS, modifies the result and
> > > > >          * passes that to SETFLAGS. So we cannot easily make SETFLAGS
> > > > >          * more restrictive than just silently masking off visible but
> > > > >          * not settable flags as we always did.
> > > > >          */
> > > > >
> > > > > if we have the xflags_mask in the new API (not only the xflags) then
> > > > > chattr(1) can set EXT4_FL_USER_MODIFIABLE in xflags_mask
> > > > > ext4_fileattr_set() can verify that
> > > > > (xflags_mask & ~EXT4_FL_USER_MODIFIABLE == 0).
> > > > >
> > > > > However, Pali, this is an important point that your RFC did not follow -
> > > > > AFAICT, the current kernel code of ext4_fileattr_set() and xfs_fileattr_set()
> > > > > (and other fs) does not return any error for unknown xflags, it just
> > > > > ignores them.
> > > > >
> > > > > This is why a new ioctl pair FS_IOC_[GS]ETFSXATTR2 is needed IMO
> > > > > before adding support to ANY new xflags, whether they are mapped to
> > > > > existing flags like in this patch or are completely new xflags.
> > > > >
> > > > > Thanks,
> > > > > Amir.
> > > >
> > > > But xflags_mask is available in this new API. It is available if the
> > > > FS_XFLAG_HASEXTFIELDS flag is set. So I think that the ext4 improvement
> > > > mentioned above can be included into this new API.
> > > >
> > > > Or I'm missing something?
> > >
> > > Yes, you are missing something very fundamental to backward compat API -
> > > You cannot change the existing kernels.
> > >
> > > You should ask yourself one question:
> > > What happens if I execute the old ioctl FS_IOC_FSSETXATTR
> > > on an existing old kernel with the new extended flags?
> >
> > It should ignore all the things it does not know about.
> >
> 
> I don't know about "should" but it certainly does, that's why I was
> wondering if a new fresh ioctl was in order before extending to new flags.

Not all filesystems ignore unknown flags. For example fs/ntfs3/file.c
function ntfs_fileattr_set() already contains:

	if (fileattr_has_fsx(fa))
		return -EOPNOTSUPP;

	if (flags & ~(FS_IMMUTABLE_FL | FS_APPEND_FL | FS_COMPR_FL))
		return -EOPNOTSUPP;

So if it is called with FS_XFLAG_HASEXTFIELDS (required for fsx_xflags2)
then kernel already returns -EOPNOTSUPP.

But some filesystems like ext4 does not contain that
fileattr_has_fsx(fa) check.

> > But the correct usage of FS_IOC_FSSETXATTR is to call
> > FS_IOC_FSGETXATTR to initialise the structure with all the current
> > inode state.
> 
> Yeh, this is how the API is being used by exiting xfs_io chattr.
> It does not mean that this is a good API.
> 
> > In this situation, the fsx.fsx_xflags field needs to
> > return a flag that says "FS_XFLAGS_HAS_WIN_ATTRS" and now userspace
> > knows that it can set/clear the MS windows attr flags.

It does not say which windows attributes. For example UDF filesystem
supports only HIDDEN windows attribute. FAT32 supports more (+ READONLY,
SYSTEM, ARCHIVE), NTFS even more (+ TEMPORARY, OFFLINE, ...). And SMB
and ReFS even more (+ INTEGRITY). And then we have there NFS4 which
supports another subset of those windows attributes.

So how would userspace know if the OFFLINE attribute is supported or
not? OFFLINE is one which more people mentioned in these discussion that
want to see support for it.

That is why I introduced new mask field, which in FS_IOC_FSGETXATTR
ioctl response is filled with supported attributes and each filesystem
will return what it supports.

> > Hence if the
> > filesystem supports windows attributes, we can require them to
> > -support the entire set-.

We cannot because even the most commonly used NTFS filesystem does not
support the entire set.

> >
> > i.e. if an attempt to set a win attr that the filesystem cannot
> > implement (e.g. compression) then it returns an error (-EINVAL),
> > otherwise it will store the attr and perform whatever operation it
> > requires.
> >
> 
> I prefer not to limit the discussion to new "win" attributes,
> especially when discussing COMPR/ENCRYPT flags which are existing
> flags that are also part of the win attributes.

+1

> To put it another way, suppose Pali did not come forward with a patch set
> to add win attribute control to smb, but to add ENCRYPT support to xfs.

I agree. I chose smb in this RFC as an example to demonstrate as much
attribute as possible.

Sure I could choice xfs or udf in RFC to implement just one attribute
(ENCRYPT in xfs, HIDDEN in udf). But such example with just one
attribute would be less useful for demonstration.

> What would have been your prefered way to set the ENCRYPT flag in xfs?
> via FS_IOC_SETFLAGS or by extending FS_IOC_FSSETXATTR?
> and if the latter, then how would xfs_io chattr be expected to know if
> setting the ENCRYPT flag is supported or not?
> 
> > IMO, the whole implementation in the patchset is wrong - there is no
> > need for a new xflags2 field,
> 
> xflags2 is needed to add more bits. I am not following.

Theoretically it is possible to move all bits from xflags2 into xflags.
But if I'm counting correctly then there would be just one or two free
bits in xflags.

> > and there is no need for whacky field
> > masks or anything like that. All it needs is a single bit to
> > indicate if the windows attributes are supported, and they are all
> > implemented as normal FS_XFLAG fields in the fsx_xflags field.
> >

If MS adds 3 new attributes then we cannot add them to fsx_xflags
because all bits of fsx_xflags would be exhausted.

> Sorry, I did not understand your vision about the API.
> On the one hand, you are saying that there is no need for xflags2.
> On the other hand, that new flags should be treated differently than
> old flags (FS_XFLAGS_HAS_WIN_ATTRS).
> Either I did not understand what you mean or the documentation of
> what you are proposing sounds terribly confusing to me.

I understood it as:
- add FS_XFLAGS_HAS_WIN_ATTRS bit into fsx_xflags
- for every windows attribute add FS_XFLAG_<attr>
That is for sure possible but the space of fsx_xflags would be
exhausted.

> > > The answer, to the best of my code emulation abilities is that
> > > old kernel will ignore the new xflags including FS_XFLAG_HASEXTFIELDS
> > > and this is suboptimal, because it would be better for the new chattr tool
> > > to get -EINVAL when trying to set new xflags and mask on an old kernel.
> >
> > What new chattr tool? I would expect that xfs_io -c "chattr ..."
> > will be extended to support all these new fields because that is the
> > historical tool we use for FS_IOC_FS{GS}ETXATTR regression test
> > support in fstests. I would also expect that the e2fsprogs chattr(1)
> > program to grow support for the new FS_XFLAGS bits as well...
> >
> 
> That's exactly what I meant by "new chattr tool".
> when user executes chattr +i or xfs_io -c "chattr +i"
> user does not care which ioctl is used and it is best if both
> utils will support the entire set of modern flags with the new ioctls
> so that eventually (after old fs are deprecated) the old ioctl may also
> be deprecated.
> 
> > > It is true that the new chattr can call the old FS_IOC_FSGETXATTR
> > > ioctl and see that it has no FS_XFLAG_HASEXTFIELDS,
> > > so I agree that a new ioctl is not absolutely necessary,
> > > but I still believe that it is a better API design.
> >
> > This is how FS{GS}ETXATTR interface has always worked. You *must*
> > do a GET before a SET so that the fsx structure has been correctly
> > initialised so the SET operation makes only the exact change being
> > asked for.
> >
> > This makes the -API pair- backwards compatible by allowing struct
> > fsxattr feature bits to be reported in the GET operation. If the
> > feature bit is set, then those optional fields can be set. If the
> > feature bit is not set by the GET operation, then if you try to use
> > it on a SET operation you'll either get an error or it will be
> > silently ignored.

I think that this is perfectly fine, it is possible to implement it.
Personally I do not see a problem with this option, just it needs to be
decided what people wants.

> Yes, I know. I still think that this is a poor API design pattern.

I agree that this is also not my preferred API design. But I also
understand the argument "it is already there, so follow it".

> Imagine that people will be interested to include the fsxattr
> in rsync or such (it has been mentioned in the context of this effort)
> The current API is pretty useless for backup/restore and for
> copying supported attributes across filesystems.

I would not say that it is useless. It is still better than nothing. And
this API also allows to fully implement backup/restore functionality.
Just would require more calls:

    if (ioctl(orig_fd, FS_IOC_FSGETXATTR, &orig_attrs) != 0) goto fail;
    if (ioctl(back_fd, FS_IOC_FSGETXATTR, &back_attrs) != 0) gott fail;
    back_attrs.fsx_xflags = orig_attrs.fsx_xflags
    if (ioctl(back_fd, FS_IOC_FSSETXATTR, &back_attrs) != 0) goto fail;
    if (ioctl(back_fd, FS_IOC_FSGETXATTR, &back_attrs) != 0) goto fail;
    if (back_attrs.fsx_xflags != orig_attrs.fsx_xflags) goto fail;

> BTW, the internal ->fileattr_[gs]et() vfs API was created so that
> overlayfs could copy flags between filesystems on copy-up,
> but right now it only copies common flags.

This can be improved/extended.

> Adding a support mask to the API will allow smarter copy of
> supported attributes between filesystems that have a more
> specific common set of supported flags.
> 
> > > Would love to hear what other fs developers prefer.
> >
> > Do not overcomplicate the problem. Use the interface as intended:
> > GET then SET, and GET returns feature bits in the xflags field to
> > indicate what is valid in the overall struct fsxattr. It's trivial
> > to probe for native fs support of windows attributes like this. It
> > also allows filesystems to be converted one at a time to support the
> > windows attributes (e.g. as they have on-disk format support for
> > them added). Applications like Samba can then switch behaviours
> > based on what they probe from the underlying filesystem...
> >
> 
> OK, so we have two opinions.
> Debating at design time is much better than after API is released...
> 
> I'd still like to hear from other stakeholders before we perpetuate
> the existing design pattern which requires apps to GET before SET
> and treat new (WIN) flags differently than old flags.
> 
> Thanks,
> Amir.

I would like to know how to move forward. If we want to remove mask
fields or let them here. If we want to move all xflags2 to xflags or let
them split. I think that all of those are options which I can implement.
I'm open for any variant.

Just having only one FS_XFLAGS_HAS_WIN_ATTRS flag for determining windows
attribute support is not enough, as it would not say anything useful for
userspace.

On Tue, Feb 18, 2025 at 08:27:01PM +0100, Pali Rohár wrote:
> On Tuesday 18 February 2025 10:13:46 Amir Goldstein wrote:
> > > and there is no need for whacky field
> > > masks or anything like that. All it needs is a single bit to
> > > indicate if the windows attributes are supported, and they are all
> > > implemented as normal FS_XFLAG fields in the fsx_xflags field.
> > >
> 
> If MS adds 3 new attributes then we cannot add them to fsx_xflags
> because all bits of fsx_xflags would be exhausted.

And then we can discuss how to extend the fsxattr structure to
implement more flags.

In this scenario we'd also need another flag bit to indicate that
there is a second set of windows attributes that are supported...

i.e. this isn't a problem we need to solve right now.

> Just having only one FS_XFLAGS_HAS_WIN_ATTRS flag for determining windows
> attribute support is not enough, as it would not say anything useful for
> userspace.

IDGI.

That flag is only needed to tell userspace "this filesystem supports
windows attributes". Then GET will return the ones that are set,
and SET will return -EINVAL for those that it can't set (e.g.
compress, encrypt). What more does userspace actually need?

-Dave.
-- 
Dave Chinner
david@fromorbit.com

On Wednesday 19 February 2025 09:56:28 Dave Chinner wrote:
> On Tue, Feb 18, 2025 at 08:27:01PM +0100, Pali Rohár wrote:
> > On Tuesday 18 February 2025 10:13:46 Amir Goldstein wrote:
> > > > and there is no need for whacky field
> > > > masks or anything like that. All it needs is a single bit to
> > > > indicate if the windows attributes are supported, and they are all
> > > > implemented as normal FS_XFLAG fields in the fsx_xflags field.
> > > >
> > 
> > If MS adds 3 new attributes then we cannot add them to fsx_xflags
> > because all bits of fsx_xflags would be exhausted.
> 
> And then we can discuss how to extend the fsxattr structure to
> implement more flags.
> 
> In this scenario we'd also need another flag bit to indicate that
> there is a second set of windows attributes that are supported...
> 
> i.e. this isn't a problem we need to solve right now.

Ok, that is possible solution for now.

> > Just having only one FS_XFLAGS_HAS_WIN_ATTRS flag for determining windows
> > attribute support is not enough, as it would not say anything useful for
> > userspace.
> 
> IDGI.
> 
> That flag is only needed to tell userspace "this filesystem supports
> windows attributes". Then GET will return the ones that are set,
> and SET will return -EINVAL for those that it can't set (e.g.
> compress, encrypt). What more does userspace actually need?

Userspace backup utility would like to backup as many attributes as
possible by what is supported by the target filesystem. What would such
utility would do if the target filesystem supports only HIDDEN
attribute, and source file has all windows attributes set? It would be
needed to issue 2*N syscalls in the worst case to set attributes.
It would be combination of GET+SET for every one windows attribute
because userspace does not know what is supported and what not.

IMHO this is suboptimal. If filesystem would provide API to get list of
supported attributes then this can be done by 2-3 syscalls.

On Wed, Feb 19, 2025 at 12:06 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Wednesday 19 February 2025 09:56:28 Dave Chinner wrote:
> > On Tue, Feb 18, 2025 at 08:27:01PM +0100, Pali Rohár wrote:
> > > On Tuesday 18 February 2025 10:13:46 Amir Goldstein wrote:
> > > > > and there is no need for whacky field
> > > > > masks or anything like that. All it needs is a single bit to
> > > > > indicate if the windows attributes are supported, and they are all
> > > > > implemented as normal FS_XFLAG fields in the fsx_xflags field.
> > > > >
> > >
> > > If MS adds 3 new attributes then we cannot add them to fsx_xflags
> > > because all bits of fsx_xflags would be exhausted.
> >
> > And then we can discuss how to extend the fsxattr structure to
> > implement more flags.
> >
> > In this scenario we'd also need another flag bit to indicate that
> > there is a second set of windows attributes that are supported...
> >
> > i.e. this isn't a problem we need to solve right now.
>
> Ok, that is possible solution for now.
>
> > > Just having only one FS_XFLAGS_HAS_WIN_ATTRS flag for determining windows
> > > attribute support is not enough, as it would not say anything useful for
> > > userspace.
> >
> > IDGI.
> >
> > That flag is only needed to tell userspace "this filesystem supports
> > windows attributes". Then GET will return the ones that are set,
> > and SET will return -EINVAL for those that it can't set (e.g.
> > compress, encrypt). What more does userspace actually need?

Let me state my opinion clearly.
I think this API smells.
I do not object to it, but I think we can do better.

I do however object to treating different flags in fsx_xflags
differently - this is unacceptable IMO.

The difference I am referring to is a nuance, but IMO an important one -

It's fine for GET to raise a flag "this filesystem does not accept SET
of any unknown flags".

It's not fine IMO for GET to raise a flag "this filesystem does not accept
SET of unknown flags except for a constant subset of flags that filesystem
may ignore".
It's not fine IMO, because it makes userspace life harder for no good reason.

This former still allows filesystems to opt-in one by one to the extended API,
but it does not assume anything about the subset of windows attributes
and legacy Linux attributes that need to be supported.

For example, adding support for set/get hidden/system/archive/readonly
fo fs/fat, does not require supporting all FS_XFLAG_COMMON by fs/fat
and an attempt to set unsupported FS_XFLAG_COMMON flags would
result in -EINVAL just as an attempt to set an unsupported win flag.

But if we agree on setting one special flag in GET to say "new SET
semantics", I do not understand the objection to fsx_xflags_mask.

Dave, if you actually object to fsx_xflags_mask please explain why.
"What more does userspace actually need?" is not a good enough
reason IMO. Userspace could make use of fsx_xflags_mask.

>
> Userspace backup utility would like to backup as many attributes as
> possible by what is supported by the target filesystem. What would such
> utility would do if the target filesystem supports only HIDDEN
> attribute, and source file has all windows attributes set? It would be
> needed to issue 2*N syscalls in the worst case to set attributes.
> It would be combination of GET+SET for every one windows attribute
> because userspace does not know what is supported and what not.
>
> IMHO this is suboptimal. If filesystem would provide API to get list of
> supported attributes then this can be done by 2-3 syscalls.

I agree that getting the "attributes supported by filesystem" is important
and even getting the "gettable" subset and "settable" subset and I also
agree with Dave that this could be done once and no need to do it for
every file (although different file types may support a different subsets).

Let's stop for a moment to talk about statx.

I think you should include a statx support path in your series - not later.
If anything, statx support for exporting those flags should be done
before adding the GET/SET API.

Why? because there is nothing controversial about it.
- add a bunch of new STATX_ATTR_ flags
- filesystems that support them will publish that in stx_attributes_mask
- COMPR/ENCRYPT are already exported in statx

With that, backup/sync programs are able to query filesystem support
even without fsx_xflags_mask.

I think this is a hacky way around a proper GET/SET API, but possible.

Thanks,
Amir.

I think a few people were talking past each other, because there are two
fileds in struct fileattr --- flags, and fsx_xflags.  The flags field
is what was originally used by FS_IOC_EXT2_[GS]ETFLAGS, which later
started getting used by many other file systems, starting with
resierfs and btrfs, and so it became FS_IOC_[GS]ETFLAGS.  The bits in
that flags word were both the ioctl ABI and the on-disk encoding, and
because we were now allowing multiple file systems to allocate bits,
and we needed to avoid stepping on each other (for example since btrfs
started using FS_NOCOW_FL, that bit position wouldn't be used by ext4,
at least not for a publically exported flag).

So we started running out of space in the FS_FLAG_*_FL namespace, and
that's why we created FS_IOC_[GS]ETXATTR and the struct fsxattr.  The
FS_XFLAG_*_FL space has plenty of space; there are 14 unassigned bit
positions, by my count.

As far as the arguments about "proper interface design", as far as
Linux is concerned, backwards compatibility trumps "we should have
done if it differently".  The one and only guarantee that we have that
FS_IOC_GETXATTR followed by FS_IOC_SETXATTR will work.  Nothing else.

The use case of "what if a backup program wants to backup the flags
and restore on a different file system" is one that hasn't been
considered, and I don't think any backup programs do it today.  For
that matter, some of the flags, such as the NODUMP flag, are designed
to be instructions to a dump/restore system, and not really one that
*should* be backed up.  Again, the only semantic that was guaranteed
is GETXATTR or GETXATTR followed by SETXATTR.

We can define some new interface for return what xflags are supported
by a particular file system.  This could either be the long-debated,
but never implemented statfsx() system call.  Or it could be extending
what gets returned by FS_IOC_GETXATTR by using one of the fs_pad
fields in struct fsxattr.  This is arguably the simplest way of
dealing with the problem.

I suppose the field could double as the bitmask field when
FS_IOC_SETXATTR is called, but that just seems to be an overly complex
set of semantics.  If someone really wants to do that, I wouldn't
really complain, but then what we would actually call the field
"flags_supported_on_get_bitmask_on_set" would seem a bit wordy.  :-)

				      	     - Ted

On Fri, Feb 21, 2025 at 11:34:43AM -0500, Theodore Ts'o wrote:
> I think a few people were talking past each other, because there are two
> fileds in struct fileattr --- flags, and fsx_xflags.  The flags field
> is what was originally used by FS_IOC_EXT2_[GS]ETFLAGS, which later

I don't think anyone has been confusing the two - the entire
discussion has been about fsx_xflags and the struct fsxattr...

> started getting used by many other file systems, starting with
> resierfs and btrfs, and so it became FS_IOC_[GS]ETFLAGS.  The bits in
> that flags word were both the ioctl ABI and the on-disk encoding, and
> because we were now allowing multiple file systems to allocate bits,
> and we needed to avoid stepping on each other (for example since btrfs
> started using FS_NOCOW_FL, that bit position wouldn't be used by ext4,
> at least not for a publically exported flag).
> 
> So we started running out of space in the FS_FLAG_*_FL namespace, and
> that's why we created FS_IOC_[GS]ETXATTR and the struct fsxattr.  The

No, that is most certainly not how this API came about. 

The FS_IOC_[GS]ETXATTR ioctls were first implement on IRIX close on
30 years ago. They were ported to Linux with the XFS linux port over
2 decades ago. Indeed, we've been using them for xfsdump/xfs_restore
since before XFS was ported to linux.

They got lifted to the VFS back in 2016 so that ext4 could use the
interface for getting/setting project IDs on files. This was done so
that existing userspace functionality for setting up
project/directory quotas on XFS could also be used on ext4.

> FS_XFLAG_*_FL space has plenty of space; there are 14 unassigned bit
> positions, by my count.
> 
> As far as the arguments about "proper interface design", as far as
> Linux is concerned, backwards compatibility trumps "we should have
> done if it differently".  The one and only guarantee that we have that
> FS_IOC_GETXATTR followed by FS_IOC_SETXATTR will work.  Nothing else.

That's a somewhat naive understanding of the overall API. The struct
fsxattr information is also directly exported to userspace via the
XFS blukstat ioctls. i.e. extent size hints, fsx_xflags, project
IDs, etc are all exported to userspace via multiple ioctl
interfaces.

This is all used by xfsdump/xfs_restore to be able to back up and
restore the inode state that is exposed/controlled by the
GET/SETXATTR interfaces.

> The use case of "what if a backup program wants to backup the flags
> and restore on a different file system" is one that hasn't been
> considered, and I don't think any backup programs do it today.

Wrong. As I've already said: we have been doing exactly this for 20+
years with xfsdump/restore.

xfsdump uses the bulkstat version of the GET interface, whilst
restore uses the FS_IOC_SETXATTR interface.

> For
> that matter, some of the flags, such as the NODUMP flag, are designed
> to be instructions to a dump/restore system, and not really one that
> *should* be backed up.

Yes. xfsdump sees this in the bulkstat flags field for the inode and
then omits the inode from the dump.

Further, xfs_fsr (the online file defragmenter for XFS) uses
bulkstat and looks at the FS_XFLAGS returned from bulkstat for each
inode it scans.

> Again, the only semantic that was guaranteed
> is GETXATTR or GETXATTR followed by SETXATTR.

For making a single delta state change, yes.

For the dump/restore case, calling SETXATTR on a newly created file
with a preconstructed struct fsxattr state retreived at dump time is
also supported.

This is not a general use case - it will destroy any existing state
that file was created with (e.g. override admin inheritence
settings) by overwriting it with the state from the backup.

It should also be noted that xfs_restore does this in two SETXATTR
calls, not one. i.e. it splits the set operation into a
pre-data restore SETXATTR, and one post-data restore SETXATTR.

Why?

Because stuff like extent size hints and realtime state needs to be
restored before any data is written whilst others can only be set
after the data has been written because they would otherwise prevent
data restoration:

/* extended inode flags that can only be set after all data
 * has been restored to a file.
 */
#define POST_DATA_XFLAGS        (XFS_XFLAG_IMMUTABLE |          \
                                  XFS_XFLAG_APPEND |            \
                                  XFS_XFLAG_SYNC)

Yup, you can't restore data to the file if it has already been
marked as immutable....

IOWs, any changes to the flag space also needs to be compatible with
the XFS bulkstat shadowing of the fsxattr fields+flags and the
existing usage of these APIs by xfsdump, xfs_restore and xfs_fsr.

> We can define some new interface for return what xflags are supported
> by a particular file system.

Why do we even care?

On the get side, it just doesn't matter - if the flag isn't set, it
either is not active or not supported. Either way, it doesn't
matter if there's a "this is supported mask".

On the set side, adding a mask isn't going to change historic
behaviour: existing applications will ignore the new mask because
they haven't been coded to understand it. And vice versa, an old
kernel will ignore the feature mask if the app uses it because it
ignores unknown flags/fields.

IOWs, adding a feature mask doesn't solve any of the problems
related to forwards/backwards compatibility of new features, and so
we are back to needing to use the API as a GET/SET pair where the
GET sets all the known state correctly such that a SET operation
will either do nothing, change the state or return an error because
an invalid combination of known parameters was passed.

> I suppose the field could double as the bitmask field when
> FS_IOC_SETXATTR is called, but that just seems to be an overly complex
> set of semantics.  If someone really wants to do that, I wouldn't
> really complain, but then what we would actually call the field
> "flags_supported_on_get_bitmask_on_set" would seem a bit wordy.  :-)

That effectively prevents the existing dump/restore usage of the
API.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

On Feb 21, 2025, at 9:34 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> 
> We can define some new interface for return what xflags are supported
> by a particular file system.  This could either be the long-debated,
> but never implemented statfsx() system call.  Or it could be extending
> what gets returned by FS_IOC_GETXATTR by using one of the fs_pad
> fields in struct fsxattr.  This is arguably the simplest way of
> dealing with the problem.
> 
> I suppose the field could double as the bitmask field when
> FS_IOC_SETXATTR is called, but that just seems to be an overly complex
> set of semantics.  If someone really wants to do that, I wouldn't
> really complain, but then what we would actually call the field
> "flags_supported_on_get_bitmask_on_set" would seem a bit wordy.  :-)

The nice thing about allowing the bitmask on SET to mean "only set/clear
the specified fields" is that this allows a race-free mechanism to change
the flags, whereas GET+SET could be racy between two callers.

I don't think the two uses are incompatible.  If called as GET+SET, where
the GET will return the flags+mask, then any flag bits set/cleared should
also be in mask when SET, and all of the other bits are reset to the same
value.  If called as "SET flags+mask" with a limited number of bits, only
those bits in mask would be set/cleared, and the other bits would be left
as-is.

Cheers, Andreas

On Fri, Feb 21, 2025 at 5:34 PM Theodore Ts'o <tytso@mit.edu> wrote:
>
> I think a few people were talking past each other, because there are two
> fileds in struct fileattr --- flags, and fsx_xflags.  The flags field
> is what was originally used by FS_IOC_EXT2_[GS]ETFLAGS, which later
> started getting used by many other file systems, starting with
> resierfs and btrfs, and so it became FS_IOC_[GS]ETFLAGS.  The bits in
> that flags word were both the ioctl ABI and the on-disk encoding, and
> because we were now allowing multiple file systems to allocate bits,
> and we needed to avoid stepping on each other (for example since btrfs
> started using FS_NOCOW_FL, that bit position wouldn't be used by ext4,
> at least not for a publically exported flag).
>
> So we started running out of space in the FS_FLAG_*_FL namespace, and
> that's why we created FS_IOC_[GS]ETXATTR and the struct fsxattr.  The
> FS_XFLAG_*_FL space has plenty of space; there are 14 unassigned bit
> positions, by my count.
>
> As far as the arguments about "proper interface design", as far as
> Linux is concerned, backwards compatibility trumps "we should have
> done if it differently".  The one and only guarantee that we have that
> FS_IOC_GETXATTR followed by FS_IOC_SETXATTR will work.  Nothing else.
>
> The use case of "what if a backup program wants to backup the flags
> and restore on a different file system" is one that hasn't been
> considered, and I don't think any backup programs do it today.  For
> that matter, some of the flags, such as the NODUMP flag, are designed
> to be instructions to a dump/restore system, and not really one that
> *should* be backed up.  Again, the only semantic that was guaranteed
> is GETXATTR or GETXATTR followed by SETXATTR.
>

Thanks for chiming in, Ted!
Notes from the original author of FS_IOC_EXT2_[GS]ETFLAGS
are valuable.

> We can define some new interface for return what xflags are supported
> by a particular file system.  This could either be the long-debated,
> but never implemented statfsx() system call.  Or it could be extending
> what gets returned by FS_IOC_GETXATTR by using one of the fs_pad
> fields in struct fsxattr.  This is arguably the simplest way of
> dealing with the problem.
>

That is also what I think.
fsx_xflags_mask semantics for GET are pretty clear
and follows the established pattern of  stx_attributes_mask
Even if it is not mandatory for userspace, it can be useful.

I asked Dave if he objects to fsx_xflags_mask and got silence,
so IMO, if Pali wants to implement fsx_xflags_mask for the API
I see no reason to resist it.

> I suppose the field could double as the bitmask field when
> FS_IOC_SETXATTR is called, but that just seems to be an overly complex
> set of semantics.  If someone really wants to do that, I wouldn't
> really complain, but then what we would actually call the field
> "flags_supported_on_get_bitmask_on_set" would seem a bit wordy.  :-)

If we follow the old rule of SET after GET should always work
then fsx_xflags_mask will always be a superset of fsx_xflags,
so I think it would be sane to return -EINVAL in the case
of (fsx_xflags_mask && fsx_xflags & ~fsx_xflags_mask),
which is anyway the correct behavior for filesystems when the
user is trying to set flags that the filesystem does not support.

As far as I could see, all in-tree filesystems behave this way
when the user is trying to set unsupported flags either via
FS_IOC_SETFLAGS or via FS_IOC_SETXATTR
except xfs, which ignores unsupported fsx_xflags from
FS_IOC_SETXATTR.

Changing the behavior of xfs to return -EINVAL for unsupported
fsx_xflags if fsx_xflags_mask is non zero is NOT going to break UAPI,
because as everyone keeps saying, the only guarantee from
FS_IOC_SETXATTR was that FS_IOC_SETXATTR after
FS_IOC_GETXATTR works and that guarantee will not be broken.

Thanks,
Amir.

On Fri, Feb 21, 2025 at 06:11:43PM +0100, Amir Goldstein wrote:
> On Fri, Feb 21, 2025 at 5:34 PM Theodore Ts'o <tytso@mit.edu> wrote:
> >
> > I think a few people were talking past each other, because there are two
> > fileds in struct fileattr --- flags, and fsx_xflags.  The flags field
> > is what was originally used by FS_IOC_EXT2_[GS]ETFLAGS, which later
> > started getting used by many other file systems, starting with
> > resierfs and btrfs, and so it became FS_IOC_[GS]ETFLAGS.  The bits in
> > that flags word were both the ioctl ABI and the on-disk encoding, and
> > because we were now allowing multiple file systems to allocate bits,
> > and we needed to avoid stepping on each other (for example since btrfs
> > started using FS_NOCOW_FL, that bit position wouldn't be used by ext4,
> > at least not for a publically exported flag).
> >
> > So we started running out of space in the FS_FLAG_*_FL namespace, and
> > that's why we created FS_IOC_[GS]ETXATTR and the struct fsxattr.  The
> > FS_XFLAG_*_FL space has plenty of space; there are 14 unassigned bit
> > positions, by my count.
> >
> > As far as the arguments about "proper interface design", as far as
> > Linux is concerned, backwards compatibility trumps "we should have
> > done if it differently".  The one and only guarantee that we have that
> > FS_IOC_GETXATTR followed by FS_IOC_SETXATTR will work.  Nothing else.
> >
> > The use case of "what if a backup program wants to backup the flags
> > and restore on a different file system" is one that hasn't been
> > considered, and I don't think any backup programs do it today.  For
> > that matter, some of the flags, such as the NODUMP flag, are designed
> > to be instructions to a dump/restore system, and not really one that
> > *should* be backed up.  Again, the only semantic that was guaranteed
> > is GETXATTR or GETXATTR followed by SETXATTR.
> >
> 
> Thanks for chiming in, Ted!
> Notes from the original author of FS_IOC_EXT2_[GS]ETFLAGS
> are valuable.

..... except when they are completely wrong. FS_IOC_[GS]ETXATTR was
promoted from XFs to the VFS to enable ext4 to use the existing
project ID and quota infrastructure (e.g. for testing with fstests).

> 
> > We can define some new interface for return what xflags are supported
> > by a particular file system.  This could either be the long-debated,
> > but never implemented statfsx() system call.  Or it could be extending
> > what gets returned by FS_IOC_GETXATTR by using one of the fs_pad
> > fields in struct fsxattr.  This is arguably the simplest way of
> > dealing with the problem.
> >
> 
> That is also what I think.
> fsx_xflags_mask semantics for GET are pretty clear
> and follows the established pattern of  stx_attributes_mask
> Even if it is not mandatory for userspace, it can be useful.

You keep saying "it can be useful" without actually giving an
example of when it will be a significant improvement. Then you
demand proof that it isn't useful to userspace to justify a NACK.
That's not the way development works - you want the functionality,
you have to prove to that it is a significant improvement, not
demand that people prove that it isn't.

As it is, a lot of the "masks won't work" reasoning is in the
response I jsut wrote to Ted's email.  There appears to be a
significant lack of knowledge of the scope of the GET/SETXATTR
interfaces and how we've been using them for the past 20+ years
amongst the people arguing they should be fundamentally changed
right now.

> I asked Dave if he objects to fsx_xflags_mask and got silence,
> so IMO, if Pali wants to implement fsx_xflags_mask for the API
> I see no reason to resist it.

You didn't get silence - I only work 3 days a week and I'm
explicitly not responding to upstream devel list email outside of
work hours. So it may take several days before I find time to
respond to any given discussion thread.

As I've told you many, many times before, Amir: slow down and have
patience. There is no rush, nor is there a need to force the
discussion to a rapid conclusion.


> > I suppose the field could double as the bitmask field when
> > FS_IOC_SETXATTR is called, but that just seems to be an overly complex
> > set of semantics.  If someone really wants to do that, I wouldn't
> > really complain, but then what we would actually call the field
> > "flags_supported_on_get_bitmask_on_set" would seem a bit wordy.  :-)
> 
> If we follow the old rule of SET after GET should always work
> then fsx_xflags_mask will always be a superset of fsx_xflags,

Doesn't work for xfsdump/restore use case.

Firstly, there's no space in the dump media format for extended
xflags (i.e. requires new code and backwards incompatible media dump
formats to be created).

Secondly, if the destination kernel and/or filesystem does not
support the flags mask or features defined in the flags mask, what
do we do then?  The policy decision made a couple of decades ago was
that it is better for the kernel to ignore file attrs it doesn't
understand on restore and let the restore continue than it is to
abort the restore....

Better to restore what we support than not be able to restore
anything, yes?

> so I think it would be sane to return -EINVAL in the case
> of (fsx_xflags_mask && fsx_xflags & ~fsx_xflags_mask),
> which is anyway the correct behavior for filesystems when the
> user is trying to set flags that the filesystem does not support.
> 
> As far as I could see, all in-tree filesystems behave this way
> when the user is trying to set unsupported flags either via
> FS_IOC_SETFLAGS or via FS_IOC_SETXATTR
> except xfs, which ignores unsupported fsx_xflags from
> FS_IOC_SETXATTR.
> 
> Changing the behavior of xfs to return -EINVAL for unsupported
> fsx_xflags if fsx_xflags_mask is non zero is NOT going to break UAPI,

Except that it's completely undesireable behaviour for the existing
dump/restore usage of this interface.

If we just add the windows attributes to the fsx_flags field as
I have suggested, then xfsdump/xfs_restore would natively supports
backing up and restoring windows attributes on XFS files the moment
that the kernel XFS code supports windows attributes. 

It will not require any dump/restore code or dump media format
changes, and with the existing SET policy if the destination kernel
and/or filesystem doesn't support those attributes then they will be
silently dropped on restore...

Seriously, I said no because I undestand how these interfaces have
been used for the past 20 years. If you want to change the
fundamental behavioural characteristics of the API, then you have to
make sure that it works with the way xfsdump/restore use the API
across the dump media format. i.e. the consumer of the GET
information can be a SET operation on a different kernel and
filesystem.

I've already said no to a new syscall because it's just a set
of feature bits that can be added to FS_XFLAGS. And the added
advantage of that is that any backup program that already supports
backing up the fsxattr information will also support it without API
or dump media format changes. i.e. we get widespread support for the
windows attributes pretty much for free.

Yet here we are, with people instead wanting to construct complex
new APIs which will require entirely new infrastructure across the
board to support.

Your call - windows attribute support via a small amount of work for
an existing API addition, or a huge amount of work across the entire
filesystem ecosystem for a whole new API. The end functionality will
be identical, but one path is a whole lot less work for everyone
than the other....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

On Tuesday 25 February 2025 16:41:37 Dave Chinner wrote:
> Your call - windows attribute support via a small amount of work for
> an existing API addition, or a huge amount of work across the entire
> filesystem ecosystem for a whole new API. The end functionality will
> be identical, but one path is a whole lot less work for everyone
> than the other....
> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

So, would you rather see a change which defines all windows attributes
in fsx_xflags field, without any mask fields, and if there is no space
in fsx_xflags field anymore, define new fsx_xflags2 field?

I can prepare new RFC with this approach, and we can compare and discuss
what is better.

Just to note, that NFS4 protocol for supported subset of windows
attribute has for each attribute 3 state value: unsupported, unset, set.
So for knfsd it would be beneficial from filesystem driver to provide
information if particular attribute is supported or not (and not only if
attribute is set or unset). It does not have to be via this interface.
But my approach with mask field can easily provide this information.

On Sunday 16 February 2025 21:43:02 Amir Goldstein wrote:
> On Sun, Feb 16, 2025 at 9:24 PM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Sunday 16 February 2025 21:17:55 Amir Goldstein wrote:
> > > On Sun, Feb 16, 2025 at 7:34 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > >
> > > > On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > > > > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> > > > >
> > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > >
> > > > Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> > > > this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> > > > which use that flag?
> > >
> > > As far as I can tell, after fileattr_fill_xflags() call in
> > > ioctl_fssetxattr(), the call
> > > to ext4_fileattr_set() should behave exactly the same if it came some
> > > FS_IOC_FSSETXATTR or from FS_IOC_SETFLAGS.
> > > IOW, EXT4_FL_USER_MODIFIABLE mask will still apply.
> > >
> > > However, unlike the legacy API, we now have an opportunity to deal with
> > > EXT4_FL_USER_MODIFIABLE better than this:
> > >         /*
> > >          * chattr(1) grabs flags via GETFLAGS, modifies the result and
> > >          * passes that to SETFLAGS. So we cannot easily make SETFLAGS
> > >          * more restrictive than just silently masking off visible but
> > >          * not settable flags as we always did.
> > >          */
> > >
> > > if we have the xflags_mask in the new API (not only the xflags) then
> > > chattr(1) can set EXT4_FL_USER_MODIFIABLE in xflags_mask
> > > ext4_fileattr_set() can verify that
> > > (xflags_mask & ~EXT4_FL_USER_MODIFIABLE == 0).
> > >
> > > However, Pali, this is an important point that your RFC did not follow -
> > > AFAICT, the current kernel code of ext4_fileattr_set() and xfs_fileattr_set()
> > > (and other fs) does not return any error for unknown xflags, it just
> > > ignores them.
> > >
> > > This is why a new ioctl pair FS_IOC_[GS]ETFSXATTR2 is needed IMO
> > > before adding support to ANY new xflags, whether they are mapped to
> > > existing flags like in this patch or are completely new xflags.
> > >
> > > Thanks,
> > > Amir.
> >
> > But xflags_mask is available in this new API. It is available if the
> > FS_XFLAG_HASEXTFIELDS flag is set. So I think that the ext4 improvement
> > mentioned above can be included into this new API.
> >
> > Or I'm missing something?
> 
> Yes, you are missing something very fundamental to backward compat API -
> You cannot change the existing kernels.
> 
> You should ask yourself one question:
> What happens if I execute the old ioctl FS_IOC_FSSETXATTR
> on an existing old kernel with the new extended flags?
> 
> The answer, to the best of my code emulation abilities is that
> old kernel will ignore the new xflags including FS_XFLAG_HASEXTFIELDS
> and this is suboptimal, because it would be better for the new chattr tool
> to get -EINVAL when trying to set new xflags and mask on an old kernel.
> 
> It is true that the new chattr can call the old FS_IOC_FSGETXATTR
> ioctl and see that it has no FS_XFLAG_HASEXTFIELDS,

Yes, this was my intention how the backward and forward compatibility
will work. I thought that reusing existing IOCTL is better than creating
new IOCTL and duplicating functionality.

> so I agree that a new ioctl is not absolutely necessary,
> but I still believe that it is a better API design.

If it is a bad idea then for sure I can prepare new IOCTL and move all
new functionality only into the new IOCTL, no problem.

> Would love to hear what other fs developers prefer.
> 
> Thanks,
> Amir.

On Sun, Feb 16, 2025 at 10:17 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Sunday 16 February 2025 21:43:02 Amir Goldstein wrote:
> > On Sun, Feb 16, 2025 at 9:24 PM Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Sunday 16 February 2025 21:17:55 Amir Goldstein wrote:
> > > > On Sun, Feb 16, 2025 at 7:34 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > >
> > > > > On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > > > > > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> > > > > >
> > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > >
> > > > > Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> > > > > this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> > > > > which use that flag?
> > > >
> > > > As far as I can tell, after fileattr_fill_xflags() call in
> > > > ioctl_fssetxattr(), the call
> > > > to ext4_fileattr_set() should behave exactly the same if it came some
> > > > FS_IOC_FSSETXATTR or from FS_IOC_SETFLAGS.
> > > > IOW, EXT4_FL_USER_MODIFIABLE mask will still apply.
> > > >
> > > > However, unlike the legacy API, we now have an opportunity to deal with
> > > > EXT4_FL_USER_MODIFIABLE better than this:
> > > >         /*
> > > >          * chattr(1) grabs flags via GETFLAGS, modifies the result and
> > > >          * passes that to SETFLAGS. So we cannot easily make SETFLAGS
> > > >          * more restrictive than just silently masking off visible but
> > > >          * not settable flags as we always did.
> > > >          */
> > > >
> > > > if we have the xflags_mask in the new API (not only the xflags) then
> > > > chattr(1) can set EXT4_FL_USER_MODIFIABLE in xflags_mask
> > > > ext4_fileattr_set() can verify that
> > > > (xflags_mask & ~EXT4_FL_USER_MODIFIABLE == 0).
> > > >
> > > > However, Pali, this is an important point that your RFC did not follow -
> > > > AFAICT, the current kernel code of ext4_fileattr_set() and xfs_fileattr_set()
> > > > (and other fs) does not return any error for unknown xflags, it just
> > > > ignores them.
> > > >
> > > > This is why a new ioctl pair FS_IOC_[GS]ETFSXATTR2 is needed IMO
> > > > before adding support to ANY new xflags, whether they are mapped to
> > > > existing flags like in this patch or are completely new xflags.
> > > >
> > > > Thanks,
> > > > Amir.
> > >
> > > But xflags_mask is available in this new API. It is available if the
> > > FS_XFLAG_HASEXTFIELDS flag is set. So I think that the ext4 improvement
> > > mentioned above can be included into this new API.
> > >
> > > Or I'm missing something?
> >
> > Yes, you are missing something very fundamental to backward compat API -
> > You cannot change the existing kernels.
> >
> > You should ask yourself one question:
> > What happens if I execute the old ioctl FS_IOC_FSSETXATTR
> > on an existing old kernel with the new extended flags?
> >
> > The answer, to the best of my code emulation abilities is that
> > old kernel will ignore the new xflags including FS_XFLAG_HASEXTFIELDS
> > and this is suboptimal, because it would be better for the new chattr tool
> > to get -EINVAL when trying to set new xflags and mask on an old kernel.
> >
> > It is true that the new chattr can call the old FS_IOC_FSGETXATTR
> > ioctl and see that it has no FS_XFLAG_HASEXTFIELDS,
>
> Yes, this was my intention how the backward and forward compatibility
> will work. I thought that reusing existing IOCTL is better than creating
> new IOCTL and duplicating functionality.
>
> > so I agree that a new ioctl is not absolutely necessary,
> > but I still believe that it is a better API design.
>
> If it is a bad idea then for sure I can prepare new IOCTL and move all
> new functionality only into the new IOCTL, no problem.
>

Well, there is at least one flaw in using the get ioctl for support detection,
as Eric pointed out -
the settable xflags set is a subset of the gettable flags set.

Let's ask some stake holders shall we?

Ted, Darrick, Eric,

What is your opinion on the plan presented in this patch set to extend the API:

1. Add some of the *_FL flags to FS_XFLAG_COMMON [*]
2. Add fsx_xflags2 for more xflags
3. Add fsx_xflags{,2}_mask to declare the supported in/out xflags
4. Should we use the existing FS_IOC_FSSETXATTR ioctl which ignores
   setting unknown flags or define a new v2 ioctl FS_IOC_SETFSXATTR_V2
   which properly fails on unknown flags [**]

[*] we can consider adding all of the flags used by actively maintained fs to
    FS_XFLAGS_COMMON, something like the set of F2FS_GETTABLE_FS_FL,
    maybe even split them to FS_XFLAGS_COMMON_[GS]ETTABLE
[**] we can either return EINVAL for unknown flags or make the ioctl _IOWR
     and return the set of flags that were not ignored

Thanks,
Amir.

On Sunday 16 February 2025 10:34:32 Eric Biggers wrote:
> On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> which use that flag?  In the fscrypt case it's very intentional that
> FS_ENCRYPT_FL can be gotten via FS_IOC_GETFLAGS but not set via FS_IOC_SETFLAGS.
> A simple toggle of the flag can't work, as it doesn't provide the needed
> information.  Instead there is a separate ioctl (FS_IOC_SET_ENCRYPTION_POLICY)
> for enabling encryption which takes additional parameters and only works on
> empty directories.
> 
> - Eric

This encrypt flag I have not implemented in the last cifs patch.
For SMB it needs to use additional SMB IOCTL which is not supported yet.
So I have not looked at that deeply yet.

I tested only that setting and clearing compression bit is working over
cifs SMB client, via that additional SMB IOCTL.