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