Bug 61516 - LevelDB: turn on paranoid checks and verify checksums, log errors
Summary: LevelDB: turn on paranoid checks and verify checksums, log errors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hans Wennborg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-26 03:24 PDT by Hans Wennborg
Modified: 2011-05-27 03:37 PDT (History)
2 users (show)

See Also:

Attachments
Patch (6.53 KB, patch)
2011-05-26 03:27 PDT, Hans Wennborg 		no flags Details | Formatted Diff | Diff
Patch (6.49 KB, patch)
2011-05-27 01:48 PDT, Hans Wennborg 		tonyg: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>