libext: real fsync + page-cache bridge for ext2/3/4

[gburd]

What

Three fixes to the ext (lwext4) filesystem module toward the Postgres-over-ext
goal.

1. fsync durability. ext mounts with lwext4 block-cache write-back enabled,
   so a write only reaches the disk when the block cache is flushed -- but
   vop_fsync was vop_nullop, so fsync(2)/fdatasync(2) on an ext file
   persisted nothing. ext_fsync() now flushes the device's block cache (like
   ext_sync does at unmount), making written data durable.

2. Page-cache bridge (vop_cache). The vop_cache slot was null, so mmap
   faults on ext files hit the block layer on every fault, unlike ROFS and ZFS
   which populate the shared page cache. ext_map_cached_page() reads one
   page-aligned page into a freshly allocated page and hands it to
   pagecache::map_read_cached_page(), warming the read cache so subsequent
   faults and readahead are served from it. Allocate-and-copy for now (lwext4's
   block-cache buffers are not page-aligned/shareable the way ROFS's read-around
   cache is); a zero-copy borrow-and-pin bridge like the ZFS ARC one can follow.

3. Inode deletion time. The inode-delete path freed the inode from the
   bitmap but never set its on-disk dtime, so after OSv deleted a file, Linux
   e2fsck flagged "deleted inode has zero dtime". ext_mark_inode_deleted()
   now sets it before each ext4_fs_free_inode() (lwext4's own
   ext4_inode_set_del_time is not exported from liblwext4.so, so the field is
   set directly with the byte-order-invariant 0xffffffff).

Because the module builds with -fno-rtti and <osv/pagecache.hh> pulls in
<osv/trace.hh> (typeid), the two page-cache symbols are declared minimally
instead of including the heavy header (the uio carries the hashkey opaquely).

Testing

tests/tst-ext4-rw.cc: mmap a pre-populated ext4 file and verify the pattern
survives the vop_cache bridge (first fault + cached re-read), then write a
file, fsync it, and read it back (plus fdatasync and a fresh re-open).
Verified on OSv under KVM with an ext4 second disk (mkfs.ext4 -b 4096 -O ^64bit,^metadata_csum, which lwext4 supports). After the run -- which
creates, fsyncs, and deletes a file -- Linux e2fsck -n -f on the disk exits 0
(clean), confirming both the fsync durability and the dtime fix.

[wkozaczuk]

I have a concern about potential memory leak

[Copilot]

Pull request overview

This PR improves OSv’s libext (lwext4-based) filesystem integration to support Postgres-over-ext goals by making fsync(2) durable, bridging ext-backed mmap() reads into the shared page cache via vop_cache, and fixing ext inode deletion metadata so Linux e2fsck doesn’t complain after deletes.

Changes:

• Implement ext_fsync() to flush lwext4’s device block cache so fsync()/fdatasync() are no longer no-ops.
• Add a vop_cache implementation (ext_map_cached_page()) that reads one page into an allocated page and maps it into OSv’s read page cache.
• Set a non-zero inode deletion_time before freeing inodes to satisfy Linux e2fsck, and add a new ext4 read/write regression test.

Reviewed changes

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

File	Description
tests/tst-ext4-rw.cc	Adds a regression test for ext vop_cache mmap-read caching and fsync durability.
modules/tests/Makefile	Includes the new tst-ext4-rw.so in the built test set.
modules/libext/ext_vnops.cc	Implements ext_fsync, adds vop_cache page-cache warming, and fixes inode deletion time before freeing.
.gitignore	Ignores local ext_images/ used for ext4 disk images.

Comments suppressed due to low confidence (2)

modules/libext/ext_vnops.cc:1526

• ext_map_cached_page() always allocates a fresh page and unconditionally passes it to pagecache::map_read_cached_page(). If another thread already inserted the same key, map_read_cached_page() returns false and the newly allocated page remains owned by this caller; currently it is leaked.

    pagecache::map_read_cached_page((pagecache::hashkey *)uio->uio_iov->iov_base,
                                    page);
    uio->uio_resid = 0;

tests/tst-ext4-rw.cc:61

• If /data is not mounted read-write (or doesn't exist), creating written.dat will assert-fail. After adding SKIP handling above, it would be more consistent to SKIP here too so the test doesn't fail due to environment rather than a regression in libext.

    fd = open(out.c_str(), O_CREAT | O_TRUNC | O_RDWR, 0644);
    assert(fd >= 0);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gburd]

Good catch, you're exactly right -- it was a page leak, and I traced why the return went unchecked: this file forward-declares map_read_cached_page locally (to avoid the heavy pagecache headers) and I'd declared it as returning void, which masked the real bool return that signals "another thread already cached this key; ownership was NOT taken."

Fixed (folded into the page-cache-bridge commit):

1. Corrected the local prototype to bool map_read_cached_page(hashkey*, void*).
2. Handle the false return by freeing our now-redundant page -- mirroring the ZFS caller in core/pagecache.cc (if (!map_read_cached_page(...)) { memory::free_page(page); }), which handles this exact concurrent-same-key race.

    if (!pagecache::map_read_cached_page(
            (pagecache::hashkey *)uio->uio_iov->iov_base, page)) {
        // Another thread cached the same key first; map_read_cached_page did
        // not take ownership, so free our now-redundant copy to avoid a leak
        // (mirrors the ZFS caller in core/pagecache.cc).
        memory::free_page(page);
    }

Also removed the stray ponytail: marker comment (it was a leftover note-to-self; reworded to a plain NOTE explaining the allocate-and-copy choice vs a future zero-copy borrow-and-pin). Pushed.

[gburd]

Follow-up on the round of comments from the last push, now in the current tip (rebased onto master):

• Page leak on the map_read_cached_page race (wkozaczuk): fixed as described earlier. The local forward declaration is now bool (it had been void, which masked the real return), and the caller frees the freshly allocated page when insertion returns false (another thread cached the same key first). The ownership comment above the call already spells this out.
• "What is ponytail?" (wkozaczuk): that word is gone from the file.
• Copilot: map_read_cached_page ownership comment: the comment now states ownership transfers only on success and stays with the caller (returns false) on a concurrent-insert race, matching the pagecache contract and the code.
• Copilot: tst-ext4-rw hard-asserts /data/readme.dat in the default suite: right, this test needs an ext4 second disk mounted at /data and would abort a default run without it. It now treats a missing /data/readme.dat as a SKIP (prints a message and returns 0) instead of asserting.

Needs a build to confirm the test compiles and skips cleanly without the ext4 disk, and passes with it; queued.

[gburd]

Build-validated on x86_64: image=tests builds. Both paths of the test change confirmed: with no /data disk, tst-ext4-rw SKIPs cleanly (exit 0, no abort, so the default suite no longer fails without an ext4 disk); with a real ext4 disk mounted at /data, tst-ext4-rw passes (mmap/page-cache bridge + fsync assertions).

[wkozaczuk]

I left a comment about a leak.