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.
Thanks,
Amir.
