Skip to content

[release-7.3] Clip read-ahead cache reads to file size for small/last blocks#13293

Merged
akankshamahajan15 merged 1 commit into
apple:release-7.3from
arnav-ag:arnav/backport-read-ahead-fix-7.3
May 30, 2026
Merged

[release-7.3] Clip read-ahead cache reads to file size for small/last blocks#13293
akankshamahajan15 merged 1 commit into
apple:release-7.3from
arnav-ag:arnav/backport-read-ahead-fix-7.3

Conversation

@arnav-ag
Copy link
Copy Markdown
Contributor

Backport of #13264 to release-7.3.

Problem

AsyncFileReadAheadCache::read_impl issues every block read with length = m_block_size, regardless of where the block sits in the file. readBlock then allocates a CacheBlock of exactly that size and calls m_f->read(..., m_block_size, offset).

For a file smaller than one block, or the trailing block of any file, this means:

  • The cache over-allocates: e.g. a 100-byte file with m_block_size = 1 MiB allocates a full 1 MiB buffer.
  • The underlying IAsyncFile::read is asked for bytes past EOF. Most backends tolerate this (they return a short read), but some, notably HTTP-range blob store reads, can reject or misbehave on a range that extends past the object's end, surfacing as a spurious read failure on small files.

The function already clips the overall request to fileSize - offset near the top, but the per-block reads still happen at the full m_block_size.

Solution

Clip each per-block read to the bytes remaining in the file, matching the blockStart/std::min pattern already used a few lines further down in the copy loop:

int64_t blockStart = (int64_t)blockNum * f->m_block_size;
int readLength = std::min<int64_t>(f->m_block_size, fileSize - blockStart);
fblock = readBlock(f.getPtr(), readLength, blockStart);

fileSize is captured at the top of the function and lastBlockToStart is already clamped to lastBlockNumInFile, so fileSize - blockStart is strictly positive within this loop. readLength is bounded above by m_block_size (int), so the narrowing into readBlock(..., int length, ...) is safe.

The only difference vs. the main PR is the filename: in release-7.3 the header is AsyncFileReadAhead.actor.h; the surrounding code is identical.

@foundationdb-ci
Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-73 on Linux RHEL 9

  • Commit ID: d12faa8
  • Duration 0:58:00
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-cluster-tests-73 on Linux RHEL 9

  • Commit ID: d12faa8
  • Duration 1:00:38
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@foundationdb-ci
Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang-73 on Linux RHEL 9

  • Commit ID: d12faa8
  • Duration 3:00:48
  • Result: ❌ FAILED
  • Error: Build has timed out.
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang-73 on Linux RHEL 9

  • Commit ID: d12faa8
  • Duration 0:36:14
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@akankshamahajan15 akankshamahajan15 merged commit fa6e7e1 into apple:release-7.3 May 30, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants