On Mon, Jan 20, 2025 at 8:29 PM Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > > > > > ---
> > > > > >
> > > > > > 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@xxxxxxx>
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@xxxxxxx>
Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
Thanks,
Amir.
