On 6 Sep 2019, at 16:50, Chuck Lever wrote:
On Sep 6, 2019, at 4:47 PM, Jason L Tibbitts III <tibbs@xxxxxxxxxxx>
wrote:
"JBF" == J Bruce Fields <bfields@xxxxxxxxxxxx> writes:
JBF> Those readdir changes were client-side, right? Based on that
I'd
JBF> been assuming a client bug, but maybe it'd be worth getting a
full
JBF> packet capture of the readdir reply to make sure it's legit.
I have been working with bcodding on IRC for the past couple of days
on
this. Fortunately I was able to come up with way to fill up a
directory
in such a way that it will fail with certainty and as a bonus doesn't
include any user data so I can feel OK about sharing packet captures.
I
have a capture alongside a kernel trace of the problematic operation
in
https://www.math.uh.edu/~tibbs/nfs/. Not that I can particularly
tell
anything useful from that, but bcodding says that it seems to point
to
some issue in sunrpc.
And because I can easily reproduce this and I was able to do a
bisect:
2c94b8eca1a26cd46010d6e73a23da5f2e93a19d is the first bad commit
commit 2c94b8eca1a26cd46010d6e73a23da5f2e93a19d
Author: Chuck Lever <chuck.lever@xxxxxxxxxx>
Date: Mon Feb 11 11:25:41 2019 -0500
SUNRPC: Use au_rslack when computing reply buffer size
au_rslack is significantly smaller than (au_cslack << 2). Using
that value results in smaller receive buffers. In some cases this
eliminates an extra segment in Reply chunks (RPC/RDMA).
Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
Signed-off-by: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
:040000 040000 d4d1ce2fbe0035c5bd9df976b8c448df85dcb505
7011a792dfe72ff9cd70d66e45d353f3d7817e3e M net
But of course, I can't say whether this is the actual bad commit or
whether it just introduced a behavior change which alters the
conditions
under which the problem appears.
The first place I'd start looking is the XDR constants at the head of
fs/nfs/nfs4xdr.c
having to do with READDIR.
Well, one thing is that I wonder about decode_readdir_maxsz including
decode_verifier_maxsz since it is part of READDIR4resok, and so should
be
included in the page data.. but that really doesn't matter here, I think
we
just end up with a larger head. Moving the start of the tail two words
forward doesn't seem to fix things..
I think the mic's xdr_buf_subsegment is getting partially split between
leftover space in the pages and the tail, so the checks for the length
of
the subbuf do not match the length of mic. In that case, we usually
just
have extra room in the tail to copy it over, but now that rslack is not
so
slack, the only extra room (as determined in xdr_buf_read_netobj())
would be
extra space in the page data, which we can't use.
The problem is that xdr_buf_read_netobj() is using xdr_buf_subsegment()
with
an offset in the response data to the mic, but xdr_buf_subsegement()
decides
what to do based on offsets in the already set-up xdr_buf. If the
server's
response leaves a hole in the page_len less than the mic, then the mic
subsegment straddles the page and the tail. Then we try to copy it to
the
end of the tail, but now there's not enough room.
Instead, I think we want to make sure the mic falls squarely into the
tail
every time.
xdr_buf_read_netobj() is only used for the mic, so it seems like we can
refactor it to just use the tail, or get rid of it altogether. I'll see
if
I can do that.
Read on, if you want some numbers to check my work..
Here's how to reproduce:
Init an xdr_buf of 176, so rq_rcvsize is:
4 RPC_REPHDRSIZE
19 auth->au_rslack
21 .p_replen
-----
44<<2 = 176
Then xdr_inline_pages() with:
- offset 140:
21 resp.hdr
4 RPC_REPHDRSIZE
11 auth->au_ralign
-1 xdr pad fixup
-----
35<<2 = 140
- base of 88 (that's static entries for . and ..)
- len of 262056 (64 pages - base of 88)
This gives us the following xdr_buf:
struct xdr_buf {
head->iov_len = 140,
tail->iov_len = 32,
page_len = 262056,
buflen = 262232
}
Then, get a READDIR response of total length 232208:
56 bytes to GSS data
76 bytes to READDIR4resok
262044 bytes of READDIR4resok
32 bytes of mic
That puts the mic at offset 262176.
That all looks right, and the response is well-formed.
Things go badly in xdr_read_netobj() with offset 262176:
We return -ENOMEM when
obj->len (28) is > than buf->buflen (262232) - buf->len (262208)
Ben
