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@xxxxxxx> 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@xxxxxxxxxxxxx
