In D57681#1322466, @kib wrote:

In D57681#1322456, @rmacklem wrote:

This looks ok to me, although I'll admit I am not
conversant with UFS semantics, so I'll leave that
to others.

All existing callers pass tsa == NULL, so the intent is that the semantic does not change.
It is the question whether your case is served when tsa != NULL is provided.

I guess it does. I don't know UFS, but it does seem "inefficient" to first do
UFS_INODE_SET_FLAG_SHARED(), which locks/unlocks the vnode interlock and
then call ufs_itimes(), which locks the vnode interlock again and calls ufs_itimes_locked().

This looks ok to me, although I'll admit I am not
conversant with UFS semantics, so I'll leave that
to others.

In D57681#1322365, @kib wrote:

What is not clear to me, should ufs_itimes() only use the passed timespec for atime update, or it shall be used for all times as I did in the initial version of the patch.
Perhaps this is the question to Rick, since I believe the only caller with tsa != NULL would be nfs server.

Made changes as suggested by kib@.

Changed return value to EBUSY when the nfsd
are running and got rid of the locking around
reading the list head, as suggested by markj@.

Add vfs_netexport_reset() as suggested by markj@.
Also, add KASSERT()s to check ne_ref > 0.

In D57555#1320278, @glebius wrote:

Mark, again thanks a lot for fixing my bug that I was too slow (extremely slow!) to fix. I feel shame for that. Rick, very sorry for delaying looking into the problem for two months.

In D57554#1320166, @kib wrote:

Do you need a kernel thread and code to create the placeholder files? Could it be done in userspace?

Updated with changes recommended by markj@
and kib@. Mostly removal of unnecessary locking
calls, plus a couple of important typo fixes.

Added vfs_netexport_acquire() as suggested by markj@
and renamed vfs_netexport_free() to vfs_netexport_release(),
since that seemed more consistent.

This looks fine to me and seemed to behave when I
tested with lotsa printf()s.

Ok, so I finally think I remember how this works;-)
Your patch tests out well, but I think it needs something like the following:

• sys/rpc/rpcsec_tls/rpctls_impl.c.yyy 2026-06-14 09:08:45.910463000 -0700

+++ sys/rpc/rpcsec_tls/rpctls_impl.c 2026-06-14 09:01:06.722976000 -0700
@@ -298,8 +298,18 @@ printf("rpctls_connect soref so=%p\n", so);

	stat = rpctlscd_connect_2(&arg, &res, rpctls_connect_handle);
	if (stat == RPC_SUCCESS)
		*reterr = res.reterr;

• else

+ else {
+ struct ct_data *ct = (struct ct_data *)newclient->cl_private;
+
+printf("clnt at rpctls_rpc_failed so=%p\n", so);

		rpctls_rpc_failed(&ups, so);

+ /*
+ * Since rpctls_rpc_failed() will do a soclose() if the
+ * daemon is not running, set ct_tlsstate to avoid doing
+ * a close in clnt_vc_destroy().
+ */
+ ct->ct_tlsstate = RPCTLS_INHANDSHAKE;
+ }

In D57555#1319373, @rmacklem wrote:

Ok, so now I see what you've done. You are now taking the
soref() when it is inserted in the RB tree instead of when it
is taken out.

I suspect the "else" case in rpctls_rpc_failed() needs a
sorele() along with the soshutdown(), to get rid of the
reference.

Now that I am testing it, this does not seem to be needed.
(You get to the "else" case if rpc.tlsservd is running on the
server and you use your little tls_trigger.c test against it.)

Once I've put it through some testing, I'll update the patch.

Ok, so now I see what you've done. You are now taking the
soref() when it is inserted in the RB tree instead of when it
is taken out.

Fix the memset() so that it only zeros out
part of the structure, as spotted by markj@.

I don't see why adding another soref() stops a socket
from being leaked?

Use refcount_init() as suggested by kib@.

Modified patch to use a local static bool instead
of kgss_gssd_cl. I also moved the svc_nl_create()
and svc_reg() function calls in gssd.c to before
the daemon() call, to avoid any race with nfsd
startup.
As suggested by glebius@.

Do you need me to commit it or do you have a
commit bit for src?

Thanks for the comments.

In D57455#1317682, @glebius wrote:

I think this change also creates a potential process level race with gssd. The rc(8) can run nfsd before gssd instantiated the RPC server. Together with this change in the gssd the daemon(3) call needs to be moved down after svc_nl_create() and svc_reg().

In D57455#1317681, @glebius wrote:

But why do you need a second pointer? You may have a static bool that marks the kgss_gssd_handle has been successfully checked with a NULL RPC. I would actually suggest not to store a second pointer neither a bool. Make kgss_gssd_client() always return a checked client.

In D57455#1317599, @glebius wrote:

Sorry, can't understand why do we need a second CLIENT * for this NULL RPC.

I'll just leave things in their current state.

In D57180#1310528, @pen_lysator.liu.se wrote:

Hmm.. Yeah, I didn't want to change that part of the code. But now when I read the fine print in the man page for strtoul() and strtol() I think we should just get rid of that extra check and just use "*endp != '\0'"...

Hmm.. Shouldn't we use strtoumax() and strtoimax() while we're at it...? Perhaps something like:

intmax_t iv;
uintmax_t uv;
uid_t id;
...
if (*name == -) {
        iv = strtoimax(name,&endp,0);
} else {
        uv = strtoumax(name,&endp,0);
}
if (name == endp || *endp != '\0') {
        errno = EINVAL; /* No or invalid number */
        return (-1);
}
if (*name == '-') {
        id = iv;
        if (int32_t) id != iv) {
               errno = EINVAL; /* Overflow */
               return (-1);
       }
} else {
        id = uv;
        if (id != uv) {
                errno = EINVAL; /* Overflow */
                return (-1);
        }
}

Hmm.. Not sure (or I'm too tired right now) how to write a better and fully portably check for under/overflow in the negative case that works if we get 64 bit uid_t/gid_t. :-)

I'll commit it authored by you, since I don't recall
that you are a committer.

Oh, and I'd leave the comment (or something like it)
in the code, so that it is obvious why the cast'ng is done.

If we cast the ids to (uintmax_t) and then use %ju,
it will still work if uid_t/gid_t ever becomes 64bits.

In D55203#1309629, @kib wrote:

I think it could be set to 'enabled' for machines with DMAP, i.e. amd64/arm64, and might be risc-v, if anybody ever uses nfs server on it. For other arches, the knob almost surely should be kept disabled.
mb_unmapped_to_ext() uses non-privately mapped sfbufs for all extents of mbufs. On DMAP systems, it is free. On other arches, allocating such sfbuf causes global IPI, and the whole chain of sfbufs is freed only after the mbufs are released by the network card. Besides the cost of allocating , this would make sfbufs scarce resource for other consumers and even for nfs server itself. There are around ~1K of sfbufs, and mb_unmapped_to_ext() seems to drop packet if an sfbuf cannot be allocated immediately.

In D55203#1309547, @gallatin wrote:

In D55203#1309439, @rmacklem wrote:

In D55203#1309344, @gallatin wrote:

In D55203#1309194, @glebius wrote:

I don't think this is a right fix. A route lookup now doesn't guarantee the same interface will be used in the future. There is dynamic routing, weighted routing, policy routing, etc etc etc.

There should be some generic gate that would convert mbufs otherwise, we will need to add a code like this every module that generates mbufs. Again, a code that is not correct when routing isn't static.

As Rick said in the description, this is just a hint, and the conversion routine in ip_output() will handle any case where the egress NIC changes. However, just using extpg mbufs all the time seems like a better solution. That's what SW ktls offload does. mb_unmapped_to_ext() is not super expensive, and most high-ish performance NICs are aware of extpgs these days (iflib + mlx5 + cxgbe covers most 10Gb or more NICs in practice).

Kostik preferred not enabling it all the time.
See thtis:
https://lists.freebsd.org/archives/freebsd-net/2026-May/008766.html

The case of concern is some corner case, where
the "always enabled" causes a regression. I asked on freebsd-net@
to try and determine if such a case exists?

Kostik didn't indicate if he thought such a case exists, but was
concerned that the user wouldn't understand why the regression
happened (and the NFS patch includes a sysctl that turns it off).
--> Right now, it is never enabled, so not enabling it does not

 introduce a regression.
Enabling it unconditionally makes a regression more likely
than only enabling it when this hint thinks the NIC can do it
and that hint is incorrect.

I doubt NFS servers have frequent routing changes, but I do not
know that for certain?

You guys can debate it. If there is no consensus, I'll just leave
it disabled as it is now.

I think assuming a performance reduction from mb_unmapped_to_ext() may be a bad assumption. Eg, in my experience from ~2017 or so (when Netflix had enough non-https traffic to matter), using M_EXTPG mbufs for sendfile and doing the conversion at the edge in ip_output() was *faster* than using plain mbufs. I attributed this to avoiding cold-cache pointer chasing in socket buffers. Note that sendfile's use of ext pgs is gated by kern.ipc.mb_use_ext_pgs, which has been 1 since its inception at Netflix. We should probably default it to 1 upstream as well.

In D55203#1309344, @gallatin wrote:

In D55203#1309194, @glebius wrote:

I don't think this is a right fix. A route lookup now doesn't guarantee the same interface will be used in the future. There is dynamic routing, weighted routing, policy routing, etc etc etc.

There should be some generic gate that would convert mbufs otherwise, we will need to add a code like this every module that generates mbufs. Again, a code that is not correct when routing isn't static.

As Rick said in the description, this is just a hint, and the conversion routine in ip_output() will handle any case where the egress NIC changes. However, just using extpg mbufs all the time seems like a better solution. That's what SW ktls offload does. mb_unmapped_to_ext() is not super expensive, and most high-ish performance NICs are aware of extpgs these days (iflib + mlx5 + cxgbe covers most 10Gb or more NICs in practice).

In D55203#1306664, @rmacklem wrote:

In D55203#1306641, @zlei wrote:

In D55203#1305963, @rmacklem wrote:

Could you please take another look at this?
kib@ seems to think this is preferred to just
enabling it for all NICs.

I understand that it is only a "hint", since
routing can change at any time.
However, for an NFS server, routing is
unlikely to change without a NFS server
restart.

Typically a high available NAS server wants link aggregation or ECMP route. The latter may introduce route change if one of the links fails ( planned or un-planned ).

I have a two ports Chelsio T520-CR ethernet card which supports MEXTPG. I'd like to setup an NFS server to test firstly.

Sure. After you have applied this patch, apply the patch here..
https://people.freebsd.org/~rmacklem/new-extpg.patch

I had one person test it who had Mellanox NICs, but no one
who has Chelsio.

Thanks, rick

In D55203#1306641, @zlei wrote:

In D55203#1305963, @rmacklem wrote:

Could you please take another look at this?
kib@ seems to think this is preferred to just
enabling it for all NICs.

I understand that it is only a "hint", since
routing can change at any time.
However, for an NFS server, routing is
unlikely to change without a NFS server
restart.

Typically a high available NAS server wants link aggregation or ECMP route. The latter may introduce route change if one of the links fails ( planned or un-planned ).

I have a two ports Chelsio T520-CR ethernet card which supports MEXTPG. I'd like to setup an NFS server to test firstly.

Could you please take another look at this?
kib@ seems to think this is preferred to just
enabling it for all NICs.

Increase the "off" sanity limit and declare ndm_eocookie
as u_int as suggested by markj@.

Add a sanity limit for "off" to avoid an
overflow when calculating "pos".
This ensures a NULL reply for overly
large offsets.