Details

Reviewers

kib
jhb
vinicius_ferrao.net.br
markj

Summary

This module implements RDMA for the NFS client.

It implements RFC-8166 and RFC-8267 using FRWR
where possible.

It does not, as yet, use Kostik's helper function for
access to a file's pages to avoid data copying to/from
them.

It implements a layer above the kernel OFED verbs
that is used by the client side krpc.

I realize this is a rather large chunk of code, so I
understand if you cannot review it.
(I'm putting it up now to give people lots of time,
since I am not planning on committing this to main
for at least 2 months.)

Thanks go to the Netperf lab for providing a test
facility and thanks goes to the Netperf lab folk
for their help with a newbie (me) getting set up.

I'd also like to thank Vinicius Ferrao for his server
implementation for FreeBSD. I would have never
been able to test this code without it.

Test Plan

Only minimal testing sofar. I will continue over
the coming weeks and hope that, by making
both the client and server modules available,
there may be other testers.

I will be announcing how to download the
modules on freebsd-current@ soon.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

rmacklem created this revision.Aug 30 2026, 10:46 PM

Herald added a subscriber: imp. · View Herald TranscriptAug 30 2026, 10:46 PM

rmacklem requested review of this revision.Aug 30 2026, 10:46 PM

Oh, I should have noted that I either wrote the code
or cribbed it from Linux sources that carry the same
dual license as the OFED sources.

I was careful to avoid copying from any of the Linux
files that are only GPL'd.

This update fixes a bunch of issues.

xprt_verbs.c started out as a dual Linux file called
net/sunrpc/xprtrdma/verbs.c (dual licensed like the
OFED stack). It used wait_for_completion_interrupt_timeout().
--> These calls caused a lot of trouble, such as "use after free" or

crashes caused by structures being used after they were free'd.

So now the code uses wait_for_completion() and seems to be working
well. (If I run into a "hard hang" of the client, I may have to revisit this.)

A bug where the client used the wrong xid value has been fixed by
putting the call_msg on the stack instead of using a global one.
(This bug caused Read reduction to fail on the server.)

The code has been modified to handle retries correctly. Still not
really tested, but it is at least close to correct.

It now seems to be working ok for normal mount usage.

It still has a problem after the server is rebooted.

It makes a new connection (qp) after the server reboots, but for some reason, all RPCs get a RPC_PROGUNAVAIL error reply after that. However, new mounts work.

I think this is a bug in the current "glue" in the NFS server.
It calls svc_listen() before svc_reg() has been called in nfsrvd_addsock(),
resulting in RPC_RPOGUNAVAIL replies to RPCs when the client
connects right after a server reboot.
I think a single call to svc_reg() done in svc_rdma_sro_newconn() for
the first connection after loading will fix this, but I haven't tested it yet.

A small optimization where, instead of using m_adj() after
converting an mbuf list from M_EXTPG mbufs to clusters
via mb_unmapped_to_ext(), it now trims the M_EXTPG mbuf
list before calling mb_unmapped_to_ext().

Plus assorted other changes I can no longer recall.

Rick, overall, it looks good, but theres a lot of code here. The only way to be sure is testing that.

However, if we keep the original args chain intact, as suggested in the review, and leave the metadata buffer mr linked in it, we also need to adjust how mr is freed. It could add issues if we don't clean up it correctly.

clnt_rdma.c
557–571	Rick, I think this block is modifying the wrong list. `m1` is the copy being modified to be sent, but the for loop moves through the original args. Doesn't `*mreduce_prev = m2->m_next;` modify the original argument list? It also looks to me that the assingment, in `m2->m_next` will be always `NULL` inside the `if`, because it's never set. Maybe we should build the outgoing copy without the metadata, instead of copying everything and then trying to remove it?

rmacklem marked an inline comment as done.Mon, Sep 7, 9:51 PM

rmacklem added inline comments.

clnt_rdma.c
557–571	Yes. Good catch. The code should remove the mbuf that indicates reduction from m1 and not args. It needs to remain in args, so that a retry works. I'll fix this for the next update.

vinicius_ferrao.net.br added inline comments.Mon, Sep 7, 10:17 PM

clnt_rdma.c
496	If we came from `call_again` label this may not be true and this entire block is skipped.
523	This will not be set if we didn't get through.
652–659	I think `rpos += tlen2` is adding too much for a reduced write. It already includes the RPC args and the RDMA header, it should only have been only the RPC. There should be a way to remove the RDMA block from it, maybe we should save the state before adding the RDMA header to `tlen2`.
897	This may become an issue. We should track the position independently. If we reach `goto call_again`, `rpos` may have been changed while reading the reply. And `reduce_chp` will be already set there, effectively skipping the `rpos` original value at line 522. Maybe what we should do is just do something like: /* Save the position / reduce_pos = rb->pos; / Calculate the postion for each write / read_pos = reduce_pos + rpc_prefix_len; / Here we keep rpos for decoding */ XDR_GETINT32(&xdrs, &rpos); This would avoid reusing `rpos` for both the request and the reply.

Rick, there are some goto shenanigans happening.

clnt_rdma.c
895	This may be also affected by the `call_again` label if we came from a `goto`. Should we have a cleanup goto label, so it's easy to retry?

Recent changes to "fix" handling of retries introduced
the bug Vinicius spotted. It actually didn't break anything
for the non-retry case, since it just meant the "reduce" mbuf
wasn't removed from the copy of the mbuf request list.

Since for the read RPC, that mbuf is at the end, it wouldn't
actually break the RPC message (except maybe leaving the
length too long, but the NFS code doesn't care about trailing
junk in the request, once it has parsed all it wants).

Anyhow, this version is fixed so that it removes the reduce
mbuf from the copy.

Since my test setup is broken at the moment, it hasn't actually
been tested.

We may be missing the error treatment for the case where reduce_chp is NULL.

clnt_rdma.c
513	`reduce_cmp` can be `NULL` here if `xprt_rdma_create_chunk` errors out.
551–556	Maybe we should check if `reduce_chp` was `NULL` here to also call `goto cannotencode`?

If we reach the final cleanup without a new mr we may run in a non necessary second free request.

clnt_rdma.c
1040	Add `mr = NULL` after for consistency to avoid trying to `free` it later.
1078–1079	We would have another `free` request even if `mr` was already freed.

rmacklem added inline comments.Mon, Sep 7, 11:05 PM

clnt_rdma.c
496	I think this is correct. Since "goto call_again" happens when the Kerberos credentials time out (based on the expiry of the TGT), I'm assuming that the reduction for a read or write will not have been done by the NFS server and that the reduce chunk formed by this block is still valid. (It is "yet another piece" not yet tested, because that requires Kerberos and setting that up in the Netperf lab setup is something I have not yet done.) --> If it turns out that the NFS server "swallows" the reduction chunk (a writelist chunk in RFC8166 terminology), then this code will have to be rewritten.
523	As above, the first pass through this code should always succeed. (I currently do not expect xprt_rdma_create_chunk() to fail and return NULL, but I won't claim it can never happen, since I'm still pretty sketchy on the OFED stuff.) On a subsequent retry, due to a Kerberos ticket expiry, I assume that the "reduce_chp" is still valid and does not need to be recreated.
557–571	Now fixed, although it would have only broken a retry done from above the clnt_rdma_call() and not the "goto call_again" case. It was broken in the sense that it didn't take the mbuf that "tags" the reduction out of the m1 copy of the arguments list, it turns out that, for a read RPC it is just trailing junk the NFS code didn't care about. For write reduction, the code has not yet been tested because the server does not currently do it.
652–659	Good question. This is for write reduction, that is not yet tested. My read of RFC8166 is that the position includes both the RDMA and RPC headers, but I could be wrong. I don't know if anyone has implemented this yet? If I am wrong and the RDMA header is not supposed to be in the position count, it can easily be taken out. See line# 601.
897	Yes, I agree that reuse of rpos was not wise and I don't think it is correctly calculated at this time. (Even ignoring that it might be trashed for a retry.) Since the write reduction code is no tested, I am not surprised. I'll revisit rpos for the next rendition. Of course, the FreeBSD server should learn how to do write reductions someday. I can work with you on that one, once this code is stable.

Use a separate variable called reduce_pos for the
reduction position, which is now calculated by the
NFS glue code, as suggested by Vinicius.

This is part of the untested code that does write
reduction, so I suspect there are more issues with it.

That is the same as the mr issue.

clnt_rdma.c
574–575	I think this is the same double `free` if it get through a `goto` path. We should add `mreq = NULL;` after `m_freem();`.
1063–1064	Here!

Add a couple of mxxx = NULL statements to avoid
a double free of the mbuf lists, as suggested by Vinicius.

Also, the code now uses a helper function in the generic
RPC code to remove the reduction mbuf from the argument
mbuf list.

This function is also now used by nfsrpc_readrpc(), but the
patch for this is not in main as yet.

Added a 2nd argument to rpc_remove_mreduce() so
that it can, optionally, not free the mbuf.

Moved the rpcrdma_regwr structure arrays from the
rpcrdma_chunk_priv structure that is allocated for each
chunk into rpcrdma_ep, which is for the connection (qp).

These changes do not do any actual fixes.

The main changes are using pages for the inline send/recv
buffers instead of malloc'd buffers, which google claims is
more efficient. I have not yet implemented RFC8797, which
should allow the use of the full 4K for servers that will allow it.

I've fixed up the recovery code so that it now seems to retry
RPCs successfully and fixed a few potential leaks spotted by
Vinicius.

The main glitch I know of at this point is that, about once in
a few million RPCs, this gets thrown out onto the console:
Sep 12 03:28:24 mercat1 kernel: mlx5_1: WARN: dump_cqe:273:(pid 100228): dump error cqe
Sep 12 03:28:24 mercat1 kernel: 00000000 00000000 00000000 00000000
Sep 12 03:28:24 mercat1 syslogd: last message repeated 2 times
Sep 12 03:28:24 mercat1 kernel: 00000000 08007806 25000907 1b0405d3
Sep 12 03:28:24 mercat1 kernel: rpcrdma_send_done: failed opcode=0 status=6
Sep 12 03:28:24 mercat1 kernel: rpcrdma_send_done: pg=0x400003163 sgeaddr=0x0 len=0 lkey=0x0 num_sge=0 send_flags=0x0

The WC is obviously trashed, but there is a KASSERT() an line#1154 in xprt_verbs.c
that sanity checks the WC just before ib_post_send() and this KASSERT() does not
get triggered. So, how does it get trashed??

The structure it is in gets allocated when the connection (QP) is established
and isn't free'd until disconnect, so it isn't a "use after free" problem.
There is no pointer in the code that wanders around this structure.
re_send[ind].wr is the first array in the structure, so a negative index
on subsequent arrays could do it.
But I have KASSERT()s checking the array index (usually called "ind") for
being out-of-bounds and those KASSERT()s don't get triggered, either.
--> So, it seems more likely that something in the OFED code or even the

NIC firmware might be doing this?

The NICs are ConnectX4's and they don't have the latest and greatest firmware
in them.
--> I am curious to see if newer NICs exhibit the same behaviour.

This isn't too serious, since the client code establishes a new connection (QP)
and then continues on after a few seconds.

Rick, I'm looking again at this code. Two more comments.

clnt_rdma.c
901–906	`rpos` should check if it matches the number of segments we offered in `chp`. Then we get the `num_segment` from `reduce_chp` or `reduce_chp`, whatever is valid here.
916–923	Rick, I think we should compare these values with the segment we offered before adding length to tlen. This is specified in the RFC here: https://www.rfc-editor.org/rfc/rfc8166.html#section-3.4.6 The reply chunk information is also here: https://www.rfc-editor.org/rfc/rfc8166.html#section-4.3.3

rmacklem marked an inline comment as done.Sun, Sep 13, 7:42 PM

rmacklem added inline comments.

clnt_rdma.c
901–906	Why? The client "mind-set" is quite different than the server one, imho. For the client, it doesn't care if the server replies with bogus info, so long as it doesn't make it crash. Put another way.. The server should try to conform with the RFCs as closely as possible, to minimize interoperability issues. However, I think the client should be as lenient as possible with servers when it comes to conformance to the RFCs. (I don't see any need for a client to not work with a broken server unless it cannot work correctly.) For example, there are places where the Linux knfsd server does not conform with the RFCs. I know it, the Linux impementors know it, but I've coded the client so it works anyhow. For the client, the only two things it needs to know from the server's reply about chunks is.. Was the chunk used. What length was read/written. So, the client shouldn't bother checking if the rest of it is correct or not, imho. (Again, assuming it won't cause a crash.)
916–923	I'll look. If it can cause a crash, then yes, otherwise, as above, why bother.

I finally got around to debugging the backchannel
stuff. I ended up creating separate svc_rdma_backchannel_xxx()
functions instead of trying to overload the svc_vc_backchannel()
ones.

It also has some other fixes for problems I ran into while testing.

I fixed everything that Vinicius pointed out except for validation
of the reply position, which the client has no use for, so I didn't
see any need to try and verify it. (After all, if it found it invalid,
the most the client would want to do is spam the console w.r.t.
it being incorrect.)

I think it is finally ready for third party testing.

I have a few enhancements planned, but I'll do those as separate
commits:

Try and get it working with kib@'s vnode page helper functions.
Write a separate code path that avoids use of scatter/gather of pages (allocating one chunk with contigmalloc()) for NICs that do no do scatter/gather of pages (or do not do them well).
Add a mount option to specify the address of the NIC to be used. (The current code uses fib[46}_lookup() for a "best guess" at which NIC, but I suspect that is not infallible?)

I will be continuing to test it and will post on freebsd-current@ for
other testers, once the "unofficial ports" of the modules is organized.

nfsclrdma.ko: Client side NFS over RDMA module
Needs ReviewPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 186550

clnt_rdma.c

rdma/xprt_rdma.h

xprt_verbs.c

nfsclrdma.ko: Client side NFS over RDMA moduleNeeds ReviewPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 186550

clnt_rdma.c

rdma/xprt_rdma.h

xprt_verbs.c

nfsclrdma.ko: Client side NFS over RDMA module
Needs ReviewPublic
Actions

Revision Contents
Changeset List