Re: [PATCH] nfsd: map EBUSY for all operations

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

On 1/20/25 2:29 PM, Trond Myklebust wrote:
> On Mon, 2025-01-20 at 20:14 +0100, Amir Goldstein wrote:
>> On Mon, Jan 20, 2025 at 7:45 PM Trond Myklebust
>> <trondmy@hammerspace.com> wrote:
>>>
>>> On Mon, 2025-01-20 at 19:21 +0100, Amir Goldstein wrote:
>>>> On Mon, Jan 20, 2025 at 6:28 PM Trond Myklebust
>>>> <trondmy@hammerspace.com> wrote:
>>>>>
>>>>> On Mon, 2025-01-20 at 18:20 +0100, Amir Goldstein wrote:
>>>>>> v4 client maps NFS4ERR_FILE_OPEN => EBUSY for all operations.
>>>>>>
>>>>>> v4 server only maps EBUSY => NFS4ERR_FILE_OPEN for
>>>>>> rmdir()/unlink()
>>>>>> although it is also possible to get EBUSY from rename() for
>>>>>> the
>>>>>> same
>>>>>> reason (victim is a local mount point).
>>>>>>
>>>>>> Filesystems could return EBUSY for other operations, so just
>>>>>> map
>>>>>> it
>>>>>> in server for all operations.
>>>>>>
>>>>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>>>>>> ---
>>>>>>
>>>>>> Chuck,
>>>>>>
>>>>>> I ran into this error with a FUSE filesystem and returns -
>>>>>> EBUSY
>>>>>> on
>>>>>> open,
>>>>>> but I noticed that vfs can also return EBUSY at least for
>>>>>> rename().
>>>>>>
>>>>>> Thanks,
>>>>>> Amir.
>>>>>>
>>>>>>   fs/nfsd/vfs.c | 10 ++--------
>>>>>>   1 file changed, 2 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>>>>> index 29cb7b812d713..a61f99c081894 100644
>>>>>> --- a/fs/nfsd/vfs.c
>>>>>> +++ b/fs/nfsd/vfs.c
>>>>>> @@ -100,6 +100,7 @@ nfserrno (int errno)
>>>>>>                { nfserr_perm, -ENOKEY },
>>>>>>                { nfserr_no_grace, -ENOGRACE},
>>>>>>                { nfserr_io, -EBADMSG },
>>>>>> +             { nfserr_file_open, -EBUSY},
>>>>>>        };
>>>>>>        int     i;
>>>>>>
>>>>>> @@ -2006,14 +2007,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 = nfserrno(host_err);
>>>>>>   out:
>>>>>>        return err;
>>>>>>   out_unlock:
>>>>>
>>>>> If this is a transient error, then it would seem that
>>>>> NFS4ERR_DELAY
>>>>> would be more appropriate.
>>>>
>>>> It is not a transient error, not in the case of a fuse file open
>>>> (it is busy as in locked for as long as it is going to be locked)
>>>> and not in the case of failure to unlink/rename a local
>>>> mountpoint.
>>>> NFS4ERR_DELAY will cause the client to retry for a long time?
>>>>
>>>>> NFS4ERR_FILE_OPEN is not supposed to apply
>>>>> to directories, and so clients would be very confused about how
>>>>> to
>>>>> recover if you were to return it in this situation.
>>>>
>>>> Do you mean specifically for OPEN command, because commit
>>>> 466e16f0920f3 ("nfsd: check for EBUSY from vfs_rmdir/vfs_unink.")
>>>> added the NFS4ERR_FILE_OPEN response for directories five years
>>>> ago and vfs_rmdir can certainly return a non-transient EBUSY.
>>>>
>>>
>>> I'm saying that clients expect NFS4ERR_FILE_OPEN to be returned in
>>> response to LINK, REMOVE or RENAME only in situations where the
>>> error
>>> itself applies to a regular file.
>>
>> This is very far from what upstream nfsd code implements (since 2019)
>> 1. out of the above, only REMOVE returns NFS4ERR_FILE_OPEN
>> 2. NFS4ERR_FILE_OPEN is not limited to non-dir
>> 3. NFS4ERR_FILE_OPEN is not limited to silly renamed file -
>>      it will also be the response for trying to rmdir a mount point
>>      or trying to unlink a file which is a bind mount point
> 
> Fair enough. I believe the name given to this kind of server behaviour
> is "bug".

I would greatly appreciate it if a bugzilla.kernel.org bug could be
opened that lists the affected operations and what the proper
expected behavior is. Bonus points for documenting the commit hashes
that introduce the incorrect behavior.

Free <insert your preferred adult beverage here>.

-- 
Chuck Lever