| Message ID | 20250216164029.20673-2-pali@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs: Add support for Windows file attributes | expand |
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 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.
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 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 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.
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 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 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.
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.
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.
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
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(-)