On 1/23/25 10:29 AM, Amir Goldstein wrote:
On Thu, Jan 23, 2025 at 3:59 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
On 1/22/25 3:11 PM, Amir Goldstein wrote:
On Wed, Jan 22, 2025 at 8:20 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
On 1/22/25 1:53 PM, Amir Goldstein wrote:
I am fine with handling EBUSY in unlink/rmdir/rename/open
only for now if that is what everyone prefers.
As far as I can tell, NFSv2 and NFSv3 REMOVE/RMDIR are working
correctly. NFSv4 REMOVE needs to return a status code that depends
on whether the target object is a file or not. Probably not much more
than something like this:
status = vfs_unlink( ... );
+ /* RFC 8881 Section 18.25.4 paragraph 5 */
+ if (status == nfserr_file_open && !S_ISREG(...))
+ status = nfserr_access;
added to nfsd4_remove().
Don't you think it's a bit awkward mapping back and forth like this?
Yes, it's awkward. It's an artifact of the way NFSD's VFS helpers have
been co-opted for new versions of the NFS protocol over the years.
With NFSv2 and NFSv3, the operations and their permitted status codes
are roughly similar so that these VFS helpers can be re-used without
a lot of fuss. This is also why, internally, the symbolic status codes
are named without the version number in them (ie, nfserr_inval).
With NFSv4, the world is more complicated.
The NFSv4 code was prototyped 20 years ago using these NFSv2/3 helpers,
and is never revisited until there's a bug. Thus there is quite a bit of
technical debt in fs/nfsd/vfs.c that we're replacing over time.
IMO it would be better if these VFS helpers returned errno values and
then the callers should figure out the conversion to an NFS status code.
I suspect that's difficult because some of the functions invoked by the
VFS helpers (like fh_verify() ) also return NFS status codes. We just
spent some time extracting NFS version-specific code from fh_verify().
Don't you think something like this is a more sane way to keep the
mapping rules in one place:
@@ -111,6 +111,26 @@ nfserrno (int errno)
return nfserr_io;
}
+static __be32
+nfsd_map_errno(int host_err, int may_flags, int type)
+{
+ switch (host_err) {
+ case -EBUSY:
+ /*
+ * According to RFC 8881 Section 18.25.4 paragraph 5,
+ * removal of regular file can fail with NFS4ERR_FILE_OPEN.
+ * For failure to remove directory we return NFS4ERR_ACCESS,
+ * same as NFS4ERR_FILE_OPEN is mapped in v3 and v2.
+ */
+ if (may_flags == NFSD_MAY_REMOVE && type == S_IFREG)
+ return nfserr_file_open;
+ else
+ return nfserr_acces;
+ }
+
+ return nfserrno(host_err);
+}
+
/*
* Called from nfsd_lookup and encode_dirent. Check if we have crossed
* a mount point.
@@ -2006,14 +2026,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct
svc_fh *fhp, int type,
out_drop_write:
fh_drop_write(fhp);
out_nfserr:
- if (host_err == -EBUSY) {
- /* name is mounted-on. There is no perfect
- * error status.
- */
- err = nfserr_file_open;
- } else {
- err = nfserrno(host_err);
- }
+ err = nfsd_map_errno(host_err, NFSD_MAY_REMOVE, type);
out:
return err;
No, I don't.
NFSD has Kconfig options that disable support for some versions of NFS.
The code that manages which status code to return really needs to be
inside the functions that are enabled or disabled by Kconfig.
As I keep repeating: there is no good way to handle the NFS status codes
in one set of functions. Each NFS version has its variations that
require special handling.
ok.
Let's visit RENAME once that is addressed.
And then next patch would be:
@@ -1828,6 +1828,7 @@ nfsd_rename(struct svc_rqst *rqstp, struct
svc_fh *ffhp, char *fname, int flen,
__be32 err;
int host_err;
bool close_cached = false;
+ int type;
err = fh_verify(rqstp, ffhp, S_IFDIR, NFSD_MAY_REMOVE);
if (err)
@@ -1922,8 +1923,10 @@ nfsd_rename(struct svc_rqst *rqstp, struct
svc_fh *ffhp, char *fname, int flen,
out_dput_new:
dput(ndentry);
out_dput_old:
+ type = d_inode(odentry)->i_mode & S_IFMT;
dput(odentry);
out_nfserr:
- err = nfserrno(host_err);
+ err = nfsd_map_errno(host_err, NFSD_MAY_REMOVE, type);
Same problem here: the NFS version-specific status codes have to be
figured out in the callers, not in nfsd_rename(). The status codes
are not common to all NFS versions.
ok.
Then handle OPEN as a third patch, because I bet we are going to meet
some complications there.
Did you think of anything better to do for OPEN other than NFS4ERR_ACCESS?
I haven't even started to think about that yet.
ok. Let me know when you have any ideas about that.
My goal is to fix EBUSY WARN for open from FUSE.
The rest is cleanup that I don't mind doing on the way.
I've poked at nfsd4_remove(). It's not going to work the way I prefer.
Do you mean because the file type is not available there?
Yes.
But I'll take care of the clean up for remove, rename, and link.
FWIW, this is how I was going to solve this,
but I admit it is quite awkward:
I'll deal with it. In the long run, making fh_verify() return an errno
so all of these helpers can return an errno rather than status code
is where I want to take this. But for now, a simple approach is best
because that can be cleanly backported.
I now realized that truncate can also return EBUSY in my FUSE fs :/
That's why I am disappointed that there is no "fall back"
mapping for EBUSY that fits all without a warning, but I will
wait to see how the cleanup goes and we will take it from there.
It's better for us if we can identify the particular system call
that returns -EBUSY. Auditing these cases might show that a blanket
approach is fine, but we still need to do the audit no matter what.
--
Chuck Lever
