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

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

On 1/23/25 10:29 AM, Amir Goldstein wrote:
> On Thu, Jan 23, 2025 at 3:59 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On 1/22/25 3:11 PM, Amir Goldstein wrote:
>>> On Wed, Jan 22, 2025 at 8:20 PM Chuck Lever <chuck.lever@oracle.com> 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