Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mutation_cleaner extends lifetimes of data past the lifetime of its LSA migrators #3526

Closed
pdziepak opened this Issue Jun 18, 2018 · 1 comment

Comments

Projects
None yet
2 participants
@pdziepak
Copy link
Member

pdziepak commented Jun 18, 2018

Installation details
Scylla version (or git commit hash): aab6b0e

Since 'Merge "Introduce new in-memory representation for cells" from Paweł' (aab6b0e) atomic_cells use schema (abstract_type to be precise) dependent LSA migrators. They need to stay alive as long as the data is stored in LSA-managed memory.

This interacts badly with mutation_cleaner introduced in 'Merge "Avoid reactor stalls in cache with large partitions" from Tomasz' (62d0639) which extends the lifetime of mutation_partition but doesn't do the same for its schema. An additional problem is that mutation_cleaner is a member of cache_tracker which is a global object, that is destroyed after no_type_imr_descriptor (which includes a global LSA-migrator for cells that do not care about their type, e.g. the dead ones).

@pdziepak pdziepak added the bug label Jun 18, 2018

@pdziepak pdziepak self-assigned this Jun 18, 2018

@pdziepak

This comment has been minimized.

Copy link
Member Author

pdziepak commented Jun 18, 2018

Looks like only one half of this bug is active at the moment. There is type_interning_helper which, among other things, leaks user-created types until TLS destructors are run. The basic types are already in TLS which means that no type is ever destroyed in the middle of execution. Since, LSA migrators used by the IMR depend only on the type so far their lifetime is not an issue. However, this is something to watch out for when the next steps of conversion to the IMR are taken (e.g. converting keys or rows).

scylla/types.hh

Lines 892 to 913 in 79fae49

template <typename InternedType, typename... BaseTypes>
class type_interning_helper {
using key_type = std::tuple<BaseTypes...>;
using value_type = shared_ptr<const InternedType>;
struct hash_type {
size_t operator()(const key_type& k) const {
return apply(simple_tuple_hash<BaseTypes...>(), k);
}
};
using map_type = std::unordered_map<key_type, value_type, hash_type>;
static thread_local map_type _instances;
public:
static shared_ptr<const InternedType> get_instance(BaseTypes... keys) {
auto key = std::make_tuple(keys...);
auto i = _instances.find(key);
if (i == _instances.end()) {
auto v = ::make_shared<InternedType>(std::move(keys)...);
i = _instances.insert(std::make_pair(std::move(key), std::move(v))).first;
}
return i->second;
}
};

What remains of this bug is the mess caused by globals referencing globals. Fortunately, it looks like cache tracker can be deglobalised quite easily.

@slivne slivne added this to the 2.3 milestone Jun 21, 2018

avikivity added a commit that referenced this issue Jun 26, 2018

Merge "Deglobalise cache_tracker" from Paweł
"
Cache tracker is a thread-local global object that indirectly depends on
the lifetimes of other objects. In particular, a member of
cache_tracker: mutation_cleaner may extend the lifetime of a
mutation_partition until the cleaner is destroyed. The
mutation_partition itself depends on LSA migrators which are
thread-local objects. Since, there is no direct dependency between
LSA-migrators and cache_tracker it is not guarantee that the former
won't be destroyed before the latter. The easiest (barring some unit
tests that repeat the same code several billion times) solution is to
stop using globals.

This series also improves the part of LSA sanitiser that deals with
migrators.

Fixes #3526.

Tests: unit(release)
"

* tag 'deglobalise-cache-tracker/v1-rebased' of https://github.com/pdziepak/scylla:
  mutation_cleaner: add disclaimer about mutation_partition lifetime
  lsa: enhance sanitizer for migrators
  lsa: formalise migrator id requirements
  row_cache: deglobalise row cache tracker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.