2011-05-18 02:17:05

by Mi Jinlong

[permalink] [raw]
Subject: [PATCH] nfsd41: check the size of response

Compare response's size to the session se_fmaxresp_sz and
se_fmaxresp_cached at the same function.

And modify function name nfsd4_check_drc_limit to
nfsd4_check_response_size.

Signed-off-by: Mi Jinlong <[email protected]>
---
fs/nfsd/nfs4xdr.c | 20 ++++++++++++--------
1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index c6766af..a7cf985 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3224,12 +3224,12 @@ static nfsd4_enc nfsd4_enc_ops[] = {
* pad: add on 8 bytes for the next operation's op_code and status so that
* there is room to cache a failure on the next operation.
*
- * Compare this length to the session se_fmaxresp_cached.
+ * Compare this length to the session se_fmaxresp_sz and se_fmaxresp_cached.
*
* Our se_fmaxresp_cached will always be a multiple of PAGE_SIZE, and so
* will be at least a page and will therefore hold the xdr_buf head.
*/
-static int nfsd4_check_drc_limit(struct nfsd4_compoundres *resp)
+static int nfsd4_check_response_size(struct nfsd4_compoundres *resp)
{
int status = 0;
struct xdr_buf *xb = &resp->rqstp->rq_res;
@@ -3242,7 +3242,7 @@ static int nfsd4_check_drc_limit(struct nfsd4_compoundres *resp)
return status;

session = resp->cstate.session;
- if (session == NULL || slot->sl_cachethis == 0)
+ if (session == NULL)
return status;

if (resp->opcnt >= args->opcnt)
@@ -3259,10 +3259,14 @@ static int nfsd4_check_drc_limit(struct nfsd4_compoundres *resp)
dprintk("%s length %u, xb->page_len %u tlen %u pad %u\n", __func__,
length, xb->page_len, tlen, pad);

- if (length <= session->se_fchannel.maxresp_cached)
- return status;
- else
+ if (length > session->se_fchannel.maxresp_sz)
+ return nfserr_rep_too_big;
+
+ if (slot->sl_cachethis == 1 &&
+ length > session->se_fchannel.maxresp_cached)
return nfserr_rep_too_big_to_cache;
+
+ return status;
}

void
@@ -3282,8 +3286,8 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
!nfsd4_enc_ops[op->opnum]);
op->status = nfsd4_enc_ops[op->opnum](resp, op->status, &op->u);
/* nfsd4_check_drc_limit guarantees enough room for error status */
- if (!op->status && nfsd4_check_drc_limit(resp))
- op->status = nfserr_rep_too_big_to_cache;
+ if (!op->status)
+ op->status = nfsd4_check_response_size(resp);
status:
/*
* Note: We write the status directly, instead of using WRITE32(),
--
1.7.4.5




2011-05-19 01:07:02

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] nfsd41: check the size of response

On Wed, May 18, 2011 at 10:19:23AM +0800, Mi Jinlong wrote:
> Compare response's size to the session se_fmaxresp_sz and
> se_fmaxresp_cached at the same function.
>
> And modify function name nfsd4_check_drc_limit to
> nfsd4_check_response_size.

Hm. I guess that's OK. Note we have a bigger problem, though, which is
that we're waiting too late to check these limits:

http://wiki.linux-nfs.org/wiki/index.php/Server_4.0_and_4.1_issues#DRC_limit_checking

Have you looked at that list, by the way?:

http://wiki.linux-nfs.org/wiki/index.php/Server_4.0_and_4.1_issues#Highest_priority

Those are the 4.1 spec compliance problems that I think are the most
important to fix--the ones that I think we have to get done before we
can really declare the 4.1 server correct.

--b.

>
> Signed-off-by: Mi Jinlong <[email protected]>
> ---
> fs/nfsd/nfs4xdr.c | 20 ++++++++++++--------
> 1 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index c6766af..a7cf985 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3224,12 +3224,12 @@ static nfsd4_enc nfsd4_enc_ops[] = {
> * pad: add on 8 bytes for the next operation's op_code and status so that
> * there is room to cache a failure on the next operation.
> *
> - * Compare this length to the session se_fmaxresp_cached.
> + * Compare this length to the session se_fmaxresp_sz and se_fmaxresp_cached.
> *
> * Our se_fmaxresp_cached will always be a multiple of PAGE_SIZE, and so
> * will be at least a page and will therefore hold the xdr_buf head.
> */
> -static int nfsd4_check_drc_limit(struct nfsd4_compoundres *resp)
> +static int nfsd4_check_response_size(struct nfsd4_compoundres *resp)
> {
> int status = 0;
> struct xdr_buf *xb = &resp->rqstp->rq_res;
> @@ -3242,7 +3242,7 @@ static int nfsd4_check_drc_limit(struct nfsd4_compoundres *resp)
> return status;
>
> session = resp->cstate.session;
> - if (session == NULL || slot->sl_cachethis == 0)
> + if (session == NULL)
> return status;
>
> if (resp->opcnt >= args->opcnt)
> @@ -3259,10 +3259,14 @@ static int nfsd4_check_drc_limit(struct nfsd4_compoundres *resp)
> dprintk("%s length %u, xb->page_len %u tlen %u pad %u\n", __func__,
> length, xb->page_len, tlen, pad);
>
> - if (length <= session->se_fchannel.maxresp_cached)
> - return status;
> - else
> + if (length > session->se_fchannel.maxresp_sz)
> + return nfserr_rep_too_big;
> +
> + if (slot->sl_cachethis == 1 &&
> + length > session->se_fchannel.maxresp_cached)
> return nfserr_rep_too_big_to_cache;
> +
> + return status;
> }
>
> void
> @@ -3282,8 +3286,8 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
> !nfsd4_enc_ops[op->opnum]);
> op->status = nfsd4_enc_ops[op->opnum](resp, op->status, &op->u);
> /* nfsd4_check_drc_limit guarantees enough room for error status */
> - if (!op->status && nfsd4_check_drc_limit(resp))
> - op->status = nfserr_rep_too_big_to_cache;
> + if (!op->status)
> + op->status = nfsd4_check_response_size(resp);
> status:
> /*
> * Note: We write the status directly, instead of using WRITE32(),
> --
> 1.7.4.5
>
>

2011-05-19 05:33:10

by Mi Jinlong

[permalink] [raw]
Subject: Re: [PATCH] nfsd41: check the size of response



J. Bruce Fields:
> On Wed, May 18, 2011 at 10:19:23AM +0800, Mi Jinlong wrote:
>> Compare response's size to the session se_fmaxresp_sz and
>> se_fmaxresp_cached at the same function.
>>
>> And modify function name nfsd4_check_drc_limit to
>> nfsd4_check_response_size.
>
> Hm. I guess that's OK. Note we have a bigger problem, though, which is
> that we're waiting too late to check these limits:
>
> http://wiki.linux-nfs.org/wiki/index.php/Server_4.0_and_4.1_issues#DRC_limit_checking
>
> Have you looked at that list, by the way?:

I will have a look, and try to solve this problem.
Maybe it will take me some times.

--
----
thanks
Mi Jinlong

>
> http://wiki.linux-nfs.org/wiki/index.php/Server_4.0_and_4.1_issues#Highest_priority
>
> Those are the 4.1 spec compliance problems that I think are the most
> important to fix--the ones that I think we have to get done before we
> can really declare the 4.1 server correct.
>
> --b.
>
>> Signed-off-by: Mi Jinlong <[email protected]>
>> ---
>> fs/nfsd/nfs4xdr.c | 20 ++++++++++++--------
>> 1 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index c6766af..a7cf985 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -3224,12 +3224,12 @@ static nfsd4_enc nfsd4_enc_ops[] = {
>> * pad: add on 8 bytes for the next operation's op_code and status so that
>> * there is room to cache a failure on the next operation.
>> *
>> - * Compare this length to the session se_fmaxresp_cached.
>> + * Compare this length to the session se_fmaxresp_sz and se_fmaxresp_cached.
>> *
>> * Our se_fmaxresp_cached will always be a multiple of PAGE_SIZE, and so
>> * will be at least a page and will therefore hold the xdr_buf head.
>> */
>> -static int nfsd4_check_drc_limit(struct nfsd4_compoundres *resp)
>> +static int nfsd4_check_response_size(struct nfsd4_compoundres *resp)
>> {
>> int status = 0;
>> struct xdr_buf *xb = &resp->rqstp->rq_res;
>> @@ -3242,7 +3242,7 @@ static int nfsd4_check_drc_limit(struct nfsd4_compoundres *resp)
>> return status;
>>
>> session = resp->cstate.session;
>> - if (session == NULL || slot->sl_cachethis == 0)
>> + if (session == NULL)
>> return status;
>>
>> if (resp->opcnt >= args->opcnt)
>> @@ -3259,10 +3259,14 @@ static int nfsd4_check_drc_limit(struct nfsd4_compoundres *resp)
>> dprintk("%s length %u, xb->page_len %u tlen %u pad %u\n", __func__,
>> length, xb->page_len, tlen, pad);
>>
>> - if (length <= session->se_fchannel.maxresp_cached)
>> - return status;
>> - else
>> + if (length > session->se_fchannel.maxresp_sz)
>> + return nfserr_rep_too_big;
>> +
>> + if (slot->sl_cachethis == 1 &&
>> + length > session->se_fchannel.maxresp_cached)
>> return nfserr_rep_too_big_to_cache;
>> +
>> + return status;
>> }
>>
>> void
>> @@ -3282,8 +3286,8 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
>> !nfsd4_enc_ops[op->opnum]);
>> op->status = nfsd4_enc_ops[op->opnum](resp, op->status, &op->u);
>> /* nfsd4_check_drc_limit guarantees enough room for error status */
>> - if (!op->status && nfsd4_check_drc_limit(resp))
>> - op->status = nfserr_rep_too_big_to_cache;
>> + if (!op->status)
>> + op->status = nfsd4_check_response_size(resp);
>> status:
>> /*
>> * Note: We write the status directly, instead of using WRITE32(),
>> --
>> 1.7.4.5
>>
>>
>
>