This patch is bogus and will not fix the crash.
It is rpc_gss_init() that needs to set CURVNET.

In D50962#1163352, @rmacklem wrote:

In D50962#1163337, @glebius wrote:

What is the problem we are trying to fix here? What is the panic trace?

The function rpctls_connect() is always called with socket's vnet set by clnt_reconnect_connect(). In general setting vnet0 is never correct. I understand that right now NFS may not fully support the VIMAGE, but we should slowly aim towards correct support, rather than just blindly plugging the panics.

The panic occurred because the CLNT_CALL_MBUF() called clnt_nl_call()
which asserts CURVNET_ASSERT_SET(). I don't have the stack trace. It
happened during Bakeathon testing over a month ago.
I fixed it by putting CURVNET_SET(TD_TO_VNET(curthread), but that
won't work when in a jail.

The CURVNET_SET(so->so_vnet) just before rpctls_connect()
doesn't always work. My guess would be something like...

• readahead/writebehind thread (from taskqueue, so it is only a kernel thread) does a clnt_reconnect_connect(). clnt_reconnect_connect()->__rpc_nconf2socket()->socreate()->soalloc(CRED_TO_VNET(cred)) I have no idea what curthread->td_ucred is going to be for taskqueue threads.

Anyhow, I cannot recreate this on local testing (during the Bakeathon I am testing against
non-FreeBSD servers over a very slow vpn) and don't have the entire panic/crash.

All I know is that the NFS client mounts are always vnet0, since they cannot be
done in a jail, even though processes/threads running in a jail can use the mount.
(I suspect you are correct, in that the CURVNET_SET()/CURVNET_RESTORE() around
are broken and don't always work. If they can't use so->so_vnet and they can't use
CRED_TO_VNET(), setting vnet0 seems like the only option?)

Ignore the above paragraph. I found where I scribbled down the crash. The calls were:

In D50962#1163337, @glebius wrote:

What is the problem we are trying to fix here? What is the panic trace?

The function rpctls_connect() is always called with socket's vnet set by clnt_reconnect_connect(). In general setting vnet0 is never correct. I understand that right now NFS may not fully support the VIMAGE, but we should slowly aim towards correct support, rather than just blindly plugging the panics.

I realized that this is a NFS client side call,
so it needs to set vnet0.
I'll do that in the specific RPC-over-TLS code.

In D50624#1158192, @khng wrote:

By the way, given that rpcbind and nfsd will install a default pidfile to /var/run even without specifying -P: shall we document that somewhere in RELNOTES and the commit message, or do we make the errno == EEXIST case a warn() call instead of errx()? I am thinking of that because some people might spin up multiple nfsd in their setup without using the rc.d/nfsd script, and with different bound IP for each nfsd spun.

I'll leave the minor nits up to you.
Are you a committer or do you need me
to do it?

In D50627#1156884, @des wrote:

then either fix rpcbind so it doesn't print an error when started before krpc is loaded, or approve this review... your choice

In D50627#1156452, @des wrote:

In D50627#1156360, @rmacklem wrote:

Are you sure? I didn't think the krpc used rpcbind.

It's the other way around; rpcbind isn't useful without krpc, and while krpc usually ends up getting loaded sooner or later, if it's not present when rpcbind starts, you get error messages on the console.

Are you sure? I didn't think the krpc used rpcbind.

The main change is to zfs_readdir(), so that it skips over
forbidden names for a named attribute directory.

Good questions! Maybe they need to be asked on a mailing
list for FreeBSD, but at least I'd like to hear your thoughts.

Added support for _PC_HAS_NAMEDATTR, which is
mainly a new function called zfs_has_namedattr().

In D50140#1143762, @kib wrote:

Does Solaris use the pathconf() to report xattr existence for the specific node?

It seems to do so. Solaris calls it _PC_XATTR_EXISTS.
It seems to work for local files on Solaris, but returns random junk for an NFSv4 mount.
(Since things look correct on the wire, I think this is just a Solaris bug?)

In D50141#1143706, @ziaee wrote:

Are these Function Arguments? Can I alphabetize these?

Clarified when named attributes and named attribute
directories are created.

Replaced the copyright notice with the new recommended
short one, as suggested by ziaee@.

Removed the "." after my name in the copyright.

Added chdir(2) to the SEE ALSO section of the runat.1.

Marked inline comments as done.

Updated the copyrights, as suggested by ziaa@.

Looks fine to me, rick

Note that O_CLOEXEC is set in "fmode" near the top of main()
for the second open.

Marked inline comments as done.

Missed a couple of cases for .Ar file. Fixed now.

Updated man page with changes suggested by ziaa@.

Updated the man page to reflect the use of /bin/sh.

This version just does an execl() as suggested by kib@.

Oops, missed the last suggested change from kib@
the last time. It is here now.

Updated runat.c to apply kib@'s suggestions.
For some reason (don't know where I got it) I
thought that fchdir() directory change would
affect more than the process, which was why I
did the fork(). (I think it came from ancient
recollections of how 6th Edition worked in the
1970s. I'm gettin old;-)

Re-written to conform to the more Solaris-like
semantics of O_NAMEDATTR that is now implemented
in main.

Update runat.c to use the up to date more Solaris-like
O_NAMEDATTR flag.

I misinterpreted the Solaris documentation for O_XATTR.
It is used for an openat() where the path is for a file in
the file system's namespace, to open a named attribute
for the file, not a named attribute directory.