Skip to content

lsm/io: round read buffer by max of memory/disk DMA alignment #30443

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.

Sign up for GitHub

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

Merged
travisdowns merged 1 commit into from
May 12, 2026
+6 −1
Merged

lsm/io: round read buffer by max of memory/disk DMA alignment #30443

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/v/lsm/io/file_io.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

#include <seastar/core/reactor.hh>

#include <algorithm>

namespace lsm::io {

disk_file_reader::disk_file_reader(std::filesystem::path path, ss::file file)
Expand All @@ -24,10 +26,13 @@ disk_file_reader::disk_file_reader(std::filesystem::path path, ss::file file)
ss::future<ioarray> disk_file_reader::read(size_t offset, size_t n) {
size_t memory_alignment = _file.memory_dma_alignment();
size_t disk_alignment = _file.disk_read_dma_alignment();
// ioarray requires its size to be a multiple of its alignment, so use the
// max of the two seastar alignments.
size_t max_alignment = std::max(memory_alignment, disk_alignment);
Copy link
Copy Markdown
Member

@dotnwat dotnwat May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this works because we make the (totally reasonable) assumption that the larger of these two is divisible by the smaller (e.g. 4k/512)

Copy link
Copy Markdown
Contributor

@andrwng andrwng May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea in practice this feels like it'll always bee the case, but to capture intent maybe we should use std::lcm()?

Copy link
Copy Markdown
Member

@dotnwat dotnwat May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think we need to do anything, but if we wanted to do something i'd advocate for an assertion.

Copy link
Copy Markdown
Member Author

@travisdowns travisdowns May 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this works because we make the (totally reasonable) assumption that the larger of these two is divisible by the smaller (e.g. 4k/512)

These are always powers of two, effectively guaranteed by the kernel/sesatar (which both rely on it), though under-documented, so the condition holds under that assumption. That means the smaller always divides the larger.

i don't think we need to do anything, but if we wanted to do something i'd advocate for an assertion.

We almost have that assertion, the same one that triggered this fix, in the ioarray constructor (the 2nd one):

    dassert(
      (alignment & (alignment - 1)) == 0,
      "alignment {} must be a power of two",
      alignment);
    dassert(
      size % alignment == 0,
      "size {} must be a multiple of alignment {}",
      size,

It's not exactly the same since it depends on the specific size, so it wouldn't trigger every time if we had a not divisible relationship.

I think I'd rather document the guarantee in seastar and assert there. Let me try that.

Copy link
Copy Markdown
Member Author

@travisdowns travisdowns May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scylladb/seastar#3399 to lock in the n^2 guarantee

Copy link
Copy Markdown
Member

@dotnwat dotnwat May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::has_single_bit

amaze

size_t adjusted_offset = ss::align_down(offset, disk_alignment);
size_t offset_delta = offset - adjusted_offset;
auto array = ioarray::aligned(
memory_alignment, ss::align_up(n + offset_delta, disk_alignment));
memory_alignment, ss::align_up(n + offset_delta, max_alignment));
try {
size_t amt = co_await _file.dma_read(adjusted_offset, array.as_iovec());
if (amt < offset_delta + n) {
Expand Down
Loading