On 2012-01-24 16:23, Tigran Mkrtchyan wrote:
> On Tue, Jan 24, 2012 at 3:06 PM, Benny Halevy <bhalevy@xxxxxxxxxx> wrote:
>> On 2012-01-24 00:23, Tigran Mkrtchyan wrote:
>>> From: Tigran Mkrtchyan <kofemann@xxxxxxxxx>
>>>
>>>
>>> Signed-off-by: Tigran Mkrtchyan <kofemann@xxxxxxxxx>
>>> ---
>>> fs/nfsd/current_stateid.h | 1 +
>>> fs/nfsd/nfs4proc.c | 12 +++++++++---
>>> fs/nfsd/nfs4state.c | 19 +++++++++++++++----
>>> fs/nfsd/xdr4.h | 6 ++++--
>>> 4 files changed, 29 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/fs/nfsd/current_stateid.h b/fs/nfsd/current_stateid.h
>>> index d8c9992..4123551 100644
>>> --- a/fs/nfsd/current_stateid.h
>>> +++ b/fs/nfsd/current_stateid.h
>>> @@ -4,6 +4,7 @@
>>> #include "state.h"
>>> #include "xdr4.h"
>>>
>>> +extern void clear_current_stateid(struct nfsd4_compound_state *cstate);
>>> /*
>>> * functions to set current state id
>>> */
>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>> index dffc9bd..56c1977 100644
>>> --- a/fs/nfsd/nfs4proc.c
>>> +++ b/fs/nfsd/nfs4proc.c
>>> @@ -453,7 +453,10 @@ nfsd4_restorefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>> return nfserr_restorefh;
>>>
>>> fh_dup2(&cstate->current_fh, &cstate->save_fh);
>>> - cstate->current_stateid = cstate->save_stateid;
>>> + if (cstate->has_ssid) {
>>> + memcpy(&cstate->current_stateid, &cstate->save_stateid, sizeof(stateid_t));
>>> + cstate->has_csid = true;
>>> + }
>>> return nfs_ok;
>>> }
>>>
>>> @@ -465,7 +468,10 @@ nfsd4_savefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>> return nfserr_nofilehandle;
>>>
>>> fh_dup2(&cstate->save_fh, &cstate->current_fh);
>>> - cstate->save_stateid = cstate->current_stateid;
>>> + if (cstate->has_csid) {
>>> + memcpy(&cstate->save_stateid, &cstate->current_stateid, sizeof(stateid_t));
>>> + cstate->has_ssid = true;
>>> + }
>>> return nfs_ok;
>>> }
>>>
>>> @@ -1238,7 +1244,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
>>> opdesc->op_set_currentstateid(cstate, &op->u);
>>>
>>> if (opdesc->op_flags & OP_CLEAR_STATEID)
>>> - cstate->current_stateid = NULL;
>>> + clear_current_stateid(cstate);
>>>
>>> if (need_wrongsec_check(rqstp))
>>> op->status = check_nfsd_access(cstate->current_fh.fh_export, rqstp);
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index 9c5a239..c6d2980 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -4699,15 +4699,26 @@ nfs4_state_shutdown(void)
>>> static void
>>> get_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
>>> {
>>> - if (cstate->current_stateid && CURRENT_STATEID(stateid))
>>> - memcpy(stateid, cstate->current_stateid, sizeof(stateid_t));
>>> + if (cstate->has_csid && CURRENT_STATEID(stateid))
>>> + memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
>>> }
>>>
>>> static void
>>> put_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
>>> {
>>> - if (cstate->minorversion)
>>> - cstate->current_stateid = stateid;
>>> + if (cstate->minorversion) {
>>> + memcpy(&cstate->current_stateid, stateid, sizeof(stateid_t));
>>> + cstate->has_csid = true;
>>> + }
>>> +}
>>> +
>>> +void
>>> +clear_current_stateid(struct nfsd4_compound_state *cstate)
>>> +{
>>> + if (cstate->has_csid) {
>>> + memcpy(&cstate->current_stateid, &zero_stateid, sizeof(stateid_t));
>>
>> This shouldn't be needed as long as has_csid is set to false, right?
>
> correct. I can change it as following:
> if (!cstate->has_csid)
> return;
> memcpy(&cstate->current_stateid, &zero_stateid, sizeof(stateid_t));
> cstate->has_csid = false;
What I meant is if has_csid is set to false why do the memcpy at all?
cstate->current_stateid's contents should never be accessed when has_scid==false.
>
> or move it into nfsd4proc.c
>
>>
>>> + cstate->has_csid = false;
>>> + }
>>> }
>>>
>>> /*
>>> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
>>> index 2ae378e..3692ba8 100644
>>> --- a/fs/nfsd/xdr4.h
>>> +++ b/fs/nfsd/xdr4.h
>>> @@ -54,8 +54,10 @@ struct nfsd4_compound_state {
>>> size_t iovlen;
>>> u32 minorversion;
>>> u32 status;
>>> - const stateid_t *current_stateid;
>>> - const stateid_t *save_stateid;
>>> + stateid_t current_stateid;
>>> + bool has_csid; /* true if compound has current state id */
>>> + stateid_t save_stateid;
>>> + bool has_ssid; /* true if compound has saved state id */
>>
>> This is a pretty big overhead for the status bits.
>> I'm not sure what Bruce's stand but using single-bit bit fields or
>> flags would be more space efficient...
>
> Fine with me.
Great. Thanks.
Benny
>
>>
>> Benny
>>
>>> };
>>>
>>> static inline bool nfsd4_has_session(struct nfsd4_compound_state *cs)
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
