Add mutex to RegisteredOptions #10480
Conversation
Summary: I wanted to make filter_policy a mutable option, but the SetOptions race fix proposed in facebook#10480 doesn't work for object options like filter_policy. (Race still reported.) I found that when the options objects are copied for persisting to file, the table factory (and thus options) is not copied, therefore leading to a race between persisting the options and subsequent updates with SetOptions(). It appears that fixing this fixes the race that was being reported with SetOptions() in the Fixes facebook#10079 Intended follow-up: See facebook#10021 Test Plan: unit test added, crash test updated with more SetOptions and higher probability of calling it. Ran TSAN blackbox crashtest for hours with options to amplify potential race (see facebook#10079)
Summary: I wanted to make filter_policy a mutable option, but the SetOptions race fix proposed in facebook#10480 doesn't work for object options like filter_policy. (Race still reported.) I found that when the options objects are copied for persisting to file, the table factory (and thus options) is not copied, therefore leading to a race between persisting the options and subsequent updates with SetOptions(). It appears that fixing this fixes the race that was being reported with SetOptions() in the Fixes facebook#10079 Intended follow-up: See facebook#10021 Test Plan: unit test added, crash test updated with more SetOptions and higher probability of calling it. Ran TSAN blackbox crashtest for hours with options to amplify potential race (see facebook#10079)
|
I believe this change solves the write-write race but I still get read-write race, and I don't believe the other referenced PRs would fix that. The table options values are still modified in place and read from the same place without any sort of synchronization. I think we need to move table_factory to mutable options, so that it's tracked with Version, and copy-on-write to the table_factory. |
@pdillinger I do not understand how moving an option to the mutable list will solve this problem. The only way I would see that working is if any time you update the mutable object, you perform a deep copy of the mutable object (e.g. two table factories might have equivalent FilterPolicy objects but would not have the same one. But what if I want objects shared between instances -- like the block cache or persistent cache? How would I know which objects should be shared versus copied? |
|
Yes, copy-on-write. The mutable list is copied as needed between Versions (including creating a new Version on SetOptions) to avoid read-write races. The immutable list is not copied, so recursive members of immutable CF options can only be safely mutable if the owning object is thread-safe with those mutations. TableFactory is not thread-safe under mutation of its options. So I was envisioning that the configurable objects have a property like IsMutationThreadSafe that would determine whether in thread-shared contexts the only way to change options is to copy before mutating and then thread-sharing the mutated copy. Cache is thread-safe under option mutations, so doesn't need to be copied on mutation. |
Adds a mutex to the RegisteredOptions structure. Each set of registered options (one per "struct") has its own mutex and is used while that option is being processed (serialized, configured, etc).
This change, along with #10479 and #10387, closes #10079.