Re: [PATCH v3 2/2] nfsd: keep a checksum of the first 256 bytes of request

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Feb 07, 2013 at 11:23:17AM -0500, Chuck Lever wrote:
> 
> On Feb 7, 2013, at 11:00 AM, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Thu, Feb 07, 2013 at 10:51:02AM -0500, Chuck Lever wrote:
> >> 
> >> On Feb 7, 2013, at 9:51 AM, Jeff Layton <jlayton@redhat.com> wrote:
> >> 
> >>> Now that we're allowing more DRC entries, it becomes a lot easier to
> >>> hit problems with XID collisions. In order to mitigate those,
> >>> calculate the crc32 of up to the first 256 bytes of each request
> >>> coming in and store that in the cache entry, along with the total
> >>> length of the request.
> >> 
> >> I'm happy to see a checksummed DRC finally become reality for the
> >> Linux NFS server.
> >> 
> >> Have you measured the CPU utilization impact and CPU cache footprint
> >> of performing a CRC computation for every incoming RPC?
> > 
> > Note this is over the first 256 bytes of the request--which we're
> > probably just about to read for xdr decoding anyway.
> 
> XDR decoding is copying and branching.  Computing a CRC involves real math, which tends to be significantly more expensive than successfully predicted branches, especially on low-power CPUs that might be found in SOHO NAS products.

OK, I wouldn't know.

(I was just responding to the "cache footprint" question--I thought you
were concerned about reading in a bunch of the request.)  Looks like the
biggest piece of the crc32 code is a 1k lookup table?

> >> I'm wondering if a simpler checksum might be just as useful but less
> >> costly to compute.
> > 
> > What would be an example of a simpler checksum?
> 
> The same one TCP uses, like a simple additive sum, or an XOR.  Is a heavyweight checksum needed because checksums generated with a simple function are more likely to collide?
> 
> Not that this should hold up merging Jeff's work!  We can easily tweak or replace the checksum algorithm after it's upstream.  It's not kABI.
> 
> But someone should assess the impact of the additional checksum computation.  CRC seems to me heavier than is needed here.

OK, sure, may be worth looking into.

> Possible tweaks:
> 
> Why 256 bytes?  Is that too much?  Or not enough for some NFSv4
> compounds that might often start with the same data?  Could we, for
> instance, use fewer bytes for NFSv2 and NFSv3?  Or even a variable
> checksum length depending on the NFS operation?  Is 256 bytes enough
> for NFSv4.1, whose compounds always start with the same operation?

NFSv4.1 has the drc turned completely off.

> If integrity or privacy is in play, can we use that information in
> place of a separate DRC checksum?

There's a gss sequence number that's incremented even on resends of the
same rpc, so this doesn't work.  (By design: you don't want an attacker
to be able to replay an old rpc.)

--b.