On Mon, 2025-01-20 at 22:11 +0100, Amir Goldstein wrote:
> 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.
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@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.
The above would be the root cause of the bug...
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
