First a minor note that the tmpfs part fails to plug the routine, so I added it with the following:

diff --git a/sys/fs/tmpfs/tmpfs_vnops.c b/sys/fs/tmpfs/tmpfs_vnops.c
index 7dc0e1b4a870..ac158167fd12 100644
--- a/sys/fs/tmpfs/tmpfs_vnops.c
+++ b/sys/fs/tmpfs/tmpfs_vnops.c
@@ -1766,6 +1766,7 @@ struct vop_vector tmpfs_vnodeop_entries = {
        .vop_stat =                     tmpfs_stat,
        .vop_getattr =                  tmpfs_getattr,
        .vop_setattr =                  tmpfs_setattr,
+       .vop_read_pgcache =             tmpfs_read_pgcache,
        .vop_read =                     tmpfs_read,
        .vop_write =                    tmpfs_write,
        .vop_fsync =                    tmpfs_fsync,

The important part though is that the patch causes a slodown both single and multi-threaded. In vm_object ref_count and paging_in_progress are close enough to each other, and given vm_object alignment they randomly share a cacheline or not. The newly added routine updates them back to back, once with a cmpxchg loop. Shared case causes excessive ping pongs. However, even with this problem addresses I don't think this is a satisfying solution (see below for comparison against Linux).

When doing a 24-way read from the same vnode and profiling with dtrace -w -n 'profile:::profile-997 { @[arg0] = count(); } tick-5s { system("clear"); trunc(@, 40); printa("%40a %@16d\n", @); clear(@); }':

   kernel`vm_object_pip_add+0x13             8039
kernel`vm_object_pip_wakeup+0x11             8892
kernel`vm_object_deallocate+0x58            14808
     kernel`vm_page_sunbusy+0x15            14896
    kernel`vm_page_trysbusy+0x2d            15161
  kernel`tmpfs_read_pgcache+0x96            22107
  kernel`tmpfs_read_pgcache+0xb6            26864

I tested this on a 24 core vm on a Cascade Lake box provided by cperciva. It can be powered on on demand.

Linux was 5.9.0-rc4, FreeBSD is r365424 + the patch with debug.vn_io_pgcache_read_enable toggled (below as pgread)

Tests are (will-it-scale):
read2 - Same file read
pread3 - Same file pread to different offsets

-t denotes processes

I don't know if I managed to whack all differences for a fair single-threaded test, so differences should be ignored. It is provided to get an idea of a scaling factor.

If you want you can get access to perform your own tests.

I'm not familiar with this code, I suspect this can be made to scale to roughly Linux level (or higher) with the following observations:

• vm object is type stable
• there is a routine to perform lockless lookup
• pages can be busied and validated afterwards. should it turned out the page is reassigned it can fallback to regular read
• vm object teardown can be made to wait for all busy pages to drain

This would reduce work single-threaded and reduce contention points multithreaded (most notably make pread3 scale completely).

In fact tmpfs_read_pgcache can become tmpfs_read.

As a useful experiment, you could comment out the call to vm_page_activate() in uiomove_object_page().

This still performs worse than the stock kernel:

     kernel`vm_page_sunbusy+0x15              979
kernel`vm_object_deallocate+0x2d             1120
  kernel`copyout_smap_erms+0x186             1975
  kernel`tmpfs_read_pgcache+0x60             3148
    kernel`vm_page_trysbusy+0x49             4959
    kernel`vm_page_trysbusy+0x2d            12040
  kernel`tmpfs_read_pgcache+0x83            19499
kernel`vm_object_deallocate+0x27            21954
kernel`vm_object_deallocate+0x58            22291
  kernel`tmpfs_read_pgcache+0xa1            25232

trysbusy reads from the object and presumbaly ping pongs here against refing/unrefing.

Just for testing purposes I removed the ref_count manipulation. It bring scalability to the same ballpark as Linux.

Is there a fundamental problem not taking the ref here? vm obj reuse could be postponed until after the vnode is freed if that's a concern.

I can't test on the original box, the patched routine is how it looked like after my own removal of vm obj refcounting so I presume performance is the same.

I disagree with special casing of the sort though. I don't know the lifecycle for vm object, so perhaps there is a lot of badness there.

The vnode is guaranteed to have usecount > 0 and that should be sufficient to provide whatever guarantees are needed in face of forced unmount (modulo smr for ->v_data). Making sure this works will likely clean up hacks elsewhere. Is there are fundamental problem assuring this?

General note is that the current forced unmount behavior is rather cumbersome and perhaps we would way better served with something cooperating with the filesystem. Most notably it can leave ->v_data in place, making it an invariant that the pointer is *never* NULL as long as you at least hold the vnode.

In D26346#586024, @kib wrote:

I tried to do pass over mis-use of f_data for FTYPE_VNODE, probably missed some.

zfs_file_fsync() perhaps, but I'm not certain.

In D26346#586405, @markj wrote:

In D26346#586024, @kib wrote:

I tried to do pass over mis-use of f_data for FTYPE_VNODE, probably missed some.

zfs_file_fsync() perhaps, but I'm not certain.

I will write a coccinelle script to find all instances before I pass this to pho.

So again is there a problem keeping vm object in a good enough shape as long as v_usecount > 1? This would avoid adding more work on tmpfs open/close and would most likely facilitate easier support for other filesystems, at the expense of some surgery to vgone.

In D26346#587083, @mjg wrote:

So again is there a problem keeping vm object in a good enough shape as long as v_usecount > 1? This would avoid adding more work on tmpfs open/close and would most likely facilitate easier support for other filesystems, at the expense of some surgery to vgone.

The liveness of the vm_object for tmpfs VREG node is equal to the liveness of the node itself. This is a fundamental difference between tmpfs v_object vs. typical filesystem with permanent storage, where v_object is used for caching and its lifetime strictly subset of the vnode (not inode) lifetime.

It is not vgone() that must be patched but freevnode(), and it must become aware of filesystem-specific details for vnodes that are already reclaimed. This is violation of the VFS architecture, and I do not see it worth the effect. Patch currently adds single atomic to open and close on tmpfs.

[Later add: and doing it in last vdrop/freevnode means user-uncontrolled memory leak. Right now node data is kept as far as there is mmap (or with patch, an opened file). ]

In D26346#587114, @kib wrote:

In D26346#587083, @mjg wrote:

So again is there a problem keeping vm object in a good enough shape as long as v_usecount > 1? This would avoid adding more work on tmpfs open/close and would most likely facilitate easier support for other filesystems, at the expense of some surgery to vgone.

The liveness of the vm_object for tmpfs VREG node is equal to the liveness of the node itself. This is a fundamental difference between tmpfs v_object vs. typical filesystem with permanent storage, where v_object is used for caching and its lifetime strictly subset of the vnode (not inode) lifetime.

It is not vgone() that must be patched but freevnode(), and it must become aware of filesystem-specific details for vnodes that are already reclaimed. This is violation of the VFS architecture, and I do not see it worth the effect. Patch currently adds single atomic to open and close on tmpfs.

[Later add: and doing it in last vdrop/freevnode means user-uncontrolled memory leak. Right now node data is kept as far as there is mmap (or with patch, an opened file). ]

So a general point here is that the current behavior vs vgone is quite problematic, even ignoring this particular use case. And the fix would be integrating vnode recycling with filesystems, making the current change spurious in my opinion. As a step towards that, we could just make vm obj for tmpfs behave the same manner as it does for other cases.

Another example of the problem is non-lockmgr locks for vnodes. If it ever worked, it was in part because vget was a 2 step process starting with setting a flag that the vnode is needed, waiting for any in-flight stuff to settle and only then locking. The flag stuff was removed years ago and now you can get threads stuck in a custom locking primitive while the vnode gets doomed and gets op vector replaced which something only using lockmgr. This leads to all kinds of woes and the only clean fix I see is to provide filesystems with custom badops (or an equivalent) so that non-lockmgr locks are operational right until the vnode is freed.

I don't get the point about user-controlled leak. The user can keep vnodes around predominantly by opening them, which has the same side effect with the current patch.

In D26346#587295, @mjg wrote:

So a general point here is that the current behavior vs vgone is quite problematic, even ignoring this particular use case. And the fix would be integrating vnode recycling with filesystems, making the current change spurious in my opinion. As a step towards that, we could just make vm obj for tmpfs behave the same manner as it does for other cases.

Another example of the problem is non-lockmgr locks for vnodes. If it ever worked, it was in part because vget was a 2 step process starting with setting a flag that the vnode is needed, waiting for any in-flight stuff to settle and only then locking. The flag stuff was removed years ago and now you can get threads stuck in a custom locking primitive while the vnode gets doomed and gets op vector replaced which something only using lockmgr. This leads to all kinds of woes and the only clean fix I see is to provide filesystems with custom badops (or an equivalent) so that non-lockmgr locks are operational right until the vnode is freed.

Reworking forced unmounts is arguably out of scope of this patch.

I don't get the point about user-controlled leak. The user can keep vnodes around predominantly by opening them, which has the same side effect with the current patch.

I wrote "user-uncontrolled leak". Before this patch, if user mapped tmpfs node, its content memory is kept around until last unmap is done. With this patch, opening has same effect. But, if node (and v_object as the actual goal) is kept around until last reference on the vnode is dropped, content memory is kept as well, and since user cannot control for real when freenode() is done, it has a temporal leak not controlled by user.