Bug 61516

Summary: LevelDB: turn on paranoid checks and verify checksums, log errors
Product: WebKit Reporter: Hans Wennborg <hans>
Component: New BugsAssignee: Hans Wennborg <hans>
Status: RESOLVED FIXED    
Severity: Normal CC: andreip, dgrogan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch tonyg: review+

Description Hans Wennborg 2011-05-26 03:24:39 PDT
LevelDB: turn on paranoid checks and verify checksums, log errors
Comment 1 Hans Wennborg 2011-05-26 03:27:58 PDT
Created attachment 94953 [details]
Patch
Comment 2 David Grogan 2011-05-26 12:48:11 PDT
Comment on attachment 94953 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94953&action=review

LGTM

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:130
> +    if (s.IsNotFound())

What circumstances cause a Put to return IsNotFound?

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:259
> +    readOptions.verify_checksums = true;

Do you know how much performance impact this is expected to have?  What happens if a checksum doesn't check out?
Comment 3 Hans Wennborg 2011-05-27 01:48:08 PDT
Created attachment 95143 [details]
Patch
Comment 4 Hans Wennborg 2011-05-27 01:49:39 PDT
(In reply to comment #2)
> (From update of attachment 94953 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94953&action=review
> 
> LGTM
> 
> > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:130
> > +    if (s.IsNotFound())
> 
> What circumstances cause a Put to return IsNotFound?
None; good catch. Removed.
> 
> > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:259
> > +    readOptions.verify_checksums = true;
> 
> Do you know how much performance impact this is expected to have?  What happens if a checksum doesn't check out?

Then operations start failing with a Status that indicated corruption.

I don't know the exact performance impact, but I don't expect it to be bad. The checksum is just a CRC, and I'd rather turn it on now and consider relaxing it later when we start tuning if it turns up as a bottleneck.
Comment 5 Tony Gentilcore 2011-05-27 03:11:28 PDT
Comment on attachment 95143 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95143&action=review

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:257
> +    readOptions.verify_checksums = true;

To David's point, perhaps just add a FIXME here mentioning that removing this might be a perf win once the API is more stable.
Comment 6 Hans Wennborg 2011-05-27 03:36:55 PDT
(In reply to comment #5)
> (From update of attachment 95143 [details])
Thanks for the review!


> View in context: https://bugs.webkit.org/attachment.cgi?id=95143&action=review
> 
> > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:257
> > +    readOptions.verify_checksums = true;
> 
> To David's point, perhaps just add a FIXME here mentioning that removing this might be a perf win once the API is more stable.
Done.
Comment 7 Hans Wennborg 2011-05-27 03:37:43 PDT
Committed r87490: <http://trac.webkit.org/changeset/87490>