On Sat, 27 Jul 2024, Chuck Lever wrote:
> On Thu, Jul 25, 2024 at 10:21:32PM -0400, NeilBrown wrote:
> > There is code scatter around nfsd which chooses an error status based on
> > the particular version of nfs being used. It is cleaner to have the
> > version specific choices in version specific code.
> >
> > With this patch common code returns the most specific error code
> > possible and the version specific code maps that if necessary.
>
> I applaud the top-level goal of this patch. There are some details
> that I'm not comfortable with. (And I've already applied the other
> patches in this series; nice work).
>
>
> > One complication is that the NFSv4 code NFS4ERR_SYMLINK might be used
> > where v3 expects NFSERR_NOTDIR of NFSERR_INVAL. To handle this we
> > introduce an internal error code NFSERR_SYMLINK_NOT_DIR and convert that
> > as appropriate for all versions.
>
> IMO this patch is trying to do too much at once, both from a review
> standpoint and also in terms of bisectability.
>
> Can you break it up so that the SYMLINK-related changes are
> separated from, eg, the xdev and other changes which are somewhat
> less controversial?
Yes, I can break it up as you suggest - makes sense.
>
> More specific comments below.
>
>
> > diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> > index a7a07470c1f8..8d75759c600d 100644
> > --- a/fs/nfsd/nfs3xdr.c
> > +++ b/fs/nfsd/nfs3xdr.c
> > @@ -111,6 +111,23 @@ svcxdr_encode_nfsstat3(struct xdr_stream *xdr, __be32 status)
> > {
> > __be32 *p;
> >
> > + switch (status) {
> > + case nfserr_symlink_not_dir:
> > + status = nfserr_notdir;
> > + break;
> > + case nfserr_symlink:
> > + case nfserr_wrong_type:
> > + status = nfserr_inval;
> > + break;
> > + case nfserr_nofilehandle:
> > + status = nfserr_badhandle;
> > + break;
> > + case nfserr_wrongsec:
> > + case nfserr_file_open:
> > + status = nfserr_acces;
> > + break;
> > + }
> > +
>
> This is entirely the wrong place for this logic. It is specific to
> symlinks, and it is not XDR layer functionality. I prefer to see
> this moved to the proc functions that need it.
The logic in not specific to symlinks. It is specific to RPC procedures
which act on things that are not expected to be symlinks. There are
lots of procedures like that.
We could put the mapping in each .pc_func function, or between the
proc->pc_func() call in nfsd_dispatch() and the following
proc->pc_encode() call, or in each .pc_encode call.
Putting it in each .pc_func would mean changing the "return status;" at
the end of each nfs3_proc_* function to "return map_status(status);"
Putting it between ->pc_func() and ->pc_encode() would mean defining a
new ->pc_map_status() for each program/version and calling that.
Putting it in each .pc_encode is what I did.
It isn't clear to me that either of the first two are better than the
third, but neither are terrible. However the second wouldn't work for
NFSv4.0 mapping (as individual ops need to be mapped). But v4.0 needs
special handling anyway.
Where would you prefer I put it?
>
> The layering here is that XDR should handle only data serialization.
> Transformations like this go upstairs in the proc layer. I know NFSD
> doesn't always adhere to this layering model, very often out of
> convenience, but I want new code to try to stick to a stricter
> layering model.
>
> Again, the reason for this is that eventually much of the existing
> XDR code will be replaced by machine-generated code. Adding bespoke
> bits of logic here makes the task of converting this code more
> difficult.
I suspect it we won't be able to avoid the machine generated code
occasionally calling bespoke code - but we won't know until we try.
> >
> > /* error codes for internal use */
> > +enum {
> > + NFSERR_DROPIT = NFS4ERR_FIRST_FREE,
> > /* if a request fails due to kmalloc failure, it gets dropped.
> > * Client should resend eventually
> > */
> > -#define nfserr_dropit cpu_to_be32(30000)
> > +#define nfserr_dropit cpu_to_be32(NFSERR_DROPIT)
> > +
> > /* end-of-file indicator in readdir */
> > -#define nfserr_eof cpu_to_be32(30001)
> > + NFSERR_EOF,
> > +#define nfserr_eof cpu_to_be32(NFSERR_EOF)
> > +
> > /* replay detected */
> > -#define nfserr_replay_me cpu_to_be32(11001)
> > + NFSERR_REPLAY_ME,
> > +#define nfserr_replay_me cpu_to_be32(NFSERR_REPLAY_ME)
> > +
> > /* nfs41 replay detected */
> > -#define nfserr_replay_cache cpu_to_be32(11002)
> > + NFSERR_REPLAY_CACHE,
> > +#define nfserr_replay_cache cpu_to_be32(NFSERR_REPLAY_CACHE)
> > +
> > +/* symlink found where dir expected - handled differently to
> > + * other symlink found errors by NSv3.
>
> ^NSv3^NFSv3
>
Thanks.
>
> > + */
> > + NFSERR_SYMLINK_NOT_DIR,
> > +#define nfserr_symlink_not_dir cpu_to_be32(NFSERR_SYMLINK_NOT_DIR)
> > +};
>
> It's confusing to me that we're using the same naming scheme for
> symbolic status codes defined by spec and the ones defined for
> internal use only. It would be nice to rename these, say, with an
> _INT_ or _INTL_ in their name somewhere.
Well the spec say v4 statuses start NFS4ERR_ but we don't follow that.
It isn't clear to me that disambiguation the name would help. At the
point where the error is returned the important thing is what the error
means, that that is spelt out in the text after nfserr_*. At the point
where the error is interpreted the meaning is obvious in the code.
