Prevent segment appender from writing into already dispatched head buffer #28249
+298
−91
Conversation
This file contains hidden or 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
mmaslankaprv
force-pushed
the
segment-appender-rework-2
branch
from
8396f85 to
d3756c9
Compare
|
/dt |
mmaslankaprv
force-pushed
the
segment-appender-rework-2
branch
from
d3756c9 to
180b333
Compare
|
/dt |
Retry command for Build#75218please wait until all jobs are finished before running the slash command |
CI test resultstest results on build#75218
test results on build#75299
test results on build#75472
|
mmaslankaprv
force-pushed
the
segment-appender-rework-2
branch
2 times, most recently
from
98d8d0d to
8d9bf0c
Compare
|
/dt |
1 similar comment
|
/dt |
Added method that copies the reminder that spans from the last page aligned address to the end of the buffer. The method copies data and sets the chunk positions to make sure the number of not flushed bytes is preserved. Signed-off-by: Michał Maślanka <michal@redpanda.com>
This commit introduces a logic that prevents the head chunk from being appended if it is dispatched i.e. its content is being DMA written. Previously the head chunk buffer might have been passed to the `file::dma_write_call` but appends could still write to that chunk. This caused a problem which manifested on some filesystems and some hardware RAID controllers. The problem manifested as corruption in the last page of a file on disk. This PR prevents the head being written to disk from being concurrently appended. The mechanism used here is copying the reminder which wasn't flushed and spans beyond the last page aligned address to a new chunk. The reminder is copied only if the current head write was dispatched. After copying the chunk writer is requested to recycle the chunk and return it to the cache. Signed-off-by: Michał Maślanka <michal@redpanda.com>
Signed-off-by: Michał Maślanka <michal@redpanda.com>
Sometimes the benchmark can take too long to execute. Increasing timeout to eliminate intermittent failures. Signed-off-by: Michał Maślanka <michal@redpanda.com>
mmaslankaprv
force-pushed
the
segment-appender-rework-2
branch
from
c6c178a to
b10ea9b
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a mechanism to prevent the segment appender from writing to the head chunk buffer while it's being DMA written to disk. This addresses data corruption issues that could occur on certain filesystems and hardware RAID controllers when concurrent appends modified data that was already dispatched for writing.
Key changes:
- Prevents concurrent modification of dispatched head chunks by copying unwritten data to a new chunk
- Adds tracking for bytes copied in chunk reminders to detect when the prevention mechanism is active
- Introduces comprehensive tests to verify the fix works correctly under various concurrent scenarios
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/v/storage/segment_appender.cc |
Core logic to detect dispatched writes and copy chunk reminders to prevent concurrent modification |
src/v/storage/segment_appender.h |
API changes including new stats tracking and chunk dispatch detection methods |
src/v/storage/segment_appender_chunk.h |
New copy_reminder_from method to safely copy unwritten data between chunks |
src/v/storage/tests/segment_appender_test.cc |
Comprehensive test coverage for concurrent flush scenarios and chunk copying behavior |
src/v/storage/tests/BUILD |
Test timeout configuration update |
| * pending_aligned_begin() i.e. it is the part that overflows last full page | ||
| * boundary. | ||
| * | ||
| * This method preservers the position and flushed position relative to the |
There was a problem hiding this comment.
Corrected spelling of 'preservers' to 'preserves'.
Suggested change
| * This method preservers the position and flushed position relative to the | |
| * This method preserves the position and flushed position relative to the |
| * | ||
| * This method preservers the position and flushed position relative to the | ||
| * pending_aligned_begin() | ||
| * IMPORTANT: this method will reset the chunk conent before copying the |
There was a problem hiding this comment.
Corrected spelling of 'conent' to 'content'.
Suggested change
| * IMPORTANT: this method will reset the chunk conent before copying the | |
| * IMPORTANT: this method will reset the chunk content before copying the |
Comment on lines
+241
to
+243
| // true if the write extends to the end of the chunk or current write is | ||
| // the last one using current chunk i.e., this is the last write that | ||
| // will use the current chunk before it is recycled |
There was a problem hiding this comment.
The comment should be updated to reflect the new field name. The field was renamed from 'full' to 'last_write_to_current_chunk' but the comment still references the old meaning.
Suggested change
| // true if the write extends to the end of the chunk or current write is | |
| // the last one using current chunk i.e., this is the last write that | |
| // will use the current chunk before it is recycled | |
| // true if this is the last write that will use the current chunk before | |
| // it is recycled. This does not necessarily mean the write extends to | |
| // the end of the chunk. |
|
|
||
| chunk_1.flush(); | ||
| auto chunk_3 = make_chunk(16_KiB); | ||
| // only last 2 KiB should be |
There was a problem hiding this comment.
The comment on line 323 is incomplete. It should specify what the 'only last 2 KiB should be' refers to (e.g., 'only last 2 KiB should be copied').
Suggested change
| // only last 2 KiB should be | |
| // only last 2 KiB should be copied to chunk_3 from chunk_1 after flush |
Retry command for Build#75472please wait until all jobs are finished before running the slash command |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This commit introduces a logic that prevents the head chunk from being
appended if it is dispatched i.e. its content is being DMA written.
Previously the head chunk buffer might have been passed to the
file::dma_write_callbut appends could still write to that chunk. Thiscaused a problem which manifested on some filesystems and some hardware
RAID controllers. The problem manifested as corruption in the last page
of a file on disk.
This PR prevents the head being written to disk from being concurrently
appended. The mechanism used here is copying the reminder which wasn't
flushed and spans beyond the last page aligned address to a new chunk.
The reminder is copied only if the current head write was dispatched.
After copying the chunk writer is requested to recycle the chunk and
return it to the cache.
Backports Required
Release Notes
Improvements