Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20220111074309.GA12918@kili> <Yd1ETmx/HCigOrzl@infradead.org>
In-Reply-To: <Yd1ETmx/HCigOrzl@infradead.org>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Wed, 12 Jan 2022 09:57:58 +0200
Message-ID: <CAOQ4uxg9V4Jsg3jRPnsk2AN7gPrNY8jRAc87tLvGW+TqH9OU-A@mail.gmail.com>
Subject: Re: [bug report] NFS: Support statx_get and statx_set ioctls
To:     Christoph Hellwig <hch@infradead.org>,
        Trond Myklebust <trond.myklebust@hammerspace.com>
Cc:     Dan Carpenter <dan.carpenter@oracle.com>,
        richard.sharpe@primarydata.com,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        lance.shelton@hammerspace.com,
        Anna Schumaker <Anna.Schumaker@netapp.com>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Linux API <linux-api@vger.kernel.org>,
        CIFS <linux-cifs@vger.kernel.org>, ntfs3@lists.linux.dev,
        Steve French <sfrench@samba.org>,
        Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jan 12, 2022 at 4:10 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Jan 11, 2022 at 10:43:09AM +0300, Dan Carpenter wrote:
> > Hello Richard Sharpe,
> >
> > This is a semi-automatic email about new static checker warnings.
> >
> > The patch bc66f6805766: "NFS: Support statx_get and statx_set ioctls"
> > from Dec 27, 2021, leads to the following Smatch complaint:
>
> Yikes, how did that crap get merged?

Did it? The bots are scanning through patches on ML:

https://lore.kernel.org/linux-nfs/20211227190504.309612-1-trondmy@kernel.org/

> Why the f**k does a remote file system need to duplicate stat?
> This kind of stuff needs a proper discussion on linux-fsdevel.

+ntfs3 +linux-cifs +linux-api

The proposal of statx_get() is very peculiar.
statx() was especially designed to be extended and accommodate
a diversity of filesystem attributes.

Moreover, NFSv4 is not the only fs that supports those extra attributes.
ntfs3 supports set/get of dos attrib bits via xattr SYSTEM_NTFS_ATTRIB.
cifs support set/get of CIFS_XATTR_ATTRIB and CIFS_XATTR_CREATETIME.

Not only that, but Linux now has ksmbd which actually emulates
those attributes on the server side (like samba) by storing a samba
formatted blob in user.DOSATTRIB xattr.
It should have a way to get/set them on filesystems that support them
natively.

The whole thing shouts for standardization.

Samba should be able to get/set the extra attributes by statx() and
ksmbd should be able to get them from the filesystem by vfs_getattr().

WRT statx_set(), standardization is also in order, both for userspace
API and for vfs API to be used by ksmbd and nfsd v4.

The new-ish vfs API fileattr_get/set() comes to mind when considering
a method to set the dos attrib bits.
Heck, FS_NODUMP_FL is the same as FILE_ATTRIBUTE_ARCHIVE.
That will also make it easy for filesystems that support the fileattr flags
to add support for FS_SYSTEM_FL, FS_HIDDEN_FL.

There is a use case for that. It can be inferred from samba config options
"map hidden/system/archive" that are used to avoid the cost of getxattr
per file during a "readdirplus" query. I recently quantified this cost on a
standard file server and it was very high.

Which leaves us with an API to set the 'time backup' attribute, which
is a "mutable creation time" [*].
cifs supports setting it via setxattr and I guess ntfs3 could use an
API to set it as well.

One natural interface that comes to mind is:

struct timespec times[3] = {/* atime, mtime, crtime */}
utimensat(dirfd, path, times, AT_UTIMES_ARCHIVE);

and add ia_crtime with ATTR_CRTIME to struct iattr.

Trond,

Do you agree to rework your patches in this direction?
Perhaps as the first stage, just use statx() and ioctls to set the
attributes to give enough time for bikeshedding the set APIs
and follow up with the generic set API patches later?

Thanks,
Amir.

[*] I find it convenient to use the statx() terminology of "btime"
to refer to the immutable birth time provided by some filesystems
and to use "crtime" for the mutable creation time for archiving,
so that at some point, some filesystems may provide both of
these times independently.