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?

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.

If we came from call_again label this may not be true and this entire block is skipped.

This will not be set if we didn't get through.

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.

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.

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?

reduce_cmp can be NULL here if xprt_rdma_create_chunk errors out.

Maybe we should check if reduce_chp was NULL here to also call goto cannotencode?

Add mr = NULL after for consistency to avoid trying to free it later.

We would have another free request even if mr was already freed.

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.

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.

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.

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.

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.

I think this is the same double free if it get through a goto path.

We should add mreq = NULL; after m_freem();.

Here!