Skip to content

Validate file-perf I/O depth and block size - #138

Open
fallintoplace wants to merge 1 commit into
ClickHouse:mainfrom
fallintoplace:fallintoplace/file-perf-validation
Open

Validate file-perf I/O depth and block size#138
fallintoplace wants to merge 1 commit into
ClickHouse:mainfrom
fallintoplace:fallintoplace/file-perf-validation

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

  • Reject --iodepth 0 before starting the benchmark.
  • Reject block sizes outside the uint32_t range before narrowing.
  • Add regression coverage for both invalid CLI values.

--iodepth 0 leads to invalid slot access and a modulo-by-zero path. A size such as 4g was narrowed to zero before the existing block-size checks.

Tests

  • git diff --check
  • ./bb fmt --check
  • Targeted x86-64 Linux build of file-perf
  • Targeted CTest cases for zero I/O depth and oversized block size

@praktika-gh

praktika-gh Bot commented Aug 23, 2026

Copy link
Copy Markdown

Workflow [PR], commit [c474a8e]


Code Review

Result: ✅ No issues found

What changed: Validates that file-perf I/O depth is nonzero and block size fits in a nonzero uint32_t before benchmark setup. It also detects overflow in suffixed size parsing and adds CTest regression coverage for invalid values.

No issues found. The previously reported suffix-multiplication overflow is now checked before multiplication and covered by a regression test.

Investigation: 3/13 rounds, 16 tool calls.

Comment thread src/perf/file-perf.cpp Outdated
@fallintoplace
fallintoplace force-pushed the fallintoplace/file-perf-validation branch from a32f4c4 to b04bef5 Compare August 23, 2026 21:07
@fallintoplace fallintoplace changed the title file-perf: validate I/O depth and block size Validate file-perf I/O depth and block size Aug 25, 2026
@fallintoplace
fallintoplace force-pushed the fallintoplace/file-perf-validation branch from b04bef5 to 80a593b Compare August 25, 2026 09:22
@fallintoplace
fallintoplace force-pushed the fallintoplace/file-perf-validation branch from 80a593b to c474a8e Compare August 25, 2026 09:36
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.

1 participant