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?
> 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:
https://github.com/amir73il/linux/commits/nfsd-fixes/
> Understood that fixing OPEN is your main priority.
>
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.
Thanks,
Amir.
