Re: [PATCH v2] nfsd: map EBUSY to NFS4ERR_ACCESS for all operations

[Chuck Lever <chuck.lever@oracle.com>]

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.


>> 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.


>> 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.


-- 
Chuck Lever