On Fri, Sep 25, 2020 at 09:59:54AM -0400, Chuck Lever wrote: > > > > On Sep 24, 2020, at 7:45 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote: > > > > On Mon, Sep 21, 2020 at 02:13:07PM -0400, Chuck Lever wrote: > >> This is follow-on work to the tracepoints added in the NFS server's > >> duplicate reply cache. Here, tracepoints are introduced that report > >> replies from cache as well as encoding and decoding errors. > >> > >> The NFSv2, v3, and v4 dispatcher requirements have diverged over > >> time, leaving us with a little bit of technical debt. In addition, > >> I wanted to add a tracepoint for NFSv2 and NFSv3 similar to the > >> nfsd4_compound/compoundstatus tracepoints. Lastly, removing some > >> conditional branches from this hot path helps optimize CPU > >> utilization. So, I've duplicated the nfsd_dispatcher function. > > > > Comparing current nfsd_dispatch to the nfsv2/v3 nfsd_dispatch: the only > > thing I spotted removed from the v2/v3-specific dispatch is the > > rq_lease_breaker = NULL. (I think that's not correct, actually. We > > could remove the need for that to be set in the v2/v3 case, but with the > > current code it does need to be set.) > > Noted with thanks. > > > > Comparing current nfsd_dispatch to the nfsv4 nfsd4_dispatch, the > > v4-specific dispatch does away with nfs_request_too_big() and the > > v2-specific shortcut in the error encoding case. > > > > So these still look *very* similar. I don't feel like we're getting a > > lot of benefit out of splitting these out. > > I don't disagree with that at all. At this point I'm just noodling > to see what's possible. I'm now toying with other ways to add high- > value tracing in the legacy ULPs. In the end I might end up avoiding > significant changes in the dispatchers in order to add tracing. OK. > However, a few thoughts I had while learning how the dispatcher > code works. > > There are some opportunities for reducing instruction path length > and the number of conditional branches in here. It's a hot path, > so I think we should consider some careful micro-optimizations > even if they don't add significant new features or do add some > code duplication. > > In user space, the library (iirc) assumes each ULP provides it's > own dispatcher function. I'd consider duplicating and removing > svc_generic_dispatcher() to simplify the pasta in svc_process(), > again as a micro-optimization and for better code legibility. Not sure you even have to duplicate it, just export the generic dispatcher and let individual programs point to it, right? > lockd's pc_func returns an RPC accept_stat, but the NFSD pc_func > methods return an NFS status. The latter feels like a layering > violation for the sake of reducing a small amount of code > duplication. I'd rather see encoding of the NFS status handled in > the NFS Reply encoders, since that is an XDR function, and because > that logic seems slightly different for NFSv2, support for which > we'd like to deprecate at some point. > > Note also that *statp in nfsd_dispatch is never explicitly set to > rpc_success in the normal execution flow. It relies on the > equivalence of rpc_success and nfs_ok, which is convenient, but > confusing to read. It might be cleaner if *statp was made an enum > to make it explicit what set of values go in that return variable. OK. --b.
