[PATCH] NFS4: Retry destroy session when getting -NFS4ERR_DELAY

[PATCH] NFS4: Retry destroy session when getting -NFS4ERR_DELAY

[Kinglong Mee]

When umounting a client, it cost near ten seconds.
Dump the request, got
   client                     server
DELEGRETURN              ---->
DESTROY_SESSION          ----> 
          NFS4ERR_DELAY  <----  DESTROY_SESSION reply
                NFS4_OK  <----  DELEGRETURN reply
DESTROY_CLIENTID          ---->
  NFS4ERR_CLIENTID_BUSY  <----  DESTROY_CLIENTID reply
DESTROY_CLIENTID          ---->
  NFS4ERR_CLIENTID_BUSY  <----  DESTROY_CLIENTID reply
         ... ....                  ... ... 
There are ten DESTROY_CLIENTID requests.
This patch retry DESTROY_SESSION when getting NFS4ERR_DELAY,
try the best to destroy the session as destroy clientid.

With this patch, only cost more than 1 seconds, as,
   client                     server
DELEGRETURN          ---->
DESTROY_SESSION      ----> 
     NFS4ERR_DELAY  <----  DESTROY_SESSION reply
           NFS4_OK  <----  DELEGRETURN reply
DESTROY_SESSION      ----> 
           NFS4_OK  <----  DESTROY_SESSION reply
DESTROY_CLIENTID     ---->
           NFS4_OK  <----  DESTROY_CLIENTID reply

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfs/nfs4proc.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 627f37c..2631dc2 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -7320,7 +7320,7 @@ out:
  * Issue the over-the-wire RPC DESTROY_SESSION.
  * The caller must serialize access to this routine.
  */
-int nfs4_proc_destroy_session(struct nfs4_session *session,
+static int _nfs4_proc_destroy_session(struct nfs4_session *session,
 		struct rpc_cred *cred)
 {
 	struct rpc_message msg = {
@@ -7332,10 +7332,6 @@ int nfs4_proc_destroy_session(struct nfs4_session *session,
 
 	dprintk("--> nfs4_proc_destroy_session\n");
 
-	/* session is still being setup */
-	if (!test_and_clear_bit(NFS4_SESSION_ESTABLISHED, &session->session_state))
-		return 0;
-
 	status = rpc_call_sync(session->clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT);
 	trace_nfs4_destroy_session(session->clp, status);
 
@@ -7347,6 +7343,26 @@ int nfs4_proc_destroy_session(struct nfs4_session *session,
 	return status;
 }
 
+int nfs4_proc_destroy_session(struct nfs4_session *session,
+		struct rpc_cred *cred)
+{
+	unsigned int loop;
+	int ret;
+
+	/* session is still being setup */
+	if (!test_and_clear_bit(NFS4_SESSION_ESTABLISHED, &session->session_state))
+		return 0;
+
+	for (loop = NFS4_MAX_LOOP_ON_RECOVER; loop != 0; loop--) {
+		ret = _nfs4_proc_destroy_session(session, cred);
+		if (ret != -NFS4ERR_DELAY)
+			break;
+		ssleep(1);
+	}
+
+	return ret;
+}
+
 /*
  * Renew the cl_session lease.
  */
-- 
2.3.3

Re: [PATCH] NFS4: Retry destroy session when getting -NFS4ERR_DELAY

[Trond Myklebust]

On Fri, Mar 20, 2015 at 4:31 AM, Kinglong Mee <kinglongmee@gmail.com> wrote:
> When umounting a client, it cost near ten seconds.
> Dump the request, got
>    client                     server
> DELEGRETURN              ---->
> DESTROY_SESSION          ---->
>           NFS4ERR_DELAY  <----  DESTROY_SESSION reply
>                 NFS4_OK  <----  DELEGRETURN reply
> DESTROY_CLIENTID          ---->
>   NFS4ERR_CLIENTID_BUSY  <----  DESTROY_CLIENTID reply
> DESTROY_CLIENTID          ---->
>   NFS4ERR_CLIENTID_BUSY  <----  DESTROY_CLIENTID reply
>          ... ....                  ... ...
> There are ten DESTROY_CLIENTID requests.
> This patch retry DESTROY_SESSION when getting NFS4ERR_DELAY,
> try the best to destroy the session as destroy clientid.
>
> With this patch, only cost more than 1 seconds, as,
>    client                     server
> DELEGRETURN          ---->
> DESTROY_SESSION      ---->
>      NFS4ERR_DELAY  <----  DESTROY_SESSION reply
>            NFS4_OK  <----  DELEGRETURN reply
> DESTROY_SESSION      ---->
>            NFS4_OK  <----  DESTROY_SESSION reply
> DESTROY_CLIENTID     ---->
>            NFS4_OK  <----  DESTROY_CLIENTID reply
>
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfs/nfs4proc.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 627f37c..2631dc2 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -7320,7 +7320,7 @@ out:
>   * Issue the over-the-wire RPC DESTROY_SESSION.
>   * The caller must serialize access to this routine.
>   */
> -int nfs4_proc_destroy_session(struct nfs4_session *session,
> +static int _nfs4_proc_destroy_session(struct nfs4_session *session,
>                 struct rpc_cred *cred)
>  {
>         struct rpc_message msg = {
> @@ -7332,10 +7332,6 @@ int nfs4_proc_destroy_session(struct nfs4_session *session,
>
>         dprintk("--> nfs4_proc_destroy_session\n");
>
> -       /* session is still being setup */
> -       if (!test_and_clear_bit(NFS4_SESSION_ESTABLISHED, &session->session_state))
> -               return 0;
> -
>         status = rpc_call_sync(session->clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT);
>         trace_nfs4_destroy_session(session->clp, status);
>
> @@ -7347,6 +7343,26 @@ int nfs4_proc_destroy_session(struct nfs4_session *session,
>         return status;
>  }
>
> +int nfs4_proc_destroy_session(struct nfs4_session *session,
> +               struct rpc_cred *cred)
> +{
> +       unsigned int loop;
> +       int ret;
> +
> +       /* session is still being setup */
> +       if (!test_and_clear_bit(NFS4_SESSION_ESTABLISHED, &session->session_state))
> +               return 0;
> +
> +       for (loop = NFS4_MAX_LOOP_ON_RECOVER; loop != 0; loop--) {
> +               ret = _nfs4_proc_destroy_session(session, cred);
> +               if (ret != -NFS4ERR_DELAY)
> +                       break;
> +               ssleep(1);
> +       }
> +
> +       return ret;
> +}
> +
>  /*
>   * Renew the cl_session lease.
>   */
> --
> 2.3.3
>

I don't understand. All you've done is paper over the problem AFAICS.
How is that useful?


-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com

Re: [PATCH] NFS4: Retry destroy session when getting -NFS4ERR_DELAY

[Kinglong Mee]

On 2015/3/23 3:14, Trond Myklebust wrote:
> On Fri, Mar 20, 2015 at 4:31 AM, Kinglong Mee <kinglongmee@gmail.com> wrote:
>> When umounting a client, it cost near ten seconds.
>> Dump the request, got
>>    client                     server
>> DELEGRETURN              ---->
>> DESTROY_SESSION          ---->
>>           NFS4ERR_DELAY  <----  DESTROY_SESSION reply
>>                 NFS4_OK  <----  DELEGRETURN reply
>> DESTROY_CLIENTID          ---->
>>   NFS4ERR_CLIENTID_BUSY  <----  DESTROY_CLIENTID reply
>> DESTROY_CLIENTID          ---->
>>   NFS4ERR_CLIENTID_BUSY  <----  DESTROY_CLIENTID reply
>>          ... ....                  ... ...
>> There are ten DESTROY_CLIENTID requests.
>> This patch retry DESTROY_SESSION when getting NFS4ERR_DELAY,
>> try the best to destroy the session as destroy clientid.
>>
>> With this patch, only cost more than 1 seconds, as,
>>    client                     server
>> DELEGRETURN          ---->
>> DESTROY_SESSION      ---->
>>      NFS4ERR_DELAY  <----  DESTROY_SESSION reply
>>            NFS4_OK  <----  DELEGRETURN reply
>> DESTROY_SESSION      ---->
>>            NFS4_OK  <----  DESTROY_SESSION reply
>> DESTROY_CLIENTID     ---->
>>            NFS4_OK  <----  DESTROY_CLIENTID reply
>>
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>> ---
>>  fs/nfs/nfs4proc.c | 26 +++++++++++++++++++++-----
>>  1 file changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 627f37c..2631dc2 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -7320,7 +7320,7 @@ out:
>>   * Issue the over-the-wire RPC DESTROY_SESSION.
>>   * The caller must serialize access to this routine.
>>   */
>> -int nfs4_proc_destroy_session(struct nfs4_session *session,
>> +static int _nfs4_proc_destroy_session(struct nfs4_session *session,
>>                 struct rpc_cred *cred)
>>  {
>>         struct rpc_message msg = {
>> @@ -7332,10 +7332,6 @@ int nfs4_proc_destroy_session(struct nfs4_session *session,
>>
>>         dprintk("--> nfs4_proc_destroy_session\n");
>>
>> -       /* session is still being setup */
>> -       if (!test_and_clear_bit(NFS4_SESSION_ESTABLISHED, &session->session_state))
>> -               return 0;
>> -
>>         status = rpc_call_sync(session->clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT);
>>         trace_nfs4_destroy_session(session->clp, status);
>>
>> @@ -7347,6 +7343,26 @@ int nfs4_proc_destroy_session(struct nfs4_session *session,
>>         return status;
>>  }
>>
>> +int nfs4_proc_destroy_session(struct nfs4_session *session,
>> +               struct rpc_cred *cred)
>> +{
>> +       unsigned int loop;
>> +       int ret;
>> +
>> +       /* session is still being setup */
>> +       if (!test_and_clear_bit(NFS4_SESSION_ESTABLISHED, &session->session_state))
>> +               return 0;
>> +
>> +       for (loop = NFS4_MAX_LOOP_ON_RECOVER; loop != 0; loop--) {
>> +               ret = _nfs4_proc_destroy_session(session, cred);
>> +               if (ret != -NFS4ERR_DELAY)
>> +                       break;
>> +               ssleep(1);
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>>  /*
>>   * Renew the cl_session lease.
>>   */
>> --
>> 2.3.3
>>
> 
> I don't understand. All you've done is paper over the problem AFAICS.
> How is that useful?

Sorry for missing more comments.
When unmounting nfs with delegation, client will return delegation first,
then call nfs41_shutdown_client() destory session and clientid.

DELEGRETURN using asynchronous RPC，destroy session will be send immediately.
If sever processes DELEGRETUEN slowly, destory session maybe processed before.
so that, destory session will be denied with NFS4ERR_DELAY.

NFS client don't care the return value of DESTROY_SESSION,
and call DESTROY_CLIENTID directly, so that, all DESTROY_CLIENTID will fail
with NFS4ERR_CLIENTID_BUSY, retry DESTROY_CLIENTID is usefulness. 

After that, nfs client have destroy all information about nfs server,
but nfs server also records client's information for DESTORY_CLIENTID and 
DESTROY_SESSION failed.

This patch make sure the DESTROY_SESSION/DESTORY_CLIENTID success,
first, cut down the cost of umount as I said above.
second, make sure server clean the recording of client not until expired.

thanks,
Kinglong Mee

Re: [PATCH] NFS4: Retry destroy session when getting -NFS4ERR_DELAY

[Trond Myklebust]

On Mon, 2015-03-23 at 09:16 +0800, Kinglong Mee wrote:
> On 2015/3/23 3:14, Trond Myklebust wrote:
> > On Fri, Mar 20, 2015 at 4:31 AM, Kinglong Mee <kinglongmee@gmail.com> wrote:
> >> When umounting a client, it cost near ten seconds.
> >> Dump the request, got
> >>    client                     server
> >> DELEGRETURN              ---->
> >> DESTROY_SESSION          ---->
> >>           NFS4ERR_DELAY  <----  DESTROY_SESSION reply
> >>                 NFS4_OK  <----  DELEGRETURN reply
> >> DESTROY_CLIENTID          ---->
> >>   NFS4ERR_CLIENTID_BUSY  <----  DESTROY_CLIENTID reply
> >> DESTROY_CLIENTID          ---->
> >>   NFS4ERR_CLIENTID_BUSY  <----  DESTROY_CLIENTID reply
> >>          ... ....                  ... ...
> >> There are ten DESTROY_CLIENTID requests.
> >> This patch retry DESTROY_SESSION when getting NFS4ERR_DELAY,
> >> try the best to destroy the session as destroy clientid.
> >>
> >> With this patch, only cost more than 1 seconds, as,
> >>    client                     server
> >> DELEGRETURN          ---->
> >> DESTROY_SESSION      ---->
> >>      NFS4ERR_DELAY  <----  DESTROY_SESSION reply
> >>            NFS4_OK  <----  DELEGRETURN reply
> >> DESTROY_SESSION      ---->
> >>            NFS4_OK  <----  DESTROY_SESSION reply
> >> DESTROY_CLIENTID     ---->
> >>            NFS4_OK  <----  DESTROY_CLIENTID reply
> >>
> >> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> >> ---
> >>  fs/nfs/nfs4proc.c | 26 +++++++++++++++++++++-----
> >>  1 file changed, 21 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> >> index 627f37c..2631dc2 100644
> >> --- a/fs/nfs/nfs4proc.c
> >> +++ b/fs/nfs/nfs4proc.c
> >> @@ -7320,7 +7320,7 @@ out:
> >>   * Issue the over-the-wire RPC DESTROY_SESSION.
> >>   * The caller must serialize access to this routine.
> >>   */
> >> -int nfs4_proc_destroy_session(struct nfs4_session *session,
> >> +static int _nfs4_proc_destroy_session(struct nfs4_session *session,
> >>                 struct rpc_cred *cred)
> >>  {
> >>         struct rpc_message msg = {
> >> @@ -7332,10 +7332,6 @@ int nfs4_proc_destroy_session(struct nfs4_session *session,
> >>
> >>         dprintk("--> nfs4_proc_destroy_session\n");
> >>
> >> -       /* session is still being setup */
> >> -       if (!test_and_clear_bit(NFS4_SESSION_ESTABLISHED, &session->session_state))
> >> -               return 0;
> >> -
> >>         status = rpc_call_sync(session->clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT);
> >>         trace_nfs4_destroy_session(session->clp, status);
> >>
> >> @@ -7347,6 +7343,26 @@ int nfs4_proc_destroy_session(struct nfs4_session *session,
> >>         return status;
> >>  }
> >>
> >> +int nfs4_proc_destroy_session(struct nfs4_session *session,
> >> +               struct rpc_cred *cred)
> >> +{
> >> +       unsigned int loop;
> >> +       int ret;
> >> +
> >> +       /* session is still being setup */
> >> +       if (!test_and_clear_bit(NFS4_SESSION_ESTABLISHED, &session->session_state))
> >> +               return 0;
> >> +
> >> +       for (loop = NFS4_MAX_LOOP_ON_RECOVER; loop != 0; loop--) {
> >> +               ret = _nfs4_proc_destroy_session(session, cred);
> >> +               if (ret != -NFS4ERR_DELAY)
> >> +                       break;
> >> +               ssleep(1);
> >> +       }
> >> +
> >> +       return ret;
> >> +}
> >> +
> >>  /*
> >>   * Renew the cl_session lease.
> >>   */
> >> --
> >> 2.3.3
> >>
> > 
> > I don't understand. All you've done is paper over the problem AFAICS.
> > How is that useful?
> 
> Sorry for missing more comments.
> When unmounting nfs with delegation, client will return delegation first,
> then call nfs41_shutdown_client() destory session and clientid.
> 
> DELEGRETURN using asynchronous RPC，destroy session will be send immediately.
> If sever processes DELEGRETUEN slowly, destory session maybe processed before.
> so that, destory session will be denied with NFS4ERR_DELAY.
> 
> NFS client don't care the return value of DESTROY_SESSION,
> and call DESTROY_CLIENTID directly, so that, all DESTROY_CLIENTID will fail
> with NFS4ERR_CLIENTID_BUSY, retry DESTROY_CLIENTID is usefulness. 
> 
> After that, nfs client have destroy all information about nfs server,
> but nfs server also records client's information for DESTORY_CLIENTID and 
> DESTROY_SESSION failed.
> 
> This patch make sure the DESTROY_SESSION/DESTORY_CLIENTID success,
> first, cut down the cost of umount as I said above.
> second, make sure server clean the recording of client not until expired.

Hi Kinglong,

Ultimately, what you are saying above is that we need to drain the
session before destroying it. I agree with that goal, however not the
method that you choose to achieve it.

Please consider the following patch instead.

Cheers,
 Trond

8<---------------------------------------------------------------

Re: [PATCH] NFS4: Retry destroy session when getting -NFS4ERR_DELAY

[Trond Myklebust]

On Mon, Mar 23, 2015 at 10:09 AM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> 8<---------------------------------------------------------------
> From 21fb62639ad69ecc5c443dba5b41ad2bd64c6e76 Mon Sep 17 00:00:00 2001
> From: Trond Myklebust <trond.myklebust@primarydata.com>
> Date: Mon, 23 Mar 2015 09:51:41 -0400
> Subject: [PATCH] NFSv4: Ensure that we drain the session before shutting it
>  down
>
> Kinglong Mee reports that the call to DESTROY_SESSION in NFSv4.1
> are racing with the asynchronous DELEGRETURN calls that precede it.
> This points to the root cause being that we're not waiting for the
> session to drain before we destroy it.
>
> This patch ensures that we do so for both NFSv4 and NFSv4.1.
>
> Reported-by: Kinglong Mee <kinglongmee@gmail.com>
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
>  fs/nfs/nfs4_fs.h    |  3 +++
>  fs/nfs/nfs4client.c |  7 ++-----
>  fs/nfs/nfs4state.c  | 18 ++++++++++++++++++
>  3 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> index fdef424b0cd3..594f53c3aee5 100644
> --- a/fs/nfs/nfs4_fs.h
> +++ b/fs/nfs/nfs4_fs.h
> @@ -257,6 +257,8 @@ extern int nfs4_set_rw_stateid(nfs4_stateid *stateid,
>                 const struct nfs_lock_context *l_ctx,
>                 fmode_t fmode);
>
> +extern void nfs40_shutdown_session(struct nfs_client *clp);
> +
>  #if defined(CONFIG_NFS_V4_1)
>  static inline struct nfs4_session *nfs4_get_session(const struct nfs_server *server)
>  {
> @@ -269,6 +271,7 @@ extern int nfs41_setup_sequence(struct nfs4_session *session,
>  extern int nfs41_sequence_done(struct rpc_task *, struct nfs4_sequence_res *);
>  extern int nfs4_proc_create_session(struct nfs_client *, struct rpc_cred *);
>  extern int nfs4_proc_destroy_session(struct nfs4_session *, struct rpc_cred *);
> +extern void nfs41_shutdown_session(struct nfs_client *clp, struct nfs4_session *session);
>  extern int nfs4_proc_get_lease_time(struct nfs_client *clp,
>                 struct nfs_fsinfo *fsinfo);
>  extern int nfs4_proc_layoutcommit(struct nfs4_layoutcommit_data *data,
> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
> index 86d6214ea022..bdabbf9b6322 100644
> --- a/fs/nfs/nfs4client.c
> +++ b/fs/nfs/nfs4client.c
> @@ -160,7 +160,7 @@ void nfs41_shutdown_client(struct nfs_client *clp)
>  {
>         if (nfs4_has_session(clp)) {
>                 nfs4_shutdown_ds_clients(clp);
> -               nfs4_destroy_session(clp->cl_session);
> +               nfs41_shutdown_session(clp, clp->cl_session);
>                 nfs4_destroy_clientid(clp);
>         }
>
> @@ -169,10 +169,7 @@ void nfs41_shutdown_client(struct nfs_client *clp)
>
>  void nfs40_shutdown_client(struct nfs_client *clp)
>  {
> -       if (clp->cl_slot_tbl) {
> -               nfs4_shutdown_slot_table(clp->cl_slot_tbl);
> -               kfree(clp->cl_slot_tbl);
> -       }
> +       nfs40_shutdown_session(clp);
>  }
>
>  struct nfs_client *nfs4_alloc_client(const struct nfs_client_initdata *cl_init)
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index f95e3b58bbc3..54fa7e2bc3e3 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -2134,6 +2134,18 @@ out_unlock:
>         return status;
>  }
>
> +void nfs40_shutdown_session(struct nfs_client *clp)
> +{
> +       struct nfs4_slot_table  *tbl = clp->cl_slot_tbl;
> +
> +       if (tbl) {
> +               nfs4_drain_slot_tbl(tbl);
> +               nfs4_shutdown_slot_table(tbl);
> +               clp->cl_slot_tbl = NULL;
> +               kfree(tbl);
> +       }
> +}
> +
>  #ifdef CONFIG_NFS_V4_1
>  void nfs4_schedule_session_recovery(struct nfs4_session *session, int err)
>  {
> @@ -2314,6 +2326,12 @@ static int nfs4_bind_conn_to_session(struct nfs_client *clp)
>         }
>         return 0;
>  }
> +
> +void nfs41_shutdown_session(struct nfs_client *clp, struct nfs4_session *session)
> +{
> +       nfs4_begin_drain_session(clp);

Argh. We can't quite do this, because we do want all outstanding RPC
calls to complete, not just the ones that have already been allocated
slots. Let me respin.

> +       nfs4_destroy_session(session);
> +}
>  #else /* CONFIG_NFS_V4_1 */
>  static int nfs4_reset_session(struct nfs_client *clp) { return 0; }
>
> --
> 2.1.0
>
>
>
>



-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com

[PATCH v2 1/2] NFSv4: Ensure that we drain the session before shutting it down

[Trond Myklebust]

Kinglong Mee reports that the call to DESTROY_SESSION in NFSv4.1
are racing with the asynchronous DELEGRETURN calls that precede it.
This points to the root cause being that we're not waiting for the
session to drain before we destroy it.

This patch ensures that we do so for both NFSv4 and NFSv4.1.

Reported-by: Kinglong Mee <kinglongmee@gmail.com>
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/nfs4client.c  |  7 ++-----
 fs/nfs/nfs4session.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
 fs/nfs/nfs4session.h |  4 ++++
 fs/nfs/nfs4state.c   | 15 ++++++++++++---
 4 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index 86d6214ea022..ffb12efb1596 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -160,7 +160,7 @@ void nfs41_shutdown_client(struct nfs_client *clp)
 {
 	if (nfs4_has_session(clp)) {
 		nfs4_shutdown_ds_clients(clp);
-		nfs4_destroy_session(clp->cl_session);
+		nfs41_shutdown_session(clp->cl_session);
 		nfs4_destroy_clientid(clp);
 	}
 
@@ -169,10 +169,7 @@ void nfs41_shutdown_client(struct nfs_client *clp)
 
 void nfs40_shutdown_client(struct nfs_client *clp)
 {
-	if (clp->cl_slot_tbl) {
-		nfs4_shutdown_slot_table(clp->cl_slot_tbl);
-		kfree(clp->cl_slot_tbl);
-	}
+	nfs40_shutdown_session(clp);
 }
 
 struct nfs_client *nfs4_alloc_client(const struct nfs_client_initdata *cl_init)
diff --git a/fs/nfs/nfs4session.c b/fs/nfs/nfs4session.c
index e23366effcfb..67c002a24d8f 100644
--- a/fs/nfs/nfs4session.c
+++ b/fs/nfs/nfs4session.c
@@ -59,8 +59,14 @@ static void nfs4_shrink_slot_table(struct nfs4_slot_table  *tbl, u32 newsize)
  */
 void nfs4_slot_tbl_drain_complete(struct nfs4_slot_table *tbl)
 {
-	if (nfs4_slot_tbl_draining(tbl))
-		complete(&tbl->complete);
+	/* Note: no need for atomicity between test and clear, so we can
+	 * optimise for the read-only case where NFS4_SLOT_TBL_WAIT_EMPTY
+	 * is not set.
+	 */
+	if (test_bit(NFS4_SLOT_TBL_WAIT_EMPTY, &tbl->slot_tbl_state)) {
+		clear_bit(NFS4_SLOT_TBL_WAIT_EMPTY, &tbl->slot_tbl_state);
+		complete_all(&tbl->complete);
+	}
 }
 
 /*
@@ -319,7 +325,42 @@ void nfs41_wake_slot_table(struct nfs4_slot_table *tbl)
 	}
 }
 
+void nfs40_shutdown_session(struct nfs_client *clp)
+{
+	struct nfs4_slot_table  *tbl = clp->cl_slot_tbl;
+
+	if (tbl) {
+		nfs4_wait_empty_slot_tbl(tbl);
+		nfs4_shutdown_slot_table(tbl);
+		clp->cl_slot_tbl = NULL;
+		kfree(tbl);
+	}
+}
+
 #if defined(CONFIG_NFS_V4_1)
+static void nfs41_shutdown_session_bc(struct nfs4_session *session)
+{
+	if (session->flags & SESSION4_BACK_CHAN) {
+		session->flags &= ~SESSION4_BACK_CHAN;
+		/* wait for back channel to drain */
+		nfs4_wait_empty_slot_tbl(&session->bc_slot_table);
+	}
+}
+
+static void nfs41_shutdown_session_fc(struct nfs4_session *session)
+{
+	/* just wait for forward channel to drain */
+	nfs4_wait_empty_slot_tbl(&session->bc_slot_table);
+}
+
+void nfs41_shutdown_session(struct nfs4_session *session)
+{
+	if (!test_bit(NFS4_SESSION_ESTABLISHED, &session->session_state))
+		return;
+	nfs41_shutdown_session_bc(session);
+	nfs41_shutdown_session_fc(session);
+	nfs4_destroy_session(session);
+}
 
 static void nfs41_set_max_slotid_locked(struct nfs4_slot_table *tbl,
 		u32 target_highest_slotid)
diff --git a/fs/nfs/nfs4session.h b/fs/nfs/nfs4session.h
index e3ea2c5324d6..1912b250fcab 100644
--- a/fs/nfs/nfs4session.h
+++ b/fs/nfs/nfs4session.h
@@ -27,6 +27,7 @@ struct nfs4_slot {
 /* Sessions */
 enum nfs4_slot_tbl_state {
 	NFS4_SLOT_TBL_DRAINING,
+	NFS4_SLOT_TBL_WAIT_EMPTY,
 };
 
 #define SLOT_TABLE_SZ DIV_ROUND_UP(NFS4_MAX_SLOT_TABLE, 8*sizeof(long))
@@ -78,10 +79,12 @@ extern int nfs4_setup_slot_table(struct nfs4_slot_table *tbl,
 extern void nfs4_shutdown_slot_table(struct nfs4_slot_table *tbl);
 extern struct nfs4_slot *nfs4_alloc_slot(struct nfs4_slot_table *tbl);
 extern void nfs4_free_slot(struct nfs4_slot_table *tbl, struct nfs4_slot *slot);
+extern int nfs4_wait_empty_slot_tbl(struct nfs4_slot_table *tbl);
 extern void nfs4_slot_tbl_drain_complete(struct nfs4_slot_table *tbl);
 bool nfs41_wake_and_assign_slot(struct nfs4_slot_table *tbl,
 		struct nfs4_slot *slot);
 void nfs41_wake_slot_table(struct nfs4_slot_table *tbl);
+extern void nfs40_shutdown_session(struct nfs_client *clp);
 
 static inline bool nfs4_slot_tbl_draining(struct nfs4_slot_table *tbl)
 {
@@ -101,6 +104,7 @@ extern struct nfs4_session *nfs4_alloc_session(struct nfs_client *clp);
 extern void nfs4_destroy_session(struct nfs4_session *session);
 extern int nfs4_init_session(struct nfs_client *clp);
 extern int nfs4_init_ds_session(struct nfs_client *, unsigned long);
+extern void nfs41_shutdown_session(struct nfs4_session *session);
 
 /*
  * Determine if sessions are in use.
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index f95e3b58bbc3..bd5293db4e5b 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -239,12 +239,13 @@ static void nfs4_end_drain_session(struct nfs_client *clp)
 	}
 }
 
-static int nfs4_drain_slot_tbl(struct nfs4_slot_table *tbl)
+int nfs4_wait_empty_slot_tbl(struct nfs4_slot_table *tbl)
 {
-	set_bit(NFS4_SLOT_TBL_DRAINING, &tbl->slot_tbl_state);
 	spin_lock(&tbl->slot_tbl_lock);
 	if (tbl->highest_used_slotid != NFS4_NO_SLOT) {
-		reinit_completion(&tbl->complete);
+		if (!test_and_set_bit(NFS4_SLOT_TBL_WAIT_EMPTY,
+					&tbl->slot_tbl_state))
+			reinit_completion(&tbl->complete);
 		spin_unlock(&tbl->slot_tbl_lock);
 		return wait_for_completion_interruptible(&tbl->complete);
 	}
@@ -252,6 +253,14 @@ static int nfs4_drain_slot_tbl(struct nfs4_slot_table *tbl)
 	return 0;
 }
 
+int nfs4_drain_slot_tbl(struct nfs4_slot_table *tbl)
+{
+	/* Block new RPC calls */
+	set_bit(NFS4_SLOT_TBL_DRAINING, &tbl->slot_tbl_state);
+	/* Wait on outstanding RPC calls to complete */
+	return nfs4_wait_empty_slot_tbl(tbl);
+}
+
 static int nfs4_begin_drain_session(struct nfs_client *clp)
 {
 	struct nfs4_session *ses = clp->cl_session;
-- 
2.1.0

[PATCH v2 2/2] NFSv4: Cleanup - move slot_table drain functions into fs/nfs/nfs4session.c

[Trond Myklebust]

Also make nfs4_shutdown_slot_table() static now that it is no longer used
outside nfs4session.c.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/nfs4session.c | 33 ++++++++++++++++++++++++++++++++-
 fs/nfs/nfs4session.h |  4 ++--
 fs/nfs/nfs4state.c   | 31 -------------------------------
 3 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/fs/nfs/nfs4session.c b/fs/nfs/nfs4session.c
index 67c002a24d8f..ee8a8ee7c27d 100644
--- a/fs/nfs/nfs4session.c
+++ b/fs/nfs/nfs4session.c
@@ -69,6 +69,37 @@ void nfs4_slot_tbl_drain_complete(struct nfs4_slot_table *tbl)
 	}
 }
 
+static int nfs4_wait_empty_slot_tbl(struct nfs4_slot_table *tbl)
+{
+	spin_lock(&tbl->slot_tbl_lock);
+	if (tbl->highest_used_slotid != NFS4_NO_SLOT) {
+		if (!test_and_set_bit(NFS4_SLOT_TBL_WAIT_EMPTY,
+					&tbl->slot_tbl_state))
+			reinit_completion(&tbl->complete);
+		spin_unlock(&tbl->slot_tbl_lock);
+		return wait_for_completion_interruptible(&tbl->complete);
+	}
+	spin_unlock(&tbl->slot_tbl_lock);
+	return 0;
+}
+
+int nfs4_drain_slot_tbl(struct nfs4_slot_table *tbl)
+{
+	/* Block new RPC calls */
+	set_bit(NFS4_SLOT_TBL_DRAINING, &tbl->slot_tbl_state);
+	/* Wait on outstanding RPC calls to complete */
+	return nfs4_wait_empty_slot_tbl(tbl);
+}
+
+void nfs4_end_drain_slot_table(struct nfs4_slot_table *tbl)
+{
+	if (test_and_clear_bit(NFS4_SLOT_TBL_DRAINING, &tbl->slot_tbl_state)) {
+		spin_lock(&tbl->slot_tbl_lock);
+		nfs41_wake_slot_table(tbl);
+		spin_unlock(&tbl->slot_tbl_lock);
+	}
+}
+
 /*
  * nfs4_free_slot - free a slot and efficiently update slot table.
  *
@@ -250,7 +281,7 @@ static void nfs4_release_slot_table(struct nfs4_slot_table *tbl)
  * @tbl: slot table to shut down
  *
  */
-void nfs4_shutdown_slot_table(struct nfs4_slot_table *tbl)
+static void nfs4_shutdown_slot_table(struct nfs4_slot_table *tbl)
 {
 	nfs4_release_slot_table(tbl);
 	rpc_destroy_wait_queue(&tbl->slot_tbl_waitq);
diff --git a/fs/nfs/nfs4session.h b/fs/nfs/nfs4session.h
index 1912b250fcab..9181f7e1374a 100644
--- a/fs/nfs/nfs4session.h
+++ b/fs/nfs/nfs4session.h
@@ -76,10 +76,10 @@ enum nfs4_session_state {
 
 extern int nfs4_setup_slot_table(struct nfs4_slot_table *tbl,
 		unsigned int max_reqs, const char *queue);
-extern void nfs4_shutdown_slot_table(struct nfs4_slot_table *tbl);
 extern struct nfs4_slot *nfs4_alloc_slot(struct nfs4_slot_table *tbl);
 extern void nfs4_free_slot(struct nfs4_slot_table *tbl, struct nfs4_slot *slot);
-extern int nfs4_wait_empty_slot_tbl(struct nfs4_slot_table *tbl);
+extern int nfs4_drain_slot_tbl(struct nfs4_slot_table *tbl);
+extern void nfs4_end_drain_slot_table(struct nfs4_slot_table *tbl);
 extern void nfs4_slot_tbl_drain_complete(struct nfs4_slot_table *tbl);
 bool nfs41_wake_and_assign_slot(struct nfs4_slot_table *tbl,
 		struct nfs4_slot *slot);
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index bd5293db4e5b..8fbf770d27ac 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -215,15 +215,6 @@ out:
 	return cred;
 }
 
-static void nfs4_end_drain_slot_table(struct nfs4_slot_table *tbl)
-{
-	if (test_and_clear_bit(NFS4_SLOT_TBL_DRAINING, &tbl->slot_tbl_state)) {
-		spin_lock(&tbl->slot_tbl_lock);
-		nfs41_wake_slot_table(tbl);
-		spin_unlock(&tbl->slot_tbl_lock);
-	}
-}
-
 static void nfs4_end_drain_session(struct nfs_client *clp)
 {
 	struct nfs4_session *ses = clp->cl_session;
@@ -239,28 +230,6 @@ static void nfs4_end_drain_session(struct nfs_client *clp)
 	}
 }
 
-int nfs4_wait_empty_slot_tbl(struct nfs4_slot_table *tbl)
-{
-	spin_lock(&tbl->slot_tbl_lock);
-	if (tbl->highest_used_slotid != NFS4_NO_SLOT) {
-		if (!test_and_set_bit(NFS4_SLOT_TBL_WAIT_EMPTY,
-					&tbl->slot_tbl_state))
-			reinit_completion(&tbl->complete);
-		spin_unlock(&tbl->slot_tbl_lock);
-		return wait_for_completion_interruptible(&tbl->complete);
-	}
-	spin_unlock(&tbl->slot_tbl_lock);
-	return 0;
-}
-
-int nfs4_drain_slot_tbl(struct nfs4_slot_table *tbl)
-{
-	/* Block new RPC calls */
-	set_bit(NFS4_SLOT_TBL_DRAINING, &tbl->slot_tbl_state);
-	/* Wait on outstanding RPC calls to complete */
-	return nfs4_wait_empty_slot_tbl(tbl);
-}
-
 static int nfs4_begin_drain_session(struct nfs_client *clp)
 {
 	struct nfs4_session *ses = clp->cl_session;
-- 
2.1.0

Re: [PATCH v2 1/2] NFSv4: Ensure that we drain the session before shutting it down

[Kinglong Mee]

With those two patches, the problem also exist.
After debugging, i found,
1. Before unmount, nfs client holds a read delegation.
2. When unmountting, nfs_kill_super will be called.
2.1, In nfs_kill_super, generic_shutdown_super() will be called,
     which will causes delegation return (async).
     DELEGRETURN is sent though **server->client**.
2.2, after that (delegreturn's reply maybe not received), 
     nfs_free_server() will be called.
3. In nfs_free_server(), 
3.1, rpc_shutdown_client(server->client); will call rpc_killall_tasks()
     to killing all tasks in server->client RPC client.
     So, DELEGRETURN maybe killed here.
3.2, nfs_put_client(server->nfs_client); will destroy session 
     and clientid.
     DESTROY_SESSION and DESTROY_CLIENTID are sent though 
     *server->nfs_client->cl_rpcclient*.

And, server->client is a clone of server->nfs_client->cl_rpcclient,
they are two different rpcclient.

So, the really problem is 3.1, rpc_killall_tasks may kill DELEGRETURN,
which maybe be processed by nfsd, nfs will **clean the used slot**.

Before DELEGRETURN reply, the used slot have be cleaned, so that,
3.2's DESTROY_SESSION request will be sent to nfsd immediately,
and return an error NFS4ERR_DELAY for client's delegation.

thanks,
Kinglong Mee

On 2015/3/24 4:21, Trond Myklebust wrote:
> Kinglong Mee reports that the call to DESTROY_SESSION in NFSv4.1
> are racing with the asynchronous DELEGRETURN calls that precede it.
> This points to the root cause being that we're not waiting for the
> session to drain before we destroy it.
> 
> This patch ensures that we do so for both NFSv4 and NFSv4.1.
> 
> Reported-by: Kinglong Mee <kinglongmee@gmail.com>
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
>  fs/nfs/nfs4client.c  |  7 ++-----
>  fs/nfs/nfs4session.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
>  fs/nfs/nfs4session.h |  4 ++++
>  fs/nfs/nfs4state.c   | 15 ++++++++++++---
>  4 files changed, 61 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
> index 86d6214ea022..ffb12efb1596 100644
> --- a/fs/nfs/nfs4client.c
> +++ b/fs/nfs/nfs4client.c
> @@ -160,7 +160,7 @@ void nfs41_shutdown_client(struct nfs_client *clp)
>  {
>  	if (nfs4_has_session(clp)) {
>  		nfs4_shutdown_ds_clients(clp);
> -		nfs4_destroy_session(clp->cl_session);
> +		nfs41_shutdown_session(clp->cl_session);
>  		nfs4_destroy_clientid(clp);
>  	}
>  
> @@ -169,10 +169,7 @@ void nfs41_shutdown_client(struct nfs_client *clp)
>  
>  void nfs40_shutdown_client(struct nfs_client *clp)
>  {
> -	if (clp->cl_slot_tbl) {
> -		nfs4_shutdown_slot_table(clp->cl_slot_tbl);
> -		kfree(clp->cl_slot_tbl);
> -	}
> +	nfs40_shutdown_session(clp);
>  }
>  
>  struct nfs_client *nfs4_alloc_client(const struct nfs_client_initdata *cl_init)
> diff --git a/fs/nfs/nfs4session.c b/fs/nfs/nfs4session.c
> index e23366effcfb..67c002a24d8f 100644
> --- a/fs/nfs/nfs4session.c
> +++ b/fs/nfs/nfs4session.c
> @@ -59,8 +59,14 @@ static void nfs4_shrink_slot_table(struct nfs4_slot_table  *tbl, u32 newsize)
>   */
>  void nfs4_slot_tbl_drain_complete(struct nfs4_slot_table *tbl)
>  {
> -	if (nfs4_slot_tbl_draining(tbl))
> -		complete(&tbl->complete);
> +	/* Note: no need for atomicity between test and clear, so we can
> +	 * optimise for the read-only case where NFS4_SLOT_TBL_WAIT_EMPTY
> +	 * is not set.
> +	 */
> +	if (test_bit(NFS4_SLOT_TBL_WAIT_EMPTY, &tbl->slot_tbl_state)) {
> +		clear_bit(NFS4_SLOT_TBL_WAIT_EMPTY, &tbl->slot_tbl_state);
> +		complete_all(&tbl->complete);
> +	}
>  }
>  
>  /*
> @@ -319,7 +325,42 @@ void nfs41_wake_slot_table(struct nfs4_slot_table *tbl)
>  	}
>  }
>  
> +void nfs40_shutdown_session(struct nfs_client *clp)
> +{
> +	struct nfs4_slot_table  *tbl = clp->cl_slot_tbl;
> +
> +	if (tbl) {
> +		nfs4_wait_empty_slot_tbl(tbl);
> +		nfs4_shutdown_slot_table(tbl);
> +		clp->cl_slot_tbl = NULL;
> +		kfree(tbl);
> +	}
> +}
> +
>  #if defined(CONFIG_NFS_V4_1)
> +static void nfs41_shutdown_session_bc(struct nfs4_session *session)
> +{
> +	if (session->flags & SESSION4_BACK_CHAN) {
> +		session->flags &= ~SESSION4_BACK_CHAN;
> +		/* wait for back channel to drain */
> +		nfs4_wait_empty_slot_tbl(&session->bc_slot_table);
> +	}
> +}
> +
> +static void nfs41_shutdown_session_fc(struct nfs4_session *session)
> +{
> +	/* just wait for forward channel to drain */
> +	nfs4_wait_empty_slot_tbl(&session->bc_slot_table);
> +}
> +
> +void nfs41_shutdown_session(struct nfs4_session *session)
> +{
> +	if (!test_bit(NFS4_SESSION_ESTABLISHED, &session->session_state))
> +		return;
> +	nfs41_shutdown_session_bc(session);
> +	nfs41_shutdown_session_fc(session);
> +	nfs4_destroy_session(session);
> +}
>  
>  static void nfs41_set_max_slotid_locked(struct nfs4_slot_table *tbl,
>  		u32 target_highest_slotid)
> diff --git a/fs/nfs/nfs4session.h b/fs/nfs/nfs4session.h
> index e3ea2c5324d6..1912b250fcab 100644
> --- a/fs/nfs/nfs4session.h
> +++ b/fs/nfs/nfs4session.h
> @@ -27,6 +27,7 @@ struct nfs4_slot {
>  /* Sessions */
>  enum nfs4_slot_tbl_state {
>  	NFS4_SLOT_TBL_DRAINING,
> +	NFS4_SLOT_TBL_WAIT_EMPTY,
>  };
>  
>  #define SLOT_TABLE_SZ DIV_ROUND_UP(NFS4_MAX_SLOT_TABLE, 8*sizeof(long))
> @@ -78,10 +79,12 @@ extern int nfs4_setup_slot_table(struct nfs4_slot_table *tbl,
>  extern void nfs4_shutdown_slot_table(struct nfs4_slot_table *tbl);
>  extern struct nfs4_slot *nfs4_alloc_slot(struct nfs4_slot_table *tbl);
>  extern void nfs4_free_slot(struct nfs4_slot_table *tbl, struct nfs4_slot *slot);
> +extern int nfs4_wait_empty_slot_tbl(struct nfs4_slot_table *tbl);
>  extern void nfs4_slot_tbl_drain_complete(struct nfs4_slot_table *tbl);
>  bool nfs41_wake_and_assign_slot(struct nfs4_slot_table *tbl,
>  		struct nfs4_slot *slot);
>  void nfs41_wake_slot_table(struct nfs4_slot_table *tbl);
> +extern void nfs40_shutdown_session(struct nfs_client *clp);
>  
>  static inline bool nfs4_slot_tbl_draining(struct nfs4_slot_table *tbl)
>  {
> @@ -101,6 +104,7 @@ extern struct nfs4_session *nfs4_alloc_session(struct nfs_client *clp);
>  extern void nfs4_destroy_session(struct nfs4_session *session);
>  extern int nfs4_init_session(struct nfs_client *clp);
>  extern int nfs4_init_ds_session(struct nfs_client *, unsigned long);
> +extern void nfs41_shutdown_session(struct nfs4_session *session);
>  
>  /*
>   * Determine if sessions are in use.
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index f95e3b58bbc3..bd5293db4e5b 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -239,12 +239,13 @@ static void nfs4_end_drain_session(struct nfs_client *clp)
>  	}
>  }
>  
> -static int nfs4_drain_slot_tbl(struct nfs4_slot_table *tbl)
> +int nfs4_wait_empty_slot_tbl(struct nfs4_slot_table *tbl)
>  {
> -	set_bit(NFS4_SLOT_TBL_DRAINING, &tbl->slot_tbl_state);
>  	spin_lock(&tbl->slot_tbl_lock);
>  	if (tbl->highest_used_slotid != NFS4_NO_SLOT) {
> -		reinit_completion(&tbl->complete);
> +		if (!test_and_set_bit(NFS4_SLOT_TBL_WAIT_EMPTY,
> +					&tbl->slot_tbl_state))
> +			reinit_completion(&tbl->complete);
>  		spin_unlock(&tbl->slot_tbl_lock);
>  		return wait_for_completion_interruptible(&tbl->complete);
>  	}
> @@ -252,6 +253,14 @@ static int nfs4_drain_slot_tbl(struct nfs4_slot_table *tbl)
>  	return 0;
>  }
>  
> +int nfs4_drain_slot_tbl(struct nfs4_slot_table *tbl)
> +{
> +	/* Block new RPC calls */
> +	set_bit(NFS4_SLOT_TBL_DRAINING, &tbl->slot_tbl_state);
> +	/* Wait on outstanding RPC calls to complete */
> +	return nfs4_wait_empty_slot_tbl(tbl);
> +}
> +
>  static int nfs4_begin_drain_session(struct nfs_client *clp)
>  {
>  	struct nfs4_session *ses = clp->cl_session;
> 

Re: [PATCH v2 1/2] NFSv4: Ensure that we drain the session before shutting it down

[Kinglong Mee]

On 2015/3/25 1:00, Kinglong Mee wrote:
> With those two patches, the problem also exist.
> After debugging, i found,
> 1. Before unmount, nfs client holds a read delegation.
> 2. When unmountting, nfs_kill_super will be called.
> 2.1, In nfs_kill_super, generic_shutdown_super() will be called,
>      which will causes delegation return (async).
>      DELEGRETURN is sent though **server->client**.
> 2.2, after that (delegreturn's reply maybe not received), 
>      nfs_free_server() will be called.
> 3. In nfs_free_server(), 
> 3.1, rpc_shutdown_client(server->client); will call rpc_killall_tasks()
>      to killing all tasks in server->client RPC client.
>      So, DELEGRETURN maybe killed here.
> 3.2, nfs_put_client(server->nfs_client); will destroy session 
>      and clientid.
>      DESTROY_SESSION and DESTROY_CLIENTID are sent though 
>      *server->nfs_client->cl_rpcclient*.
> 
> And, server->client is a clone of server->nfs_client->cl_rpcclient,
> they are two different rpcclient.
> 
> So, the really problem is 3.1, rpc_killall_tasks may kill DELEGRETURN,
> which maybe be processed by nfsd, nfs will **clean the used slot**.
> 
> Before DELEGRETURN reply, the used slot have be cleaned, so that,
> 3.2's DESTROY_SESSION request will be sent to nfsd immediately,
> and return an error NFS4ERR_DELAY for client's delegation.

After fixing the new race, I will test with those patches.

thanks,
Kinglong Mee

> On 2015/3/24 4:21, Trond Myklebust wrote:
>> Kinglong Mee reports that the call to DESTROY_SESSION in NFSv4.1
>> are racing with the asynchronous DELEGRETURN calls that precede it.
>> This points to the root cause being that we're not waiting for the
>> session to drain before we destroy it.
>>
>> This patch ensures that we do so for both NFSv4 and NFSv4.1.
>>
>> Reported-by: Kinglong Mee <kinglongmee@gmail.com>
>> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>> ---
>>  fs/nfs/nfs4client.c  |  7 ++-----
>>  fs/nfs/nfs4session.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
>>  fs/nfs/nfs4session.h |  4 ++++
>>  fs/nfs/nfs4state.c   | 15 ++++++++++++---
>>  4 files changed, 61 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
>> index 86d6214ea022..ffb12efb1596 100644
>> --- a/fs/nfs/nfs4client.c
>> +++ b/fs/nfs/nfs4client.c
>> @@ -160,7 +160,7 @@ void nfs41_shutdown_client(struct nfs_client *clp)
>>  {
>>  	if (nfs4_has_session(clp)) {
>>  		nfs4_shutdown_ds_clients(clp);
>> -		nfs4_destroy_session(clp->cl_session);
>> +		nfs41_shutdown_session(clp->cl_session);
>>  		nfs4_destroy_clientid(clp);
>>  	}
>>  
>> @@ -169,10 +169,7 @@ void nfs41_shutdown_client(struct nfs_client *clp)
>>  
>>  void nfs40_shutdown_client(struct nfs_client *clp)
>>  {
>> -	if (clp->cl_slot_tbl) {
>> -		nfs4_shutdown_slot_table(clp->cl_slot_tbl);
>> -		kfree(clp->cl_slot_tbl);
>> -	}
>> +	nfs40_shutdown_session(clp);
>>  }
>>  
>>  struct nfs_client *nfs4_alloc_client(const struct nfs_client_initdata *cl_init)
>> diff --git a/fs/nfs/nfs4session.c b/fs/nfs/nfs4session.c
>> index e23366effcfb..67c002a24d8f 100644
>> --- a/fs/nfs/nfs4session.c
>> +++ b/fs/nfs/nfs4session.c
>> @@ -59,8 +59,14 @@ static void nfs4_shrink_slot_table(struct nfs4_slot_table  *tbl, u32 newsize)
>>   */
>>  void nfs4_slot_tbl_drain_complete(struct nfs4_slot_table *tbl)
>>  {
>> -	if (nfs4_slot_tbl_draining(tbl))
>> -		complete(&tbl->complete);
>> +	/* Note: no need for atomicity between test and clear, so we can
>> +	 * optimise for the read-only case where NFS4_SLOT_TBL_WAIT_EMPTY
>> +	 * is not set.
>> +	 */
>> +	if (test_bit(NFS4_SLOT_TBL_WAIT_EMPTY, &tbl->slot_tbl_state)) {
>> +		clear_bit(NFS4_SLOT_TBL_WAIT_EMPTY, &tbl->slot_tbl_state);
>> +		complete_all(&tbl->complete);
>> +	}
>>  }
>>  
>>  /*
>> @@ -319,7 +325,42 @@ void nfs41_wake_slot_table(struct nfs4_slot_table *tbl)
>>  	}
>>  }
>>  
>> +void nfs40_shutdown_session(struct nfs_client *clp)
>> +{
>> +	struct nfs4_slot_table  *tbl = clp->cl_slot_tbl;
>> +
>> +	if (tbl) {
>> +		nfs4_wait_empty_slot_tbl(tbl);
>> +		nfs4_shutdown_slot_table(tbl);
>> +		clp->cl_slot_tbl = NULL;
>> +		kfree(tbl);
>> +	}
>> +}
>> +
>>  #if defined(CONFIG_NFS_V4_1)
>> +static void nfs41_shutdown_session_bc(struct nfs4_session *session)
>> +{
>> +	if (session->flags & SESSION4_BACK_CHAN) {
>> +		session->flags &= ~SESSION4_BACK_CHAN;
>> +		/* wait for back channel to drain */
>> +		nfs4_wait_empty_slot_tbl(&session->bc_slot_table);
>> +	}
>> +}
>> +
>> +static void nfs41_shutdown_session_fc(struct nfs4_session *session)
>> +{
>> +	/* just wait for forward channel to drain */
>> +	nfs4_wait_empty_slot_tbl(&session->bc_slot_table);
>> +}
>> +
>> +void nfs41_shutdown_session(struct nfs4_session *session)
>> +{
>> +	if (!test_bit(NFS4_SESSION_ESTABLISHED, &session->session_state))
>> +		return;
>> +	nfs41_shutdown_session_bc(session);
>> +	nfs41_shutdown_session_fc(session);
>> +	nfs4_destroy_session(session);
>> +}
>>  
>>  static void nfs41_set_max_slotid_locked(struct nfs4_slot_table *tbl,
>>  		u32 target_highest_slotid)
>> diff --git a/fs/nfs/nfs4session.h b/fs/nfs/nfs4session.h
>> index e3ea2c5324d6..1912b250fcab 100644
>> --- a/fs/nfs/nfs4session.h
>> +++ b/fs/nfs/nfs4session.h
>> @@ -27,6 +27,7 @@ struct nfs4_slot {
>>  /* Sessions */
>>  enum nfs4_slot_tbl_state {
>>  	NFS4_SLOT_TBL_DRAINING,
>> +	NFS4_SLOT_TBL_WAIT_EMPTY,
>>  };
>>  
>>  #define SLOT_TABLE_SZ DIV_ROUND_UP(NFS4_MAX_SLOT_TABLE, 8*sizeof(long))
>> @@ -78,10 +79,12 @@ extern int nfs4_setup_slot_table(struct nfs4_slot_table *tbl,
>>  extern void nfs4_shutdown_slot_table(struct nfs4_slot_table *tbl);
>>  extern struct nfs4_slot *nfs4_alloc_slot(struct nfs4_slot_table *tbl);
>>  extern void nfs4_free_slot(struct nfs4_slot_table *tbl, struct nfs4_slot *slot);
>> +extern int nfs4_wait_empty_slot_tbl(struct nfs4_slot_table *tbl);
>>  extern void nfs4_slot_tbl_drain_complete(struct nfs4_slot_table *tbl);
>>  bool nfs41_wake_and_assign_slot(struct nfs4_slot_table *tbl,
>>  		struct nfs4_slot *slot);
>>  void nfs41_wake_slot_table(struct nfs4_slot_table *tbl);
>> +extern void nfs40_shutdown_session(struct nfs_client *clp);
>>  
>>  static inline bool nfs4_slot_tbl_draining(struct nfs4_slot_table *tbl)
>>  {
>> @@ -101,6 +104,7 @@ extern struct nfs4_session *nfs4_alloc_session(struct nfs_client *clp);
>>  extern void nfs4_destroy_session(struct nfs4_session *session);
>>  extern int nfs4_init_session(struct nfs_client *clp);
>>  extern int nfs4_init_ds_session(struct nfs_client *, unsigned long);
>> +extern void nfs41_shutdown_session(struct nfs4_session *session);
>>  
>>  /*
>>   * Determine if sessions are in use.
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index f95e3b58bbc3..bd5293db4e5b 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -239,12 +239,13 @@ static void nfs4_end_drain_session(struct nfs_client *clp)
>>  	}
>>  }
>>  
>> -static int nfs4_drain_slot_tbl(struct nfs4_slot_table *tbl)
>> +int nfs4_wait_empty_slot_tbl(struct nfs4_slot_table *tbl)
>>  {
>> -	set_bit(NFS4_SLOT_TBL_DRAINING, &tbl->slot_tbl_state);
>>  	spin_lock(&tbl->slot_tbl_lock);
>>  	if (tbl->highest_used_slotid != NFS4_NO_SLOT) {
>> -		reinit_completion(&tbl->complete);
>> +		if (!test_and_set_bit(NFS4_SLOT_TBL_WAIT_EMPTY,
>> +					&tbl->slot_tbl_state))
>> +			reinit_completion(&tbl->complete);
>>  		spin_unlock(&tbl->slot_tbl_lock);
>>  		return wait_for_completion_interruptible(&tbl->complete);
>>  	}
>> @@ -252,6 +253,14 @@ static int nfs4_drain_slot_tbl(struct nfs4_slot_table *tbl)
>>  	return 0;
>>  }
>>  
>> +int nfs4_drain_slot_tbl(struct nfs4_slot_table *tbl)
>> +{
>> +	/* Block new RPC calls */
>> +	set_bit(NFS4_SLOT_TBL_DRAINING, &tbl->slot_tbl_state);
>> +	/* Wait on outstanding RPC calls to complete */
>> +	return nfs4_wait_empty_slot_tbl(tbl);
>> +}
>> +
>>  static int nfs4_begin_drain_session(struct nfs_client *clp)
>>  {
>>  	struct nfs4_session *ses = clp->cl_session;
>>

Re: [PATCH v2 1/2] NFSv4: Ensure that we drain the session before shutting it down

[Olga Kornievskaia]

Hi Trond, Kinglong,

What has happened to this patch?

I believe the lack of this patch is causing the following problem on
the nfsv4.1 mount:
1. client has delegations
2. unmount is initiated which as Kinglong points out:
-- initiates asynchronous DELEGRETURNs.
-- then in nfs_free_server() it ends up killing ongoing rpc tasks with
DELEGRETURN.
-- nfs41_proc_sequence() takes a reference on the client structure.
However since the RPCs are killed there is no call to nfs_put_client()
which is done in the nfs4_sequence_release()
-- then in nfs_put_client() the reference count doesn't go down to 0
and DESTROY_SESSION isn't called. The user's umount succeeds but we
still have the client structure with a session.

I believe this patch that waits for the session to be drained would
not have the reference count problem.


On Mon, Mar 23, 2015 at 4:21 PM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> Kinglong Mee reports that the call to DESTROY_SESSION in NFSv4.1
> are racing with the asynchronous DELEGRETURN calls that precede it.
> This points to the root cause being that we're not waiting for the
> session to drain before we destroy it.
>
> This patch ensures that we do so for both NFSv4 and NFSv4.1.
>
> Reported-by: Kinglong Mee <kinglongmee@gmail.com>
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
>  fs/nfs/nfs4client.c  |  7 ++-----
>  fs/nfs/nfs4session.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
>  fs/nfs/nfs4session.h |  4 ++++
>  fs/nfs/nfs4state.c   | 15 ++++++++++++---
>  4 files changed, 61 insertions(+), 10 deletions(-)
>
> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
> index 86d6214ea022..ffb12efb1596 100644
> --- a/fs/nfs/nfs4client.c
> +++ b/fs/nfs/nfs4client.c
> @@ -160,7 +160,7 @@ void nfs41_shutdown_client(struct nfs_client *clp)
>  {
>         if (nfs4_has_session(clp)) {
>                 nfs4_shutdown_ds_clients(clp);
> -               nfs4_destroy_session(clp->cl_session);
> +               nfs41_shutdown_session(clp->cl_session);
>                 nfs4_destroy_clientid(clp);
>         }
>
> @@ -169,10 +169,7 @@ void nfs41_shutdown_client(struct nfs_client *clp)
>
>  void nfs40_shutdown_client(struct nfs_client *clp)
>  {
> -       if (clp->cl_slot_tbl) {
> -               nfs4_shutdown_slot_table(clp->cl_slot_tbl);
> -               kfree(clp->cl_slot_tbl);
> -       }
> +       nfs40_shutdown_session(clp);
>  }
>
>  struct nfs_client *nfs4_alloc_client(const struct nfs_client_initdata *cl_init)
> diff --git a/fs/nfs/nfs4session.c b/fs/nfs/nfs4session.c
> index e23366effcfb..67c002a24d8f 100644
> --- a/fs/nfs/nfs4session.c
> +++ b/fs/nfs/nfs4session.c
> @@ -59,8 +59,14 @@ static void nfs4_shrink_slot_table(struct nfs4_slot_table  *tbl, u32 newsize)
>   */
>  void nfs4_slot_tbl_drain_complete(struct nfs4_slot_table *tbl)
>  {
> -       if (nfs4_slot_tbl_draining(tbl))
> -               complete(&tbl->complete);
> +       /* Note: no need for atomicity between test and clear, so we can
> +        * optimise for the read-only case where NFS4_SLOT_TBL_WAIT_EMPTY
> +        * is not set.
> +        */
> +       if (test_bit(NFS4_SLOT_TBL_WAIT_EMPTY, &tbl->slot_tbl_state)) {
> +               clear_bit(NFS4_SLOT_TBL_WAIT_EMPTY, &tbl->slot_tbl_state);
> +               complete_all(&tbl->complete);
> +       }
>  }
>
>  /*
> @@ -319,7 +325,42 @@ void nfs41_wake_slot_table(struct nfs4_slot_table *tbl)
>         }
>  }
>
> +void nfs40_shutdown_session(struct nfs_client *clp)
> +{
> +       struct nfs4_slot_table  *tbl = clp->cl_slot_tbl;
> +
> +       if (tbl) {
> +               nfs4_wait_empty_slot_tbl(tbl);
> +               nfs4_shutdown_slot_table(tbl);
> +               clp->cl_slot_tbl = NULL;
> +               kfree(tbl);
> +       }
> +}
> +
>  #if defined(CONFIG_NFS_V4_1)
> +static void nfs41_shutdown_session_bc(struct nfs4_session *session)
> +{
> +       if (session->flags & SESSION4_BACK_CHAN) {
> +               session->flags &= ~SESSION4_BACK_CHAN;
> +               /* wait for back channel to drain */
> +               nfs4_wait_empty_slot_tbl(&session->bc_slot_table);
> +       }
> +}
> +
> +static void nfs41_shutdown_session_fc(struct nfs4_session *session)
> +{
> +       /* just wait for forward channel to drain */
> +       nfs4_wait_empty_slot_tbl(&session->bc_slot_table);
> +}
> +
> +void nfs41_shutdown_session(struct nfs4_session *session)
> +{
> +       if (!test_bit(NFS4_SESSION_ESTABLISHED, &session->session_state))
> +               return;
> +       nfs41_shutdown_session_bc(session);
> +       nfs41_shutdown_session_fc(session);
> +       nfs4_destroy_session(session);
> +}
>
>  static void nfs41_set_max_slotid_locked(struct nfs4_slot_table *tbl,
>                 u32 target_highest_slotid)
> diff --git a/fs/nfs/nfs4session.h b/fs/nfs/nfs4session.h
> index e3ea2c5324d6..1912b250fcab 100644
> --- a/fs/nfs/nfs4session.h
> +++ b/fs/nfs/nfs4session.h
> @@ -27,6 +27,7 @@ struct nfs4_slot {
>  /* Sessions */
>  enum nfs4_slot_tbl_state {
>         NFS4_SLOT_TBL_DRAINING,
> +       NFS4_SLOT_TBL_WAIT_EMPTY,
>  };
>
>  #define SLOT_TABLE_SZ DIV_ROUND_UP(NFS4_MAX_SLOT_TABLE, 8*sizeof(long))
> @@ -78,10 +79,12 @@ extern int nfs4_setup_slot_table(struct nfs4_slot_table *tbl,
>  extern void nfs4_shutdown_slot_table(struct nfs4_slot_table *tbl);
>  extern struct nfs4_slot *nfs4_alloc_slot(struct nfs4_slot_table *tbl);
>  extern void nfs4_free_slot(struct nfs4_slot_table *tbl, struct nfs4_slot *slot);
> +extern int nfs4_wait_empty_slot_tbl(struct nfs4_slot_table *tbl);
>  extern void nfs4_slot_tbl_drain_complete(struct nfs4_slot_table *tbl);
>  bool nfs41_wake_and_assign_slot(struct nfs4_slot_table *tbl,
>                 struct nfs4_slot *slot);
>  void nfs41_wake_slot_table(struct nfs4_slot_table *tbl);
> +extern void nfs40_shutdown_session(struct nfs_client *clp);
>
>  static inline bool nfs4_slot_tbl_draining(struct nfs4_slot_table *tbl)
>  {
> @@ -101,6 +104,7 @@ extern struct nfs4_session *nfs4_alloc_session(struct nfs_client *clp);
>  extern void nfs4_destroy_session(struct nfs4_session *session);
>  extern int nfs4_init_session(struct nfs_client *clp);
>  extern int nfs4_init_ds_session(struct nfs_client *, unsigned long);
> +extern void nfs41_shutdown_session(struct nfs4_session *session);
>
>  /*
>   * Determine if sessions are in use.
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index f95e3b58bbc3..bd5293db4e5b 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -239,12 +239,13 @@ static void nfs4_end_drain_session(struct nfs_client *clp)
>         }
>  }
>
> -static int nfs4_drain_slot_tbl(struct nfs4_slot_table *tbl)
> +int nfs4_wait_empty_slot_tbl(struct nfs4_slot_table *tbl)
>  {
> -       set_bit(NFS4_SLOT_TBL_DRAINING, &tbl->slot_tbl_state);
>         spin_lock(&tbl->slot_tbl_lock);
>         if (tbl->highest_used_slotid != NFS4_NO_SLOT) {
> -               reinit_completion(&tbl->complete);
> +               if (!test_and_set_bit(NFS4_SLOT_TBL_WAIT_EMPTY,
> +                                       &tbl->slot_tbl_state))
> +                       reinit_completion(&tbl->complete);
>                 spin_unlock(&tbl->slot_tbl_lock);
>                 return wait_for_completion_interruptible(&tbl->complete);
>         }
> @@ -252,6 +253,14 @@ static int nfs4_drain_slot_tbl(struct nfs4_slot_table *tbl)
>         return 0;
>  }
>
> +int nfs4_drain_slot_tbl(struct nfs4_slot_table *tbl)
> +{
> +       /* Block new RPC calls */
> +       set_bit(NFS4_SLOT_TBL_DRAINING, &tbl->slot_tbl_state);
> +       /* Wait on outstanding RPC calls to complete */
> +       return nfs4_wait_empty_slot_tbl(tbl);
> +}
> +
>  static int nfs4_begin_drain_session(struct nfs_client *clp)
>  {
>         struct nfs4_session *ses = clp->cl_session;
> --
> 2.1.0
>
> --
> 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

Re: [PATCH v2 1/2] NFSv4: Ensure that we drain the session before shutting it down

[Trond Myklebust]

> On Aug 19, 2016, at 09:41, Olga Kornievskaia <aglo@umich.edu> wrote:
>=20
> Hi Trond, Kinglong,
>=20
> What has happened to this patch?

It was dropped, since it should be unnecessary. The delegreturn calls from =
nfs4_evict_inode() should now be synchronous, and other calls should normal=
ly grab a reference to the inode+super block.

>=20
> I believe the lack of this patch is causing the following problem on
> the nfsv4.1 mount:
> 1. client has delegations
> 2. unmount is initiated which as Kinglong points out:
> -- initiates asynchronous DELEGRETURNs.
> -- then in nfs_free_server() it ends up killing ongoing rpc tasks with
> DELEGRETURN.
> -- nfs41_proc_sequence() takes a reference on the client structure.
> However since the RPCs are killed there is no call to nfs_put_client()
> which is done in the nfs4_sequence_release()

task->tk_ops->rpc_release() is guaranteed to be run.

> -- then in nfs_put_client() the reference count doesn't go down to 0
> and DESTROY_SESSION isn't called. The user's umount succeeds but we
> still have the client structure with a session.