On Oct. 22, 2009, 15:32 +0200, "William A. (Andy) Adamson" <[email protected]> wrote:
> On Thu, Oct 22, 2009 at 9:20 AM, Benny Halevy <[email protected]> wrote:
>> On Oct. 21, 2009, 20:24 +0200, [email protected] wrote:
>>> From: Andy Adamson <[email protected]>
>>>
>>> Do not clear the NFS4CLNT_SESSION_SETUP bit until after the session has been
>>> reset (a possible EXCHANGE_ID, a DESTROY_SESSION, and a CREATE_SESSION) to
>>> prevent a race with nfs41_setup_sequence assigning a slot on a
>>> partially reset session.
>>>
>>> Signed-off-by: Andy Adamson <[email protected]>
>>> ---
>>> fs/nfs/nfs4proc.c | 3 +++
>>> fs/nfs/nfs4state.c | 15 +++++++++------
>>> 2 files changed, 12 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index eb245a1..80764e2 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -4788,6 +4788,9 @@ int nfs4_proc_create_session(struct nfs_client *clp, int reset)
>>> if (status)
>>> goto out;
>>>
>>> + /* Signal nfs41_setup_sequence that the session is ready for use */
>>> + clear_bit(NFS4CLNT_SESSION_SETUP, &clp->cl_state);
>>> +
>>> ptr = (unsigned *)&session->sess_id.data[0];
>>> dprintk("%s client>seqid %d sessionid %u:%u:%u:%u\n", __func__,
>>> clp->cl_seqid, ptr[0], ptr[1], ptr[2], ptr[3]);
>>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>>> index 1394dfb..09ca30b 100644
>>> --- a/fs/nfs/nfs4state.c
>>> +++ b/fs/nfs/nfs4state.c
>>> @@ -1252,17 +1252,20 @@ static void nfs4_state_manager(struct nfs_client *clp)
>>> continue;
>>> }
>>> /* Initialize or reset the session */
>>> - if (test_and_clear_bit(NFS4CLNT_SESSION_SETUP, &clp->cl_state)
>>> - && nfs4_has_session(clp)) {
>>> + if (test_bit(NFS4CLNT_SESSION_SETUP, &clp->cl_state)
>>> + && nfs4_has_session(clp)) {
>>> if (clp->cl_cons_state == NFS_CS_SESSION_INITING)
>>> status = nfs4_initialize_session(clp);
>>> else
>>> status = nfs4_reset_session(clp);
>>> - if (status) {
>>> - if (status == -NFS4ERR_STALE_CLIENTID)
>>> - continue;
>>> + if (status == -NFS4ERR_STALE_CLIENTID)
>>> + continue;
>>> + /* For error case. On success the bit is cleared in
>>> + * nfs4_proc_create_session */
>> Why the separation?
>> Why not handle both success and error cases here?
>
> We don't clear the bit on NFS4ERR_STALE_CLIENTID because we first need
> an EXCHANGE_ID and then session reset and we don't want any rpc's
> queued on the slot_tbl_waitq to run. The check for
> NFS4ERR_STALE_CLIENTID is in the nfs4_state_manager. Also, all other
> cases in the state manager clear the bits used - so I thought it best
> to follow suit.
I agree, so why clear the NFS4CLNT_SESSION_SETUP bit on success
in nfs4_proc_create_session()
Also, nfs4_proc_create_session() clears NFS4CLNT_LEASE_EXPIRED
which is test_and_cleared by the state manager...
Benny
>
> -->Andy
>
>> Benny
>>
>>> + clear_bit(NFS4CLNT_SESSION_SETUP, &clp->cl_state);
>>> +
>>> + if (status)
>>> goto out_error;
>>> - }
>>> }
>>> /* First recover reboot state... */
>>> if (test_and_clear_bit(NFS4CLNT_RECLAIM_REBOOT, &clp->cl_state)) {
>> _______________________________________________
>> pNFS mailing list
>> [email protected]
>> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>>
On Thu, Oct 22, 2009 at 9:47 AM, Benny Halevy <[email protected]> wro=
te:
> On Oct. 22, 2009, 15:32 +0200, "William A. (Andy) Adamson" <androsada=
[email protected]> wrote:
>> On Thu, Oct 22, 2009 at 9:20 AM, Benny Halevy <[email protected]> =
wrote:
>>> On Oct. 21, 2009, 20:24 +0200, [email protected] wrote:
>>>> From: Andy Adamson <[email protected]>
>>>>
>>>> Do not clear the NFS4CLNT_SESSION_SETUP bit until after the sessio=
n has been
>>>> reset (a possible EXCHANGE_ID, a DESTROY_SESSION, and a CREATE_SES=
SION) to
>>>> prevent a race with nfs41_setup_sequence assigning a slot on a
>>>> partially reset session.
>>>>
>>>> Signed-off-by: Andy Adamson <[email protected]>
>>>> ---
>>>> =A0fs/nfs/nfs4proc.c =A0| =A0 =A03 +++
>>>> =A0fs/nfs/nfs4state.c | =A0 15 +++++++++------
>>>> =A02 files changed, 12 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>> index eb245a1..80764e2 100644
>>>> --- a/fs/nfs/nfs4proc.c
>>>> +++ b/fs/nfs/nfs4proc.c
>>>> @@ -4788,6 +4788,9 @@ int nfs4_proc_create_session(struct nfs_clie=
nt *clp, int reset)
>>>> =A0 =A0 =A0 if (status)
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out;
>>>>
>>>> + =A0 =A0 /* Signal nfs41_setup_sequence that the session is ready=
for use */
>>>> + =A0 =A0 clear_bit(NFS4CLNT_SESSION_SETUP, &clp->cl_state);
>>>> +
>>>> =A0 =A0 =A0 ptr =3D (unsigned *)&session->sess_id.data[0];
>>>> =A0 =A0 =A0 dprintk("%s client>seqid %d sessionid %u:%u:%u:%u\n", =
__func__,
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 clp->cl_seqid, ptr[0], ptr[1], ptr[2],=
ptr[3]);
>>>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>>>> index 1394dfb..09ca30b 100644
>>>> --- a/fs/nfs/nfs4state.c
>>>> +++ b/fs/nfs/nfs4state.c
>>>> @@ -1252,17 +1252,20 @@ static void nfs4_state_manager(struct nfs_=
client *clp)
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 contin=
ue;
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Initialize or reset the session */
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 if (test_and_clear_bit(NFS4CLNT_SESSION_=
SETUP, &clp->cl_state)
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0&& nfs4_has_session(clp)) {
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 if (test_bit(NFS4CLNT_SESSION_SETUP, &cl=
p->cl_state)
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 && nfs4_has_session(clp)) {
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (clp->cl_cons_state=
=3D=3D NFS_CS_SESSION_INITING)
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 status=
=3D nfs4_initialize_session(clp);
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 else
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 status=
=3D nfs4_reset_session(clp);
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (status) {
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (stat=
us =3D=3D -NFS4ERR_STALE_CLIENTID)
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 continue;
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (status =3D=3D -NFS4E=
RR_STALE_CLIENTID)
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue=
;
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* For error case. On su=
ccess the bit is cleared in
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* nfs4_proc_create_se=
ssion */
>>> Why the separation?
>>> Why not handle both success and error cases here?
>>
>> We don't clear the bit on NFS4ERR_STALE_CLIENTID because we first ne=
ed
>> an EXCHANGE_ID and then session reset and we don't want any rpc's
>> queued on the slot_tbl_waitq to run. The check for
>> NFS4ERR_STALE_CLIENTID is in the nfs4_state_manager. Also, all other
>> cases in the state manager clear the bits used - so I thought it bes=
t
>> to follow suit.
>
> I agree, so why clear the NFS4CLNT_SESSION_SETUP bit on success
> in nfs4_proc_create_session()
so that nfs4_proc_get_lease_time can use the newly created session.
> Also, nfs4_proc_create_session() clears NFS4CLNT_LEASE_EXPIRED
> which is test_and_cleared by the state manager...
True and harmless.
-->Andy
>
> Benny
>
>>
>> -->Andy
>>
>>> Benny
>>>
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 clear_bit(NFS4CLNT_SESSI=
ON_SETUP, &clp->cl_state);
>>>> +
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (status)
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto o=
ut_error;
>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* First recover reboot state... */
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (test_and_clear_bit(NFS4CLNT_RECLAI=
M_REBOOT, &clp->cl_state)) {
>>> _______________________________________________
>>> pNFS mailing list
>>> [email protected]
>>> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>>>
>