Repository navigation
fix(libkernel): append O_APPEND writes atomically on Windows - #5831
Conversation
|
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.
|
|
Windows cross-check of head Windows 11 Pro 26220, Ryzen 7 5700X, RTX 4070 (driver 596.21), WinLibs MinGW-w64 GCC 15.2.0 r7, Ninja, Release.
Titles, 60 s each, relinked once with
There is no stop line in any run. FPS varied with other load on the machine. One observation outside this PR's path: PS5MC's Verdict: confirmed on Windows. Concurrent appends through separate descriptors no longer lose data, and there are no test or title regressions. AI-assisted (Claude Code). |
|
Thanks @screamm for the cross-check, and for the negative control with main's libraries. On the overlap with #6228 flagged above: it's textual only. Both PRs change neighbouring lines in On the libc streams: agreed, that's a separate path. |
jotitadev
left a comment
There was a problem hiding this comment.
Linux run (Ubuntu 26.04, Intel i7-8700, GCC 15.2, full build, non-GPU tests), head 17d69f14a merged onto main ce991f1ec. The description says Linux was not tested.
- Builds;
guest_append_writes, which is registered on every platform, passes 30 of 30 repeated runs.guest_file_vector,guest_kernel_fcntl,guest_filesystemandguest_file_timestampspass too. - With the five libkernel files from
main(the PR's test kept),guest_append_writesalso passes 30 of 30. That matches the description: the hostwrite/writevon Linux already append atomically, so the new test is safe on the Linux CI and only guards the Windows path.
Non-GPU suite (ctest -E "^agc"): same result as main; only file_writer_binary and file_writer_text fail, and they fail on main too on this machine (Ubuntu 26.04 coreutils).
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
On Windows the C runtime appends by seeking to the end and then writing, so concurrent appends through different descriptors overwrite each other and lose data. Write and writev on an O_APPEND descriptor now go through WriteFile at the end-of-file offset, which Windows appends atomically, and writev gathers its buffers into one append as FreeBSD writes the whole vector at the end.
17d69f1 to
e8c97bf
Compare
|
Rebased onto |
What
On Windows,
NativeOpenhands the file to the C runtime with_O_APPEND. The runtime appends by seeking to the end and then writing, two separate steps. When two descriptors append to the same file at once, both seek to the same end and the second write overwrites the first. FreeBSD'svn_writesetsIO_APPENDfor anO_APPENDfile and writes at the end under the vnode lock, so appends never collide there.A standalone Windows program shows the size of the loss: 4 threads each append 2,000 lines through their own descriptor. With the C runtime's
_O_APPEND, 6,999 of the 8,000 lines arrive. WithWriteFileat offset0xFFFFFFFFFFFFFFFF, all 8,000 arrive. That offset is Windows' atomic write to end of file, "functionally equivalent" toFILE_APPEND_DATA, and it leaves the file pointer after the data, as FreeBSD leaves the offset at the end.The change:
sceKernelOpenalready records the guest open flags withFile::RememberDescriptor, and those flags are shared acrossdup,dup2andF_DUPFD. The newFile::Appends(fd)reads itsO_APPENDbit.NativeWrite(sceKernelWrite,write,_write) andNativeTransfer(writev,sceKernelWritev) send writes on such a descriptor through the newFile::WriteAtEnd. It callsWriteFileat the end-of-file offset, only for disk files. Pipes and devices keep the existing path.writevon anO_APPENDdescriptor gathers its buffers into one write, because FreeBSD appends the whole vector at once. Until now, each buffer was written separately, so other writers could land between them. This is also in the Windows code: Linux calls the host'swritev, which already appends the whole vector at once.pwriteis unchanged.guest_append_writescovers four concurrent writers on one file:sceKernelWriteon its own descriptor;writeon another descriptor;sceKernelWriteon adupof the first descriptor;writev.Each writes 1,500 fixed-length lines. The test checks that the file holds exactly 6,000 intact lines, each exactly once. It then checks that a write after
lseek(0, SEEK_SET)lands at the end and leaves the offset after it. Onmainit fails at the size check in 10 of 10 runs; with the change it passes.Overlap with #5833
#5833 holds the descriptor's position lock across a whole vectored transfer and splits
NativeTransferinto a lock-freeNativeTransferBufferand a locking wrapper. The two changes don't overlap in purpose: #5833 keeps the aliases of one descriptor from interleaving, and this PR makesO_APPENDwrites atomic across independent opens. Agreed with @Ondra97 on #5833: #5833 lands first, and this PR is then rebased onto it with two adjustments:File::Appendsbranch moves intoNativeTransferBuffer;writevgathering callsNativeTransferBuffer, because with fix(libkernel): serialize Windows vectored file transfers #5833TransferIovecsalready holds the lock, and callingNativeTransferthere would lock the samestd::mutextwice.With
main84b01f9, #5833 and this PR resolved that way,guest_append_writes,guest_file_vector(with #5833's concurrent records) andguest_kernel_fcntlpass 5 of 5 runs.Tested
Windows 11 Pro (26200), WinLibs GCC 15.2.0 r7, Release,
BUILD_TESTING=ON:main9a3e2ac, the full build and fullctestpass: 1172 tests, 10 of them skipped by their own environment checks.libkernel.prxsucceeds.main8ffbb49, whose new commits don't touch these files,guest_append_writes,guest_kernel_fcntl,guest_file_vectorandguest_system_configurationpass (3 runs).Java 25 LAN / DNS (PPSA99869, #5501) was run with #5563, #5643 and #5666 stacked, which take it past its remaining missing imports (#5502 and #5187 are merged), and relinked with
--trap-sdk-syscall-stubs. It keeps a trace in/download0/java25-vm-trace.log, which its threads append to through separateO_APPENDdescriptors. During the DNS stage (two threads), without this change 86 lines lose their start and 99 lines lose their newline and run into the next. With it, no line is cut and no newline is lost. The 174 lines that still join are each followed by an empty line, because the title writes a line and its newline separately.Linux not tested; nothing changes there.
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)