On Fri, 29 Mar 2013 19:57:06 -0000 malahal naineni <malahal@xxxxxxxxxx> wrote:
> This bug seems to be present in v2.6.37 or lower versions. The code was
> re-organized in v2.6.38 that eliminated the bug. Current upstream code
> doesn't have this bug. This may be applicable to some longterm releases!
>
> Here are the bug details:
>
> 1. nfs_read_rpcsetup(), args.count and res.count are both set to the
> actual number of bytes to be read. Let us assume that the request is
> for 16K, so arg.count = res.count = 16K
> 2. nfs3_xdr_readres() conditionally sets res.count to to the actual
> number of bytes read. This condition is true for the first response
> as res.count was set to args.count before the first request. Let us
> say the server returned only 4K bytes. res.count=4K
> 3. Another read request is sent for the remaining data. Note that
> res.count is NOT updated. It is still set to the actual amount of
> bytes we got in the first response. The client will send a READ
> request for the remaining 12K.
This is looks like a real bug, but I think the "NOT" above is the best thing
to fix.
i.e. when another read request is set, res.count *SHOULD*BE* updated. That
makes it consistent with the original send, and consistency is good!
So this patch.
Index: linux-2.6.32-SLE11-SP1-LTSS/fs/nfs/read.c
===================================================================
--- linux-2.6.32-SLE11-SP1-LTSS.orig/fs/nfs/read.c 2013-03-20 16:24:31.426605189 +1100
+++ linux-2.6.32-SLE11-SP1-LTSS/fs/nfs/read.c 2013-04-11 11:19:57.670724540 +1000
@@ -368,6 +368,7 @@ static void nfs_readpage_retry(struct rp
argp->offset += resp->count;
argp->pgbase += resp->count;
argp->count -= resp->count;
+ resp->count = argp->count;
nfs4_restart_rpc(task, NFS_SERVER(data->inode)->nfs_client);
return;
out:
This would old affect clients with servers which would sometimes return
partial reads, and I don't think the Linux NFS server does. What server have
you seen this against?
NeilBrown
> 4. Assume that the server gave all 12K bytes. We still think we got ony
> 4K bytes due to conditional update in nfs3_xdr_readres(). The client
> sends further requests, if not EOF! If this response includes EOF, it
> truncates pages beyond 4K+4K causing data corruption beyond 8K. The
> corrupted data is filled with zeros!
>
> Signed-off-by: Malahal Naineni <malahal@xxxxxxxxxx>
>
> ---
> fs/nfs/nfs2xdr.c | 3 +--
> fs/nfs/nfs3xdr.c | 3 +--
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
> index 5914a19..710ca56 100644
> --- a/fs/nfs/nfs2xdr.c
> +++ b/fs/nfs/nfs2xdr.c
> @@ -287,8 +287,7 @@ nfs_xdr_readres(struct rpc_rqst *req, __be32 *p, struct nfs_readres *res)
> }
>
> dprintk("RPC: readres OK count %u\n", count);
> - if (count < res->count)
> - res->count = count;
> + res->count = count;
>
> return count;
> }
> diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
> index f6cc60f..152a5e4 100644
> --- a/fs/nfs/nfs3xdr.c
> +++ b/fs/nfs/nfs3xdr.c
> @@ -914,8 +914,7 @@ nfs3_xdr_readres(struct rpc_rqst *req, __be32 *p, struct nfs_readres *res)
> res->eof = 0;
> }
>
> - if (count < res->count)
> - res->count = count;
> + res->count = count;
>
> return count;
> }
Attachment:
signature.asc
Description: PGP signature
