Re: [PATCH 02/29] nfsd41: encode create_session result into cache

Andy Adamson
2009-04-24T13:52:04Z

On Apr 23, 2009, at 7:21 PM, J. Bruce Fields wrote:

> On Thu, Apr 23, 2009 at 12:42:41PM -0400, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> CREATE_SESSION can be preceeded by a SEQUENCE operation and the
>> create session single slot cache must be maintained. Encode the
>> results of
>> a create session call into the cache at the end of processing.
>>
>> The create session result will also be encoded into the RPC
>> response, and if
>> it is preceeded by a SEQUENCE operation, will also be encoded into
>> the
>> session slot table cache.
>>
>> Errors that do not change the create session cache:
>> A create session NFS4ERR_STALE_CLIENTID error means that a client
>> record
>> (and associated create session slot) could not be found and
>> therefore can't
>> be changed. NFSERR_SEQ_MISORDERED errors do not change the slot
>> cache.
>>
>> All other errors get cached.
>
> Thanks, this all sounds sensible. (One simple thing I think I was
> confused about: the create_session sequencing is done at a layer above
> sessions; so in the case of a create_session preceded by a sequence,
> if
> we discover at the point of processing the sequence that this is a
> replay, we use the sequence replay cache and never even get to any
> create_session processing. OK!)

Yes! The double DRC of a sequence:create_session is a bit confusing.
It works correctly because the create session or clientid cache is a
single slot. So if there is a sequence replay of a compound with a
create session it's safe to replay the create session stored in the
sequence slot cache because: sequence replay -> client did not get
the response -> create session slot sequence # was not incremented.

-->Andy

>
>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>> fs/nfsd/nfs4state.c | 6 +++++-
>> fs/nfsd/nfs4xdr.c | 19 +++++++++++++++++++
>> include/linux/nfsd/xdr4.h | 2 ++
>> 3 files changed, 26 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 27ad37f..279b47e 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -1377,7 +1377,7 @@ nfsd4_create_session(struct svc_rqst *rqstp,
>> if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
>> (ip_addr != unconf->cl_addr)) {
>
> Could we apply the patch removing these ip_addr checks before any of
> these patches? Also I seem to recall expressing some other doubts
> about
> the correctness of exchange_id and create_session--I'd like to see
> those
> addressed first, as this builds on those.
>
>> status = nfserr_clid_inuse;
>> - goto out;
>> + goto out_cache;
>> }
>>
>> slot = &unconf->cl_slot;
>> @@ -1424,6 +1424,10 @@ nfsd4_create_session(struct svc_rqst *rqstp,
>> cstate->slot = slot;
>> /* Ensure a page is used for the cache */
>> slot->sl_cache_entry.ce_cachethis = 1;
>
> Is this still needed?
>
> --b.
>
>> +out_cache:
>> + /* cache solo and embedded create sessions under the state lock */
>> + nfsd4_cache_create_session(cr_ses, slot, status);
>> +
>> out:
>> nfs4_unlock_state();
>> dprintk("%s returns %d\n", __func__, ntohl(status));
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index b2f8d74..6b34fac 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -3046,6 +3046,25 @@ nfsd4_encode_create_session(struct
>> nfsd4_compoundres *resp, int nfserr,
>> return 0;
>> }
>>
>> +/*
>> + * Encode the create_session result into the create session single
>> DRC
>> + * slot cache. Do this for solo or embedded create session
>> operations.
>> + */
>> +void
>> +nfsd4_cache_create_session(struct nfsd4_create_session *cr_ses,
>> + struct nfsd4_slot *slot, int nfserr)
>> +{
>> + struct nfsd4_cache_entry *entry = &slot->sl_cache_entry;
>> + __be32 *p = (__be32 *)entry->ce_datav.iov_base;
>> + struct nfsd4_compoundres tmp = {
>> + .p = p,
>> + .end = p + XDR_QUADLEN(CS_MAX_ENC_SZ),
>> + }, *resp = &tmp;
>> +
>> + entry->ce_status = nfsd4_encode_create_session(resp, nfserr,
>> cr_ses);
>> + entry->ce_datav.iov_len = (char *)resp->p - (char *)p;
>> +}
>> +
>> static __be32
>> nfsd4_encode_destroy_session(struct nfsd4_compoundres *resp, int
>> nfserr,
>> struct nfsd4_destroy_session *destroy_session)
>> diff --git a/include/linux/nfsd/xdr4.h b/include/linux/nfsd/xdr4.h
>> index afd4464..cc50ca0 100644
>> --- a/include/linux/nfsd/xdr4.h
>> +++ b/include/linux/nfsd/xdr4.h
>> @@ -517,6 +517,8 @@ extern __be32 nfsd4_setclientid_confirm(struct
>> svc_rqst *rqstp,
>> extern void nfsd4_store_cache_entry(struct nfsd4_compoundres *resp);
>> extern __be32 nfsd4_replay_cache_entry(struct nfsd4_compoundres
>> *resp,
>> struct nfsd4_sequence *seq);
>> +extern void nfsd4_cache_create_session(struct nfsd4_create_session
>> *cr_ses,
>> + struct nfsd4_slot *slot, int nfserr);
>> extern __be32 nfsd4_exchange_id(struct svc_rqst *rqstp,
>> struct nfsd4_compound_state *,
>> struct nfsd4_exchange_id *);
>> --
>> 1.5.4.3
>>