Message-ID: <CAOQ4uxjwQJiKAqyjEmKUnq-VihyeSsxyEy2F+J38NXwrAXurFQ@mail.gmail.com>
Date: Fri, 21 Feb 2025 18:11:43 +0100
From: Amir Goldstein <amir73il@...il.com>
To: "Theodore Ts'o" <tytso@....edu>
Cc: Pali Rohár <pali@...nel.org>, 
	Dave Chinner <david@...morbit.com>, Eric Biggers <ebiggers@...nel.org>, 
	"Darrick J. Wong" <djwong@...nel.org>, ronnie sahlberg <ronniesahlberg@...il.com>, 
	Chuck Lever <chuck.lever@...cle.com>, Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, 
	Steve French <sfrench@...ba.org>, Alexander Viro <viro@...iv.linux.org.uk>, 
	linux-fsdevel@...r.kernel.org, linux-cifs@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED
 for FS_IOC_FS[GS]ETXATTR API

On Fri, Feb 21, 2025 at 5:34 PM Theodore Ts'o <tytso@....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.

Powered by blists - more mailing lists