Close files that were opened by guest VM via 9pfs

[njain15_protonmail.com]

Summary

See: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=294327

It seems this bug appears because calling close() on an open file in the guest will not send a clunk request to the P9 server. This is deferred until the vnode is reclaimed, which can be after a while.

The suggested fix clunks the open FID if the calling process is the last one using it.

[dave_freedave.net]

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.

[jlduran]

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);
}

[dave_freedave.net]

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.

[njain15_protonmail.com]

@dave_freedave.net Could you try a bulk poudriere run now? I seem to have gotten a successful run.

[markj]

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.

sys/fs/p9fs/p9fs_vnops.c

869

Looks like an unrelated change.

1283

Are you wrapping these lines because they're too long? Please avoid mixing stylistic and functional changes in the same diff.

[kib]

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.

[njain15_protonmail.com]

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.

[kib]

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.

[njain15_protonmail.com]

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?

[kib]

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.

[njain15_protonmail.com]

I have made the following changes:

• Instead of performing a reverse path-lookup every time we don't find a VFID, eagerly attach the user during VOP_LOOKUP. Also, when a new p9 node is created, create a generic VFID for it. This is used for fallback if some process doesn't go through the entire lookup routine to create FIDs.
• This removes the need to maintain a node session list and track the parent of each node, which means we don't need vref().
• Removed v_opens counts. This was not being used anywhere. Clunking now happens in VOP_INACTIVE/RECLAIM.
• Refactored p9fs_[get/release]_open_fid to not go through VOP_OPEN/CLOSE. Instead, maintain a local VOFID for them that is clunked after use.
• Bumped up our max scatter-gather support, since our MTU was recently changed from 8192 to 128KiB (commit 882181b). Leaving it at 20 caused sgl_list to fail on appends after some time.

I have done some poudriere builds, and some basic file operations testing (and running fio). I would like to test cases where we fallback to the generic FID more, but am unsure how to do so (ex. something fsync related that triggers VOP_STRATEGY?).

[kib]

sys/fs/p9fs/p9fs.h

111

Now there are three mutexes in single node. Could it be collapsed to one? Or even (ab)using the vnode interlock for the internal use.

sys/fs/p9fs/p9fs_vnops.c

177

How could this happen?

The vnode there must be locked, and it must be not reclaimed for the p9fs_inactive() to be called.

[njain15_protonmail.com]

sys/fs/p9fs/p9fs.h

111

Hmm. I have reused the VFID lock here now. I have also migrated the VFID lock to be shared/exclusive, since fids were being read more than added.

[kib]

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

[njain15_protonmail.com]

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.

[kib]

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