Re: USE_SHA1DC is broken in pu

From: Jeff King <peff@peff.net>
Date: Mar 23, 2017, 17:47 UTC
Message-ID: <20170323174750.xyucxmfhuc6dbrzc@sigill.intra.peff.net>
In-Reply-To: <CA+55aFyFS2r6isdQU_i5FUQnrzjvWhZdmEVE=KmBVTrMukBbkw@mail.gmail.com>

On Thu, Mar 23, 2017 at 10:16:23AM -0700, Linus Torvalds wrote:

Show 9 quoted lines

> > If I write out an index, I should not suffer the slowdown from detecting
> > collisions.
> 
> The index case is a complete red herring.
> 
> As already noted, the proper fix for the index case is to simply do it
> asynchronously on read. On write, it's harder to do asynchronously,
> but for a 300MB index file you're likely going to be doing IO in the
> middle, so it's probably not even noticeable.

I think there were some earlier timings that show OpenSSL had a small but measurable improvement over block-sha1 in this massive case (and sha1dc is about 1.75x slower than block-sha1, so it will be a little worse).

So I am mildly sympathetic. BUT. I mostly agree with:

> But the fact is, if you don't want SHA1DC, and you have crazy special
> cases, you just continue to build with openssl support instead. Nobody
> else should ever have to worry about *your* crazy cases.

If somebody who has such a crazy special case wants to tweak the build to link in a second sha1 implementation and appropriately call it from non-security spots, I don't have a problem with that. But IMHO that's the itch of the crazy-case person to scratch.

  Side note: I also have a feeling that any operation that cares about
  non-object sha1 performance is probably ripe for other, bigger
  optimizations. If you update 300MB worth of index entries, then the
  cost of computing a checksum over it isn't a big deal. But if you have
  a 300MB index file and you update one entry (or you just want to read
  one entry), maybe we ought to consider solutions that don't involve
  the whole 300MB in the first place. I know that's a much harder change
  because it may involve new on-disk formats. But it seems like that's
  the right long-term path forward.

-Peff