Fix readers cache truncation deadlock #5681

mmaslankaprv · 2022-07-27T15:41:43Z

Cover letter

Fixed condition checking if range is locked. Incorrect check resulted in
situations in which log truncation was blocked by pending readers.

Log truncation is multi step process that ends with grabbing necessary
segments write logs and data deletion. In order for truncation to grab
the write lock all readers which own read lock units from range being
truncated must be evicted from readers_cache. Since log truncation
contains multiple scheduling points it may interleave with another fiber
creating a reader for log that is currently being truncated. This reader
MUST not be cached as truncation would need to wait for it to be
evicted. Additionally no new readers can be created as truncation
related write lock request is waiting in the read_write_lock
underlying semaphore waiters queue.

In order to prevent readers requesting truncated range from being cached
readers cache maintain list of locked ranges i.e. ranges for which
readers can not be cached.

Previously an incorrect condition checking if reader belongs to locked
range allow it to be cached preventing the truncate action to
continue. This stopped all other writes and truncation for 60 seconds,
after this duration the reader was evicted from the cache, its lease was
released and truncation was able to finish.

Fixed incorrect condition checking if reader is within the locked
range.

Fixes: #5510

Release notes

Fixed move constructor and operator of `readers_cache::range_lock_holder`. Previously moving the `range_lock_holder` would result in lock being released. Signed-off-by: Michal Maslanka <michal@redpanda.com>

ajfabbri · 2022-07-27T21:39:45Z

src/v/storage/readers_cache.cc

+      _locked_offset_ranges.end(),
+      [reader_base_offset, reader_end_offset](const offset_range& range) {
+          return reader_base_offset <= range.second
+                 && reader_end_offset >= range.first;


Story time: When I finished grad school and interviewed at my first big job, this was the last coding problem in a long day of interviews. This part in particular is error prone. Also, the guy who gave me this problem is now making autonomous robots that kill things (weeds) with lasers.

ajfabbri · 2022-07-27T21:44:10Z

src/v/storage/readers_cache.cc

+      _locked_offset_ranges.begin(),
+      _locked_offset_ranges.end(),
+      [reader_base_offset, reader_end_offset](const offset_range& range) {
+          return reader_base_offset <= range.second


Good find. This condition looks correct to me. Previous one didn't make any sense to me. Good call putting it in a helper function.

ajfabbri · 2022-07-27T21:47:15Z

src/v/storage/tests/readers_cache_test.cc

+} // namespace storage
+using namespace storage;
+
+FIXTURE_TEST(test_range_is_correctly_locked, readers_cache_test_fixture) {


Thanks for adding a unit test.

ajfabbri · 2022-07-27T21:50:04Z

src/v/storage/tests/storage_e2e_test.cc

+          });
+      });
+
+    produce.get();


I have some questions about this integration test.. nothing that should block this from being merged, IMO. We can discuss separately.

ajfabbri

+1 (pending CI): The new range overlap condition makes much more sense than before. Thanks for adding tests. For "continuous improvement", let's remind each other during review that all new APIs / modules should come with a unit test.

andrwng · 2022-07-27T22:16:35Z

src/v/storage/readers_cache.cc

-    auto lock_it = std::find_if(
-      _locked_offset_ranges.begin(),
-      _locked_offset_ranges.end(),
-      [&reader](const offset_range& range) {
-          return reader->lease_range_base_offset() > range.second
-                 || reader->lease_range_end_offset() < range.first;
-      });


Nice catch! The fix seems reasonable, though I'm curious how else this bug may manifest itself. Is the worst that can happen a minute-long deadlock? Or is there any way this can result in a segfault or segments being deleted incorrectly?

one minute deadlock is how it manifests, it can not lead to any other more serious issue as read/write/truncate concurrency control is separate from readers cache.

dotnwat

lgtm pending the CI failure audit

nice catch. that's great find.

dotnwat · 2022-07-28T01:41:07Z

src/v/storage/readers_cache.h

-            std::erase(_cache->_locked_offset_ranges, _range);
+            if (_range) {
+                std::erase(_cache->_locked_offset_ranges, _range.value());
+            }


gosh. nice catch

src/v/storage/readers_cache.cc

src/v/storage/tests/storage_e2e_test.cc

dotnwat · 2022-07-28T01:57:08Z

src/v/storage/tests/readers_cache_test.cc

+        auto holder = cache.evict_prefix_truncate(model::offset(10)).get();
+
+        // [0,1] is in [0,10]
+        test_within_range(0, 1, true);


Fixed condition checking if range is locked. Incorrect check resulted in situations in which log truncation was blocked by pending readers. Log truncation is multi step process that ends with grabbing necessary segments write logs and data deletion. In order for truncation to grab the write lock all readers which own read lock units from range being truncated must be evicted from `readers_cache`. Since log truncation contains multiple scheduling points it may interleave with another fiber creating a reader for log that is currently being truncated. This reader MUST not be cached as truncation would need to wait for it to be evicted. Additionally no new readers can be created as truncation related write lock request is waiting in the `read_write_lock` underlying semaphore waiters queue. In order to prevent readers requesting truncated range from being cached readers cache maintain list of locked ranges i.e. ranges for which readers can not be cached. Previously an incorrect condition checking if reader belongs to locked range allow it to be cached preventing the `truncate` action to continue. This stopped all other writes and truncation for 60 seconds, after this duration the reader was evicted from the cache, its lease was released and trucation was able to finish. Fixed incorrect condition checking if reader is within the locked range. Fixes: redpanda-data#5510 Signed-off-by: Michal Maslanka <michal@redpanda.com>

Signed-off-by: Michal Maslanka <michal@redpanda.com>

s/readers_cache: made readers_cache::range_lock_holder moveable

cafa7dd

Fixed move constructor and operator of `readers_cache::range_lock_holder`. Previously moving the `range_lock_holder` would result in lock being released. Signed-off-by: Michal Maslanka <michal@redpanda.com>

mmaslankaprv requested review from dotnwat, LenaAn and VadimPlh as code owners July 27, 2022 15:41

github-actions bot added the area/redpanda label Jul 27, 2022

ajfabbri reviewed Jul 27, 2022

View reviewed changes

ajfabbri previously approved these changes Jul 27, 2022

View reviewed changes

andrwng reviewed Jul 27, 2022

View reviewed changes

dotnwat previously approved these changes Jul 28, 2022

View reviewed changes

ajfabbri mentioned this pull request Jul 28, 2022

PartitionMoveInterruption.test_cancelling_partition_move.replication_factor=3.unclean_abort=True.recovery=restart_recovery #5612

Closed

mmaslankaprv added 2 commits July 28, 2022 07:54

s/tests: added readers_cache range locking test

1c31f91

Signed-off-by: Michal Maslanka <michal@redpanda.com>

mmaslankaprv dismissed stale reviews from dotnwat and ajfabbri via 1c31f91 July 28, 2022 06:48

mmaslankaprv force-pushed the fix-readers-cache-truncation-deadlock branch from 9a9f31a to 1c31f91 Compare July 28, 2022 06:48

mmedenjak added kind/bug Something isn't working area/tests ci-failure labels Jul 28, 2022

andrwng mentioned this pull request Jul 28, 2022

Failure in ConsumerOffsetsMigrationTest.test_cluster_is_available_during_upgrade_without_group_topic #4848

Closed

mmaslankaprv requested review from dotnwat, andrwng and ajfabbri July 28, 2022 15:31

dotnwat approved these changes Jul 28, 2022

View reviewed changes

mmaslankaprv merged commit 16d30b1 into redpanda-data:dev Jul 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix readers cache truncation deadlock #5681

Fix readers cache truncation deadlock #5681

mmaslankaprv commented Jul 27, 2022 •

edited

Loading

ajfabbri Jul 27, 2022 •

edited

Loading

ajfabbri Jul 27, 2022 •

edited

Loading

ajfabbri Jul 27, 2022 •

edited

Loading

ajfabbri Jul 27, 2022

ajfabbri left a comment

andrwng Jul 27, 2022

mmaslankaprv Jul 28, 2022

dotnwat left a comment

dotnwat Jul 28, 2022

dotnwat Jul 28, 2022

Fix readers cache truncation deadlock #5681

Fix readers cache truncation deadlock #5681

Conversation

mmaslankaprv commented Jul 27, 2022 • edited Loading

Cover letter

Release notes

ajfabbri Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

ajfabbri Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

ajfabbri Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

ajfabbri Jul 27, 2022

Choose a reason for hiding this comment

ajfabbri left a comment

Choose a reason for hiding this comment

andrwng Jul 27, 2022

Choose a reason for hiding this comment

mmaslankaprv Jul 28, 2022

Choose a reason for hiding this comment

dotnwat left a comment

Choose a reason for hiding this comment

dotnwat Jul 28, 2022

Choose a reason for hiding this comment

dotnwat Jul 28, 2022

Choose a reason for hiding this comment

mmaslankaprv commented Jul 27, 2022 •

edited

Loading

ajfabbri Jul 27, 2022 •

edited

Loading

ajfabbri Jul 27, 2022 •

edited

Loading

ajfabbri Jul 27, 2022 •

edited

Loading