On 5/13/2014 4:44 PM, Chuck Lever wrote:
+static int rdma_read_chunks(struct svcxprt_rdma *xprt,
+ struct rpcrdma_msg *rmsgp,
+ struct svc_rqst *rqstp,
+ struct svc_rdma_op_ctxt *head)
{
- struct ib_send_wr read_wr;
- struct ib_send_wr inv_wr;
- int err = 0;
- int ch_no;
- int ch_count;
- int byte_count;
- int sge_count;
- u64 sgl_offset;
+ int page_no, ch_count, ret;
struct rpcrdma_read_chunk *ch;
- struct svc_rdma_op_ctxt *ctxt = NULL;
- struct svc_rdma_req_map *rpl_map;
- struct svc_rdma_req_map *chl_map;
+ u32 page_offset, byte_count;
+ u64 rs_offset;
+ rdma_reader_fn reader;
/* If no read list is present, return 0 */
ch = svc_rdma_get_read_chunk(rmsgp);
@@ -408,122 +368,53 @@ static int rdma_read_xdr(struct svcxprt_rdma *xprt,
if (ch_count > RPCSVC_MAXPAGES)
return -EINVAL;
I'm not sure what this ^^^ ch_count check is doing, but maybe I haven't had
enough caffeine this morning. Shouldn't this code be comparing the segment
count, not the chunk count, with MAXPAGES?
Isn't ch_count really the number of chunks in the RPC?
Sort of. Block comment in front of svc_rdma_rcl_chunk_counts() reads:
74 * Determine number of chunks and total bytes in chunk list. The chunk
75 * list has already been verified to fit within the RPCRDMA header.
So, called from rdma_read_chunks(), ch_count is the number of chunks in
the Read list.
As you point out below, NFS/RDMA passes either a Read or a Write list,
not both. So it amounts to the same thing as the total number of chunks
in the RPC.
I'm not sure what "segment count” you are talking about?
AFAICT a chunk is a list of segments. Thus one chunk can reference
many pages, one per segment.
Ok yes. rdma_read_chunk_frmr()/rdma_read_chunk_lcl() walk the segments
creating rdma read work requests to pull the data for this chunk.
If you print ch_count, it is 2 for NFS WRITEs from a Linux client,
no matter how large the write payload is. Therefore I think the check
as it is written is not particularly useful.
Why are there 2?
The returned ch_count here is not used beyond that check, after the
refactoring patch is applied. The returned byte_count includes all
chunks in the passed-in chunk list, whereas I think it should count
only the first chunk in the list.
So I'll investigate this more with the goal of only grabbing the first
chunk.
- /* Allocate temporary reply and chunk maps */
- rpl_map = svc_rdma_get_req_map();
- chl_map = svc_rdma_get_req_map();
-
- if (!xprt->sc_frmr_pg_list_len)
- sge_count = map_read_chunks(xprt, rqstp, hdr_ctxt, rmsgp,
- rpl_map, chl_map, ch_count,
- byte_count);
- else
- sge_count = fast_reg_read_chunks(xprt, rqstp, hdr_ctxt, rmsgp,
- rpl_map, chl_map, ch_count,
- byte_count);
- if (sge_count < 0) {
- err = -EIO;
- goto out;
- }
-
- sgl_offset = 0;
- ch_no = 0;
+ /* The request is completed when the RDMA_READs complete. The
+ * head context keeps all the pages that comprise the
+ * request.
+ */
+ head->arg.head[0] = rqstp->rq_arg.head[0];
+ head->arg.tail[0] = rqstp->rq_arg.tail[0];
+ head->arg.pages = &head->pages[head->count];
+ head->hdr_count = head->count;
+ head->arg.page_base = 0;
+ head->arg.page_len = 0;
+ head->arg.len = rqstp->rq_arg.len;
+ head->arg.buflen = rqstp->rq_arg.buflen + PAGE_SIZE;
+ page_no = 0; page_offset = 0;
for (ch = (struct rpcrdma_read_chunk *)&rmsgp->rm_body.rm_chunks[0];
- ch->rc_discrim != 0; ch++, ch_no++) {
- u64 rs_offset;
-next_sge:
- ctxt = svc_rdma_get_context(xprt);
- ctxt->direction = DMA_FROM_DEVICE;
- ctxt->frmr = hdr_ctxt->frmr;
- ctxt->read_hdr = NULL;
- clear_bit(RDMACTXT_F_LAST_CTXT, &ctxt->flags);
- clear_bit(RDMACTXT_F_FAST_UNREG, &ctxt->flags);
+ ch->rc_discrim != 0; ch++) {
As I understand it, this loop is iterating over the items in the RPC
header's read chunk list.
RFC 5667 section 4 says
"The server MUST ignore any Read list for other NFS procedures, as well
as additional Read list entries beyond the first in the list."
I interpret this to say the server should ignore an RDMA Write list, if present, when
processing an NFS WRITE (because the server emits RDMA Reads to the client in this case).
Similarly, it should ignore an RDMA Read list when processing an NFS READ (which generates
RDMA Writes from the server->client to fulfill the NFS READ).
That’s already stated explicitly in a later paragraph. So this text
is getting at something else, IMO. Here’s the full context:
4. NFS Versions 2 and 3 Mapping
A single RDMA Write list entry MAY be posted by the client to receive
either the opaque file data from a READ request or the pathname from
a READLINK request. The server MUST ignore a Write list for any
other NFS procedure, as well as any Write list entries beyond the
first in the list.
Similarly, a single RDMA Read list entry MAY be posted by the client
to supply the opaque file data for a WRITE request or the pathname
for a SYMLINK request. The server MUST ignore any Read list for
other NFS procedures, as well as additional Read list entries beyond
the first in the list.
Because there are no NFS version 2 or 3 requests that transfer bulk
data in both directions, it is not necessary to post requests
containing both Write and Read lists. Any unneeded Read or Write
lists are ignored by the server.
If there is a Read list, it can contain one chunk for NFS WRITE and
SYMLINK operations, otherwise it should contain zero chunks. Anything
else MUST be ignored (ie, the server mustn’t process the additional
chunks, and mustn’t throw an error).
Processing the second chunk from the client during NFS WRITE operations
is incorrect. The additional chunk should be ignored, and the first
chunk should be padded on the server to make things work right in the
upper layer (the server’s NFS WRITE XDR decoder).
Ok. Padding is another issue, and I'd like to defer it until we get
this refactor patch merged.
But rdma_read_chunks() still expects multiple chunks in the incoming list.
An RDMA Read list can have multiple chunks, yes?
It can. But RFC 5667 suggests that particularly for NFS requests, only
a Read list of zero or one chunks is valid.
Should this code follow the RFC and process only the first chunk in
each RPC header, or did I misunderstand the RFC text?
I'm not sure how the RPC layer should enforce the requirement to ignore
chunks for particular classes of NFS requests. But seems like valid
NFS/RDMA requests will have only zero or one item in each chunk list.
The ramification of this change:
The Linux client sends an extra chunk which is a zero pad to satisfy XDR
alignment. This results in an extra RDMA READ on the wire for a payload
that is never more than 3 bytes of zeros.
This sounds like a logic bug in the server then.
Perhaps it’s simply that this code was designed before RFC 5667 was
published. Certainly the client expects the server might work this way.
I don't think so. Just never implemented correctly.
You see these IOs on the wire?
I see extra iterations in the RDMA READ loop on the server, both before
and after the refactoring patch.
I haven’t looked at the wire because ib_dump doesn’t work with the mthca
provider.
Section 3.7 of RFC 5666 states:
"If in turn, no data remains to be decoded from the inline portion, then
the receiver MUST conclude that roundup is present, and therefore it
advances the XDR decode position to that indicated by the next chunk (if
any). In this way, roundup is passed without ever actually transferring
additional XDR bytes."
Later, it states:
"Senders SHOULD therefore avoid encoding individual RDMA Read Chunks for
roundup whenever possible."
The Solaris NFS/RDMA server does not require this extra chunk, but Linux
appears to need it. When I enable pad optimization on the Linux client,
it works fine with the Solaris server. But the first NFS WRITE with an
odd length against the Linux server causes connection loss and the client
workload hangs.
At some point I'd like to permanently enable this optimization on the
Linux client. You can try this out with:
sudo echo 1 > /proc/sys/sunrpc/rdma_pad_optimize
The default contents of this file leave pad optimization disabled, for
compatibility with the current Linux NFS/RDMA server.
So rdma_read_chunks() needs to handle the implicit XDR length round-up.
That's probably not hard.
What I'm asking is:
Do you agree the server should be changed to comply with section 4 of 5667?
I think it is complying and you are misinterpreting. But I could be misinterpreting it
too :)
But we should fix the pad thing.
That may be enough to support a client that does not send the
pad bytes in a separate chunk. However, I wonder how such a
server would behave with the current client.
True, this isn’t a regression, per se. The server is already
behaving incorrectly without the refactoring patch.
However, Tom’s refactoring rewrites all this logic anyway, and
it wouldn’t be much of a stretch to make the server comply with
RFC 5667, at this point.
Can we defer this until the refactor patch is in, then I'll work on a
new patch for the pad stuff.
Thanks,
Steve.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
