Repository navigation
fix(libkernel): serialize Windows vectored file transfers - #5833
Conversation
|
Hi @Ondra97, the overlap comment pairs this with #5831 (atomic Whichever lands second needs two adjustments:
I built I'm happy for this one to go first and will rebase #5831 onto it once it's merged. If #5831 lands first instead, the two points above are the whole resolution. AI-assisted. |
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
adriadam10
left a comment
There was a problem hiding this comment.
Thanks, the lock change in TransferIovecs looks right. One problem:
core/libs/tests/GuestFileVector.cpp:164: the newguest_file_vectoraborts on Linux (File vector check failed at line 164, 5 of 5 runs). The concurrentpreadvat offset 0 againstpwritevat offset 0 reads torn records: on Linux a buffered read is not atomic against an overlapping write, and the Linux path does not use the Windows lock. The whole-record check on overlappingpreadv/pwritevonly holds with the Windows lock, so it must not run on Linux (guard that phase with_WIN32, or make the readers not overlap the writers), while thewritev/readvphases can stay.
Please rerun guest_file_vector on Linux after the change.
9ea6db1 to
cbb6ddc
Compare
|
Thanks @adriadam10, fixed in cbb6ddc. I guarded only the overlapping concurrent I independently reproduced the Linux failure at line 164 in 5/5 runs before the change. After the fix:
Validated against main f633042. I updated the PR description with these results. AI-assisted: yes. — Codex |
adriadam10
left a comment
There was a problem hiding this comment.
Re-checked against cbb6ddc: the _WIN32 guard on the overlapping preadv/pwritev phase fixes the Linux abort; guest_file_vector passes here on Linux with the writev/readv phases still running. TransferIovecs holds the position lock and only calls the unlocked NativeTransferBuffer/NativePositionedTransfer, the scalar wrappers keep their lock, and the technical-debt entry matches. Merging once the Windows build is green.
What
On Windows,
TransferIovecsreleases the descriptor's shared position lock between iovec chunks. Concurrentwritevcalls through duplicated descriptors can interleave parts of one record;readvcan split a record across readers. On main2aad30682, the regression finds record label 1 mixed with label 256 at byte 64 of an actual file.Hold the existing position lock across the whole vector operation. Extract the native transfer bodies so the scalar wrappers retain their locking and the vector loop can call them while already holding the lock. This also covers
preadv/pwritevand preserves the shared file cursor. The locking reference is POSIX thread interactions with regular file operations; these are reference semantics, not console measurements.Update the existing technical-debt entry to describe serialization across a descriptor's aliases and the remaining limit: independent opens and other processes do not share this lock. Socket and random-device paths keep their existing transfer mechanisms.
The overlapping concurrent
preadv/pwritevphase inguest_file_vectorruns only on Windows, where the shared lock guarantees whole records. Concurrentwritev/readvand the existing sequential positioned-I/O checks continue to run on both platforms.Tested
I validated head
cbb6ddc885c07c8a3c6ff3542505e9c30ff6b51c, based on mainf633042dfdf0d51f4af58fafbade3fa9d47fd169, using real guest file APIs and file contents.Windows 11 Pro 10.0.26300, WinLibs MinGW-w64 GCC 15.2.0, Release, bundled dependencies. Hardware: AMD Ryzen 9 7900X, 32 GB RAM, NVIDIA GeForce RTX 4070 Ti SUPER.
guest_file_vector,guest_posix_scalar_io,guest_positioned_race,guest_kernel_fcntl,guest_filesystem,file_position,file_streams, andguest_shm_open.guest_file_vectorpassed ten consecutive runs, including the overlapping positioned phase. Four threads write and read 1,024 distinct 64-byte records through duplicated descriptors, each split across 32 iovecs. Every record remains whole and appears exactly once; positioned transfers leave the shared cursor unchanged.Ubuntu 24.04.5 LTS x86-64 in VirtualBox, GCC 13.3.0, Release, bundled dependencies. The VM has 8 vCPUs and 8 GB RAM on the same host.
guest_file_vectorpassed ten consecutive runs. The overlapping positioned phase is excluded on Linux.guest_file_vectorConventions and whitespace checks passed. The original Windows defect was reproduced on main
2aad30682withwritev interleaved a record at byte 64: 1 versus 256. No speed improvement is claimed. I did not run a title, the full Linux build or the full hosted CI suite.Integration with open PRs
The agreement with #5831 remains: #5833 lands first, then #5831 rebases onto it. Its append branch belongs in
NativeTransferBuffer, and the gatheredO_APPENDwrite must call that helper while the vector already holds the position lock. Calling the lockingNativeTransferwrapper there would lock the same mutex twice.An exact-head merge preview on main
d92a8dad390af1807cc91622b924252141816a5aalso reproduces the conflict with #5835e4036135e. In the WindowsTransferIovecspath, preserve the archive-read routing beforeFile::LockPositionedTransfer(d), then keep this PR's unlockedNativePositionedTransfer/NativeTransferBuffercalls under that lock. Dropping either side loses archive routing or whole-vector serialization.This #5835 composition was inspected statically; no combined archive/native runtime build or test was run. The archive feature and vector serialization are independent, and their merge order is not yet agreed.
Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)