On Tue, Jan 21, 2025 at 12:03 AM NeilBrown <neilb@xxxxxxx> wrote:
>
> On Tue, 21 Jan 2025, 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.
> >
> > >
> > > 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.
