Bug 61516 – LevelDB: turn on paranoid checks and verify checksums, log errors

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Hans Wennborg]

LevelDB: turn on paranoid checks and verify checksums, log errors

[Hans Wennborg]

Created attachment 94953 [details]
Patch

[David Grogan]

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?

[Hans Wennborg]

Created attachment 95143 [details]
Patch

[Hans Wennborg]

(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.

[Tony Gentilcore]

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.

[Hans Wennborg]

(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.

[Hans Wennborg]

Committed r87490: <http://trac.webkit.org/changeset/87490>