Re: [PATCH] nfsd41: compare request's opcnt with session's maxops at nfsd4_sequence

[Mi Jinlong <mijinlong@cn.fujitsu.com>]

J. Bruce Fields:
> On Wed, Apr 27, 2011 at 01:48:09PM +0800, Mi Jinlong wrote:
>>
>> J. Bruce Fields:
>>> On Wed, Apr 27, 2011 at 09:09:58AM +0800, Mi Jinlong wrote:
>>>> Make sure nfs server can distinguish request contains more ops
>>>> than channel allowed.
>>> Yes, sequence looks like a reasonable op to catch this error.  The spec
>>> doesn't care as far as I can tell (and it's a buggy-client case, so why
>>> should it), and we already check that any compound not starting with a
>>> sequence has only one op.
>>>
>>>> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
>>>> ---
>>>>  fs/nfsd/nfs4state.c |    5 +++++
>>>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index fbde6f7..4f9fc68 100644
>>>> --- a/fs/nfsd/nfs4state.c
>>>> +++ b/fs/nfsd/nfs4state.c
>>>> @@ -1749,6 +1749,11 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>>>>  	if (!session)
>>>>  		goto out;
>>>>  
>>>> +	status = nfserr_too_many_ops;
>>>> +	if (((struct nfsd4_compoundargs *)rqstp->rq_argp)->opcnt >
>>> Kind of a cumbersome construction, though.
>>>
>>> Eh, maybe overkill, but how about this?:
>>   That's great!
> 
> But note we're making a bunch of the pynfs tests fail now, since they
> aren't careful about how many ops they use in a compound.  For example,
> LKPP1d fails for me now.  Note it might not fail for you if you put the
> pynfs test directory right at the root of the export tree?

  Yes, I don't get LKPP1d fail.

> I don't think pynfs hsould be using absolute paths for everything.  And
> it should probably be doing single-component lookups in a loop instead
> of depending on being able to do the whole lookup in one compound,
> unless it's willing to adapt the compounds to take into account
> maxops--which sounds to me like more trouble than it's worth.

  I agree the second idea although it's more trouble. 
  Because simulating a NFS4.1 client, we should make sure pynfs41 can 
  make request with ops which less than maxops.

> 
> The server could probably also raise its limit, though.

  I think, we can do it as raising the maxops at CREATE_SESSION ops
  when create session, I don't test it.

-- 
----
thanks
Mi Jinlong

>>> commit d5eee1629fb9d3a55e5793d156026248c14cb46c
>>> Author: Mi Jinlong <mijinlong@cn.fujitsu.com>
>>> Date:   Wed Apr 27 09:09:58 2011 +0800
>>>
>>>     nfsd41: compare request's opcnt with session's maxops at nfsd4_sequence
>>>     
>>>     Make sure nfs server errors out if request contains more ops
>>>     than channel allows.
>>>     
>>>     Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
>>>     [bfields@redhat.com: use helper function]
>>>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index fbde6f7..487ba47 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -1721,6 +1721,13 @@ static void nfsd4_sequence_check_conn(struct nfsd4_conn *new, struct nfsd4_sessi
>>>  	return;
>>>  }
>>>  
>>> +static bool nfsd4_session_too_many_ops(struct svc_rqst *rqstp, struct nfsd4_session *session)
>>> +{
>>> +	struct nfsd4_compoundargs *args = rqstp->rq_argp;
>>> +
>>> +	return args->opcnt > session->se_fchannel.maxops;
>>> +}
>>> +
>>>  __be32
>>>  nfsd4_sequence(struct svc_rqst *rqstp,
>>>  	       struct nfsd4_compound_state *cstate,
>>> @@ -1749,6 +1756,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>>>  	if (!session)
>>>  		goto out;
>>>  
>>> +	status = nfserr_too_many_ops;
>>> +	if (nfsd4_session_too_many_ops(rqstp, session))
>>> +		goto out;
>>> +
>>>  	status = nfserr_badslot;
>>>  	if (seq->slotid >= session->se_fchannel.maxreqs)
>>>  		goto out;
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>