diff mbox

[2/8] NFSv4: don't let hanging mounts block other mounts

Message ID 150304037188.30218.6679182255685764801.stgit@noble (mailing list archive)
State New, archived
Headers show

Commit Message

NeilBrown Aug. 18, 2017, 7:12 a.m. UTC 
If you try an NFSv4 mount from an inaccessible server, it will hang as
you would expect.
If you then try an NFSv4 mount from a different accessible server,
it will also hang.  This is not expected.

The second mount is blocked in
  nfs4_init_client()
  -> nfs4_discover_server_trunking()
  -> nfs40_discover_server_trunking()
  -> nfs40_walk_client_list()
  -> nfs4_match_client()
  -> nfs_wait_client_init_complete()
It is waiting for the first mount to complete so that it can then
see if the two servers are really one and the same.

It is not necessary to wait here when an nfs_client cl_cons_state is
NFS_CS_INITING.  Such a client will, after changing cl_cons_state, call
nfs4_discover_server_trunking() itself.  So if the current client just
skips those clients, trunking will still be discovered if necessary.

I am unsure of situation with NFS_CS_SESSION_INITING, but I suspect
that the comment "Wait for CREATE_SESSION to finish" implies that
it is only clients in NFS_CS_SESSION_INITING that need to be waited for.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/nfs/nfs4client.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)



--
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

Comments

Trond Myklebust Aug. 18, 2017, 7:05 p.m. UTC | #1 
T24gRnJpLCAyMDE3LTA4LTE4IGF0IDE3OjEyICsxMDAwLCBOZWlsQnJvd24gd3JvdGU6DQo+IElm
IHlvdSB0cnkgYW4gTkZTdjQgbW91bnQgZnJvbSBhbiBpbmFjY2Vzc2libGUgc2VydmVyLCBpdCB3
aWxsIGhhbmcNCj4gYXMNCj4geW91IHdvdWxkIGV4cGVjdC4NCj4gSWYgeW91IHRoZW4gdHJ5IGFu
IE5GU3Y0IG1vdW50IGZyb20gYSBkaWZmZXJlbnQgYWNjZXNzaWJsZSBzZXJ2ZXIsDQo+IGl0IHdp
bGwgYWxzbyBoYW5nLiAgVGhpcyBpcyBub3QgZXhwZWN0ZWQuDQo+IA0KPiBUaGUgc2Vjb25kIG1v
dW50IGlzIGJsb2NrZWQgaW4NCj4gICBuZnM0X2luaXRfY2xpZW50KCkNCj4gICAtPiBuZnM0X2Rp
c2NvdmVyX3NlcnZlcl90cnVua2luZygpDQo+ICAgLT4gbmZzNDBfZGlzY292ZXJfc2VydmVyX3Ry
dW5raW5nKCkNCj4gICAtPiBuZnM0MF93YWxrX2NsaWVudF9saXN0KCkNCj4gICAtPiBuZnM0X21h
dGNoX2NsaWVudCgpDQo+ICAgLT4gbmZzX3dhaXRfY2xpZW50X2luaXRfY29tcGxldGUoKQ0KPiBJ
dCBpcyB3YWl0aW5nIGZvciB0aGUgZmlyc3QgbW91bnQgdG8gY29tcGxldGUgc28gdGhhdCBpdCBj
YW4gdGhlbg0KPiBzZWUgaWYgdGhlIHR3byBzZXJ2ZXJzIGFyZSByZWFsbHkgb25lIGFuZCB0aGUg
c2FtZS4NCj4gDQo+IEl0IGlzIG5vdCBuZWNlc3NhcnkgdG8gd2FpdCBoZXJlIHdoZW4gYW4gbmZz
X2NsaWVudCBjbF9jb25zX3N0YXRlIGlzDQo+IE5GU19DU19JTklUSU5HLiAgU3VjaCBhIGNsaWVu
dCB3aWxsLCBhZnRlciBjaGFuZ2luZyBjbF9jb25zX3N0YXRlLA0KPiBjYWxsDQo+IG5mczRfZGlz
Y292ZXJfc2VydmVyX3RydW5raW5nKCkgaXRzZWxmLiAgU28gaWYgdGhlIGN1cnJlbnQgY2xpZW50
DQo+IGp1c3QNCj4gc2tpcHMgdGhvc2UgY2xpZW50cywgdHJ1bmtpbmcgd2lsbCBzdGlsbCBiZSBk
aXNjb3ZlcmVkIGlmIG5lY2Vzc2FyeS4NCj4gDQo+IEkgYW0gdW5zdXJlIG9mIHNpdHVhdGlvbiB3
aXRoIE5GU19DU19TRVNTSU9OX0lOSVRJTkcsIGJ1dCBJIHN1c3BlY3QNCj4gdGhhdCB0aGUgY29t
bWVudCAiV2FpdCBmb3IgQ1JFQVRFX1NFU1NJT04gdG8gZmluaXNoIiBpbXBsaWVzIHRoYXQNCj4g
aXQgaXMgb25seSBjbGllbnRzIGluIE5GU19DU19TRVNTSU9OX0lOSVRJTkcgdGhhdCBuZWVkIHRv
IGJlIHdhaXRlZA0KPiBmb3IuDQo+IA0KPiBTaWduZWQtb2ZmLWJ5OiBOZWlsQnJvd24gPG5laWxi
QHN1c2UuY29tPg0KPiAtLS0NCj4gIGZzL25mcy9uZnM0Y2xpZW50LmMgfCAgICAyICstDQo+ICAx
IGZpbGUgY2hhbmdlZCwgMSBpbnNlcnRpb24oKyksIDEgZGVsZXRpb24oLSkNCj4gDQo+IGRpZmYg
LS1naXQgYS9mcy9uZnMvbmZzNGNsaWVudC5jIGIvZnMvbmZzL25mczRjbGllbnQuYw0KPiBpbmRl
eCBlOWJlYTkwZGMwMTcuLmQ4YjliN2ZmMTlhOSAxMDA2NDQNCj4gLS0tIGEvZnMvbmZzL25mczRj
bGllbnQuYw0KPiArKysgYi9mcy9uZnMvbmZzNGNsaWVudC5jDQo+IEBAIC00ODIsNyArNDgyLDcg
QEAgc3RhdGljIGludCBuZnM0X21hdGNoX2NsaWVudChzdHJ1Y3QNCj4gbmZzX2NsaWVudCAgKnBv
cywgIHN0cnVjdCBuZnNfY2xpZW50ICpuZXcsDQo+ICAJICogcmVtYWluaW5nIGZpZWxkcyBpbiAi
cG9zIiwgZXNwZWNpYWxseSB0aGUgY2xpZW50DQo+ICAJICogSUQgYW5kIHNlcnZlcm93bmVyIGZp
ZWxkcy4gIFdhaXQgZm9yIENSRUFURV9TRVNTSU9ODQo+ICAJICogdG8gZmluaXNoLiAqLw0KPiAt
CWlmIChwb3MtPmNsX2NvbnNfc3RhdGUgPiBORlNfQ1NfUkVBRFkpIHsNCj4gKwlpZiAocG9zLT5j
bF9jb25zX3N0YXRlID09IE5GU19DU19TRVNTSU9OX0lOSVRJTkcpIHsNCj4gIAkJYXRvbWljX2lu
YygmcG9zLT5jbF9jb3VudCk7DQo+ICAJCXNwaW5fdW5sb2NrKCZubi0+bmZzX2NsaWVudF9sb2Nr
KTsNCg0KVGhpcyBjb3VsZCBjYXVzZSB1cyB0byBkZWNsYXJlIGEgZmFsc2UgcG9zaXRpdmUgbWF0
Y2ggd2l0aCBhIGNsaWVudA0KdGhhdCBpcyB1bmluaXRpYWxpc2VkLg0KDQotLSANClRyb25kIE15
a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyLCBQcmltYXJ5RGF0YQ0KdHJvbmQu
bXlrbGVidXN0QHByaW1hcnlkYXRhLmNvbQ0K

--
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
NeilBrown Aug. 19, 2017, 12:50 a.m. UTC | #2 
On Fri, Aug 18 2017, Trond Myklebust wrote:

> On Fri, 2017-08-18 at 17:12 +1000, NeilBrown wrote:
>> If you try an NFSv4 mount from an inaccessible server, it will hang
>> as
>> you would expect.
>> If you then try an NFSv4 mount from a different accessible server,
>> it will also hang.  This is not expected.
>> 
>> The second mount is blocked in
>>   nfs4_init_client()
>>   -> nfs4_discover_server_trunking()
>>   -> nfs40_discover_server_trunking()
>>   -> nfs40_walk_client_list()
>>   -> nfs4_match_client()
>>   -> nfs_wait_client_init_complete()
>> It is waiting for the first mount to complete so that it can then
>> see if the two servers are really one and the same.
>> 
>> It is not necessary to wait here when an nfs_client cl_cons_state is
>> NFS_CS_INITING.  Such a client will, after changing cl_cons_state,
>> call
>> nfs4_discover_server_trunking() itself.  So if the current client
>> just
>> skips those clients, trunking will still be discovered if necessary.
>> 
>> I am unsure of situation with NFS_CS_SESSION_INITING, but I suspect
>> that the comment "Wait for CREATE_SESSION to finish" implies that
>> it is only clients in NFS_CS_SESSION_INITING that need to be waited
>> for.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>  fs/nfs/nfs4client.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
>> index e9bea90dc017..d8b9b7ff19a9 100644
>> --- a/fs/nfs/nfs4client.c
>> +++ b/fs/nfs/nfs4client.c
>> @@ -482,7 +482,7 @@ static int nfs4_match_client(struct
>> nfs_client  *pos,  struct nfs_client *new,
>>  	 * remaining fields in "pos", especially the client
>>  	 * ID and serverowner fields.  Wait for CREATE_SESSION
>>  	 * to finish. */
>> -	if (pos->cl_cons_state > NFS_CS_READY) {
>> +	if (pos->cl_cons_state == NFS_CS_SESSION_INITING) {
>>  		atomic_inc(&pos->cl_count);
>>  		spin_unlock(&nn->nfs_client_lock);
>
> This could cause us to declare a false positive match with a client
> that is uninitialised.

Thanks for the review.

A positive match is reported by returning zero.
If cl_cons_state is not NFS_CS_READY, nfs4_match_client() will not
return zero.

So I don't see how a false positive is possible.

A false negative might be possible with an uninitialized client, but
once that client is initialised, it will call walk_client_list and find
the match.  Won't it?

Thanks,
NeilBrown
diff mbox

Patch

diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index e9bea90dc017..d8b9b7ff19a9 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -482,7 +482,7 @@  static int nfs4_match_client(struct nfs_client  *pos,  struct nfs_client *new,
 	 * remaining fields in "pos", especially the client
 	 * ID and serverowner fields.  Wait for CREATE_SESSION
 	 * to finish. */
-	if (pos->cl_cons_state > NFS_CS_READY) {
+	if (pos->cl_cons_state == NFS_CS_SESSION_INITING) {
 		atomic_inc(&pos->cl_count);
 		spin_unlock(&nn->nfs_client_lock);