-
Notifications
You must be signed in to change notification settings - Fork 178
mv mem fences
- merged to master -
- code complete - August 18, 2016
- development started - August 15, 2016
Note: this discussion covers parallel branches in both eleveldb and leveldb. Both branches have the name mv-mem-fences.
There has been a bug in the eleveldb code that was first seen in the fourth quarter of 2014 on a Riak cluster running on virtual machines in Poland. The customer moved to a different type of server and the bug went away. The same bug recently appeared during testing of new functionality for the Riak 2.2 release. This time the investigation actually found the likely cause.
The bug is manifested within eleveldb's c_src/refobjects.cc routine ErlRefObject::RefDec(). The last block of that routine contains an assert(). The assert() was left by mistake. It was originally used to help debug the interaction of multiple objects containing references to an iterator object that was closing. leveldb's Makefile contains the necessary build commands to disable assert() in production builds. eleveldb's build process, controlled by rebar tool, does not have the same disable commands. Therefore this assert() in refobjects.cc is live in production. This assert() is what triggered both in 2014 and recently with Riak 2.2 testing.
There is no verification test at the time of this writing. The believed bug cause is a few lines above the assert. The original code was:
if (flag)
RefObject::RefDec();
else
cur_count=0;
RefObject::RefDec() has the potential to delete the current object if its decrement brings the reference count to zero. And the code block below this one could also delete the current object. The symptoms suggest that the delete in the RefObject::RefDec() is occurring, thus the assert() is firing because the heap has been reused before reaching the assert. The correction employs a new function RefObject::RefDecNoDelete() which will never delete the current object.
The above thread sequencing was difficult to define. The first approach to this bug was to begin adding better memory fencing for the ErlRefObject and RefObject reference counting. Paul Place previously noted that the code was highly dependent upon how compiler optimizations respected the "volatile" attribute of the m_RefCount variable. Also, both bug reports occurred on virtual machines. Those machines might have memory consistency issues (just guessing, no proof or references). Therefore this branch contains changes to the RefObject class and its use to properly use atomic operations that are documented as providing memory fencing.
And finally the branch also contains couple of compiler warning and a user community reported memory leak fixes.
parse_init_options() function now lowers the worker thread count when the limited_developer_mem option is set. Each thread includes a 2Mbyte to 8Mbyte stack. Reducing the default count of 71 to 7 will likely impact performance but reduce the total memory foot print. Programmers set the limited_developer_mem option when running multiple Riak instances on a single server for development and testing purposes. The performance impact is therefor not a concern.
async_write() function now includes a "delete batch" line within the subsequent error path. A later error path does not need the same line because the WriteTask object owns the write batch object and will clean it up.
The remaining changes in this file are all conversions from accessing the m_CloseRequested volatile object directly versus using a new memory fenced GetCloseRequested() function.
The m_CloseRequested variable was previously available for "public" access. This branch changes it to "protected" access and forces external objects to use a variable accessor. The variable accessor, GetCloseRequested() is contains memory fencing code.
ErlRefObject::InitiateCloseRequest() now uses memory fenced accessors for overall reference count and m_CloseRequested read operations. It does not contain memory fenced operations for the m_CloseRequested writes. The nature of the current bug suggests that is not a possible error source. The write fencing is therefore left for a future branch when release time is not critical.
Two code lines use the new RefObject::RefDecNoDelete(). The first line changed solely to establish a consistent usage pattern. The second line change is the actual fix to address the double delete bug.
Parameters updated to reduce memory footprint of the test.
This change originated in branch mv-expiry-schema. This branch needed it copied here to match leveldb version.
Address compiler warning.
Address compiler warnings.
Address compiler warning.
Address compiler warning.
Address compiler warning.
Address compiler warning.
Eleveldb's ErlRefObject code was directly accessing m_RefCount. This member variable is now a "private" member to force all code to use accessors to get and change the reference count. This forces all get operations to use a new memory fenced GetRefCount() function. The new RefDecNoDelete() allows derived classes to use the accessor functions for decrement operations without concern for the side effect of the object self deleting.