Skip to content

feat: 添加IO性能测试工具test_ioperf#1666

Merged
fslongjin merged 1 commit intoDragonOS-Community:masterfrom
fslongjin:add-fio-like-test-program
Jan 15, 2026
Merged

feat: 添加IO性能测试工具test_ioperf#1666
fslongjin merged 1 commit intoDragonOS-Community:masterfrom
fslongjin:add-fio-like-test-program

Conversation

@fslongjin
Copy link
Member

实现一个简化的fio-like IO性能测试工具,支持顺序/随机读写模式,可配置块大小、线程数、测试时长等参数,并输出带宽、IOPS和延迟等性能指标

实现一个简化的fio-like IO性能测试工具,支持顺序/随机读写模式,可配置块大小、线程数、测试时长等参数,并输出带宽、IOPS和延迟等性能指标
@github-actions github-actions bot added the enhancement New feature or request label Jan 14, 2026
@fslongjin
Copy link
Member Author

@claude review

@dragonos-community-ai-pr-reviewer
Copy link

dragonos-community-ai-pr-reviewer bot commented Jan 14, 2026

Claude finished @fslongjin's task in 5m 58s —— View job


PR Review: feat: 添加IO性能测试工具test_ioperf

I've reviewed the new IO performance testing tool test_ioperf.c. Overall, it's a well-structured and useful addition that provides a simplified fio-like benchmark for DragonOS. The code follows a clear pattern, uses pthreads for concurrency, and supports essential parameters.

Below is a detailed review covering potential bugs, security considerations, and improvement suggestions.


Positive Aspects

  • Clean separation of concerns: parsing, normalization, worker threads, and result aggregation.
  • Proper handling of partial reads/writes with pread_full/pwrite_full loops and EINTR recovery.
  • Support for both size‑based and time‑based test modes.
  • Thread‑local file descriptors avoid sharing issues.
  • Meaningful error reporting with operation, offset, and length.

⚠️ Critical Issues

1. Deadlock when a worker fails before reaching the barrier

Location: worker() (lines 290–314)
Problem: If open_file_for_mode() or malloc() fails, the thread returns without calling pthread_barrier_wait(). Because the barrier is initialized with jobs count, the remaining threads will wait forever.
Impact: The benchmark hangs on any early error (e.g., missing file, out‑of‑memory).
Suggestion: Ensure every thread reaches the barrier, even after an error. One approach: set an error flag in ctx, call pthread_barrier_wait() (or a no‑op wrapper), then exit. A simpler fix is to move the barrier before the operations that can fail, but that would still leave malloc errors unhandled. Consider using a shared atomic error flag that all threads check after the barrier.

2. Missing cleanup on pthread_create failure

Location: main() (lines 639–642)
Problem: If pthread_create fails after some threads have already been created, the program returns 1 without joining those threads. This leaves zombie threads and may leak resources.
Fix: Keep track of successfully created threads and join them before exiting.

3. ftruncate error ignored for sequential‑write time‑based mode

Location: normalize_options() (lines 498–504)
Issue: The else branch of if (ftruncate(...) == 0) still sets file_size = req_size without reporting the error. If ftruncate fails (e.g., due to lack of space), the benchmark may later fail with confusing I/O errors.
Fix: Treat ftruncate failure as an error and return -1.


🐛 Potential Bugs & Edge Cases

4. Block‑size truncation when size_t is 32‑bit

Location: main() (line 556)
Detail: parse_size_bytes returns a uint64_t, but opt.bs is size_t. On a 32‑bit system, a block size > SIZE_MAX would be silently truncated.
Suggestion: Add a check if (v > SIZE_MAX) { fprintf(stderr, "block size too large\n"); return 2; }.

5. Offset overflow when casting uint64_t to off_t

Detail: Several places cast a uint64_t offset to off_t (e.g., next_offset_seq line 257, next_offset_rand line 279). If off_t is 32‑bit and the offset exceeds OFF_MAX, the cast is implementation‑defined and may produce a negative offset.
Suggestion: Add a runtime check if (off > OFF_MAX) { /* handle error */ } or ensure the file size never exceeds OFF_MAX (already limited by uint64_t).

6. Random offset generation for very small files

Location: next_offset_rand() (lines 262–279)
Issue: If file_size < bs, blocks becomes zero and the function always returns offset 0. This leads to repeated I/O at the same offset, which may not be intended.
Observation: The normalize_options function already enforces file_size >= bs for random modes (line 474), so this should not happen in practice. Still, the function could return 0 as a safe default.

7. Time‑based loop calls now_ns() every iteration

Location: worker() (lines 325–356)
Performance: Calling clock_gettime (via now_ns()) for every I/O operation adds overhead, especially for high‑IOPS tests.
Suggestion: Cache the deadline once and compare with a cheaper monotonic clock (e.g., clock_gettime every 1000 iterations) or use a timeout‑based approach. However, for a simple benchmark this may be acceptable.


🔧 Code Quality & Maintainability

8. Magic numbers in option parsing

Location: main() (lines 532–533)
Detail: The --fsync and --seed options use hard‑coded values 1000 and 1001. This is error‑prone if more options are added.
Suggestion: Use an enum or #define constants for readability.

9. Missing validation for --bs power‑of‑two

Detail: Some storage subsystems perform better with power‑of‑two block sizes. The tool could warn or enforce this, but it’s not required.

10. Data pattern not verified on reads

Detail: The buffer is filled with a fixed pattern (0xA5 ^ tid) for writes, but reads do not verify the content. This is fine for pure performance measurement, but if data integrity is desired, a verification mode could be added later.

11. Error‑code handling in now_ns()

Location: now_ns() (lines 51–58)
Issue: Returns 0 on clock_gettime failure, which is indistinguishable from a valid timestamp (e.g., system boot).
Suggestion: Abort on failure or return a sentinel like UINT64_MAX.

12. Suffix parsing rejects multi‑letter suffixes (e.g., "KB")

Location: parse_size_bytes() (lines 117–120)
Detail: The check if (end[1] != '\0') rejects suffixes like KB, MB. This is fine if the intended syntax is single‑letter (K, M, G), but the help text does not specify.
Suggestion: Clarify in usage or accept common two‑letter suffixes.


📈 Performance & Accuracy

  • The use of pthread_barrier for start‑time synchronization is good.
  • Calculating elapsed time as max_end - min_start across threads gives an accurate wall‑clock duration.
  • Average latency calculation correctly handles zero operations.

Recommendations

High priority:

  1. Fix the barrier deadlock (issue 如何创建磁盘镜像 #1).
  2. Clean up threads on pthread_create failure (issue Fix code scanning alert - File opened with O_CREAT flag but without mode argument #2).
  3. Handle ftruncate errors (issue 调整创建磁盘、挂载磁盘及卸载磁盘的脚本 #3).

Medium priority:
4. Add block‑size truncation check (issue #4).
5. Consider offset overflow (issue #5).

Low priority / future enhancements:

  • Add data verification mode.
  • Support O_DIRECT flag.
  • Allow multiple files per thread.
  • Add ramp‑up and steady‑state phases.

📋 Checklist for the Author

  • Ensure all threads reach the barrier even on early errors.
  • Join already‑created threads when pthread_create fails.
  • Propagate ftruncate errors in normalize_options.
  • Validate that block size fits in size_t.
  • Consider adding a runtime check for offset overflow.
  • (Optional) Reduce clock_gettime overhead in time‑based loop.

🔗 CI Status

– Claude Code Review completed successfully.

Note: The above feedback is based on a static review of the code. I have not run the tool in the DragonOS environment, so some assumptions about POSIX API availability may need verification.

Let me know if you would like me to implement any of the suggested fixes.

@fslongjin fslongjin merged commit 1c0948f into DragonOS-Community:master Jan 15, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant