On Wed, 22 Jan 2025, Amir Goldstein wrote:
> On Tue, Jan 21, 2025 at 8:45 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
> >
> > Please send patches To: the NFSD reviewers listed in MAINTAINERS and
> > Cc: linux-nfs and others. Thanks!
> >
> >
> > On 1/21/25 5:39 AM, Amir Goldstein wrote:
> > > Commit 466e16f0920f3 ("nfsd: check for EBUSY from vfs_rmdir/vfs_unink.")
> > > mapped EBUSY host error from rmdir/unlink operation to avoid unknown
> > > error server warning.
> >
> > > The same reason that casued the reported EBUSY on rmdir() (dir is a
> > > local mount point in some other bind mount) could also cause EBUSY on
> > > rename and some filesystems (e.g. FUSE) can return EBUSY on other
> > > operations like open().
> > >
> > > Therefore, to avoid unknown error warning in server, we need to map
> > > EBUSY for all operations.
> > >
> > > The original fix mapped EBUSY to NFS4ERR_FILE_OPEN in v4 server and
> > > to NFS4ERR_ACCESS in v2/v3 server.
> > >
> > > During the discussion on this issue, Trond claimed that the mapping
> > > made from EBUSY to NFS4ERR_FILE_OPEN was incorrect according to the
> > > protocol spec and specifically, NFS4ERR_FILE_OPEN is not expected
> > > for directories.
> >
> > NFS4ERR_FILE_OPEN might be incorrect when removing certain types of
> > file system objects. Here's what I find in RFC 8881 Section 18.25.4:
> >
> > > If a file has an outstanding OPEN and this prevents the removal of the
> > > file's directory entry, the error NFS4ERR_FILE_OPEN is returned.
> >
> > It's not normative, but it does suggest that any object that cannot be
> > associated with an OPEN state ID should never cause REMOVE to return
> > NFS4ERR_FILE_OPEN.
> >
> >
> > > To keep things simple and consistent and avoid the server warning,
> > > map EBUSY to NFS4ERR_ACCESS for all operations in all protocol versions.
> >
> > Generally a "one size fits all" mapping for these status codes is
> > not going to cut it. That's why we have nfsd3_map_status() and
> > nfsd_map_status() -- the set of permitted status codes for each
> > operation is different for each NFS version.
> >
> > NFSv3 has REMOVE and RMDIR. You can't pass a directory to NFSv3 REMOVE.
> >
> > NFSv4 has only REMOVE, and it removes the directory entry for the
> > object no matter its type. The set of failure modes is different for
> > this operation compared to NFSv3 REMOVE.
> >
> > Adding a specific mapping for -EBUSY in nfserrno() is going to have
> > unintended consequences for any VFS call NFSD might make that returns
> > -EBUSY.
> >
> > I think I prefer that the NFSv4 cases be dealt with in nfsd4_remove(),
> > nfsd4_rename(), and nfsd4_link(), and that -EBUSY should continue to
> > trigger a warning.
> >
> >
>
> Sorry, I didn't understand what you are suggesting.
>
> FUSE can return EBUSY for open().
> What do you suggest to do when nfsd encounters EBUSY on open()?
>
> vfs_rename() can return EBUSY.
> What do you suggest to do when nfsd v3 encounters EBUSY on rename()?
>
> This sort of assertion:
> WARN_ONCE(1, "nfsd: non-standard errno: %d\n", errno);
>
> Is a code assertion for a situation that should not be possible in the
> code and certainly not possible to trigger by userspace.
>
> Both cases above could trigger the warning from userspace.
> If you want to leave the warning it should not be a WARN_ONCE()
> assertion, but I must say that I did not understand the explanation
> for not mapping EBUSY by default to NFS4ERR_ACCESS in nfserrno().
My answer to this last question is that it isn't obvious that EBUSY
should map to NFS4ERR_ACCESS.
I would rather that nfsd explicitly checked the error from unlink/rmdir and
mapped EBUSY to NFS4ERR_ACCESS (if we all agree that is best) with a
comment (like we have now) explaining why it is best.
And nfsd should explicitly check the error from open() and map EBUSY to
whatever seems appropriate. Maybe that is also NS4ERR_ACCESS but if it
is, the reason is likely different to the reason that it is best for
rmdir.
So again, I would like a comment in the code explaining the choice with
a reference to FUSE.
Then if some other function that we haven't thought about starts
returning EBUSY, we'll get warning and have a change to think about it.
Thanks,
NeilBrown
