I updated, but I'm seeing the same errors from poudriere-bulk runs:

[00:00:54] Warning: (devel/air-go): make: /usr/ports/Mk/bsd.port.mk: Invalid argument
[00:00:54] Warning: (devel/air-go): Error: Error looking up dependencies for devel/air-go
...
[00:00:55] Warning: (devel/lua-language-server): make: /usr/ports/Mk/bsd.port.mk:1053: Cannot open /usr/ports/Mk/bsd.commands.mk
[00:00:55] Warning: (devel/lua-language-server): in /usr/share/mk/bsd.port.mk:27
[00:00:55] Warning: (devel/lua-language-server): in /usr/share/mk/bsd.port.post.mk:4
[00:00:55] Warning: (devel/lua-language-server): in /usr/ports/devel/lua-language-server/Makefile:104
...
[00:00:55] Warning: (devel/lua-language-server): make: Fatal errors encountered -- cannot continue
[00:00:55] devel/lua-luarocks depends on devel/gmake
[00:00:55] Warning: (devel/lua-language-server): Error: Error looking up dependencies for devel/lua-language-server
..

I think you have a race and will need to introduce locking or find a clever use of atomics for vofid->v_opens.

In D57096#1308375, @dave_freedave.net wrote:

I think you have a race and will need to introduce locking or find a clever use of atomics for vofid->v_opens.

Something like?:

if (atomic_fetchadd_int(&vofid->v_opens, -1) == 1) {
        p9fs_fid_remove(np, vofid, VOFID);
        p9_client_clunk(vofid);
}

In D57096#1308398, @jlduran wrote:

In D57096#1308375, @dave_freedave.net wrote:

I think you have a race and will need to introduce locking or find a clever use of atomics for vofid->v_opens.

Something like?:

if (atomic_fetchadd_int(&vofid->v_opens, -1) == 1) {
        p9fs_fid_remove(np, vofid, VOFID);
        p9_client_clunk(vofid);
}

So its going to be everywhere not just one location. If one place treats v_opens as atomic, they pretty much all have to change to check atomically that they didn't get a closed struct p9_fid and it they did to retry. Or it may be better to push all of that functionality into the functions that manage the struct p9_fid cache.

But before going either of those routes are we sure there isn't some simpler off by one bug that is preventing things from aging out today? I am not the right person to answer that, I just use p9fs a lot but with bhyve instances that I only have up briefly so it hasn't been a priority for me to look into this.

When all handles referencing a vnode are closed, VOP_INACTIVE (implemented by p9fs_inactive()) should be called. Isn't that the right place to clunk the file? From what I can see, that's currently happening in p9fs_reclaim() which is too late as you say.

In D57096#1311582, @markj wrote:

When all handles referencing a vnode are closed, VOP_INACTIVE (implemented by p9fs_inactive()) should be closed. Isn't that the right place to clunk the file? From what I can see, that's currently happening in p9fs_reclaim() which is too late as you say.

Generally VOP_INACTIVE() is a good place, but a (small) problem with it is that it might be skipped. That said, VOP_INACTIVE + VOP_RECLAIM should be good enough.

Generally VOP_INACTIVE() is a good place, but a (small) problem with it is that it might be skipped

I had initially called p9fs_fid_remove_all() in VOP_INACTIVE, but as you said, it isn't guaranteed to be called. When I tested this solution, the number of open files on the host still grew quickly. For example, after a poudriere run, lsof shows ~25k file descriptors.

Closing it in VOP_CLOSE keeps this number at <40 for me.

In D57096#1311601, @njain15_protonmail.com wrote:

Generally VOP_INACTIVE() is a good place, but a (small) problem with it is that it might be skipped

I had initially called p9fs_fid_remove_all() in VOP_INACTIVE, but as you said, it isn't guaranteed to be called. When I tested this solution, the number of open files on the host still grew quickly. For example, after a poudriere run, lsof shows ~25k file descriptors.

Closing it in VOP_CLOSE keeps this number at <40 for me.

Yes, but. I do not expect that it would be so drastic, and there is probably something else broken. VOP_INACTIVE() is skipped when the current lock in vput() is not exclusive and an immediate upgrade request failed because there are other shared lock owners. At very least this means that there are hold owners on the vnode without refs, which is not typical.

Yes, but. I do not expect that it would be so drastic, and there is probably something else broken. VOP_INACTIVE() is skipped when the current lock in vput() is not exclusive and an immediate upgrade request failed because there are other shared lock owners. At very least this means that there are hold owners on the vnode without refs, which is not typical.

It seems the problem is in p9fs_vget_common(). In creating a new Plan 9 node, we vref() the parent vnode. These directory vnodes are only vrele() when we unmount. So, while the filesystem is mounted, they will never be inactive or reclaimed. Would it suffice to use vhold/vdrop instead?

In D57096#1312676, @njain15_protonmail.com wrote:

Yes, but. I do not expect that it would be so drastic, and there is probably something else broken. VOP_INACTIVE() is skipped when the current lock in vput() is not exclusive and an immediate upgrade request failed because there are other shared lock owners. At very least this means that there are hold owners on the vnode without refs, which is not typical.

It seems the problem is in p9fs_vget_common(). In creating a new Plan 9 node, we vref() the parent vnode. These directory vnodes are only vrele() when we unmount. So, while the filesystem is mounted, they will never be inactive or reclaimed. Would it suffice to use vhold/vdrop instead?

I would say that the whole idea of holding the directories vnodes is against the FreeBSD VFS design. It means that the mp could consume arbitrary amount of both vnode count and kernel memory, since held vnodes cannot be recycled by vnlru.

Is there some reference or a spec for the p9fs protocol?

In D57096#1323730, @kib wrote:

Is there some reference or a spec for the p9fs protocol?

• https://github.com/chaos/diod/blob/master/protocol.md
• https://docs.kernel.org/filesystems/9p.html
• https://www.usenix.org/legacy/publications/library/proceedings/usenix05/tech/freenix/full_papers/hensbergen/hensbergen.pdf

I think the original commit that ported p9fs to FreeBSD used the Linux implementation as a reference, since some code structure seems the same. For example, Linux pins their dentry structure and does this same reverse-lookup. The only way they avoid this is if you set the cache=none option, which essentially reclaims the node when it goes inactive (clearly high performance penalty). See: https://github.com/torvalds/linux/blob/master/fs/9p/fid.c and https://github.com/torvalds/linux/blob/master/fs/9p/vfs_super.c.

I do not have much to comment on the VFS side of things for the patch.