Skip to content

Conversation

xemul
Copy link
Contributor

@xemul xemul commented Oct 14, 2025

The test validates several things:

  • the maximum number of writes-above-file-size doesn't exceed the limit
  • the number of writes-above-file-size actually reaches it
  • no parallel fsyncs are run if disabled

In fact, the 2nd test fails for concurrency above 1, but for XFS it's not currently configured, so OK for now.

To work, the test creates an "real" temporary file, but doesn't do real IO, instead, it runs counting-only operations. Truncations, however, happen for real, because this file impl uses raw ftruncate systemcall for it.

@xemul xemul requested review from bhalevy and Copilot October 14, 2025 15:13
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a comprehensive unit test for the append_challenged_posix_file implementation to validate append concurrency limits, write behavior, and exclusive fsync functionality. The test ensures that the file implementation correctly handles write operations that extend beyond the current file size while respecting concurrency constraints.

  • Adds a test framework class append_challenged_posix_file_test that simulates file operations without actual I/O
  • Implements validation for append concurrency limits and exclusive fsync behavior
  • Creates helper functions to construct test instances of the append-challenged file implementation

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
tests/unit/file_io_test.cc Adds the main test class and test cases for append-challenged file behavior
src/core/file.cc Implements factory function for creating test instances of append-challenged files
src/core/file-impl.hh Adds forward declarations and friend class access for testing

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.


SEASTAR_TEST_CASE(test_append_challenged_posix_file_impl) {
return tmp_dir::do_with_thread([] (tmp_dir& t) {
test_append_challenged_posix_file_concurrency(t, 0);
Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

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

Testing with concurrency 0 may not be meaningful since it suggests no concurrent operations are allowed, which could lead to undefined behavior or deadlocks in the append-challenged file implementation.

Suggested change
test_append_challenged_posix_file_concurrency(t, 0);

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0 concurrency means something else here -- it tells that no size-changing operations are allowed, so the appenc challenged file truncates it ahead of any write.

@xemul xemul force-pushed the br-append-challenged-posix-file-impl-test branch 2 times, most recently from 140b6c5 to d86d69f Compare October 14, 2025 16:21
The test validates several things:
- the maximum number of writes-above-file-size doesn't exceed the limit
- the number of writes-above-file-size actually reaches it
- no parallel fsyncs are run if disabled

In fact, the 2nd test fails for concurrency above 1, but for XFS it's
not currently configured, so OK for now.

To work, the test creates an "real" temporary file, but doesn't do real
IO, instead, it runs counting-only operations. Truncations, however,
happen for real, because this file impl uses raw ftruncate systemcall
for it.

Signed-off-by: Pavel Emelyanov <[email protected]>
@xemul xemul force-pushed the br-append-challenged-posix-file-impl-test branch from d86d69f to 1783f7c Compare October 20, 2025 09:44
@xemul
Copy link
Contributor Author

xemul commented Oct 20, 2025

upd:

  • rebased
  • use explicit dev variable for device number used to create the file-impl for testing

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.

2 participants