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
