[PATCH 1/1] nfs: fix race between renewd, umount, and the state manager in V4.1

[PATCH 1/1] nfs: fix race between renewd, umount, and the state manager in V4.1

[Alexandros Batsakis]

Signed-off-by: Alexandros Batsakis <batsakis@netapp.com>
---
 fs/nfs/client.c    |   27 +++++++++++++--------------
 fs/nfs/nfs4_fs.h   |    1 +
 fs/nfs/nfs4proc.c  |    4 ++++
 fs/nfs/nfs4state.c |    8 ++++++--
 4 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index d0b060a..6c7d407 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -164,19 +164,6 @@ error_0:
 	return ERR_PTR(err);
 }
 
-static void nfs4_shutdown_client(struct nfs_client *clp)
-{
-#ifdef CONFIG_NFS_V4
-	if (__test_and_clear_bit(NFS_CS_RENEWD, &clp->cl_res_state))
-		nfs4_kill_renewd(clp);
-	BUG_ON(!RB_EMPTY_ROOT(&clp->cl_state_owners));
-	if (__test_and_clear_bit(NFS_CS_IDMAP, &clp->cl_res_state))
-		nfs_idmap_delete(clp);
-
-	rpc_destroy_wait_queue(&clp->cl_rpcwaitq);
-#endif
-}
-
 /*
  * Destroy the NFS4 callback service
  */
@@ -206,6 +193,19 @@ static void nfs4_clear_client_minor_version(struct nfs_client *clp)
 	nfs4_destroy_callback(clp);
 }
 
+static void nfs4_shutdown_client(struct nfs_client *clp)
+{
+#ifdef CONFIG_NFS_V4
+	if (__test_and_clear_bit(NFS_CS_RENEWD, &clp->cl_res_state))
+		nfs4_kill_renewd(clp);
+	nfs4_clear_client_minor_version(clp);
+	if (__test_and_clear_bit(NFS_CS_IDMAP, &clp->cl_res_state))
+		nfs_idmap_delete(clp);
+
+	rpc_destroy_wait_queue(&clp->cl_rpcwaitq);
+#endif
+}
+
 /*
  * Destroy a shared client record
  */
@@ -213,7 +213,6 @@ static void nfs_free_client(struct nfs_client *clp)
 {
 	dprintk("--> nfs_free_client(%u)\n", clp->rpc_ops->version);
 
-	nfs4_clear_client_minor_version(clp);
 	nfs4_shutdown_client(clp);
 
 	nfs_fscache_release_client_cookie(clp);
diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index 865265b..783c986 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -262,6 +262,7 @@ extern void nfs4_renew_state(struct work_struct *);
 /* nfs4state.c */
 struct rpc_cred *nfs4_get_setclientid_cred(struct nfs_client *clp);
 struct rpc_cred *nfs4_get_renew_cred_locked(struct nfs_client *clp);
+int nfs4_begin_drain_session(struct nfs_client *clp);
 #if defined(CONFIG_NFS_V4_1)
 struct rpc_cred *nfs4_get_machine_cred_locked(struct nfs_client *clp);
 struct rpc_cred *nfs4_get_exchange_id_cred(struct nfs_client *clp);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index a4b7d69..f424b1a 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4728,6 +4728,7 @@ struct nfs4_session *nfs4_alloc_session(struct nfs_client *clp)
 
 void nfs4_destroy_session(struct nfs4_session *session)
 {
+	nfs4_begin_drain_session(session->clp);
 	nfs4_proc_destroy_session(session);
 	dprintk("%s Destroy backchannel for xprt %p\n",
 		__func__, session->clp->cl_rpcclient->cl_xprt);
@@ -4996,6 +4997,9 @@ static int nfs4_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred)
 void nfs41_sequence_call_done(struct rpc_task *task, void *data)
 {
 	struct nfs_client *clp = (struct nfs_client *)data;
+
+	if (!nfs4_has_session(clp))
+		return;
 
 	nfs41_sequence_done(clp, task->tk_msg.rpc_resp, task->tk_status);
 
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 6d263ed..09f4402 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -156,7 +156,7 @@ static void nfs4_end_drain_session(struct nfs_client *clp)
 	}
 }
 
-static int nfs4_begin_drain_session(struct nfs_client *clp)
+int nfs4_begin_drain_session(struct nfs_client *clp)
 {
 	struct nfs4_session *ses = clp->cl_session;
 	struct nfs4_slot_table *tbl = &ses->fc_slot_table;
@@ -878,7 +878,11 @@ void nfs4_schedule_state_manager(struct nfs_client *clp)
 	if (test_and_set_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state) != 0)
 		return;
 	__module_get(THIS_MODULE);
-	atomic_inc(&clp->cl_count);
+	if (!atomic_inc_not_zero(&clp->cl_count)) {
+		nfs4_clear_state_manager_bit(clp);
+		module_put(THIS_MODULE);
+		return;
+	}
 	task = kthread_run(nfs4_run_state_manager, clp, "%s-manager",
 				rpc_peeraddr2str(clp->cl_rpcclient,
 							RPC_DISPLAY_ADDR));
-- 
1.6.2.5

[PATCH 1/1] nfs: fix race between renewd, umount, and the state manager in V4.1

[Alexandros Batsakis]

Signed-off-by: Alexandros Batsakis <batsakis@netapp.com>
---
 fs/nfs/client.c    |   38 ++++++++++----------------------------
 fs/nfs/nfs4_fs.h   |    1 +
 fs/nfs/nfs4proc.c  |    3 +++
 fs/nfs/nfs4state.c |   17 ++++++++++++++++-
 4 files changed, 30 insertions(+), 29 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index d0b060a..3efde50 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -164,19 +164,6 @@ error_0:
 	return ERR_PTR(err);
 }
 
-static void nfs4_shutdown_client(struct nfs_client *clp)
-{
-#ifdef CONFIG_NFS_V4
-	if (__test_and_clear_bit(NFS_CS_RENEWD, &clp->cl_res_state))
-		nfs4_kill_renewd(clp);
-	BUG_ON(!RB_EMPTY_ROOT(&clp->cl_state_owners));
-	if (__test_and_clear_bit(NFS_CS_IDMAP, &clp->cl_res_state))
-		nfs_idmap_delete(clp);
-
-	rpc_destroy_wait_queue(&clp->cl_rpcwaitq);
-#endif
-}
-
 /*
  * Destroy the NFS4 callback service
  */
@@ -188,22 +175,18 @@ static void nfs4_destroy_callback(struct nfs_client *clp)
 #endif /* CONFIG_NFS_V4 */
 }
 
-/*
- * Clears/puts all minor version specific parts from an nfs_client struct
- * reverting it to minorversion 0.
- */
-static void nfs4_clear_client_minor_version(struct nfs_client *clp)
+static void nfs4_shutdown_client(struct nfs_client *clp)
 {
-#ifdef CONFIG_NFS_V4_1
-	if (nfs4_has_session(clp)) {
-		nfs4_destroy_session(clp->cl_session);
-		clp->cl_session = NULL;
-	}
-
-	clp->cl_call_sync = _nfs4_call_sync;
-#endif /* CONFIG_NFS_V4_1 */
-
+#ifdef CONFIG_NFS_V4
+	if (__test_and_clear_bit(NFS_CS_RENEWD, &clp->cl_res_state))
+		nfs4_kill_renewd(clp);
+	nfs4_clear_session(clp);
 	nfs4_destroy_callback(clp);
+	if (__test_and_clear_bit(NFS_CS_IDMAP, &clp->cl_res_state))
+		nfs_idmap_delete(clp);
+
+	rpc_destroy_wait_queue(&clp->cl_rpcwaitq);
+#endif
 }
 
 /*
@@ -213,7 +196,6 @@ static void nfs_free_client(struct nfs_client *clp)
 {
 	dprintk("--> nfs_free_client(%u)\n", clp->rpc_ops->version);
 
-	nfs4_clear_client_minor_version(clp);
 	nfs4_shutdown_client(clp);
 
 	nfs_fscache_release_client_cookie(clp);
diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index cd93dfc..fbeec98 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -263,6 +263,7 @@ extern void nfs4_renew_state(struct work_struct *);
 /* nfs4state.c */
 struct rpc_cred *nfs4_get_setclientid_cred(struct nfs_client *clp);
 struct rpc_cred *nfs4_get_renew_cred_locked(struct nfs_client *clp);
+void nfs4_clear_session(struct nfs_client *clp);
 #if defined(CONFIG_NFS_V4_1)
 struct rpc_cred *nfs4_get_machine_cred_locked(struct nfs_client *clp);
 struct rpc_cred *nfs4_get_exchange_id_cred(struct nfs_client *clp);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ec9f075..a99c7d3 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5004,6 +5004,9 @@ void nfs41_sequence_call_done(struct rpc_task *task, void *data)
 {
 	struct nfs_client *clp = (struct nfs_client *)data;
 
+	if (!nfs4_has_session(clp))
+		return;
+
 	nfs41_sequence_done(clp, task->tk_msg.rpc_resp, task->tk_status);
 
 	if (task->tk_status < 0) {
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 069dcb3..dc3cfbf 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -199,6 +199,16 @@ struct rpc_cred *nfs4_get_exchange_id_cred(struct nfs_client *clp)
 	return cred;
 }
 
+void nfs4_clear_session(struct nfs_client *clp)
+{
+	if (nfs4_has_session(clp)) {
+		nfs4_begin_drain_session(clp);
+		nfs4_destroy_session(clp->cl_session);
+		clp->cl_session = NULL;
+	}
+	clp->cl_call_sync = _nfs4_call_sync;
+}
+
 #endif /* CONFIG_NFS_V4_1 */
 
 struct rpc_cred *nfs4_get_setclientid_cred(struct nfs_client *clp)
@@ -878,7 +888,11 @@ void nfs4_schedule_state_manager(struct nfs_client *clp)
 	if (test_and_set_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state) != 0)
 		return;
 	__module_get(THIS_MODULE);
-	atomic_inc(&clp->cl_count);
+	if (!atomic_inc_not_zero(&clp->cl_count)) {
+		nfs4_clear_state_manager_bit(clp);
+		module_put(THIS_MODULE);
+		return;
+	}
 	task = kthread_run(nfs4_run_state_manager, clp, "%s-manager",
 				rpc_peeraddr2str(clp->cl_rpcclient,
 							RPC_DISPLAY_ADDR));
@@ -1336,6 +1350,7 @@ static int nfs4_recall_slot(struct nfs_client *clp)
 #else /* CONFIG_NFS_V4_1 */
 static int nfs4_reset_session(struct nfs_client *clp) { return 0; }
 static int nfs4_end_drain_session(struct nfs_client *clp) { return 0; }
+void nfs4_clear_session(struct nfs_client *clp) { return 0; }
 static int nfs4_recall_slot(struct nfs_client *clp) { return 0; }
 #endif /* CONFIG_NFS_V4_1 */
 
-- 
1.6.2.5

Re: [PATCH 1/1] nfs: fix race between renewd, umount, and the state manager in V4.1

[Trond Myklebust]

On Thu, 2010-01-21 at 20:51 -0800, Alexandros Batsakis wrote:

> struct rpc_cred *nfs4_get_setclientid_cred(struct nfs_client *clp)
> @@ -878,7 +888,11 @@ void nfs4_schedule_state_manager(struct nfs_client *clp)
>  	if (test_and_set_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state) != 0)
>  		return;
>  	__module_get(THIS_MODULE);
> -	atomic_inc(&clp->cl_count);
> +	if (!atomic_inc_not_zero(&clp->cl_count)) {
> +		nfs4_clear_state_manager_bit(clp);
> +		module_put(THIS_MODULE);
> +		return;
> +	}

The use of atomic_inc_not_zero() should not be necessary here. Anybody
who is calling this function without holding a reference to clp is doing
something fundamentally wrong.

Trond

RE: [PATCH 1/1] nfs: fix race between renewd, umount, and the state manager in V4.1

[Batsakis, Alexandros]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogTXlrbGVidXN0LCBUcm9u
ZA0KPiBTZW50OiBGcmlkYXksIEphbnVhcnkgMjIsIDIwMTAgMTA6NDcgQU0NCj4gVG86IEJhdHNh
a2lzLCBBbGV4YW5kcm9zDQo+IENjOiBsaW51eC1uZnNAdmdlci5rZXJuZWwub3JnOyBNeWtsZWJ1
c3QsIFRyb25kDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggMS8xXSBuZnM6IGZpeCByYWNlIGJldHdl
ZW4gcmVuZXdkLCB1bW91bnQsIGFuZCB0aGUNCj4gc3RhdGUgbWFuYWdlciBpbiBWNC4xDQo+IA0K
PiBPbiBUaHUsIDIwMTAtMDEtMjEgYXQgMjA6NTEgLTA4MDAsIEFsZXhhbmRyb3MgQmF0c2FraXMg
d3JvdGU6DQo+IA0KPiA+IHN0cnVjdCBycGNfY3JlZCAqbmZzNF9nZXRfc2V0Y2xpZW50aWRfY3Jl
ZChzdHJ1Y3QgbmZzX2NsaWVudCAqY2xwKQ0KPiA+IEBAIC04NzgsNyArODg4LDExIEBAIHZvaWQg
bmZzNF9zY2hlZHVsZV9zdGF0ZV9tYW5hZ2VyKHN0cnVjdA0KPiBuZnNfY2xpZW50ICpjbHApDQo+
ID4gIAlpZiAodGVzdF9hbmRfc2V0X2JpdChORlM0Q0xOVF9NQU5BR0VSX1JVTk5JTkcsICZjbHAt
PmNsX3N0YXRlKSAhPQ0KPiAwKQ0KPiA+ICAJCXJldHVybjsNCj4gPiAgCV9fbW9kdWxlX2dldChU
SElTX01PRFVMRSk7DQo+ID4gLQlhdG9taWNfaW5jKCZjbHAtPmNsX2NvdW50KTsNCj4gPiArCWlm
ICghYXRvbWljX2luY19ub3RfemVybygmY2xwLT5jbF9jb3VudCkpIHsNCj4gPiArCQluZnM0X2Ns
ZWFyX3N0YXRlX21hbmFnZXJfYml0KGNscCk7DQo+ID4gKwkJbW9kdWxlX3B1dChUSElTX01PRFVM
RSk7DQo+ID4gKwkJcmV0dXJuOw0KPiA+ICsJfQ0KPiANCj4gVGhlIHVzZSBvZiBhdG9taWNfaW5j
X25vdF96ZXJvKCkgc2hvdWxkIG5vdCBiZSBuZWNlc3NhcnkgaGVyZS4gQW55Ym9keQ0KPiB3aG8g
aXMgY2FsbGluZyB0aGlzIGZ1bmN0aW9uIHdpdGhvdXQgaG9sZGluZyBhIHJlZmVyZW5jZSB0byBj
bHAgaXMNCj4gZG9pbmcNCj4gc29tZXRoaW5nIGZ1bmRhbWVudGFsbHkgd3JvbmcuDQoNCkFncmVl
ZCwgYnV0IGhvdyBjYW4gd2UgZW5mb3JjZSB0aGlzIChlc3BlY2lhbGx5IHdpdGggYXN5bmNocm9u
b3VzIFJQQykgPw0KRm9yIGV4YW1wbGUgd2hhdCB3aWxsIGhhcHBlbiBpZiBfZHVyaW5nXyB1bW91
bnQgKGFmdGVyIG5mc19wdXRfY2xpZW50KCkpLCBhbiBORlMgY2FsbCB0aGF0IHdhcyBzbGVlcGlu
ZyBlLmcuIHdhaXRpbmcgZm9yIHRoZSBzZXJ2ZXIgdG8gY29tZSB1cCwgcmVhY2hlcyB0aGUgc2Vy
dmVyIGFuZCB0aGUgcmVzcG9uc2UgY2F1c2VzIHRoZSBzdGF0ZSBtYW5hZ2VyIHRvIHN0YXJ0ID8N
CkFueSBpZGVhcyA/DQoNCi1hDQoNCj4gDQo+IFRyb25kDQo=

RE: [PATCH 1/1] nfs: fix race between renewd, umount, and the state manager in V4.1

[Trond Myklebust]

On Fri, 2010-01-22 at 11:30 -0800, Batsakis, Alexandros wrote: 
> 
> > -----Original Message-----
> > From: Myklebust, Trond
> > Sent: Friday, January 22, 2010 10:47 AM
> > To: Batsakis, Alexandros
> > Cc: linux-nfs@vger.kernel.org; Myklebust, Trond
> > Subject: Re: [PATCH 1/1] nfs: fix race between renewd, umount, and the
> > state manager in V4.1
> > 
> > On Thu, 2010-01-21 at 20:51 -0800, Alexandros Batsakis wrote:
> > 
> > > struct rpc_cred *nfs4_get_setclientid_cred(struct nfs_client *clp)
> > > @@ -878,7 +888,11 @@ void nfs4_schedule_state_manager(struct
> > nfs_client *clp)
> > >  	if (test_and_set_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state) !=
> > 0)
> > >  		return;
> > >  	__module_get(THIS_MODULE);
> > > -	atomic_inc(&clp->cl_count);
> > > +	if (!atomic_inc_not_zero(&clp->cl_count)) {
> > > +		nfs4_clear_state_manager_bit(clp);
> > > +		module_put(THIS_MODULE);
> > > +		return;
> > > +	}
> > 
> > The use of atomic_inc_not_zero() should not be necessary here. Anybody
> > who is calling this function without holding a reference to clp is
> > doing
> > something fundamentally wrong.
> 
> Agreed, but how can we enforce this (especially with asynchronous RPC) ?
> For example what will happen if _during_ umount (after nfs_put_client()), an NFS call that was sleeping e.g. waiting for the server to come up, reaches the server and the response causes the state manager to start ?
> Any ideas ?

What would you expect to see running during umount? Nobody can be
holding a reference to the vfsmount at that point, so there should be no
active processes around to make NFS calls.

Trond

RE: [PATCH 1/1] nfs: fix race between renewd, umount, and the state manager in V4.1

[Batsakis, Alexandros]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogTXlrbGVidXN0LCBUcm9u
ZA0KPiBTZW50OiBGcmlkYXksIEphbnVhcnkgMjIsIDIwMTAgMTE6NDIgQU0NCj4gVG86IEJhdHNh
a2lzLCBBbGV4YW5kcm9zDQo+IENjOiBsaW51eC1uZnNAdmdlci5rZXJuZWwub3JnDQo+IFN1Ympl
Y3Q6IFJFOiBbUEFUQ0ggMS8xXSBuZnM6IGZpeCByYWNlIGJldHdlZW4gcmVuZXdkLCB1bW91bnQs
IGFuZCB0aGUNCj4gc3RhdGUgbWFuYWdlciBpbiBWNC4xDQo+IA0KPiBPbiBGcmksIDIwMTAtMDEt
MjIgYXQgMTE6MzAgLTA4MDAsIEJhdHNha2lzLCBBbGV4YW5kcm9zIHdyb3RlOg0KPiA+DQo+ID4g
PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiA+ID4gRnJvbTogTXlrbGVidXN0LCBUcm9u
ZA0KPiA+ID4gU2VudDogRnJpZGF5LCBKYW51YXJ5IDIyLCAyMDEwIDEwOjQ3IEFNDQo+ID4gPiBU
bzogQmF0c2FraXMsIEFsZXhhbmRyb3MNCj4gPiA+IENjOiBsaW51eC1uZnNAdmdlci5rZXJuZWwu
b3JnOyBNeWtsZWJ1c3QsIFRyb25kDQo+ID4gPiBTdWJqZWN0OiBSZTogW1BBVENIIDEvMV0gbmZz
OiBmaXggcmFjZSBiZXR3ZWVuIHJlbmV3ZCwgdW1vdW50LCBhbmQNCj4gdGhlDQo+ID4gPiBzdGF0
ZSBtYW5hZ2VyIGluIFY0LjENCj4gPiA+DQo+ID4gPiBPbiBUaHUsIDIwMTAtMDEtMjEgYXQgMjA6
NTEgLTA4MDAsIEFsZXhhbmRyb3MgQmF0c2FraXMgd3JvdGU6DQo+ID4gPg0KPiA+ID4gPiBzdHJ1
Y3QgcnBjX2NyZWQgKm5mczRfZ2V0X3NldGNsaWVudGlkX2NyZWQoc3RydWN0IG5mc19jbGllbnQN
Cj4gKmNscCkNCj4gPiA+ID4gQEAgLTg3OCw3ICs4ODgsMTEgQEAgdm9pZCBuZnM0X3NjaGVkdWxl
X3N0YXRlX21hbmFnZXIoc3RydWN0DQo+ID4gPiBuZnNfY2xpZW50ICpjbHApDQo+ID4gPiA+ICAJ
aWYgKHRlc3RfYW5kX3NldF9iaXQoTkZTNENMTlRfTUFOQUdFUl9SVU5OSU5HLCAmY2xwLQ0KPiA+
Y2xfc3RhdGUpICE9DQo+ID4gPiAwKQ0KPiA+ID4gPiAgCQlyZXR1cm47DQo+ID4gPiA+ICAJX19t
b2R1bGVfZ2V0KFRISVNfTU9EVUxFKTsNCj4gPiA+ID4gLQlhdG9taWNfaW5jKCZjbHAtPmNsX2Nv
dW50KTsNCj4gPiA+ID4gKwlpZiAoIWF0b21pY19pbmNfbm90X3plcm8oJmNscC0+Y2xfY291bnQp
KSB7DQo+ID4gPiA+ICsJCW5mczRfY2xlYXJfc3RhdGVfbWFuYWdlcl9iaXQoY2xwKTsNCj4gPiA+
ID4gKwkJbW9kdWxlX3B1dChUSElTX01PRFVMRSk7DQo+ID4gPiA+ICsJCXJldHVybjsNCj4gPiA+
ID4gKwl9DQo+ID4gPg0KPiA+ID4gVGhlIHVzZSBvZiBhdG9taWNfaW5jX25vdF96ZXJvKCkgc2hv
dWxkIG5vdCBiZSBuZWNlc3NhcnkgaGVyZS4NCj4gQW55Ym9keQ0KPiA+ID4gd2hvIGlzIGNhbGxp
bmcgdGhpcyBmdW5jdGlvbiB3aXRob3V0IGhvbGRpbmcgYSByZWZlcmVuY2UgdG8gY2xwIGlzDQo+
ID4gPiBkb2luZw0KPiA+ID4gc29tZXRoaW5nIGZ1bmRhbWVudGFsbHkgd3JvbmcuDQo+ID4NCj4g
PiBBZ3JlZWQsIGJ1dCBob3cgY2FuIHdlIGVuZm9yY2UgdGhpcyAoZXNwZWNpYWxseSB3aXRoIGFz
eW5jaHJvbm91cw0KPiBSUEMpID8NCj4gPiBGb3IgZXhhbXBsZSB3aGF0IHdpbGwgaGFwcGVuIGlm
IF9kdXJpbmdfIHVtb3VudCAoYWZ0ZXINCj4gbmZzX3B1dF9jbGllbnQoKSksIGFuIE5GUyBjYWxs
IHRoYXQgd2FzIHNsZWVwaW5nIGUuZy4gd2FpdGluZyBmb3IgdGhlDQo+IHNlcnZlciB0byBjb21l
IHVwLCByZWFjaGVzIHRoZSBzZXJ2ZXIgYW5kIHRoZSByZXNwb25zZSBjYXVzZXMgdGhlIHN0YXRl
DQo+IG1hbmFnZXIgdG8gc3RhcnQgPw0KPiA+IEFueSBpZGVhcyA/DQo+IA0KPiBXaGF0IHdvdWxk
IHlvdSBleHBlY3QgdG8gc2VlIHJ1bm5pbmcgZHVyaW5nIHVtb3VudD8gTm9ib2R5IGNhbiBiZQ0K
PiBob2xkaW5nIGEgcmVmZXJlbmNlIHRvIHRoZSB2ZnNtb3VudCBhdCB0aGF0IHBvaW50LCBzbyB0
aGVyZSBzaG91bGQgYmUNCj4gbm8NCj4gYWN0aXZlIHByb2Nlc3NlcyBhcm91bmQgdG8gbWFrZSBO
RlMgY2FsbHMuDQo+IA0KDQpyZW5ld2QgaXMgc3RpbGwgcnVubmluZyBhbmQgZXZlbiBpZiB3ZSBr
aWxsIHRoZSBkYWVtb24sIG1heWJlIHRoZXJlIGlzIGEgc2VxdWVuY2VfZG9uZSBwZW5kaW5nDQoN
Ci1hDQoNCj4gVHJvbmQNCg0K

RE: [PATCH 1/1] nfs: fix race between renewd, umount, and the state manager in V4.1

[Trond Myklebust]

On Fri, 2010-01-22 at 12:01 -0800, Batsakis, Alexandros wrote: 
> 
> > -----Original Message-----
> > From: Myklebust, Trond
> > Sent: Friday, January 22, 2010 11:42 AM
> > To: Batsakis, Alexandros
> > Cc: linux-nfs@vger.kernel.org
> > Subject: RE: [PATCH 1/1] nfs: fix race between renewd, umount, and the
> > state manager in V4.1
> > 
> > On Fri, 2010-01-22 at 11:30 -0800, Batsakis, Alexandros wrote:
> > >
> > > > -----Original Message-----
> > > > From: Myklebust, Trond
> > > > Sent: Friday, January 22, 2010 10:47 AM
> > > > To: Batsakis, Alexandros
> > > > Cc: linux-nfs@vger.kernel.org; Myklebust, Trond
> > > > Subject: Re: [PATCH 1/1] nfs: fix race between renewd, umount, and
> > the
> > > > state manager in V4.1
> > > >
> > > > On Thu, 2010-01-21 at 20:51 -0800, Alexandros Batsakis wrote:
> > > >
> > > > > struct rpc_cred *nfs4_get_setclientid_cred(struct nfs_client
> > *clp)
> > > > > @@ -878,7 +888,11 @@ void nfs4_schedule_state_manager(struct
> > > > nfs_client *clp)
> > > > >  	if (test_and_set_bit(NFS4CLNT_MANAGER_RUNNING, &clp-
> > >cl_state) !=
> > > > 0)
> > > > >  		return;
> > > > >  	__module_get(THIS_MODULE);
> > > > > -	atomic_inc(&clp->cl_count);
> > > > > +	if (!atomic_inc_not_zero(&clp->cl_count)) {
> > > > > +		nfs4_clear_state_manager_bit(clp);
> > > > > +		module_put(THIS_MODULE);
> > > > > +		return;
> > > > > +	}
> > > >
> > > > The use of atomic_inc_not_zero() should not be necessary here.
> > Anybody
> > > > who is calling this function without holding a reference to clp is
> > > > doing
> > > > something fundamentally wrong.
> > >
> > > Agreed, but how can we enforce this (especially with asynchronous
> > RPC) ?
> > > For example what will happen if _during_ umount (after
> > nfs_put_client()), an NFS call that was sleeping e.g. waiting for the
> > server to come up, reaches the server and the response causes the state
> > manager to start ?
> > > Any ideas ?
> > 
> > What would you expect to see running during umount? Nobody can be
> > holding a reference to the vfsmount at that point, so there should be
> > no
> > active processes around to make NFS calls.
> > 
> 
> renewd is still running and even if we kill the daemon, maybe there is a sequence_done pending

Then the correct thing to do is to fix the renewd kill process. Once
that is done, there should be no reason for any sequence_done calls to
be pending (see reason above).

Trond

RE: [PATCH 1/1] nfs: fix race between renewd, umount, and the state manager in V4.1

[Trond Myklebust]

On Fri, 2010-01-22 at 15:11 -0500, Trond Myklebust wrote: 
> Then the correct thing to do is to fix the renewd kill process. Once
> that is done, there should be no reason for any sequence_done calls to
> be pending (see reason above).

IOW: we should add a call to atomic_inc_not_zero(&clp->cl_count) in
nfs4_proc_async_renew()/nfs41_proc_async_sequence(), and have them fail
if that attempt to grab a reference fails. Then we should add a call to
nfs_put_client() in their .rpc_release method.

Trond

RE: [PATCH 1/1] nfs: fix race between renewd, umount, and the state manager in V4.1

[Batsakis, Alexandros]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogVHJvbmQgTXlrbGVidXN0
IFttYWlsdG86dHJvbmQubXlrbGVidXN0QGZ5cy51aW8ubm9dDQo+IFNlbnQ6IEZyaWRheSwgSmFu
dWFyeSAyMiwgMjAxMCAxMjoyMSBQTQ0KPiBUbzogQmF0c2FraXMsIEFsZXhhbmRyb3MNCj4gQ2M6
IGxpbnV4LW5mc0B2Z2VyLmtlcm5lbC5vcmcNCj4gU3ViamVjdDogUkU6IFtQQVRDSCAxLzFdIG5m
czogZml4IHJhY2UgYmV0d2VlbiByZW5ld2QsIHVtb3VudCwgYW5kIHRoZQ0KPiBzdGF0ZSBtYW5h
Z2VyIGluIFY0LjENCj4gDQo+IE9uIEZyaSwgMjAxMC0wMS0yMiBhdCAxNToxMSAtMDUwMCwgVHJv
bmQgTXlrbGVidXN0IHdyb3RlOg0KPiA+IFRoZW4gdGhlIGNvcnJlY3QgdGhpbmcgdG8gZG8gaXMg
dG8gZml4IHRoZSByZW5ld2Qga2lsbCBwcm9jZXNzLiBPbmNlDQo+ID4gdGhhdCBpcyBkb25lLCB0
aGVyZSBzaG91bGQgYmUgbm8gcmVhc29uIGZvciBhbnkgc2VxdWVuY2VfZG9uZSBjYWxscw0KPiB0
bw0KPiA+IGJlIHBlbmRpbmcgKHNlZSByZWFzb24gYWJvdmUpLg0KPiANCj4gSU9XOiB3ZSBzaG91
bGQgYWRkIGEgY2FsbCB0byBhdG9taWNfaW5jX25vdF96ZXJvKCZjbHAtPmNsX2NvdW50KSBpbg0K
PiBuZnM0X3Byb2NfYXN5bmNfcmVuZXcoKS9uZnM0MV9wcm9jX2FzeW5jX3NlcXVlbmNlKCksIGFu
ZCBoYXZlIHRoZW0gZmFpbA0KPiBpZiB0aGF0IGF0dGVtcHQgdG8gZ3JhYiBhIHJlZmVyZW5jZSBm
YWlscy4gVGhlbiB3ZSBzaG91bGQgYWRkIGEgY2FsbCB0bw0KPiBuZnNfcHV0X2NsaWVudCgpIGlu
IHRoZWlyIC5ycGNfcmVsZWFzZSBtZXRob2QuDQo+IA0KPiBUcm9uZA0KDQpHb3QgaXQuLi53aWxs
IGRvLiBUaGFua3MhDQoNCi1hDQo=