diff mbox

[Version,1,1/1] NFS free the layoutget slot before calling pnfs_layout_process

Message ID 20170315151851.5762-1-andros@netapp.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andy Adamson March 15, 2017, 3:18 p.m. UTC 
From: Andy Adamson <andros@netapp.com>

Otherwise no slots could be available for the potential
pnfs_layout_process getdeviceinfo call and the client hangs on pNFS I/O.

This occurs in testing against the pynfs pNFS server where the
the on-wire reply highest_slotid and slot id are zero, and the
target high slot id is 8 (negotiated in CREATE_SESSION).

The internal fore channel slot table max_slotid, the maximum allowed
table slotid value, has been reduced via nfs41_set_max_slotid_locked
 from 8 to 1.  Thus there is one slot (slotid 0) available for use but
it has not been freed by layoutget proir to the getdeviceinfo request.

So the layout get call is waiting for the getdeviceinfo call to complete,
and the getdeviceinfo call is waiting for layoutget to free the slot.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/nfs4proc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Trond Myklebust March 15, 2017, 7:09 p.m. UTC | #1 
On Wed, 2017-03-15 at 11:18 -0400, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>

> 

> Otherwise no slots could be available for the potential

> pnfs_layout_process getdeviceinfo call and the client hangs on pNFS

> I/O.

> 

> This occurs in testing against the pynfs pNFS server where the

> the on-wire reply highest_slotid and slot id are zero, and the

> target high slot id is 8 (negotiated in CREATE_SESSION).

> 

> The internal fore channel slot table max_slotid, the maximum allowed

> table slotid value, has been reduced via nfs41_set_max_slotid_locked

>  from 8 to 1.  Thus there is one slot (slotid 0) available for use

> but

> it has not been freed by layoutget proir to the getdeviceinfo

> request.

> 

> So the layout get call is waiting for the getdeviceinfo call to

> complete,

> and the getdeviceinfo call is waiting for layoutget to free the slot.

> 

> Signed-off-by: Andy Adamson <andros@netapp.com>

> ---

>  fs/nfs/nfs4proc.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c

> index 1b18368..8ca92d0 100644

> --- a/fs/nfs/nfs4proc.c

> +++ b/fs/nfs/nfs4proc.c

> @@ -8504,10 +8504,10 @@ nfs4_proc_layoutget(struct nfs4_layoutget

> *lgp, long *timeout, gfp_t gfp_flags)

>  			&lgp->res.stateid,

>  			status);

>  

> +	nfs4_sequence_free_slot(&lgp->res.seq_res);

>  	/* if layoutp->len is 0, nfs4_layoutget_prepare called

> rpc_exit */

>  	if (status == 0 && lgp->res.layoutp->len)

>  		lseg = pnfs_layout_process(lgp);

> -	nfs4_sequence_free_slot(&lgp->res.seq_res);

>  	rpc_put_task(task);

>  	dprintk("<-- %s status=%d\n", __func__, status);

>  	if (status)


The placement of the call to nfs4_sequence_free_slot() after the call
to pnfs_layout_process() is deliberate, and is needed in order to
ensure that layoutrecall callbacks are processed correctly in order
after the layoutget has been processed (see referring_call_exists()).

The call to nfs4_find_get_deviceid() needs to be moved out of
pnfs_update_layout().
In the flexfiles driver, it was moved to nfs4_ff_layout_prepare_ds(),
but it looks to me as if the files driver needs to call it earlier due
to the need to calculate stripes. Perhaps the right thing is to add a
wrapper around pnfs_update_layout() in fs/nfs/filelayout/filelayout.c
that can be called by both filelayout_pg_init_read() and
filelayout_pg_init_write()?

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com
Adamson, Andy March 15, 2017, 7:18 p.m. UTC | #2 
T0sg4oCTIEnigJlsbCB0YWtlIGEgbG9vay4NCg0KVGhhbmtzDQoNCuKGkkFuZHkNCg0KT24gMy8x
NS8xNywgMzowOSBQTSwgIlRyb25kIE15a2xlYnVzdCIgPHRyb25kbXlAcHJpbWFyeWRhdGEuY29t
PiB3cm90ZToNCg0KT24gV2VkLCAyMDE3LTAzLTE1IGF0IDExOjE4IC0wNDAwLCBhbmRyb3NAbmV0
YXBwLmNvbSB3cm90ZToNCj4gRnJvbTogQW5keSBBZGFtc29uIDxhbmRyb3NAbmV0YXBwLmNvbT4N
Cj4gDQo+IE90aGVyd2lzZSBubyBzbG90cyBjb3VsZCBiZSBhdmFpbGFibGUgZm9yIHRoZSBwb3Rl
bnRpYWwNCj4gcG5mc19sYXlvdXRfcHJvY2VzcyBnZXRkZXZpY2VpbmZvIGNhbGwgYW5kIHRoZSBj
bGllbnQgaGFuZ3Mgb24gcE5GUw0KPiBJL08uDQo+IA0KPiBUaGlzIG9jY3VycyBpbiB0ZXN0aW5n
IGFnYWluc3QgdGhlIHB5bmZzIHBORlMgc2VydmVyIHdoZXJlIHRoZQ0KPiB0aGUgb24td2lyZSBy
ZXBseSBoaWdoZXN0X3Nsb3RpZCBhbmQgc2xvdCBpZCBhcmUgemVybywgYW5kIHRoZQ0KPiB0YXJn
ZXQgaGlnaCBzbG90IGlkIGlzIDggKG5lZ290aWF0ZWQgaW4gQ1JFQVRFX1NFU1NJT04pLg0KPiAN
Cj4gVGhlIGludGVybmFsIGZvcmUgY2hhbm5lbCBzbG90IHRhYmxlIG1heF9zbG90aWQsIHRoZSBt
YXhpbXVtIGFsbG93ZWQNCj4gdGFibGUgc2xvdGlkIHZhbHVlLCBoYXMgYmVlbiByZWR1Y2VkIHZp
YSBuZnM0MV9zZXRfbWF4X3Nsb3RpZF9sb2NrZWQNCj4gIGZyb20gOCB0byAxLiAgVGh1cyB0aGVy
ZSBpcyBvbmUgc2xvdCAoc2xvdGlkIDApIGF2YWlsYWJsZSBmb3IgdXNlDQo+IGJ1dA0KPiBpdCBo
YXMgbm90IGJlZW4gZnJlZWQgYnkgbGF5b3V0Z2V0IHByb2lyIHRvIHRoZSBnZXRkZXZpY2VpbmZv
DQo+IHJlcXVlc3QuDQo+IA0KPiBTbyB0aGUgbGF5b3V0IGdldCBjYWxsIGlzIHdhaXRpbmcgZm9y
IHRoZSBnZXRkZXZpY2VpbmZvIGNhbGwgdG8NCj4gY29tcGxldGUsDQo+IGFuZCB0aGUgZ2V0ZGV2
aWNlaW5mbyBjYWxsIGlzIHdhaXRpbmcgZm9yIGxheW91dGdldCB0byBmcmVlIHRoZSBzbG90Lg0K
PiANCj4gU2lnbmVkLW9mZi1ieTogQW5keSBBZGFtc29uIDxhbmRyb3NAbmV0YXBwLmNvbT4NCj4g
LS0tDQo+ICBmcy9uZnMvbmZzNHByb2MuYyB8IDIgKy0NCj4gIDEgZmlsZSBjaGFuZ2VkLCAxIGlu
c2VydGlvbigrKSwgMSBkZWxldGlvbigtKQ0KPiANCj4gZGlmZiAtLWdpdCBhL2ZzL25mcy9uZnM0
cHJvYy5jIGIvZnMvbmZzL25mczRwcm9jLmMNCj4gaW5kZXggMWIxODM2OC4uOGNhOTJkMCAxMDA2
NDQNCj4gLS0tIGEvZnMvbmZzL25mczRwcm9jLmMNCj4gKysrIGIvZnMvbmZzL25mczRwcm9jLmMN
Cj4gQEAgLTg1MDQsMTAgKzg1MDQsMTAgQEAgbmZzNF9wcm9jX2xheW91dGdldChzdHJ1Y3QgbmZz
NF9sYXlvdXRnZXQNCj4gKmxncCwgbG9uZyAqdGltZW91dCwgZ2ZwX3QgZ2ZwX2ZsYWdzKQ0KPiAg
CQkJJmxncC0+cmVzLnN0YXRlaWQsDQo+ICAJCQlzdGF0dXMpOw0KPiAgDQo+ICsJbmZzNF9zZXF1
ZW5jZV9mcmVlX3Nsb3QoJmxncC0+cmVzLnNlcV9yZXMpOw0KPiAgCS8qIGlmIGxheW91dHAtPmxl
biBpcyAwLCBuZnM0X2xheW91dGdldF9wcmVwYXJlIGNhbGxlZA0KPiBycGNfZXhpdCAqLw0KPiAg
CWlmIChzdGF0dXMgPT0gMCAmJiBsZ3AtPnJlcy5sYXlvdXRwLT5sZW4pDQo+ICAJCWxzZWcgPSBw
bmZzX2xheW91dF9wcm9jZXNzKGxncCk7DQo+IC0JbmZzNF9zZXF1ZW5jZV9mcmVlX3Nsb3QoJmxn
cC0+cmVzLnNlcV9yZXMpOw0KPiAgCXJwY19wdXRfdGFzayh0YXNrKTsNCj4gIAlkcHJpbnRrKCI8
LS0gJXMgc3RhdHVzPSVkXG4iLCBfX2Z1bmNfXywgc3RhdHVzKTsNCj4gIAlpZiAoc3RhdHVzKQ0K
DQpUaGUgcGxhY2VtZW50IG9mIHRoZSBjYWxsIHRvIG5mczRfc2VxdWVuY2VfZnJlZV9zbG90KCkg
YWZ0ZXIgdGhlIGNhbGwNCnRvIHBuZnNfbGF5b3V0X3Byb2Nlc3MoKSBpcyBkZWxpYmVyYXRlLCBh
bmQgaXMgbmVlZGVkIGluIG9yZGVyIHRvDQplbnN1cmUgdGhhdCBsYXlvdXRyZWNhbGwgY2FsbGJh
Y2tzIGFyZSBwcm9jZXNzZWQgY29ycmVjdGx5IGluIG9yZGVyDQphZnRlciB0aGUgbGF5b3V0Z2V0
IGhhcyBiZWVuIHByb2Nlc3NlZCAoc2VlIHJlZmVycmluZ19jYWxsX2V4aXN0cygpKS4NCg0KVGhl
IGNhbGwgdG8gbmZzNF9maW5kX2dldF9kZXZpY2VpZCgpIG5lZWRzIHRvIGJlIG1vdmVkIG91dCBv
Zg0KcG5mc191cGRhdGVfbGF5b3V0KCkuDQpJbiB0aGUgZmxleGZpbGVzIGRyaXZlciwgaXQgd2Fz
IG1vdmVkIHRvIG5mczRfZmZfbGF5b3V0X3ByZXBhcmVfZHMoKSwNCmJ1dCBpdCBsb29rcyB0byBt
ZSBhcyBpZiB0aGUgZmlsZXMgZHJpdmVyIG5lZWRzIHRvIGNhbGwgaXQgZWFybGllciBkdWUNCnRv
IHRoZSBuZWVkIHRvIGNhbGN1bGF0ZSBzdHJpcGVzLiBQZXJoYXBzIHRoZSByaWdodCB0aGluZyBp
cyB0byBhZGQgYQ0Kd3JhcHBlciBhcm91bmQgcG5mc191cGRhdGVfbGF5b3V0KCkgaW4gZnMvbmZz
L2ZpbGVsYXlvdXQvZmlsZWxheW91dC5jDQp0aGF0IGNhbiBiZSBjYWxsZWQgYnkgYm90aCBmaWxl
bGF5b3V0X3BnX2luaXRfcmVhZCgpIGFuZA0KZmlsZWxheW91dF9wZ19pbml0X3dyaXRlKCk/DQoN
Ci0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXIsIFByaW1h
cnlEYXRhDQp0cm9uZC5teWtsZWJ1c3RAcHJpbWFyeWRhdGEuY29tDQoNCg0K
--
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
diff mbox

Patch

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 1b18368..8ca92d0 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -8504,10 +8504,10 @@  nfs4_proc_layoutget(struct nfs4_layoutget *lgp, long *timeout, gfp_t gfp_flags)
 			&lgp->res.stateid,
 			status);
 
+	nfs4_sequence_free_slot(&lgp->res.seq_res);
 	/* if layoutp->len is 0, nfs4_layoutget_prepare called rpc_exit */
 	if (status == 0 && lgp->res.layoutp->len)
 		lseg = pnfs_layout_process(lgp);
-	nfs4_sequence_free_slot(&lgp->res.seq_res);
 	rpc_put_task(task);
 	dprintk("<-- %s status=%d\n", __func__, status);
 	if (status)