Date: Tue, 03 Aug 1999 01:31:02 +0800 From: Peter Wemm <peter@netplex.com.au> To: Alfred Perlstein <bright@rush.net> Cc: Matthew Dillon <dillon@apollo.backplane.com>, hackers@FreeBSD.ORG Subject: Re: confusion about nfsm_srvmtofh bad behavior? Message-ID: <19990802173102.CCA101C9F@overcee.netplex.com.au> In-Reply-To: Your message of "Mon, 02 Aug 1999 13:07:25 -0400." <Pine.BSF.3.96.990802113034.20420m-100000@cygnus.rush.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Alfred Perlstein wrote:
> On Mon, 2 Aug 1999, Matthew Dillon wrote:
>
> >
> > ::> it then rewinds the mbuf pointers (i think) because of the
> > ::> over "dissection" above.
> > ::> ---
> > ::>
> > ::> why does it do the copy, then rewind it, it seems like it knows
> > ::> it's doing something wrong and instead of fixing it, it just
> > ::> compensates after the fact.
> > ::
> > ::yes, replying to my own message.
> > ::
> > ::the only thing i can think of is that the extra data is safely
> > ::ignored because the routines that use these macros seem to
> > ::pass the version of NFS to all the function that they call...
> > ::
> > ::however unless i'm wrong (which i probably am) nfsV2 stuff
> > ::could be made faster if it was correctly noted and less data
> > ::was copied. It would also DTRT and not access data it isn't
> > ::supposed to :)
> > ::
> > ::it seems like all of the V3 handles are the same length so
> > ::there isn't much to do there...
> > ::
> > ::-Alfred
> > :
> > : Well, I must say that it certainly looks like a bug. It is not going
> > : to blow anything up since the nfsm_dissect() will break out if it runs
> > : out of buffer space, but it certainly seems inefficient. I am somewha
t
> > : loath to fix anything in NFS that does not create a demonstrateable
> > : problem for fear of creating new problems, though, it is quite possibl
e
> > : that the server code depends on the extra junk in the file handle for
> > : V2 mounts - A full audit of nfs_nqlease.c and nfs_serv.c would be
> > : necessary before this could be fixed.
> > :
> > : -Matt
> > : Matthew Dillon
> > : <dillon@backplane.com>
> >
> > Oh, p.s. But in the mean time, if you or someone would like to commit
> > an XXX comment to document the potential bug / performance problem, I
> > think that would be very appropriate. e.g.
> >
> > /*
> > * Extract file handle from NFS stream. XXX note that the extraction of
> > * the file handle for an NFSv2 mount appears to be rather odd. It is copy
ing
> > * NFSX_V3FH bytes instead of NFSX_V2FH and then rewinding the mbuf index.
> > */
> >
> > #define nfsm_srvmtofh(f) \
> > { int fhlen = NFSX_V3FH; \
> > if (nfsd->nd_flag & ND_NFSV3) { \
> > nfsm_dissect(tl, u_int32_t *, NFSX_UNSIGNED); \
> > fhlen = fxdr_unsigned(int, *tl); \
> > ...
>
> The whole file needs some **** documentation. :)
> I stared at that for so long wondering why the heck it was doing that.
>
> Is there a chance you could give this some of your famous regression
> testing?
>
> http://big.endian.org/~bright/freebsd/patches/nfsm_subs.diff
>
> This is a patch that Peter Wemm proposed however he had this:
>
> ! if (fhlen < NFSX_V3FH) { \
> ! bzero((caddr_t)(f) + fhlen, NFSX_V3FH - fhlen); \
> ! } \
>
> right before the while(0) instead of the else clause with the
> full bzero.
>
> i'd rather get rid of the extra copying going on and since
> previously it was filled with garbage from the rest of the RPC
> structure i don't think it's nessesary.
Right now it seems we're generating 8 bytes of fsid and 12 (padded to 16)
bytes of handle data in the common case for a total of 24 bytes of filehandle.
Then we pad that to 32 bytes for V2 or 64 bytes for V3, with random crud.
Then we copy this around, store it all in memory, transmit it over the wire,
etc. It's a nightmare.
NFSv2 filehandles are fixed at 32 bytes long. For V3 we could probably just
transmit 24 byte filehandles rather than 64. (I'll reread the spec to make
sure there isn't a v3 minimum size).
That explicit zero on the end is probably redundant since we've been using
random data in it's place since day 1. (We do need (I think) the fhlen ==
0 bzero though).
Cheers,
-Peter
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-hackers" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?19990802173102.CCA101C9F>