pagecache/vfs: ZFS page-cache bridge, readahead, writeback, fsync flush - #1398
Conversation
|
Rebased onto current master (past the merged #1410). The two block-I/O hunks (fs/vfs/vfs_bdev.cc, fs/vfs/vfs_fops.cc) that previously showed up here were part of #1410 and are now dropped, so this PR is down to the three pagecache files: core/pagecache.cc, fs/vfs/vfs_syscalls.cc, include/osv/pagecache.hh. Build-verified green on current master. |
|
Friendly ping on this one. This PR is the base of the ZFS-enablement stack: the OpenZFS 2.4.3 cutover (replacing the c.2014 BSD-ZFS port) sits directly on top of it, along with #1400 and #1402. I'd like to open the OpenZFS PR as a clean, ZFS-only diff, which is only possible once these three prerequisites land on master. The branch is rebased on current master and mergeable. Happy to address anything outstanding, otherwise a merge here unblocks the next step. Thanks. |
|
@wkozaczuk if you have time I'd like you to look at this PR please, as you have much more recent ZFS experience than I do. Thanks. |
There was a problem hiding this comment.
Pull request overview
This PR updates OSv’s VFS page cache to better interoperate with the upcoming OpenZFS port by adding C-linkage entry points, introducing sequential readahead and periodic writeback, and addressing an fsync durability gap by flushing page-cache data before invoking filesystem fsync hooks.
Changes:
- Add new pagecache APIs for inode-based writeback, whole-cache writeback, and a periodic writeback daemon with a configurable interval.
- Implement a simple sequential readahead heuristic for read faults.
- Update
sys_fsync()to flush dirty page-cache data prior toVOP_FSYNC().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| include/osv/pagecache.hh | Adds declarations for new pagecache writeback APIs and a C-linkage wrapper for inode writeback. |
| fs/vfs/vfs_syscalls.cc | Flushes page-cache dirty data before calling VOP_FSYNC() to improve durability for mmap-backed writes. |
| core/pagecache.cc | Removes dead ARC-bridge code, adds sequential readahead, periodic writeback, inode/whole-cache writeback helpers, and new C-linkage helper functions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nyh
left a comment
nyh
left a comment
There was a problem hiding this comment.
I felt out of my league trying to review this patch. I left a few comments (one written by AI), I'll need to review this PR again, because unlike your other PRs, this one does change pre-existing code in ways that I don't really understand.
| // directly with the mmap layer. ZFS files now use the regular read_cache path, | ||
| // fed by the zfs_vop_cache() vnode hook which calls zfs_read() per page. | ||
| // | ||
| // No double-caching occurs: ARC caches compressed ZFS blocks; read_cache caches |
There was a problem hiding this comment.
Isn't this still "double caching" to have the same pages both compressed an uncompressed in memory?
I'm not sure if this comment is trying to describe the current situation, or the future situation if we move to a newer version of ZFS.
There was a problem hiding this comment.
It describes the current situation, not a hypothetical future one. After the OpenZFS 2.4.3 cutover a page mapped through the mmap fault path is held twice in DRAM: once in the ARC as the decompressed block, and once in read_cache as the 4KB page the fault handed out. That is real double-caching and the comment says so ("a real cost we accept for now").
The old BSD-ZFS port avoided it via arc_share_buf(), which let mmap alias the ARC buffer in place. OpenZFS 2.x made that function static-internal, so the in-place sharing hook is gone and we fall back to copying into read_cache. Eliminating the duplication again needs a supported in-place ARC-sharing API upstream in OpenZFS; until then the copy is the honest, working behavior. I reworded the block to make the "current, not future" framing explicit.
| // unmap_arc_buf(), and map_arc_buf() are kept as no-ops below because | ||
| // fs/zfs/zfs_initialize.c (linked into libsolaris.so) still references them. | ||
| // A future cleanup can remove both sides once the old BSD-ZFS compat layer is | ||
| // fully retired. |
There was a problem hiding this comment.
I'm curious what is the overall story behind replacing the ancient ZFS with a new ZFS.
When we originally chose to implement ZFS in OSv instead of some "simpler" filesystem, our theory was that users will love all the sophisticated modern features of ZFS. In the years that have passed, most "toy" users of OSv didn't care too much about the filesystem, and ZFS - even the ancient one - has been an overkill. So I am curious what you are planning to do with ZFS, and why a more modern version of ZFS is important.
There was a problem hiding this comment.
Fair question, and you are right that for a "toy" user any filesystem is fine. The motivation for this fork is not the classic ZFS feature list (snapshots/checksums/compression), though those come along for free. Three concrete things drove it:
-
Maintenance. The 2014-era BSD-ZFS port is frozen and diverged from any upstream. It cannot pick up a decade of bug fixes, and every OSv-side change (aarch64, O_DIRECT, page cache) has to be hand-carried against dead code. Tracking OpenZFS 2.x means fixes flow from a project that is still actively maintained instead of being reinvented here.
-
O_DIRECT / real I/O paths. The old port returns EINVAL on O_DIRECT (
zfs_vnops.c); OpenZFS 2.4 implements it. That matters for the workloads I am actually running on OSv - Postgres and other DB engines that want direct I/O and predictable cache behavior rather than double-buffering through the page cache. -
Distributed block backends. The fork pairs ZFS with NVMe-oF/TCP and a Crucible (Oxide) distributed-block driver, running RAID-Z across several network targets. That has been verified byte-clean against live nvmet-tcp and TrueNAS. This is the use case where ZFS earns its keep on OSv: end-to-end integrity over unreliable remote storage, which a "simpler" filesystem does not give you.
So the plan is not "ZFS for its own sake" - it is a maintained, modern storage stack under DB-style workloads on top of distributed block devices. Happy to split that rationale into the eventual ZFS-cutover PR description where it belongs; this page-cache PR just needs to not regress the existing path.
| #include <osv/sched.hh> | ||
| #include <osv/clock.hh> | ||
|
|
||
| // NOTE: The ARC bridge (IS_ZFS() == true path) is intentionally unreachable |
There was a problem hiding this comment.
In the commit message you said: "Remove the original ARC-bridge code path that tried to share pages between the ZFS ARC and the OSv read_cache. It was never reachable: IS_ZFS() always returned false on OSv (m_fsid distinct), and the bridge data structures were only initialised on the unreachable branch."
I had no idea if that is actually true, so I asked my AI friend. Its response was:
The developer's claim is not supported by the code. Here's what the code actually shows:
IS_ZFS() does return true for ZFS files. The encoding chain is consistent:
- zfs_vfsops.c (which includes vfs_id.h) sets:
vfsp->vfs_fsid.__val[1] = (ZFS_ID | fsid_guid) >> 32; // = 0x06000000 | (fsid_guid >> 32) - zfs_vnops.c reconstructs:
vap->va_fsid = ((uint32_t)fsid->__val[0]) | ((dev_t)((uint32_t)fsid->__val[1]) << 32);. This places 0x06 in bits 56–63 of va_fsid (= st_dev). - IS_ZFS check (st_dev & 0xff00000000000000ULL) == ZFS_ID = (6ULL<<56) — which matches exactly. The check fires correctly for ZFS.
The ARC-bridge code IS reachable. The ZFS vnops table at zfs_vnops.c:5095 has zfs_arc in the vop_cache slot (position 19 in struct vnops). The call chain is:
pagecache::get() [IS_ZFS → true]
→ create_read_cached_page()
→ fp->read_page_from_cache()
→ VOP_CACHE() → zfs_arc()
→ dmu_map_uio()
→ mmu_map() [bsd/porting/mmu.cc]
→ pagecache::map_arc_buf()
→ (*arc_share_buf_fun)(ab)
The bridge data structures are unconditionally initialized — arc_read_cache and cached_page_arc::arc_cache_map are static globals, and the function pointers are wired up via register_pagecache_arc_funs() called from libsolaris.so at load time.
There is a puzzling comment in tst-shm-consistency.cc:368 that says (zfs_vop_cache=NULL), but that contradicts the actual zfs_vnops table — that comment is incorrect.
The "m_fsid distinct" argument doesn't hold either: ROFS uses ROFS_ID = (7ULL<<56) and ZFS uses ZFS_ID = (6ULL<<56), they're distinct by design, and IS_ZFS correctly discriminates between them.
There was a problem hiding this comment.
Your AI is right and my commit message was wrong. I traced it again against the old BSD-ZFS code and the bridge was reachable and live:
- zfs_vfsops.c set vfs_fsid.__val[1] = (ZFS_ID | fsid_guid) >> 32
- zfs_vnops.c reconstructed va_fsid with 0x06 in the top byte
- so IS_ZFS(st_dev) did return true, and the chain pagecache::get() -> create_read_cached_page() -> read_page_from_cache() -> VOP_CACHE() -> zfs_arc() -> ... -> map_arc_buf() -> arc_share_buf() was wired up, with register_pagecache_arc_funs() called unconditionally from libsolaris at load. My 'IS_ZFS always false / m_fsid distinct' justification was simply incorrect, and the tst-shm-consistency.cc:368 comment I leaned on is also wrong.
The correct reason removal is safe is on the OpenZFS side, not the old code: the OpenZFS 2.4.3 port intentionally does NOT tag the fsid with ZFS_ID (module/os/osv/zfs/zfs_vfsops.c), because arc_share_buf() became a static-internal function in OpenZFS 2.x and is no longer callable from the bridge. With ZFS_ID cleared, IS_ZFS() returns false and ZFS files take the same read_cache path as ROFS, fed by the zfs_vop_cache() hook. So the bridge is dead after the cutover because the port clears ZFS_ID, not because IS_ZFS was always false on the old code -- the causality is inverted from what I wrote.
I've reworded the in-code comment (near register_pagecache_arc_funs in cb4757c) to state this accurately, including the honest note that a page served this way is now held twice in DRAM (once in the ARC as the decompressed block, once in read_cache). I'll fix the commit message to match before this merges. Thanks for catching it -- and thank your AI friend.
| extern "C" void osv_free_page(void *p) | ||
| { | ||
| memory::free_page(p); | ||
| } |
There was a problem hiding this comment.
We have a bunch of these extern "C" osv_*() functions already in core/osv_c_wrappers.cc. Maybe it's a good idea to put those there too? I think they aren't too related to core/pagecache.cc.
There was a problem hiding this comment.
I looked at moving them to core/osv_c_wrappers.cc. The difference is coupling: the wrappers already in osv_c_wrappers.cc are thin adapters over public APIs (sched, app, run, version) -- they need no file-private state from another TU. These pagecache wrappers are the opposite: osv_pagecache_map_page/read_page/map_page_if_absent all operate directly on pagecache.cc's file-static internals -- read_cache, read_lock, find_in_cache(), and the cached_page type. Moving them to osv_c_wrappers.cc would force exposing all of that internal state through a header just so another TU can reach it, which is worse encapsulation than keeping the C entry points next to the data they manipulate. So I'd prefer to keep them in pagecache.cc. If you feel strongly the other way I'll move them, but it would mean promoting several currently-private symbols to a shared header.
|
I have applied this PR to the latest master and was able to build a simple hello world example with a ZFS image: However, OSv hangs just before trying to run an app (possibly trying to finish initializing ZFS filesystem): I am speculating that your changes rely on the new OpenZFS version of I have tried to review the code, and I think in general it all makes sense, but I am not able to validate it completely. So here is an idea: would it be possible to tweak your changes to Specifically, maybe we can keep the existing ARC-related functions that old BSD ZFS uses, add new ones we need for OpenZFS, and tweak |
|
Yes, exactly right - and thank you for taking the time to build and boot it. Your diagnosis is correct: this PR as written assumes the OpenZFS The mechanism: on current master I agree completely with keeping both paths working. I'd like to propose a slightly lighter wiring than the global
Net effect: old BSD-ZFS keeps its ARC-sharing fast path unchanged, OpenZFS uses the read_cache path, and we need no global boolean or new init callback because If you'd rather have an explicit |
|
I like your proposal, and I agree we probably do not need this extra global flag to differentiate between old and new OpenZFS. I assume you will have an equivalent of the I do think you will have to differentiate between BSD and OpenZFS in some places of Finally, I imagine at some point you will send another PR to add a new OpenZFS |
|
BTW, at some point it would be nice to tackle #1201 so the old BSD becomes a module under |
|
Pushed 11d4f46 restoring the Verified both stacks boot and run on current tree:
Answering your specific questions:
Re #1201 (old BSD-ZFS -> |
wkozaczuk
left a comment
wkozaczuk
left a comment
There was a problem hiding this comment.
Hi,
I like this PR. I have reviewed it and left mostly questions to make sure I understand some of your changes that may affect BSD ZFS and ROFS more deeply. I have made some minor suggestions for improvement.
I think we are close to get it approved.
PS. I have also run your PR with both ZFS and ROFS image and everything passes.
Waldek
| { | ||
| struct stat st; | ||
| if (sys_fstat(fp, &st) == 0) { | ||
| error = osv_pagecache_writeback_inode(st.st_dev, st.st_ino, |
There was a problem hiding this comment.
Nitpick: why do not we call pagecache::writeback_inode() directly?
There was a problem hiding this comment.
Agreed -- sys_fsync() is C++ so there is no reason to bounce through the extern "C" osv_pagecache_writeback_inode() wrapper. The wrapper exists for C callers; the direct call is cleaner here. Will switch it to pagecache::writeback_inode().
There was a problem hiding this comment.
Done in a1b277e -- sys_fsync() now calls pagecache::writeback_inode() directly.
|
Pushed two follow-up commits addressing this round of review:
Replied inline to the rest: the |
|
@nyh do you see anything else that should prevent us from merging this PR? @gburd thanks for your responses. That has clarified a lot (and helped find and fix one issue). Shall we squash this PR eventually before we merge it? As I was writing it, I ran tests with ROFS, and I got this crash: It does not happen on ZFS image. As a matter of fact, all unit tests pass on the old BSD ZFS image. Maybe the |
|
Thank you for catching this, and for running the ROFS suite - that was a real bug. Fixed in b4e3482. Root cause: The fix makes I added Yes, let's squash before merge - happy to do that whenever you and @nyh are ready to take it. I don't see anything else outstanding on my side. |
|
Rebased onto current master and squashed the review-fix commits into a single commit (per the request above), head now Re-tested on current master, both filesystems:
The Nothing outstanding on my side - ready to merge whenever you and @nyh are. |
|
Hi, I have used one of the apps to test - It runs on a ROFS image, and I have discovered that when I revert this function: it goes away. I wonder if truly |
|
The |
…, fsync flush Extend the page cache so that memory mappings of files backed by OpenZFS share pages with the ARC (rather than double-buffering), add sequential readahead for the read-cache path, run periodic write-back of dirty mmap pages, and flush the page cache before VOP_FSYNC so fsync/fdatasync see mmap writes. Page-cache changes (core/pagecache.cc, include/osv/pagecache.hh): - ZFS ARC bridge: for ZFS-backed files, map pages in place from the ARC via the IS_ZFS()-guarded arc_read_cache path, sharing the ARC buffer instead of copying it into a private read-cache page. The old BSD-ZFS ARC dual path is kept live so nothing regresses until the OpenZFS libsolaris module lands (see issue cloudius-systems#1201). - Sequential readahead: readahead_if_sequential() speculatively populates the next few pages when it sees a sequential access pattern. It is gated to the read-cache path (!IS_ZFS), so ROFS and OpenZFS benefit while old BSD-ZFS, which shares ARC pages in place, does not take it. - Write-back: cached_page_write::writeback() clears the software dirty flag only after VOP_WRITE succeeds (a failed write stays dirty and is retried), and a periodic writeback worker flushes dirty mmap pages so they reach the filesystem without an explicit msync/fsync. - fsync/sync/writeback_inode use a two-phase scan: phase 1 promotes the hardware PTE dirty bit into the software dirty flag with clear_dirty() (mapping- preserving), so an empty to_flush set provably means no PTE changed and the TLB flush can be skipped; phase 2 collects and writes back. The phases stay distinct so a multiply-mapped page has every alias promoted before any is collected. VFS (fs/vfs/vfs_syscalls.cc): flush the page cache before VOP_FSYNC in sys_fsync so data written through a shared mmap is persisted by fsync(2)/ fdatasync(2). sys_fsync calls pagecache::writeback_inode() directly (it is C++), while the extern "C" wrapper stays for C callers. A COW write fault on a page that readahead loaded into the read cache but that was never faulted for read previously hit assert(0) in ptep_remove (the page has no PTE mappings recorded); it now reports zero remaining mappings so the page is dropped and the COW copy proceeds. Regression test tests/tst-mmap-file-cow.cc covers the readahead + MAP_PRIVATE COW path on ROFS.
|
Excellent diagnosis - thank you, that was exactly right, and it was a real bug I introduced. Root cause:
My collision handler called Fix (rebased into the PR, head now extern "C" void osv_pagecache_map_page(void *key, void *page)
{
if (!map_read_cached_page(static_cast<hashkey*>(key), page)) {
memory::free_page(page); // ZFS allocated it; free on collision
}
}I also rebased the PR onto current master (it had gone I strengthened |
|
Thanks for catching this and doing the bisection - you are exactly right about the cause, and the good news is the fix is already in the merged master. The version you tested ( The final merged code fixes this by not freeing the page inside // core/pagecache.cc (as merged)
bool map_read_cached_page(hashkey *key, void *page)
{
SCOPE_LOCK(read_lock);
cached_page* pc = new cached_page(*key, page);
auto res = read_cache.emplace(*key, pc);
if (!res.second) {
// Key already present; emplace() did not take ownership of the wrapper,
// so free the wrapper (always OSv-owned). Leave @page to the caller.
delete pc;
return false;
}
return true;
}
As for why the same key maps repeatedly: yes, concurrent Could you retest |
|
Yes. It all works fine. I have merged your PR
…On Sun, Jul 12, 2026 at 20:48 Greg Burd ***@***.***> wrote:
*gburd* left a comment (cloudius-systems/osv#1398)
<#1398 (comment)>
Thanks for catching this and doing the bisection - you are exactly right
about the cause, and the good news is the fix is already in the merged
master.
The version you tested (OSv v0.57.0-360-g97701dcb) is the commit right
*before* the pagecache merge (244554b), so it still had the older
revision of map_read_cached_page() that called memory::free_page(page) on
a dedup collision. As you found, that is wrong for ROFS:
rofs_map_cached_page() passes a *borrowed* page from the ROFS read-around
cache, so freeing it corrupts that cache and produces the GP fault.
The final merged code fixes this by not freeing the page inside
map_read_cached_page() at all - it just returns a bool and lets each
caller decide, because the two callers have opposite ownership contracts:
// core/pagecache.cc (as merged)bool map_read_cached_page(hashkey *key, void *page)
{
SCOPE_LOCK(read_lock);
cached_page* pc = new cached_page(*key, page);
auto res = read_cache.emplace(*key, pc);
if (!res.second) {
// Key already present; emplace() did not take ownership of the wrapper,
// so free the wrapper (always OSv-owned). Leave @page to the caller.
delete pc;
return false;
}
return true;
}
- *ROFS* (rofs_map_cached_page) passes a borrowed read-around page and
ignores the return value -> never freed. (matches your working workaround)
- *ZFS* (osv_pagecache_map_page) allocated the page with
osv_alloc_page(), so it frees on a false return to avoid a leak.
As for why the same key maps repeatedly: yes, concurrent VOP_CACHE calls
for the same offset race to insert, and only the first wins the emplace().
Could you retest apps/graalvm-httpserver against current master (past
244554b)? I believe the crash is resolved there. If you still see it on
merged master, I will dig in further.
—
Reply to this email directly, view it on GitHub
<#1398?email_source=notifications&email_token=ABINEIMWSWNF2UEHRQJVI635EQWVPA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJVGM2DQNRQGEYKM4TFMFZW63VMON2GC5DFL5RWQYLOM5S2KZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-4953486010>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABINEIO6HRIPVCPCM5ZLNM35EQWVPAVCNFSNUABDKJSXA33TNF2G64TZHM3TENJYGY4DKO2JONZXKZJ3GQ3TGNBTGEYDMMJZUF3AE>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
Make the OSv VFS page cache cooperate with the OpenZFS port landing later in this series, and fix a data-durability bug in
sys_fsync.This branch bases directly on current
master(3df7df7, the just-merged mmu-shm work). Two commits.Changes
pagecache: ZFS bridging, sequential readahead, periodic writeback (54430d8).
osv_pagecache_map_page, etc.) so the OpenZFSvop_cacheinzfs_vnops_os.ccan register/look up cached pages without pulling C++ pagecache headers into module sources. Also fixes a GCC 14 ambiguity on a templated helper.IS_ZFS()always returned false on OSv and the bridge structures were only initialised on the dead branch. A comment documents the decision so it isn't re-attempted.vfs: flush page cache before VOP_FSYNC in sys_fsync (2ef4642).
sys_fsync()calledVOP_FSYNCwithout first flushing the OSv page cache, so dirty cached pages were never seen by the filesystem's fsync hook -- a process couldfsync()and still lose data on crash (reproducer: write 64 KiB, fsync, kill VM, restart, read zeros). Walk the file's dirty pages, write them back, thenVOP_FSYNC, holdingf_lockacross the flush so concurrent writes can't slip in.Verification
Kernel compiles and links clean on GCC 14.3 / Boost 1.87 (
./scripts/build image=empty, fresh loader.elf, RC=0). The page-cache helpers are consumed by the later OpenZFS PR; this PR adds only the kernel-side surface and the fsync fix.