storage: support dirty entries in batch cache #16954
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nvartolomei
force-pushed
the
nv/write-caching-rfc-final-draft
branch
from
1ecda4f
to
49c1e29
Compare
andrwng
reviewed
Mar 8, 2024
nvartolomei
force-pushed
the
nv/write-caching-rfc-final-draft
branch
from
49c1e29
to
9767245
Compare
dotnwat
reviewed
Mar 11, 2024
nvartolomei
force-pushed
the
nv/write-caching-rfc-final-draft
branch
from
9767245
to
0409642
Compare
nvartolomei
force-pushed
the
nv/write-caching-rfc-final-draft
branch
2 times, most recently
from
d9cf0db
to
a169ce1
Compare
nvartolomei
changed the title
nvartolomei
force-pushed
the
nv/write-caching-rfc-final-draft
branch
from
a169ce1
to
ce420b9
Compare
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46224#018e3f3a-9095-4243-9c79-309ffdfc5b96 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46311#018e4444-106f-400e-aa73-bdd1e2e0f872 |
|
/cdt |
andrwng
reviewed
Mar 15, 2024
Comment on lines
+377
to
+383
| log | ||
| ->truncate( | ||
| storage::truncate_config(truncate_offset, ss::default_priority_class())) | ||
| .get(); | ||
|
|
||
| // Reclaim everything from cache. | ||
| storage::testing_details::log_manager_accessor::batch_cache(mgr).clear(); |
There was a problem hiding this comment.
nit: might also be interesting to continue appending dirty batches, to make sure we don't hit any of the vasserts ensuring monotonicity
There was a problem hiding this comment.
That's a good call. Thinking about a good place where to add this which will also cover the randomization you mentioned earlier. We have a read_write_truncate test which I'm thinking about expanding (or copying) to cover batch cache too.
There was a problem hiding this comment.
Likely a follow up PR.
nvartolomei
force-pushed
the
nv/write-caching-rfc-final-draft
branch
from
ce420b9
to
32f82e1
Compare
Maintain `stable_offset <= dirty_offset` invariant. `advance_stable_offset` may be called before the continuation attached to the `segment_appender::append` where we are advancing the dirty offset.
nvartolomei
force-pushed
the
nv/write-caching-rfc-final-draft
branch
from
32f82e1
to
f6bcdd7
Compare
andrwng
reviewed
Mar 15, 2024
| @@ -648,8 +648,13 @@ void segment::advance_stable_offset(size_t filepos) { | |||
| } | |||
|
|
|||
| _reader->set_file_size(it->first); | |||
|
|
|||
There was a problem hiding this comment.
nit: squash with the previous commit?
There was a problem hiding this comment.
🤦 git absorb and rebase didn't work great here. Will do after i get the CI results.
I plan to extend the test suite to cover write caching functionality too. In the case of write caching we will want to read past the committed offset. I couldn't infer a good motivation for bounding test readers to committed offset. I believe that not having the upper bound by default can help uncover hidden bugs both in the implementation and in the testing code. If anything needs to bound reads, it must do it explicitly like the regular application code does.
Dirty entries are ones that aren't eligible for eviction as they might
have not yet been written durably to disk. Evicting them would otherwise
lead to "disappeared" data on cache miss.
We are making an implicit assumption that readers that want to read
beyond storage stable offset will always read through the batch cache.
Otherwise, the read-after-write semantics won't hold as the data
buffered in appender chunk cache isn't exposed.
---
Batch cache entries are `batch_cache::range` data structures. A range is
a 32KiB memory arena with some ancillary data structures. It is
associated with a `batch_cache_index`. `batch_cache_index` has 1-to-1
mapping with a segment. When a batch is put in the cache, the
`batch_cache_index` will append to an existing range or allocate a new
one and insert it into the `batch_cache`.
We propose to track dirty batches at `batch_cache::range` granularity
rather than per batch.
Using the insight that dirty batches are inserted with monotonically
increasing offsets, and the fact that we always mark as clean (clear the
dirty flag) for a prefix of offsets, it is enough to track only the
maximum dirty batch offset in a range and advance it when new dirty
batches are inserted. For marking offsets as clean (on flush) we check
if `clean_offset >= max_dirty_offset` and mark the range as clean. In
the alternative case, nothing needs to be done.
A `batch_cache_index` can be associated with multiple ranges which
potentially need to be marked as clean. As an optimization, we propose
to track which ranges might contain dirty data and when the request for
marking a prefix of offsets comes in to iterate only through this subset
of ranges. We will use this information together with the already
existing index which maps offsets to ranges
`absl::btree_map<model::offset, batch_cache::entry>` to iterate only
through the entries and ranges which contain the dirty offsets we are
interested in. The complexity of this iteration is O(N) where N is the
number of dirty batches that must be marked as clean.
To this end, we are introducing a new data structure called
`batch_cache_dirty_tracker` to keep track of minimum and maximum dirty
offsets in an index that we need for the optimization.
Technically, we could achieve a similar performance optimization by
doing a binary search over the index using desired mark clean `up_to`
offset as a needle and iterate backwards through ranges marking them as
clean and stopping once we hit a clean range. However, the new data
structure has some additional benefits:
This data structure facilitates some invariants checks:
- Dirty offsets are monotonic: `vassert(_max <= range.first && _max <
range.second, ...)`
- Truncation is requested only with a clean index:
`vassert(_dirty_tracker.clean(), ...)`. See the next section for why
this is important.
- At the end of batch cache / batch cache index lifetime there are no
dirty batches left in the index. This already caught 2 bugs in the
existing code:
1) redpanda-data#17032
2) redpanda-data#17039
Also, it improves the debugging experience as we include the state of
the dirty tracker in the `batch_cache_index` ostream insertion operator.
---
Truncation needs special attention too. When truncation is requested we
must evict batches with offsets higher than truncation point but still
retain dirty batches with smaller offsets. Since we can't have holes in
the ranges and can evict only at the range granularity we require a
flush prior truncation. It is already present so no changes required.
This proves that `<` is the right comparison operator in the following
if statement of the `batch_cache_index::mark_clean` routine:
```cpp
if (_dirty_tracker.clean() || up_to < _dirty_tracker.min()) {
// No dirty data in the cache.
return;
}
```
nvartolomei
force-pushed
the
nv/write-caching-rfc-final-draft
branch
from
f6bcdd7
to
31c451a
Compare
|
Known failure: |
andrwng
approved these changes
Mar 16, 2024
Is this still true? |
dotnwat
approved these changes
Mar 19, 2024
Comment on lines
+557
to
+560
| // that case, the batch entry should be considered clean. | ||
| _tracker.stable_offset < b.last_offset() | ||
| ? batch_cache::is_dirty_entry::yes | ||
| : batch_cache::is_dirty_entry::no); |
There was a problem hiding this comment.
looks ok. curious why not to do this in the batch cache itself by tracking the cleaned-up-to offset? i guess maybe are doing that accounting outside the cache?
Comment on lines
+208
to
+234
| // Maximum dirty batch offset that is stored in this range. If a range | ||
| // contains any dirty batch offsets we prevent its eviction so that | ||
| // readers get cache hits until the data is persisted to disk and we can | ||
| // rehydrate on a miss. | ||
| // | ||
| // Dirty batches are inserted with monotonically increasing offsets and | ||
| // marked as clean with an upper bound. | ||
| // | ||
| // You can imagine the put operation moving one cursor forward and | ||
| // mark_clean operation moving another one from behind. Once they | ||
| // overlap, the range is considered clean. | ||
| // | ||
| // Let's take the following example range with clean and dirty batches: | ||
| // | ||
| // +------------+------------|------------+----------+ | ||
| // | c=10..15 | d=16..18 | d=19..25 | c=4..6 | | ||
| // +------------+------------|------------+----------+ | ||
| // ^ | ||
| // _max_dirty_offset=25 -----+ | ||
| // | ||
| // Note 1: Dirty batches always are monotonically increasing but clean | ||
| // batches might not be as they might have been added by concurrent | ||
| // read operations. | ||
| // | ||
| // Note 2: We don't actually track whether a particular batch is clean | ||
| // or dirty. The `c` and `d` on the diagram are for illustration | ||
| // purposes only. |
Comment on lines
+468
to
+469
| /// 2. Truncation is requested only with a clean index (we can evict only at | ||
| /// range level). |
There was a problem hiding this comment.
i take this truncation isn't the same as truncating a segment where i guess it would be fine to throw away dirty data?
| first != _index.end(), | ||
| "Iterator must exist if dirty tracker isn't clean."); | ||
|
|
||
| auto last = std::next(find_first(up_to_inclusive)); |
There was a problem hiding this comment.
what guarantees that find_first here doesn't return end()?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cover letter is in the first commit of the PR https://github.com/redpanda-data/redpanda/pull/16954/commits
Backports Required
Release Notes