pagecache: report page-writeback errors from sync() by return value - #1483
Open
gburd wants to merge 2 commits into
Open
pagecache: report page-writeback errors from sync() by return value#1483gburd wants to merge 2 commits into
gburd wants to merge 2 commits into
Conversation
pagecache::sync() threw a C++ error when a page writeback failed. Its only caller is the munmap / msync path (vfs_file::sync, invoked from mmu::file_vma::sync), which already converts the outcome to a return value and surfaces any error through its own sys_fsync() tail. No caller depended on the exception. Throwing from that path is both unnecessary and fragile: raising an exception during munmap can abort when the C++ unwinder cannot complete in the calling context, turning a recoverable writeback error into a fatal abort() that takes down the whole instance. Return the writeback errno from sync() instead; vfs_file::sync() consumes it and file_vma::sync() continues to report the error via sys_fsync(). Behavior is unchanged on success. On writeback error, sync() now returns the errno rather than throwing.
The base file::sync(off_t,off_t) threw make_error(ENOSYS). It is called from the munmap / msync page-writeback path (mmu::file_vma::sync -> _file->sync()), so unmapping a file-backed mapping whose file type does not override sync() raised a C++ exception during munmap(). That exception cannot be unwound from this path (it reaches __cxa_throw / _Unwind_RaiseException and bottoms out in abort()), taking down the instance. A file type that does not implement sync() has no cached writable pages to flush, so the correct default is to do nothing. vfs_file, the only type that caches writable file pages, overrides sync() with the real page-cache writeback (which reports errors by return value, per the previous commit). Make the default a no-op so munmap of any other file-backed mapping cannot raise an exception. Companion to the pagecache::sync() change: together they remove all C++ throws from the munmap page-writeback path.
Contributor
Author
|
Added a companion commit: the base |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pagecache::sync()threw a C++ error when a page writeback (cached_page_write::writeback()) failed. Its only caller is the munmap / msync path:vfs_file::sync(), invoked frommmu::file_vma::sync(), which already converts the outcome to a return value and surfaces any error through its ownsys_fsync()tail. Thefsync(2)syscall uses a separateVOP_FSYNCpath and never relied on this throw, so no caller depended on the exception.Throwing from the munmap path is both unnecessary and fragile: raising an exception during
munmap()can abort when the C++ unwinder cannot complete in the calling context, turning a recoverable page-writeback error into a fatalabort()that takes down the whole instance.Change
pagecache::sync()returnsint(0 on success, or the writeback errno) instead of throwing.vfs_file::sync()consumes the return value;mmu::file_vma::sync()continues to report the error viasys_fsync().include/osv/pagecache.hhis updated to match.Behavior is unchanged on success. On a writeback error,
sync()now returns the errno rather than throwing, and the page is left marked dirty for retry exactly as before.