Made the changes suggested by kib@.

In D44906#1023894, @olce wrote:

In D44906#1023893, @markj wrote:

Oh hmm, not unrelated, but at a glance I think it is superseded by the privport check. That is, all code paths which lead there first have to go through the privport check. But I could be wrong about this.

It feels like NFS_REQRSVPORT is older but apparently it was introduced by the same (initial) commit as the if (nfs_privport ... check (9ec7b004d0edb). Maybe replacing NFS_REQRSVPORT by a runtime check for nfs_privport would make sense, but I don't really know this code. It may be instead that the code under NFS_REQRSVPORT is redundant, I'm not sure either. In any case, this would be a separate change.

lgtm. I cannot recall if there is any man page change
needed for this?

markj@ has a better patch in D44614.

I made a couple of comments, but feel free to ignore
them, since I think the patch is fine without the changes.

In D44499#1015512, @markj wrote:

I'd somewhat prefer to make this more prominent, having its own paragraph in the description section. I understand that the problem at hand is a natural consequence of how exports(5) works, but it's really easy for someone new to NFS to miss that they can't "safely" export a single subdirectory from a local filesystem on the server.

Made the changes suggested by emaste@.

This version looks fine to me. I'll let you decide whether or
not to leave the TCP check in.

In D44395#1012518, @wollman wrote:

Very minor comment: there should be markup for sysctl variables, but I haven't checked another manual page to be sure which one we're using. (My gut says .Li would be the expected formatting.)

This version has Mike Karel's suggested change.

va_holey may be the way to go for FreeBSD15, so long as
it is easy to calculate. Having a file system do something
akin to Seek_Hole in VOP_GETATTR() would defeat the purpose.

Updated the comment as suggested by kib@.

In D44002#1009718, @jhb wrote:

In D44002#1006603, @rmacklem wrote:

Hmm. Sounds like it should only be enabled if TLS is
not enabled?
For the TLS case, the receive code in clnt_vc.c does
upcalls to userland for non-application data records
and these are handled by the OpenSSL library using
a SSL_read() call.

No, it's fine to enable if TLS is enabled, but we do want to enable TLS first to give TLS offload priority over DDP.
You can still use DDP for data copied out to userspace, it's just not as useful an optimization in that case.

This patch would enable it before TLS unconditionally,
I think?

Yes.

If you look at clnt_rc.c, you'll find the rpctls_connect()
call right after the clnt_vc_create() call. That is what enables
the ktls via an upcall to the daemon that does a SSL_connect().
It sounds like that is where the socket option should be set?

Yes, for the client I can move it back to where it was before, but leave off the CLSET_DDP complexity.

Yes, that sounds fine to me.

Hmm. Sounds like it should only be enabled if TLS is
not enabled?
For the TLS case, the receive code in clnt_vc.c does
upcalls to userland for non-application data records
and these are handled by the OpenSSL library using
a SSL_read() call.

One more "bigger picture" comment.
If setting TCP_USE_DDP is something
that the NFS client will always want to
try and do, then I would scrap the client
side changes in this patch and put the
so_setsockopt() right in clnt_vc_create().

One more comment. If this socket option must/should be
set right away (before any data travels on the TCP
connection), it should be left in clnt_reconnect_coonect(),
but should be moved to before the rpctls_connect() call block.
(Right after clnt_vc_create() at line#202.)

Oh, and to make it work on the client side, the
CLSET_TCP_DDP needs to be set in newnfs_connect().
(In the nmp != NULL section.)

As noted in the second inline comment, the socket
opt would normally be set in clnt_vc.c and clnt_rc.c
would just do a CLSET on the client to do that.

Looks ok to me.

I ran a test cycle mounting the client laptop locally
(which is all I can do at this time) and with another
experimental patch removed and...
-> I see no performance degradation and do not

go anywhere need the runningbuf limit (it never
exceeded 1Mbyte, from what I saw).

In D43249#987015, @imp wrote:

In D43249#987014, @rmacklem wrote:

Although I have not found any correctness issue with this patch,
I am seeing a little performance degradation.
For example, a kernel build over NFS takes about 5% longer
on the setup/hardware I have.

I do not know if 5% is worth worrying about?

Are you hitting the runningbufs limit?

vfs.hirunningspace: 16777216
vfs.lorunningspace: 11206656
vfs.runningbufspace: 0

The first two are control, and the last one is the currently used space...

Although I have not found any correctness issue with this patch,
I am seeing a little performance degradation.
For example, a kernel build over NFS takes about 5% longer
on the setup/hardware I have.

Yes, I think a FreeBSD NFS server exporting msdosfs
file systems will change file handles when a rename
occurs.

Looks ok to me, although I have limited knowledge w.r.t.
the buffer cache.

Added kib@'s lines to the calculation as an extra
safety belt (and to avoid any overflow to a negative
value).

This looks ok to me and testing with the patch has not
found any regression.

This looks fine to me. I did a little testing and did not
find any regression.