| Message ID | 20201118221939.20715-3-trondmy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] NFSv4: Fix the alignment of page data in the getdeviceinfo reply | expand |
> On Nov 18, 2020, at 5:19 PM, trondmy@kernel.org wrote: > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > When doing a read() into a page, we also don't care if the nul padding > stays in that last page when the data length is not 32-bit aligned. What if the READ payload lands in the middle of a file? The pad on the end will overwrite file content just past where the READ payload lands. > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > --- > fs/nfs/nfs2xdr.c | 2 +- > fs/nfs/nfs3xdr.c | 2 +- > fs/nfs/nfs4xdr.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c > index db9c265ad9e1..468bfbfe44d7 100644 > --- a/fs/nfs/nfs2xdr.c > +++ b/fs/nfs/nfs2xdr.c > @@ -102,7 +102,7 @@ static int decode_nfsdata(struct xdr_stream *xdr, struct nfs_pgio_res *result) > if (unlikely(!p)) > return -EIO; > count = be32_to_cpup(p); > - recvd = xdr_read_pages(xdr, count); > + recvd = xdr_read_pages(xdr, xdr_align_size(count)); > if (unlikely(count > recvd)) > goto out_cheating; > out: > diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c > index d3e1726d538b..8ef7c961d3e2 100644 > --- a/fs/nfs/nfs3xdr.c > +++ b/fs/nfs/nfs3xdr.c > @@ -1611,7 +1611,7 @@ static int decode_read3resok(struct xdr_stream *xdr, > ocount = be32_to_cpup(p++); > if (unlikely(ocount != count)) > goto out_mismatch; > - recvd = xdr_read_pages(xdr, count); > + recvd = xdr_read_pages(xdr, xdr_align_size(count)); > if (unlikely(count > recvd)) > goto out_cheating; > out: > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > index 755b556e85c3..5baa767106dc 100644 > --- a/fs/nfs/nfs4xdr.c > +++ b/fs/nfs/nfs4xdr.c > @@ -5202,7 +5202,7 @@ static int decode_read(struct xdr_stream *xdr, struct rpc_rqst *req, > return -EIO; > eof = be32_to_cpup(p++); > count = be32_to_cpup(p); > - recvd = xdr_read_pages(xdr, count); > + recvd = xdr_read_pages(xdr, xdr_align_size(count)); > if (count > recvd) { > dprintk("NFS: server cheating in read reply: " > "count %u > recvd %u\n", count, recvd); > -- > 2.28.0 > -- Chuck Lever
On Thu, 2020-11-19 at 09:17 -0500, Chuck Lever wrote: > > > > On Nov 18, 2020, at 5:19 PM, trondmy@kernel.org wrote: > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > When doing a read() into a page, we also don't care if the nul > > padding > > stays in that last page when the data length is not 32-bit aligned. > > What if the READ payload lands in the middle of a file? The > pad on the end will overwrite file content just past where > the READ payload lands. If the size > buf->page_len, then it gets truncated in xdr_align_pages() afaik. > > > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > > --- > > fs/nfs/nfs2xdr.c | 2 +- > > fs/nfs/nfs3xdr.c | 2 +- > > fs/nfs/nfs4xdr.c | 2 +- > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c > > index db9c265ad9e1..468bfbfe44d7 100644 > > --- a/fs/nfs/nfs2xdr.c > > +++ b/fs/nfs/nfs2xdr.c > > @@ -102,7 +102,7 @@ static int decode_nfsdata(struct xdr_stream > > *xdr, struct nfs_pgio_res *result) > > if (unlikely(!p)) > > return -EIO; > > count = be32_to_cpup(p); > > - recvd = xdr_read_pages(xdr, count); > > + recvd = xdr_read_pages(xdr, xdr_align_size(count)); > > if (unlikely(count > recvd)) > > goto out_cheating; > > out: > > diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c > > index d3e1726d538b..8ef7c961d3e2 100644 > > --- a/fs/nfs/nfs3xdr.c > > +++ b/fs/nfs/nfs3xdr.c > > @@ -1611,7 +1611,7 @@ static int decode_read3resok(struct > > xdr_stream *xdr, > > ocount = be32_to_cpup(p++); > > if (unlikely(ocount != count)) > > goto out_mismatch; > > - recvd = xdr_read_pages(xdr, count); > > + recvd = xdr_read_pages(xdr, xdr_align_size(count)); > > if (unlikely(count > recvd)) > > goto out_cheating; > > out: > > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > > index 755b556e85c3..5baa767106dc 100644 > > --- a/fs/nfs/nfs4xdr.c > > +++ b/fs/nfs/nfs4xdr.c > > @@ -5202,7 +5202,7 @@ static int decode_read(struct xdr_stream > > *xdr, struct rpc_rqst *req, > > return -EIO; > > eof = be32_to_cpup(p++); > > count = be32_to_cpup(p); > > - recvd = xdr_read_pages(xdr, count); > > + recvd = xdr_read_pages(xdr, xdr_align_size(count)); > > if (count > recvd) { > > dprintk("NFS: server cheating in read reply: " > > "count %u > recvd %u\n", count, > > recvd); > > -- > > 2.28.0 > > > > -- > Chuck Lever > > >
> On Nov 19, 2020, at 9:30 AM, Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Thu, 2020-11-19 at 09:17 -0500, Chuck Lever wrote: >> >> >>> On Nov 18, 2020, at 5:19 PM, trondmy@kernel.org wrote: >>> >>> From: Trond Myklebust <trond.myklebust@hammerspace.com> >>> >>> When doing a read() into a page, we also don't care if the nul >>> padding >>> stays in that last page when the data length is not 32-bit aligned. >> >> What if the READ payload lands in the middle of a file? The >> pad on the end will overwrite file content just past where >> the READ payload lands. > > If the size > buf->page_len, then it gets truncated in > xdr_align_pages() afaik. I will need to check how RPC/RDMA behaves. It might build a chunk that includes the pad in this case, which would break things. >>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> >>> --- >>> fs/nfs/nfs2xdr.c | 2 +- >>> fs/nfs/nfs3xdr.c | 2 +- >>> fs/nfs/nfs4xdr.c | 2 +- >>> 3 files changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c >>> index db9c265ad9e1..468bfbfe44d7 100644 >>> --- a/fs/nfs/nfs2xdr.c >>> +++ b/fs/nfs/nfs2xdr.c >>> @@ -102,7 +102,7 @@ static int decode_nfsdata(struct xdr_stream >>> *xdr, struct nfs_pgio_res *result) >>> if (unlikely(!p)) >>> return -EIO; >>> count = be32_to_cpup(p); >>> - recvd = xdr_read_pages(xdr, count); >>> + recvd = xdr_read_pages(xdr, xdr_align_size(count)); >>> if (unlikely(count > recvd)) >>> goto out_cheating; >>> out: >>> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c >>> index d3e1726d538b..8ef7c961d3e2 100644 >>> --- a/fs/nfs/nfs3xdr.c >>> +++ b/fs/nfs/nfs3xdr.c >>> @@ -1611,7 +1611,7 @@ static int decode_read3resok(struct >>> xdr_stream *xdr, >>> ocount = be32_to_cpup(p++); >>> if (unlikely(ocount != count)) >>> goto out_mismatch; >>> - recvd = xdr_read_pages(xdr, count); >>> + recvd = xdr_read_pages(xdr, xdr_align_size(count)); >>> if (unlikely(count > recvd)) >>> goto out_cheating; >>> out: >>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c >>> index 755b556e85c3..5baa767106dc 100644 >>> --- a/fs/nfs/nfs4xdr.c >>> +++ b/fs/nfs/nfs4xdr.c >>> @@ -5202,7 +5202,7 @@ static int decode_read(struct xdr_stream >>> *xdr, struct rpc_rqst *req, >>> return -EIO; >>> eof = be32_to_cpup(p++); >>> count = be32_to_cpup(p); >>> - recvd = xdr_read_pages(xdr, count); >>> + recvd = xdr_read_pages(xdr, xdr_align_size(count)); >>> if (count > recvd) { >>> dprintk("NFS: server cheating in read reply: " >>> "count %u > recvd %u\n", count, >>> recvd); >>> -- >>> 2.28.0 >>> >> >> -- >> Chuck Lever >> >> >> > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com -- Chuck Lever
On Thu, 2020-11-19 at 09:31 -0500, Chuck Lever wrote: > > > > On Nov 19, 2020, at 9:30 AM, Trond Myklebust < > > trondmy@hammerspace.com> wrote: > > > > On Thu, 2020-11-19 at 09:17 -0500, Chuck Lever wrote: > > > > > > > > > > On Nov 18, 2020, at 5:19 PM, trondmy@kernel.org wrote: > > > > > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > > > > > When doing a read() into a page, we also don't care if the nul > > > > padding > > > > stays in that last page when the data length is not 32-bit > > > > aligned. > > > > > > What if the READ payload lands in the middle of a file? The > > > pad on the end will overwrite file content just past where > > > the READ payload lands. > > > > If the size > buf->page_len, then it gets truncated in > > xdr_align_pages() afaik. > > I will need to check how RPC/RDMA behaves. It might build a > chunk that includes the pad in this case, which would break > things. That would be a bug in the existing code too, then. It shouldn't be writing beyond the buffer size we set in the NFS layer. > > > > Signed-off-by: Trond Myklebust > > > > <trond.myklebust@hammerspace.com> > > > > --- > > > > fs/nfs/nfs2xdr.c | 2 +- > > > > fs/nfs/nfs3xdr.c | 2 +- > > > > fs/nfs/nfs4xdr.c | 2 +- > > > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c > > > > index db9c265ad9e1..468bfbfe44d7 100644 > > > > --- a/fs/nfs/nfs2xdr.c > > > > +++ b/fs/nfs/nfs2xdr.c > > > > @@ -102,7 +102,7 @@ static int decode_nfsdata(struct xdr_stream > > > > *xdr, struct nfs_pgio_res *result) > > > > if (unlikely(!p)) > > > > return -EIO; > > > > count = be32_to_cpup(p); > > > > - recvd = xdr_read_pages(xdr, count); > > > > + recvd = xdr_read_pages(xdr, xdr_align_size(count)); > > > > if (unlikely(count > recvd)) > > > > goto out_cheating; > > > > out: > > > > diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c > > > > index d3e1726d538b..8ef7c961d3e2 100644 > > > > --- a/fs/nfs/nfs3xdr.c > > > > +++ b/fs/nfs/nfs3xdr.c > > > > @@ -1611,7 +1611,7 @@ static int decode_read3resok(struct > > > > xdr_stream *xdr, > > > > ocount = be32_to_cpup(p++); > > > > if (unlikely(ocount != count)) > > > > goto out_mismatch; > > > > - recvd = xdr_read_pages(xdr, count); > > > > + recvd = xdr_read_pages(xdr, xdr_align_size(count)); > > > > if (unlikely(count > recvd)) > > > > goto out_cheating; > > > > out: > > > > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > > > > index 755b556e85c3..5baa767106dc 100644 > > > > --- a/fs/nfs/nfs4xdr.c > > > > +++ b/fs/nfs/nfs4xdr.c > > > > @@ -5202,7 +5202,7 @@ static int decode_read(struct xdr_stream > > > > *xdr, struct rpc_rqst *req, > > > > return -EIO; > > > > eof = be32_to_cpup(p++); > > > > count = be32_to_cpup(p); > > > > - recvd = xdr_read_pages(xdr, count); > > > > + recvd = xdr_read_pages(xdr, xdr_align_size(count)); > > > > if (count > recvd) { > > > > dprintk("NFS: server cheating in read reply: " > > > > "count %u > recvd %u\n", count, > > > > recvd); > > > > -- > > > > 2.28.0
> On Nov 19, 2020, at 9:34 AM, Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Thu, 2020-11-19 at 09:31 -0500, Chuck Lever wrote: >> >> >>> On Nov 19, 2020, at 9:30 AM, Trond Myklebust < >>> trondmy@hammerspace.com> wrote: >>> >>> On Thu, 2020-11-19 at 09:17 -0500, Chuck Lever wrote: >>>> >>>> >>>>> On Nov 18, 2020, at 5:19 PM, trondmy@kernel.org wrote: >>>>> >>>>> From: Trond Myklebust <trond.myklebust@hammerspace.com> >>>>> >>>>> When doing a read() into a page, we also don't care if the nul >>>>> padding >>>>> stays in that last page when the data length is not 32-bit >>>>> aligned. >>>> >>>> What if the READ payload lands in the middle of a file? The >>>> pad on the end will overwrite file content just past where >>>> the READ payload lands. >>> >>> If the size > buf->page_len, then it gets truncated in >>> xdr_align_pages() afaik. >> >> I will need to check how RPC/RDMA behaves. It might build a >> chunk that includes the pad in this case, which would break >> things. > > That would be a bug in the existing code too, then. It shouldn't be > writing beyond the buffer size we set in the NFS layer. Testing now with xfstests, which should include fsx with direct I/O of odd sizes. So far I haven't seen any unexpected behavior. But I'm not sure what copy you're trying to avoid. This one in xdr_align_pages() ? 1189 else if (nwords < xdr->nwords) { 1190 /* Truncate page data and move it into the tail */ 1191 offset = buf->page_len - len; 1192 copied = xdr_shrink_pagelen(buf, offset); 1193 trace_rpc_xdr_alignment(xdr, offset, copied); 1194 xdr->nwords = XDR_QUADLEN(buf->len - cur); 1195 } We set up the receive buffer already to avoid this copy. It should rarely, if ever, happen. That's the point of rpc_prepare_reply_pages(). >>>>> Signed-off-by: Trond Myklebust >>>>> <trond.myklebust@hammerspace.com> >>>>> --- >>>>> fs/nfs/nfs2xdr.c | 2 +- >>>>> fs/nfs/nfs3xdr.c | 2 +- >>>>> fs/nfs/nfs4xdr.c | 2 +- >>>>> 3 files changed, 3 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c >>>>> index db9c265ad9e1..468bfbfe44d7 100644 >>>>> --- a/fs/nfs/nfs2xdr.c >>>>> +++ b/fs/nfs/nfs2xdr.c >>>>> @@ -102,7 +102,7 @@ static int decode_nfsdata(struct xdr_stream >>>>> *xdr, struct nfs_pgio_res *result) >>>>> if (unlikely(!p)) >>>>> return -EIO; >>>>> count = be32_to_cpup(p); >>>>> - recvd = xdr_read_pages(xdr, count); >>>>> + recvd = xdr_read_pages(xdr, xdr_align_size(count)); >>>>> if (unlikely(count > recvd)) >>>>> goto out_cheating; >>>>> out: >>>>> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c >>>>> index d3e1726d538b..8ef7c961d3e2 100644 >>>>> --- a/fs/nfs/nfs3xdr.c >>>>> +++ b/fs/nfs/nfs3xdr.c >>>>> @@ -1611,7 +1611,7 @@ static int decode_read3resok(struct >>>>> xdr_stream *xdr, >>>>> ocount = be32_to_cpup(p++); >>>>> if (unlikely(ocount != count)) >>>>> goto out_mismatch; >>>>> - recvd = xdr_read_pages(xdr, count); >>>>> + recvd = xdr_read_pages(xdr, xdr_align_size(count)); >>>>> if (unlikely(count > recvd)) >>>>> goto out_cheating; >>>>> out: >>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c >>>>> index 755b556e85c3..5baa767106dc 100644 >>>>> --- a/fs/nfs/nfs4xdr.c >>>>> +++ b/fs/nfs/nfs4xdr.c >>>>> @@ -5202,7 +5202,7 @@ static int decode_read(struct xdr_stream >>>>> *xdr, struct rpc_rqst *req, >>>>> return -EIO; >>>>> eof = be32_to_cpup(p++); >>>>> count = be32_to_cpup(p); >>>>> - recvd = xdr_read_pages(xdr, count); >>>>> + recvd = xdr_read_pages(xdr, xdr_align_size(count)); >>>>> if (count > recvd) { >>>>> dprintk("NFS: server cheating in read reply: " >>>>> "count %u > recvd %u\n", count, >>>>> recvd); >>>>> -- >>>>> 2.28.0 > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > > -- Chuck Lever
On Thu, 2020-11-19 at 17:58 -0500, Chuck Lever wrote: > > > > On Nov 19, 2020, at 9:34 AM, Trond Myklebust < > > trondmy@hammerspace.com> wrote: > > > > On Thu, 2020-11-19 at 09:31 -0500, Chuck Lever wrote: > > > > > > > > > > On Nov 19, 2020, at 9:30 AM, Trond Myklebust < > > > > trondmy@hammerspace.com> wrote: > > > > > > > > On Thu, 2020-11-19 at 09:17 -0500, Chuck Lever wrote: > > > > > > > > > > > > > > > > On Nov 18, 2020, at 5:19 PM, trondmy@kernel.org wrote: > > > > > > > > > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > > > > > > > > > When doing a read() into a page, we also don't care if the > > > > > > nul > > > > > > padding > > > > > > stays in that last page when the data length is not 32-bit > > > > > > aligned. > > > > > > > > > > What if the READ payload lands in the middle of a file? The > > > > > pad on the end will overwrite file content just past where > > > > > the READ payload lands. > > > > > > > > If the size > buf->page_len, then it gets truncated in > > > > xdr_align_pages() afaik. > > > > > > I will need to check how RPC/RDMA behaves. It might build a > > > chunk that includes the pad in this case, which would break > > > things. > > > > That would be a bug in the existing code too, then. It shouldn't be > > writing beyond the buffer size we set in the NFS layer. > > Testing now with xfstests, which should include fsx with direct > I/O of odd sizes. So far I haven't seen any unexpected behavior. > > But I'm not sure what copy you're trying to avoid. This one in > xdr_align_pages() ? > > 1189 else if (nwords < xdr->nwords) { > 1190 /* Truncate page data and move it into the tail > */ > 1191 offset = buf->page_len - len; > 1192 copied = xdr_shrink_pagelen(buf, offset); > 1193 trace_rpc_xdr_alignment(xdr, offset, copied); > 1194 xdr->nwords = XDR_QUADLEN(buf->len - cur); > 1195 } > > We set up the receive buffer already to avoid this copy. It should > rarely, if ever, happen. That's the point of > rpc_prepare_reply_pages(). ...and the point of padding here is to avoid unaligned access to memory. That is completely broken by this whole mechanism, which causes us to place the real data in the tail in an unaligned buffer that follows this padding. Furthermore, rpc_prepare_reply_pages() only ever places the padding in the tail _if_ our buffer size is already not 32-bit aligned. Otherwise, we're engaging in this pointless exercise of making the tail buffer data unaligned after the fact. > > > > > > > Signed-off-by: Trond Myklebust > > > > > > <trond.myklebust@hammerspace.com> > > > > > > --- > > > > > > fs/nfs/nfs2xdr.c | 2 +- > > > > > > fs/nfs/nfs3xdr.c | 2 +- > > > > > > fs/nfs/nfs4xdr.c | 2 +- > > > > > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c > > > > > > index db9c265ad9e1..468bfbfe44d7 100644 > > > > > > --- a/fs/nfs/nfs2xdr.c > > > > > > +++ b/fs/nfs/nfs2xdr.c > > > > > > @@ -102,7 +102,7 @@ static int decode_nfsdata(struct > > > > > > xdr_stream > > > > > > *xdr, struct nfs_pgio_res *result) > > > > > > if (unlikely(!p)) > > > > > > return -EIO; > > > > > > count = be32_to_cpup(p); > > > > > > - recvd = xdr_read_pages(xdr, count); > > > > > > + recvd = xdr_read_pages(xdr, xdr_align_size(count)); > > > > > > if (unlikely(count > recvd)) > > > > > > goto out_cheating; > > > > > > out: > > > > > > diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c > > > > > > index d3e1726d538b..8ef7c961d3e2 100644 > > > > > > --- a/fs/nfs/nfs3xdr.c > > > > > > +++ b/fs/nfs/nfs3xdr.c > > > > > > @@ -1611,7 +1611,7 @@ static int decode_read3resok(struct > > > > > > xdr_stream *xdr, > > > > > > ocount = be32_to_cpup(p++); > > > > > > if (unlikely(ocount != count)) > > > > > > goto out_mismatch; > > > > > > - recvd = xdr_read_pages(xdr, count); > > > > > > + recvd = xdr_read_pages(xdr, xdr_align_size(count)); > > > > > > if (unlikely(count > recvd)) > > > > > > goto out_cheating; > > > > > > out: > > > > > > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > > > > > > index 755b556e85c3..5baa767106dc 100644 > > > > > > --- a/fs/nfs/nfs4xdr.c > > > > > > +++ b/fs/nfs/nfs4xdr.c > > > > > > @@ -5202,7 +5202,7 @@ static int decode_read(struct > > > > > > xdr_stream > > > > > > *xdr, struct rpc_rqst *req, > > > > > > return -EIO; > > > > > > eof = be32_to_cpup(p++); > > > > > > count = be32_to_cpup(p); > > > > > > - recvd = xdr_read_pages(xdr, count); > > > > > > + recvd = xdr_read_pages(xdr, xdr_align_size(count)); > > > > > > if (count > recvd) { > > > > > > dprintk("NFS: server cheating in read > > > > > > reply: " > > > > > > "count %u > recvd %u\n", > > > > > > count, > > > > > > recvd); > > > > > > -- > > > > > > 2.28.0 > > > > -- > > Trond Myklebust > > Linux NFS client maintainer, Hammerspace > > trond.myklebust@hammerspace.com > > > > > > -- > Chuck Lever > > >
> On Nov 19, 2020, at 7:58 PM, Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Thu, 2020-11-19 at 17:58 -0500, Chuck Lever wrote: >> >> >>> On Nov 19, 2020, at 9:34 AM, Trond Myklebust < >>> trondmy@hammerspace.com> wrote: >>> >>> On Thu, 2020-11-19 at 09:31 -0500, Chuck Lever wrote: >>>> >>>> >>>>> On Nov 19, 2020, at 9:30 AM, Trond Myklebust < >>>>> trondmy@hammerspace.com> wrote: >>>>> >>>>> On Thu, 2020-11-19 at 09:17 -0500, Chuck Lever wrote: >>>>>> >>>>>> >>>>>>> On Nov 18, 2020, at 5:19 PM, trondmy@kernel.org wrote: >>>>>>> >>>>>>> From: Trond Myklebust <trond.myklebust@hammerspace.com> >>>>>>> >>>>>>> When doing a read() into a page, we also don't care if the >>>>>>> nul >>>>>>> padding >>>>>>> stays in that last page when the data length is not 32-bit >>>>>>> aligned. >>>>>> >>>>>> What if the READ payload lands in the middle of a file? The >>>>>> pad on the end will overwrite file content just past where >>>>>> the READ payload lands. >>>>> >>>>> If the size > buf->page_len, then it gets truncated in >>>>> xdr_align_pages() afaik. >>>> >>>> I will need to check how RPC/RDMA behaves. It might build a >>>> chunk that includes the pad in this case, which would break >>>> things. >>> >>> That would be a bug in the existing code too, then. It shouldn't be >>> writing beyond the buffer size we set in the NFS layer. >> >> Testing now with xfstests, which should include fsx with direct >> I/O of odd sizes. So far I haven't seen any unexpected behavior. >> >> But I'm not sure what copy you're trying to avoid. This one in >> xdr_align_pages() ? >> >> 1189 else if (nwords < xdr->nwords) { >> 1190 /* Truncate page data and move it into the tail >> */ >> 1191 offset = buf->page_len - len; >> 1192 copied = xdr_shrink_pagelen(buf, offset); >> 1193 trace_rpc_xdr_alignment(xdr, offset, copied); >> 1194 xdr->nwords = XDR_QUADLEN(buf->len - cur); >> 1195 } >> >> We set up the receive buffer already to avoid this copy. It should >> rarely, if ever, happen. That's the point of >> rpc_prepare_reply_pages(). > > > ...and the point of padding here is to avoid unaligned access to > memory. That is completely broken by this whole mechanism, which causes > us to place the real data in the tail in an unaligned buffer that > follows this padding. I've never seen run-time complaints about unaligned accesses of the tail data. Do you have a reproducer? (And obviously this applies only to NFSv4 COMPOUND results, right?) > Furthermore, rpc_prepare_reply_pages() only ever places the padding in > the tail _if_ our buffer size is already not 32-bit aligned. Otherwise, > we're engaging in this pointless exercise of making the tail buffer > data unaligned after the fact. Architecturally, I agree that it would be best if the tail buffer presented the XDR data items aligned on 4 bytes. But I do not agree that a pad goes in the pages. Some transports do not send an XDR pad for unaligned data payloads. Transports have to have a way of avoiding that pad. If it no longer goes in the tail, then how will they do that? We could perhaps have a new flag, XDRBUF_IMPLICIT_PAD, that means the transport has to add a pad to xdr->pages on send, or did not add a pad on receive. >>>>>>> Signed-off-by: Trond Myklebust >>>>>>> <trond.myklebust@hammerspace.com> >>>>>>> --- >>>>>>> fs/nfs/nfs2xdr.c | 2 +- >>>>>>> fs/nfs/nfs3xdr.c | 2 +- >>>>>>> fs/nfs/nfs4xdr.c | 2 +- >>>>>>> 3 files changed, 3 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c >>>>>>> index db9c265ad9e1..468bfbfe44d7 100644 >>>>>>> --- a/fs/nfs/nfs2xdr.c >>>>>>> +++ b/fs/nfs/nfs2xdr.c >>>>>>> @@ -102,7 +102,7 @@ static int decode_nfsdata(struct >>>>>>> xdr_stream >>>>>>> *xdr, struct nfs_pgio_res *result) >>>>>>> if (unlikely(!p)) >>>>>>> return -EIO; >>>>>>> count = be32_to_cpup(p); >>>>>>> - recvd = xdr_read_pages(xdr, count); >>>>>>> + recvd = xdr_read_pages(xdr, xdr_align_size(count)); >>>>>>> if (unlikely(count > recvd)) >>>>>>> goto out_cheating; >>>>>>> out: >>>>>>> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c >>>>>>> index d3e1726d538b..8ef7c961d3e2 100644 >>>>>>> --- a/fs/nfs/nfs3xdr.c >>>>>>> +++ b/fs/nfs/nfs3xdr.c >>>>>>> @@ -1611,7 +1611,7 @@ static int decode_read3resok(struct >>>>>>> xdr_stream *xdr, >>>>>>> ocount = be32_to_cpup(p++); >>>>>>> if (unlikely(ocount != count)) >>>>>>> goto out_mismatch; >>>>>>> - recvd = xdr_read_pages(xdr, count); >>>>>>> + recvd = xdr_read_pages(xdr, xdr_align_size(count)); >>>>>>> if (unlikely(count > recvd)) >>>>>>> goto out_cheating; >>>>>>> out: >>>>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c >>>>>>> index 755b556e85c3..5baa767106dc 100644 >>>>>>> --- a/fs/nfs/nfs4xdr.c >>>>>>> +++ b/fs/nfs/nfs4xdr.c >>>>>>> @@ -5202,7 +5202,7 @@ static int decode_read(struct >>>>>>> xdr_stream >>>>>>> *xdr, struct rpc_rqst *req, >>>>>>> return -EIO; >>>>>>> eof = be32_to_cpup(p++); >>>>>>> count = be32_to_cpup(p); >>>>>>> - recvd = xdr_read_pages(xdr, count); >>>>>>> + recvd = xdr_read_pages(xdr, xdr_align_size(count)); >>>>>>> if (count > recvd) { >>>>>>> dprintk("NFS: server cheating in read >>>>>>> reply: " >>>>>>> "count %u > recvd %u\n", >>>>>>> count, >>>>>>> recvd); >>>>>>> -- >>>>>>> 2.28.0 >>> >>> -- >>> Trond Myklebust >>> Linux NFS client maintainer, Hammerspace >>> trond.myklebust@hammerspace.com >>> >>> >> >> -- >> Chuck Lever >> >> >> > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com -- Chuck Lever
On Fri, 2020-11-20 at 09:52 -0500, Chuck Lever wrote: > > > > On Nov 19, 2020, at 7:58 PM, Trond Myklebust < > > trondmy@hammerspace.com> wrote: > > > > On Thu, 2020-11-19 at 17:58 -0500, Chuck Lever wrote: > > > > > > > > > > On Nov 19, 2020, at 9:34 AM, Trond Myklebust < > > > > trondmy@hammerspace.com> wrote: > > > > > > > > On Thu, 2020-11-19 at 09:31 -0500, Chuck Lever wrote: > > > > > > > > > > > > > > > > On Nov 19, 2020, at 9:30 AM, Trond Myklebust < > > > > > > trondmy@hammerspace.com> wrote: > > > > > > > > > > > > On Thu, 2020-11-19 at 09:17 -0500, Chuck Lever wrote: > > > > > > > > > > > > > > > > > > > > > > On Nov 18, 2020, at 5:19 PM, trondmy@kernel.org wrote: > > > > > > > > > > > > > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > > > > > > > > > > > > > When doing a read() into a page, we also don't care if > > > > > > > > the > > > > > > > > nul > > > > > > > > padding > > > > > > > > stays in that last page when the data length is not 32- > > > > > > > > bit > > > > > > > > aligned. > > > > > > > > > > > > > > What if the READ payload lands in the middle of a file? > > > > > > > The > > > > > > > pad on the end will overwrite file content just past > > > > > > > where > > > > > > > the READ payload lands. > > > > > > > > > > > > If the size > buf->page_len, then it gets truncated in > > > > > > xdr_align_pages() afaik. > > > > > > > > > > I will need to check how RPC/RDMA behaves. It might build a > > > > > chunk that includes the pad in this case, which would break > > > > > things. > > > > > > > > That would be a bug in the existing code too, then. It > > > > shouldn't be > > > > writing beyond the buffer size we set in the NFS layer. > > > > > > Testing now with xfstests, which should include fsx with direct > > > I/O of odd sizes. So far I haven't seen any unexpected behavior. > > > > > > But I'm not sure what copy you're trying to avoid. This one in > > > xdr_align_pages() ? > > > > > > 1189 else if (nwords < xdr->nwords) { > > > 1190 /* Truncate page data and move it into the > > > tail > > > */ > > > 1191 offset = buf->page_len - len; > > > 1192 copied = xdr_shrink_pagelen(buf, offset); > > > 1193 trace_rpc_xdr_alignment(xdr, offset, > > > copied); > > > 1194 xdr->nwords = XDR_QUADLEN(buf->len - cur); > > > 1195 } > > > > > > We set up the receive buffer already to avoid this copy. It > > > should > > > rarely, if ever, happen. That's the point of > > > rpc_prepare_reply_pages(). > > > > > > ...and the point of padding here is to avoid unaligned access to > > memory. That is completely broken by this whole mechanism, which > > causes > > us to place the real data in the tail in an unaligned buffer that > > follows this padding. > > I've never seen run-time complaints about unaligned accesses of > the tail data. Do you have a reproducer? (And obviously this > applies only to NFSv4 COMPOUND results, right?) I don't think we can trigger it anywhere right now mainly because we're careful never to put any further ops after a read, readdir or readlink, even though we probably should append a VERIFY to some of those so as to protect data caching (particularly for readlink). > > > Furthermore, rpc_prepare_reply_pages() only ever places the padding > > in > > the tail _if_ our buffer size is already not 32-bit aligned. > > Otherwise, > > we're engaging in this pointless exercise of making the tail buffer > > data unaligned after the fact. > > Architecturally, I agree that it would be best if the tail buffer > presented the XDR data items aligned on 4 bytes. > > But I do not agree that a pad goes in the pages. Some transports do > not send an XDR pad for unaligned data payloads. Transports have to > have a way of avoiding that pad. If it no longer goes in the tail, > then how will they do that? > > We could perhaps have a new flag, XDRBUF_IMPLICIT_PAD, that means > the transport has to add a pad to xdr->pages on send, or did not add > a pad on receive. > I don't see why we would need any of this. If the pad is being directly placed in the tail by the RPC transport layer, then there is nothing in xdr_read_pages() that will move it or change the alignment of the existing code in the tail. The call just notes that xdr_align_pages() returned a length that was shorter than the one we supplied, and continues to skip the padding in the tail. The one thing that perhaps is broken here is that if we supply a length to xdr_read_pages() that is in fact larger than xdr_align_size(xdr- >buf->page_len), then we should skip additional data that was placed in the tail, but that is a bug with the existing xdr_read_pages() and is unaffected by these patches. > > > > > > > > > Signed-off-by: Trond Myklebust > > > > > > > > <trond.myklebust@hammerspace.com> > > > > > > > > --- > > > > > > > > fs/nfs/nfs2xdr.c | 2 +- > > > > > > > > fs/nfs/nfs3xdr.c | 2 +- > > > > > > > > fs/nfs/nfs4xdr.c | 2 +- > > > > > > > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > > > diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c > > > > > > > > index db9c265ad9e1..468bfbfe44d7 100644 > > > > > > > > --- a/fs/nfs/nfs2xdr.c > > > > > > > > +++ b/fs/nfs/nfs2xdr.c > > > > > > > > @@ -102,7 +102,7 @@ static int decode_nfsdata(struct > > > > > > > > xdr_stream > > > > > > > > *xdr, struct nfs_pgio_res *result) > > > > > > > > if (unlikely(!p)) > > > > > > > > return -EIO; > > > > > > > > count = be32_to_cpup(p); > > > > > > > > - recvd = xdr_read_pages(xdr, count); > > > > > > > > + recvd = xdr_read_pages(xdr, > > > > > > > > xdr_align_size(count)); > > > > > > > > if (unlikely(count > recvd)) > > > > > > > > goto out_cheating; > > > > > > > > out: > > > > > > > > diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c > > > > > > > > index d3e1726d538b..8ef7c961d3e2 100644 > > > > > > > > --- a/fs/nfs/nfs3xdr.c > > > > > > > > +++ b/fs/nfs/nfs3xdr.c > > > > > > > > @@ -1611,7 +1611,7 @@ static int > > > > > > > > decode_read3resok(struct > > > > > > > > xdr_stream *xdr, > > > > > > > > ocount = be32_to_cpup(p++); > > > > > > > > if (unlikely(ocount != count)) > > > > > > > > goto out_mismatch; > > > > > > > > - recvd = xdr_read_pages(xdr, count); > > > > > > > > + recvd = xdr_read_pages(xdr, > > > > > > > > xdr_align_size(count)); > > > > > > > > if (unlikely(count > recvd)) > > > > > > > > goto out_cheating; > > > > > > > > out: > > > > > > > > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > > > > > > > > index 755b556e85c3..5baa767106dc 100644 > > > > > > > > --- a/fs/nfs/nfs4xdr.c > > > > > > > > +++ b/fs/nfs/nfs4xdr.c > > > > > > > > @@ -5202,7 +5202,7 @@ static int decode_read(struct > > > > > > > > xdr_stream > > > > > > > > *xdr, struct rpc_rqst *req, > > > > > > > > return -EIO; > > > > > > > > eof = be32_to_cpup(p++); > > > > > > > > count = be32_to_cpup(p); > > > > > > > > - recvd = xdr_read_pages(xdr, count); > > > > > > > > + recvd = xdr_read_pages(xdr, > > > > > > > > xdr_align_size(count)); > > > > > > > > if (count > recvd) { > > > > > > > > dprintk("NFS: server cheating in read > > > > > > > > reply: " > > > > > > > > "count %u > recvd > > > > > > > > %u\n", > > > > > > > > count, > > > > > > > > recvd); > > > > > > > > -- > > > > > > > > 2.28.0 > > > > > > > > -- > > > > Trond Myklebust > > > > Linux NFS client maintainer, Hammerspace > > > > trond.myklebust@hammerspace.com > > > > > > > > > > > > > > -- > > > Chuck Lever > > > > > > > > > > > > > -- > > Trond Myklebust > > Linux NFS client maintainer, Hammerspace > > trond.myklebust@hammerspace.com > > -- > Chuck Lever > > >
> On Nov 20, 2020, at 12:14 PM, Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Fri, 2020-11-20 at 09:52 -0500, Chuck Lever wrote: >> >> >>> On Nov 19, 2020, at 7:58 PM, Trond Myklebust < >>> trondmy@hammerspace.com> wrote: >>> >>> On Thu, 2020-11-19 at 17:58 -0500, Chuck Lever wrote: >>>> >>>> >>>>> On Nov 19, 2020, at 9:34 AM, Trond Myklebust < >>>>> trondmy@hammerspace.com> wrote: >>>>> >>>>> On Thu, 2020-11-19 at 09:31 -0500, Chuck Lever wrote: >>>>>> >>>>>> >>>>>>> On Nov 19, 2020, at 9:30 AM, Trond Myklebust < >>>>>>> trondmy@hammerspace.com> wrote: >>>>>>> >>>>>>> On Thu, 2020-11-19 at 09:17 -0500, Chuck Lever wrote: >>>>>>>> >>>>>>>> >>>>>>>>> On Nov 18, 2020, at 5:19 PM, trondmy@kernel.org wrote: >>>>>>>>> >>>>>>>>> From: Trond Myklebust <trond.myklebust@hammerspace.com> >>>>>>>>> >>>>>>>>> When doing a read() into a page, we also don't care if >>>>>>>>> the >>>>>>>>> nul >>>>>>>>> padding >>>>>>>>> stays in that last page when the data length is not 32- >>>>>>>>> bit >>>>>>>>> aligned. >>>>>>>> >>>>>>>> What if the READ payload lands in the middle of a file? >>>>>>>> The >>>>>>>> pad on the end will overwrite file content just past >>>>>>>> where >>>>>>>> the READ payload lands. >>>>>>> >>>>>>> If the size > buf->page_len, then it gets truncated in >>>>>>> xdr_align_pages() afaik. >>>>>> >>>>>> I will need to check how RPC/RDMA behaves. It might build a >>>>>> chunk that includes the pad in this case, which would break >>>>>> things. >>>>> >>>>> That would be a bug in the existing code too, then. It >>>>> shouldn't be >>>>> writing beyond the buffer size we set in the NFS layer. >>>> >>>> Testing now with xfstests, which should include fsx with direct >>>> I/O of odd sizes. So far I haven't seen any unexpected behavior. >>>> >>>> But I'm not sure what copy you're trying to avoid. This one in >>>> xdr_align_pages() ? >>>> >>>> 1189 else if (nwords < xdr->nwords) { >>>> 1190 /* Truncate page data and move it into the >>>> tail >>>> */ >>>> 1191 offset = buf->page_len - len; >>>> 1192 copied = xdr_shrink_pagelen(buf, offset); >>>> 1193 trace_rpc_xdr_alignment(xdr, offset, >>>> copied); >>>> 1194 xdr->nwords = XDR_QUADLEN(buf->len - cur); >>>> 1195 } >>>> >>>> We set up the receive buffer already to avoid this copy. It >>>> should >>>> rarely, if ever, happen. That's the point of >>>> rpc_prepare_reply_pages(). >>> >>> >>> ...and the point of padding here is to avoid unaligned access to >>> memory. That is completely broken by this whole mechanism, which >>> causes >>> us to place the real data in the tail in an unaligned buffer that >>> follows this padding. >> >> I've never seen run-time complaints about unaligned accesses of >> the tail data. Do you have a reproducer? (And obviously this >> applies only to NFSv4 COMPOUND results, right?) > > I don't think we can trigger it anywhere right now mainly because we're > careful never to put any further ops after a read, readdir or readlink, > even though we probably should append a VERIFY to some of those so as > to protect data caching (particularly for readlink). > >> >>> Furthermore, rpc_prepare_reply_pages() only ever places the padding >>> in >>> the tail _if_ our buffer size is already not 32-bit aligned. >>> Otherwise, >>> we're engaging in this pointless exercise of making the tail buffer >>> data unaligned after the fact. >> >> Architecturally, I agree that it would be best if the tail buffer >> presented the XDR data items aligned on 4 bytes. >> >> But I do not agree that a pad goes in the pages. Some transports do >> not send an XDR pad for unaligned data payloads. Transports have to >> have a way of avoiding that pad. If it no longer goes in the tail, >> then how will they do that? >> >> We could perhaps have a new flag, XDRBUF_IMPLICIT_PAD, that means >> the transport has to add a pad to xdr->pages on send, or did not add >> a pad on receive. >> > > I don't see why we would need any of this. Right. I was conflating the send and receive side. You're talking about only receive. This would be a problem when the structure of the reply doesn't allow the client to align the pages and tail[] in advance. So, READ/READLINK/READDIR/LISTXATTRS, on either TCP, or with RDMA and krb5i and krb5p where XDR pads have to stay inline. I'm still a little troubled by what happens to the pad during a short READ. The common case for READ requests is page-aligned payloads, so I don't think tail content alignment is a performance issue. Do you concur? > If the pad is being directly > placed in the tail by the RPC transport layer, then there is nothing in > xdr_read_pages() that will move it or change the alignment of the > existing code in the tail. The call just notes that xdr_align_pages() > returned a length that was shorter than the one we supplied, and > continues to skip the padding in the tail. That's what I observe today. I'm trying to understand the usage scenario where the tail content is not word-aligned. Probably can happen on server-side receives too. > The one thing that perhaps is broken here is that if we supply a length > to xdr_read_pages() that is in fact larger than xdr_align_size(xdr- >> buf->page_len), then we should skip additional data that was placed in > the tail, but that is a bug with the existing xdr_read_pages() and is > unaffected by these patches. I think I understand what these patches do well enough to drop my objection, pending further exercise with a testing barrage. >>>>>>>>> Signed-off-by: Trond Myklebust >>>>>>>>> <trond.myklebust@hammerspace.com> >>>>>>>>> --- >>>>>>>>> fs/nfs/nfs2xdr.c | 2 +- >>>>>>>>> fs/nfs/nfs3xdr.c | 2 +- >>>>>>>>> fs/nfs/nfs4xdr.c | 2 +- >>>>>>>>> 3 files changed, 3 insertions(+), 3 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c >>>>>>>>> index db9c265ad9e1..468bfbfe44d7 100644 >>>>>>>>> --- a/fs/nfs/nfs2xdr.c >>>>>>>>> +++ b/fs/nfs/nfs2xdr.c >>>>>>>>> @@ -102,7 +102,7 @@ static int decode_nfsdata(struct >>>>>>>>> xdr_stream >>>>>>>>> *xdr, struct nfs_pgio_res *result) >>>>>>>>> if (unlikely(!p)) >>>>>>>>> return -EIO; >>>>>>>>> count = be32_to_cpup(p); >>>>>>>>> - recvd = xdr_read_pages(xdr, count); >>>>>>>>> + recvd = xdr_read_pages(xdr, >>>>>>>>> xdr_align_size(count)); >>>>>>>>> if (unlikely(count > recvd)) >>>>>>>>> goto out_cheating; >>>>>>>>> out: >>>>>>>>> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c >>>>>>>>> index d3e1726d538b..8ef7c961d3e2 100644 >>>>>>>>> --- a/fs/nfs/nfs3xdr.c >>>>>>>>> +++ b/fs/nfs/nfs3xdr.c >>>>>>>>> @@ -1611,7 +1611,7 @@ static int >>>>>>>>> decode_read3resok(struct >>>>>>>>> xdr_stream *xdr, >>>>>>>>> ocount = be32_to_cpup(p++); >>>>>>>>> if (unlikely(ocount != count)) >>>>>>>>> goto out_mismatch; >>>>>>>>> - recvd = xdr_read_pages(xdr, count); >>>>>>>>> + recvd = xdr_read_pages(xdr, >>>>>>>>> xdr_align_size(count)); >>>>>>>>> if (unlikely(count > recvd)) >>>>>>>>> goto out_cheating; >>>>>>>>> out: >>>>>>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c >>>>>>>>> index 755b556e85c3..5baa767106dc 100644 >>>>>>>>> --- a/fs/nfs/nfs4xdr.c >>>>>>>>> +++ b/fs/nfs/nfs4xdr.c >>>>>>>>> @@ -5202,7 +5202,7 @@ static int decode_read(struct >>>>>>>>> xdr_stream >>>>>>>>> *xdr, struct rpc_rqst *req, >>>>>>>>> return -EIO; >>>>>>>>> eof = be32_to_cpup(p++); >>>>>>>>> count = be32_to_cpup(p); >>>>>>>>> - recvd = xdr_read_pages(xdr, count); >>>>>>>>> + recvd = xdr_read_pages(xdr, >>>>>>>>> xdr_align_size(count)); >>>>>>>>> if (count > recvd) { >>>>>>>>> dprintk("NFS: server cheating in read >>>>>>>>> reply: " >>>>>>>>> "count %u > recvd >>>>>>>>> %u\n", >>>>>>>>> count, >>>>>>>>> recvd); >>>>>>>>> -- >>>>>>>>> 2.28.0 >>>>> >>>>> -- >>>>> Trond Myklebust >>>>> Linux NFS client maintainer, Hammerspace >>>>> trond.myklebust@hammerspace.com >>>>> >>>>> >>>> >>>> -- >>>> Chuck Lever >>>> >>>> >>>> >>> >>> -- >>> Trond Myklebust >>> Linux NFS client maintainer, Hammerspace >>> trond.myklebust@hammerspace.com >> >> -- >> Chuck Lever >> >> >> > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com -- Chuck Lever
diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c index db9c265ad9e1..468bfbfe44d7 100644 --- a/fs/nfs/nfs2xdr.c +++ b/fs/nfs/nfs2xdr.c @@ -102,7 +102,7 @@ static int decode_nfsdata(struct xdr_stream *xdr, struct nfs_pgio_res *result) if (unlikely(!p)) return -EIO; count = be32_to_cpup(p); - recvd = xdr_read_pages(xdr, count); + recvd = xdr_read_pages(xdr, xdr_align_size(count)); if (unlikely(count > recvd)) goto out_cheating; out: diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c index d3e1726d538b..8ef7c961d3e2 100644 --- a/fs/nfs/nfs3xdr.c +++ b/fs/nfs/nfs3xdr.c @@ -1611,7 +1611,7 @@ static int decode_read3resok(struct xdr_stream *xdr, ocount = be32_to_cpup(p++); if (unlikely(ocount != count)) goto out_mismatch; - recvd = xdr_read_pages(xdr, count); + recvd = xdr_read_pages(xdr, xdr_align_size(count)); if (unlikely(count > recvd)) goto out_cheating; out: diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index 755b556e85c3..5baa767106dc 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -5202,7 +5202,7 @@ static int decode_read(struct xdr_stream *xdr, struct rpc_rqst *req, return -EIO; eof = be32_to_cpup(p++); count = be32_to_cpup(p); - recvd = xdr_read_pages(xdr, count); + recvd = xdr_read_pages(xdr, xdr_align_size(count)); if (count > recvd) { dprintk("NFS: server cheating in read reply: " "count %u > recvd %u\n", count, recvd);