Re: [bug report] NFS: Support statx_get and statx_set ioctls

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2022-01-12 at 09:57 +0200, Amir Goldstein wrote:
> On Wed, Jan 12, 2022 at 4:10 AM Christoph Hellwig <hch@xxxxxxxxxxxxx>
> 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@xxxxxxxxxx/
> 
> > 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.

I'll give it a shot. I've asked Anna to remove these patches from the
pull request for this merge Window, so I'll try to rework the retrieval
side using statx() and then work on a statx_set() API to enable setting
of these new variables.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx






[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux