Page MenuHomeFreeBSD

Close files that were opened by guest VM via 9pfs
Needs ReviewPublic

Authored by njain15_protonmail.com on Tue, May 19, 4:31 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, May 26, 2:26 PM
Unknown Object (File)
Tue, May 26, 7:56 AM
Unknown Object (File)
Tue, May 26, 7:54 AM
Unknown Object (File)
Mon, May 25, 9:12 PM
Unknown Object (File)
Fri, May 22, 11:17 PM
Unknown Object (File)
Fri, May 22, 8:23 PM
Unknown Object (File)
Fri, May 22, 6:49 PM
Unknown Object (File)
Thu, May 21, 9:35 PM

Details

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.

Test Plan

Tested the following on bhyve and KVM by monitoring output of lsof.

  • Created and deleting single file.
  • Open and close same file multiple times.
  • tail -f <shared_file> to check read keeps file open until tail is exited.
  • @jim.chen.1827_gmail.com tested concurrent appends with
#!/bin/sh
for i in <some list>
do
  find . -iregex <something here> | xargs -I{} sh -c "echo <something> >> {}" &
done

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

njain15_protonmail.com retitled this revision from Bug fix for PR294327 to Close files that were opened by guest VM via 9pfs.Tue, May 19, 5:15 PM
njain15_protonmail.com edited the summary of this revision. (Show Details)

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.

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

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 removed a reviewer: emaste.

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

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
851

Looks like an unrelated change.

1269

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

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.

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.