[PATCH] nfsd: map EBUSY for all operations

[PATCH] nfsd: map EBUSY for all operations

[Amir Goldstein]

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:
-- 
2.34.1

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

[Trond Myklebust]

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

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

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

[Amir Goldstein]

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.

Thanks,
Amir.

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

[Trond Myklebust]

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.
The protocol says that the client can expect this return value to mean
it is dealing with a server with Windows-like semantics that doesn't
allow these particular operations while the file is being held open. It
says nothing about expecting the same behaviour for mountpoints, and
since the latter have a very different life cycle than file open state
does, you should not treat those cases as being the same.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

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

[Amir Goldstein]

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

> The protocol says that the client can expect this return value to mean
> it is dealing with a server with Windows-like semantics that doesn't
> allow these particular operations while the file is being held open. It
> says nothing about expecting the same behaviour for mountpoints, and
> since the latter have a very different life cycle than file open state
> does, you should not treat those cases as being the same.

The two cases are currently indistinguishable in nfsd_unlink(), but
it could check DCACHE_NFSFS_RENAMED flag if we want to
limit NFS4ERR_FILE_OPEN to this specific case - again, this is
upstream code - nothing to do with my patch.

FWIW, my observed behavior of Linux nfs client for this error
is about 1 second retries and failure with -EBUSY, which is fine
for my use case, but if you think there is a better error to map
EBUSY it's fine with me. nfsv3 maps it to EACCES anyway.

Thanks,
Amir.

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

[Trond Myklebust]

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

> 
> > The protocol says that the client can expect this return value to
> > mean
> > it is dealing with a server with Windows-like semantics that
> > doesn't
> > allow these particular operations while the file is being held
> > open. It
> > says nothing about expecting the same behaviour for mountpoints,
> > and
> > since the latter have a very different life cycle than file open
> > state
> > does, you should not treat those cases as being the same.
> 
> The two cases are currently indistinguishable in nfsd_unlink(), but
> it could check DCACHE_NFSFS_RENAMED flag if we want to
> limit NFS4ERR_FILE_OPEN to this specific case - again, this is
> upstream code - nothing to do with my patch.
> 
> FWIW, my observed behavior of Linux nfs client for this error
> is about 1 second retries and failure with -EBUSY, which is fine
> for my use case, but if you think there is a better error to map
> EBUSY it's fine with me. nfsv3 maps it to EACCES anyway.
> 
> 

When doing LINK, RENAME, REMOVE on a mount point, I'd suggest returning
NFS4ERR_XDEV, since that is literally a case of trying to perform the
operation across a filesystem boundary.

Otherwise, since Linux doesn't implement Windows behaviour w.r.t. link,
rename or remove, it would seem that NFS4ERR_ACCESS is indeed the most
appropriate error, no? It's certainly the right behaviour for
sillyrenamed files.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

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

[Chuck Lever]

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

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

[Amir Goldstein]

On Mon, Jan 20, 2025 at 8:29 PM Trond Myklebust <trondmy@hammerspace.com> 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".
>
> >
> > > The protocol says that the client can expect this return value to
> > > mean
> > > it is dealing with a server with Windows-like semantics that
> > > doesn't
> > > allow these particular operations while the file is being held
> > > open. It
> > > says nothing about expecting the same behaviour for mountpoints,
> > > and
> > > since the latter have a very different life cycle than file open
> > > state
> > > does, you should not treat those cases as being the same.
> >
> > The two cases are currently indistinguishable in nfsd_unlink(), but
> > it could check DCACHE_NFSFS_RENAMED flag if we want to
> > limit NFS4ERR_FILE_OPEN to this specific case - again, this is
> > upstream code - nothing to do with my patch.
> >
> > FWIW, my observed behavior of Linux nfs client for this error
> > is about 1 second retries and failure with -EBUSY, which is fine
> > for my use case, but if you think there is a better error to map
> > EBUSY it's fine with me. nfsv3 maps it to EACCES anyway.
> >
> >
>
> When doing LINK, RENAME, REMOVE on a mount point, I'd suggest returning
> NFS4ERR_XDEV, since that is literally a case of trying to perform the
> operation across a filesystem boundary.

I would not recommend doing that. vfs hides those tests in vfs_rename(), etc
I don't think that nfsd should repeat them for this specialize interpretation,
because to be clear, this is specially not an EXDEV situation as far as vfs
is concerned.

>
> Otherwise, since Linux doesn't implement Windows behaviour w.r.t. link,
> rename or remove, it would seem that NFS4ERR_ACCESS is indeed the most
> appropriate error, no? It's certainly the right behaviour for
> sillyrenamed files.

If NFS4ERR_ACCESS is acceptable for sillyrenamed files, we can map
EBUSY to NFS4ERR_ACCESS always and be done with it, but TBH,
reading the explanation for the chosen error code, I tend to agree with it.
It is a very nice added benefit for me that the NFS clients get EBUSY when
the server gets an EBUSY, so I don't see what's the problem with that.

commit 466e16f0920f3ffdfa49713212fa334fb3dc08f1
Author: NeilBrown <neilb@suse.de>
Date:   Thu Nov 28 13:56:43 2019 +1100

    nfsd: check for EBUSY from vfs_rmdir/vfs_unink.

    vfs_rmdir and vfs_unlink can return -EBUSY if the
    target is a mountpoint.  This currently gets passed to
    nfserrno() by nfsd_unlink(), and that results in a WARNing,
    which is not user-friendly.

    Possibly the best NFSv4 error is NFS4ERR_FILE_OPEN, because
    there is a sense in which the object is currently in use
    by some other task.  The Linux NFSv4 client will map this
    back to EBUSY, which is an added benefit.

    For NFSv3, the best we can do is probably NFS3ERR_ACCES, which isn't
    true, but is not less true than the other options.

    Signed-off-by: NeilBrown <neilb@suse.de>
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>


Thanks,
Amir.

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

[Trond Myklebust]

On Mon, 2025-01-20 at 22:11 +0100, Amir Goldstein wrote:
> On Mon, Jan 20, 2025 at 8:29 PM Trond Myklebust
> <trondmy@hammerspace.com> 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".
> > 
> > > 
> > > > The protocol says that the client can expect this return value
> > > > to
> > > > mean
> > > > it is dealing with a server with Windows-like semantics that
> > > > doesn't
> > > > allow these particular operations while the file is being held
> > > > open. It
> > > > says nothing about expecting the same behaviour for
> > > > mountpoints,
> > > > and
> > > > since the latter have a very different life cycle than file
> > > > open
> > > > state
> > > > does, you should not treat those cases as being the same.
> > > 
> > > The two cases are currently indistinguishable in nfsd_unlink(),
> > > but
> > > it could check DCACHE_NFSFS_RENAMED flag if we want to
> > > limit NFS4ERR_FILE_OPEN to this specific case - again, this is
> > > upstream code - nothing to do with my patch.
> > > 
> > > FWIW, my observed behavior of Linux nfs client for this error
> > > is about 1 second retries and failure with -EBUSY, which is fine
> > > for my use case, but if you think there is a better error to map
> > > EBUSY it's fine with me. nfsv3 maps it to EACCES anyway.
> > > 
> > > 
> > 
> > When doing LINK, RENAME, REMOVE on a mount point, I'd suggest
> > returning
> > NFS4ERR_XDEV, since that is literally a case of trying to perform
> > the
> > operation across a filesystem boundary.
> 
> I would not recommend doing that. vfs hides those tests in
> vfs_rename(), etc
> I don't think that nfsd should repeat them for this specialize
> interpretation,
> because to be clear, this is specially not an EXDEV situation as far
> as vfs
> is concerned.

That's not how protocols work.

The server is required to use the appropriate error as determined by
the protocol spec. It is then up to the client to interpret that error
according to its context.

IOW: It doesn't matter what POSIX may say here, or what errors the VFS
may want to see on the client. The server should be using NFS4ERR_XDEV
because the spec says it should use that error to signal this
condition.

It is then up to the client to convert that back into an EBUSY or
whatever it wants to display to the application.

> 
> > 
> > Otherwise, since Linux doesn't implement Windows behaviour w.r.t.
> > link,
> > rename or remove, it would seem that NFS4ERR_ACCESS is indeed the
> > most
> > appropriate error, no? It's certainly the right behaviour for
> > sillyrenamed files.
> 
> If NFS4ERR_ACCESS is acceptable for sillyrenamed files, we can map
> EBUSY to NFS4ERR_ACCESS always and be done with it, but TBH,
> reading the explanation for the chosen error code, I tend to agree
> with it.
> It is a very nice added benefit for me that the NFS clients get EBUSY
> when
> the server gets an EBUSY, so I don't see what's the problem with
> that.
> 
> commit 466e16f0920f3ffdfa49713212fa334fb3dc08f1
> Author: NeilBrown <neilb@suse.de>
> Date:   Thu Nov 28 13:56:43 2019 +1100
> 
>     nfsd: check for EBUSY from vfs_rmdir/vfs_unink.
> 
>     vfs_rmdir and vfs_unlink can return -EBUSY if the
>     target is a mountpoint.  This currently gets passed to
>     nfserrno() by nfsd_unlink(), and that results in a WARNing,
>     which is not user-friendly.
> 
>     Possibly the best NFSv4 error is NFS4ERR_FILE_OPEN, because
>     there is a sense in which the object is currently in use
>     by some other task.  The Linux NFSv4 client will map this
>     back to EBUSY, which is an added benefit.
> 
>     For NFSv3, the best we can do is probably NFS3ERR_ACCES, which
> isn't
>     true, but is not less true than the other options.
> 
>     Signed-off-by: NeilBrown <neilb@suse.de>
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> 
> Thanks,
> Amir.


The above would be the root cause of the bug...

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

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

[Amir Goldstein]

On Mon, Jan 20, 2025 at 11:15 PM Trond Myklebust
<trondmy@hammerspace.com> wrote:
>
> On Mon, 2025-01-20 at 22:11 +0100, Amir Goldstein wrote:
> > On Mon, Jan 20, 2025 at 8:29 PM Trond Myklebust
> > <trondmy@hammerspace.com> 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".
> > >
> > > >
> > > > > The protocol says that the client can expect this return value
> > > > > to
> > > > > mean
> > > > > it is dealing with a server with Windows-like semantics that
> > > > > doesn't
> > > > > allow these particular operations while the file is being held
> > > > > open. It
> > > > > says nothing about expecting the same behaviour for
> > > > > mountpoints,
> > > > > and
> > > > > since the latter have a very different life cycle than file
> > > > > open
> > > > > state
> > > > > does, you should not treat those cases as being the same.
> > > >
> > > > The two cases are currently indistinguishable in nfsd_unlink(),
> > > > but
> > > > it could check DCACHE_NFSFS_RENAMED flag if we want to
> > > > limit NFS4ERR_FILE_OPEN to this specific case - again, this is
> > > > upstream code - nothing to do with my patch.
> > > >
> > > > FWIW, my observed behavior of Linux nfs client for this error
> > > > is about 1 second retries and failure with -EBUSY, which is fine
> > > > for my use case, but if you think there is a better error to map
> > > > EBUSY it's fine with me. nfsv3 maps it to EACCES anyway.
> > > >
> > > >
> > >
> > > When doing LINK, RENAME, REMOVE on a mount point, I'd suggest
> > > returning
> > > NFS4ERR_XDEV, since that is literally a case of trying to perform
> > > the
> > > operation across a filesystem boundary.
> >
> > I would not recommend doing that. vfs hides those tests in
> > vfs_rename(), etc
> > I don't think that nfsd should repeat them for this specialize
> > interpretation,
> > because to be clear, this is specially not an EXDEV situation as far
> > as vfs
> > is concerned.
>
> That's not how protocols work.
>
> The server is required to use the appropriate error as determined by
> the protocol spec. It is then up to the client to interpret that error
> according to its context.
>
> IOW: It doesn't matter what POSIX may say here, or what errors the VFS
> may want to see on the client. The server should be using NFS4ERR_XDEV
> because the spec says it should use that error to signal this
> condition.
>

Which condition exactly?
I really doubt that the spec know and refers to bind mounts
wrt NFS4ERR_XDEV

~# mount /dev/vdf /vdf
~# mount --bind /vdf /mnt/
~# mount --make-private /mnt/
~# mkdir /mnt/emptydir
~# mount -t tmpfs none /mnt/emptydir
~# rmdir /vdf/emptydir/
rmdir: failed to remove '/vdf/emptydir/': Device or resource busy
~# touch /vdf/a
~# strace -e renameat2 mv /vdf/a /vdf/emptydir/a
renameat2(AT_FDCWD, "/vdf/a", AT_FDCWD, "/vdf/emptydir/a", RENAME_NOREPLACE) = 0

The definition of this condition is outside the scope of the NFS protocol,
so I'd rather not special case it for nfsd.

Thanks,
Amir.

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

[NeilBrown]

On Tue, 21 Jan 2025, Amir Goldstein wrote:
> On Mon, Jan 20, 2025 at 8:29 PM Trond Myklebust <trondmy@hammerspace.com> 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".
> >
> > >
> > > > The protocol says that the client can expect this return value to
> > > > mean
> > > > it is dealing with a server with Windows-like semantics that
> > > > doesn't
> > > > allow these particular operations while the file is being held
> > > > open. It
> > > > says nothing about expecting the same behaviour for mountpoints,
> > > > and
> > > > since the latter have a very different life cycle than file open
> > > > state
> > > > does, you should not treat those cases as being the same.
> > >
> > > The two cases are currently indistinguishable in nfsd_unlink(), but
> > > it could check DCACHE_NFSFS_RENAMED flag if we want to
> > > limit NFS4ERR_FILE_OPEN to this specific case - again, this is
> > > upstream code - nothing to do with my patch.
> > >
> > > FWIW, my observed behavior of Linux nfs client for this error
> > > is about 1 second retries and failure with -EBUSY, which is fine
> > > for my use case, but if you think there is a better error to map
> > > EBUSY it's fine with me. nfsv3 maps it to EACCES anyway.
> > >
> > >
> >
> > When doing LINK, RENAME, REMOVE on a mount point, I'd suggest returning
> > NFS4ERR_XDEV, since that is literally a case of trying to perform the
> > operation across a filesystem boundary.
> 
> I would not recommend doing that. vfs hides those tests in vfs_rename(), etc
> I don't think that nfsd should repeat them for this specialize interpretation,
> because to be clear, this is specially not an EXDEV situation as far as vfs
> is concerned.
> 
> >
> > Otherwise, since Linux doesn't implement Windows behaviour w.r.t. link,
> > rename or remove, it would seem that NFS4ERR_ACCESS is indeed the most
> > appropriate error, no? It's certainly the right behaviour for
> > sillyrenamed files.
> 
> If NFS4ERR_ACCESS is acceptable for sillyrenamed files, we can map
> EBUSY to NFS4ERR_ACCESS always and be done with it, but TBH,
> reading the explanation for the chosen error code, I tend to agree with it.
> It is a very nice added benefit for me that the NFS clients get EBUSY when
> the server gets an EBUSY, so I don't see what's the problem with that.

I agreed with it when I wrote it :-) but now I find Trond's argument to
be quite compelling.  Fomr rfc5661:

15.1.4.5.  NFS4ERR_FILE_OPEN (Error Code 10046)

   The operation is not allowed because a file involved in the operation
   is currently open.  Servers may, but are not required to, disallow
   linking-to, removing, or renaming open files.

This doesn't seem to cover "rmdir" of a mountpoint.

However NFS4ERR_XDEV is only permitted for LINK and RENAME, not for
REMOVE, so we cannot use that.

NFS4ERR_ACCESS says "Indicates permission denied" but there is no
permission issue here.

NFS4ERR_INVAL might be ok.  "The arguments for this operation are not
valid for some reason" is suitably vague.

NFS4ERR_NOTEMPTY "An attempt was made to remove a directory that was not
empty." could be argued as it "contains" a mountpoint in some sense.

I'd favour NFS4ERR_INVAL today.  I might change my mind again tomorrow.

NeilBrown

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

[Amir Goldstein]

On Tue, Jan 21, 2025 at 12:03 AM NeilBrown <neilb@suse.de> wrote:
>
> On Tue, 21 Jan 2025, Amir Goldstein wrote:
> > On Mon, Jan 20, 2025 at 8:29 PM Trond Myklebust <trondmy@hammerspace.com> 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".
> > >
> > > >
> > > > > The protocol says that the client can expect this return value to
> > > > > mean
> > > > > it is dealing with a server with Windows-like semantics that
> > > > > doesn't
> > > > > allow these particular operations while the file is being held
> > > > > open. It
> > > > > says nothing about expecting the same behaviour for mountpoints,
> > > > > and
> > > > > since the latter have a very different life cycle than file open
> > > > > state
> > > > > does, you should not treat those cases as being the same.
> > > >
> > > > The two cases are currently indistinguishable in nfsd_unlink(), but
> > > > it could check DCACHE_NFSFS_RENAMED flag if we want to
> > > > limit NFS4ERR_FILE_OPEN to this specific case - again, this is
> > > > upstream code - nothing to do with my patch.
> > > >
> > > > FWIW, my observed behavior of Linux nfs client for this error
> > > > is about 1 second retries and failure with -EBUSY, which is fine
> > > > for my use case, but if you think there is a better error to map
> > > > EBUSY it's fine with me. nfsv3 maps it to EACCES anyway.
> > > >
> > > >
> > >
> > > When doing LINK, RENAME, REMOVE on a mount point, I'd suggest returning
> > > NFS4ERR_XDEV, since that is literally a case of trying to perform the
> > > operation across a filesystem boundary.
> >
> > I would not recommend doing that. vfs hides those tests in vfs_rename(), etc
> > I don't think that nfsd should repeat them for this specialize interpretation,
> > because to be clear, this is specially not an EXDEV situation as far as vfs
> > is concerned.
> >
> > >
> > > Otherwise, since Linux doesn't implement Windows behaviour w.r.t. link,
> > > rename or remove, it would seem that NFS4ERR_ACCESS is indeed the most
> > > appropriate error, no? It's certainly the right behaviour for
> > > sillyrenamed files.
> >
> > If NFS4ERR_ACCESS is acceptable for sillyrenamed files, we can map
> > EBUSY to NFS4ERR_ACCESS always and be done with it, but TBH,
> > reading the explanation for the chosen error code, I tend to agree with it.
> > It is a very nice added benefit for me that the NFS clients get EBUSY when
> > the server gets an EBUSY, so I don't see what's the problem with that.
>
> I agreed with it when I wrote it :-) but now I find Trond's argument to
> be quite compelling.  Fomr rfc5661:
>
> 15.1.4.5.  NFS4ERR_FILE_OPEN (Error Code 10046)
>
>    The operation is not allowed because a file involved in the operation
>    is currently open.  Servers may, but are not required to, disallow
>    linking-to, removing, or renaming open files.
>
> This doesn't seem to cover "rmdir" of a mountpoint.
>
> However NFS4ERR_XDEV is only permitted for LINK and RENAME, not for
> REMOVE, so we cannot use that.
>
> NFS4ERR_ACCESS says "Indicates permission denied" but there is no
> permission issue here.
>
> NFS4ERR_INVAL might be ok.  "The arguments for this operation are not
> valid for some reason" is suitably vague.
>
> NFS4ERR_NOTEMPTY "An attempt was made to remove a directory that was not
> empty." could be argued as it "contains" a mountpoint in some sense.
>
> I'd favour NFS4ERR_INVAL today.  I might change my mind again tomorrow.
>

I will follow the guidance of 2019 Neil and go with NFS4ERR_ACCESS
because it matches the current behavior of NFS v2,3 and because
Trond also said it is the appropriate error for sillyrenamed files.

Thanks,
Amir.