[PATCH 1/2] NFSv4.1: Get rid of redundant NFS4CLNT_LAYOUTRECALL tests

[PATCH 1/2] NFSv4.1: Get rid of redundant NFS4CLNT_LAYOUTRECALL tests

[Trond Myklebust]

The NFS4CLNT_LAYOUTRECALL tests in pnfs_layout_process and
pnfs_update_layout are redundant.

In the case of a bulk layout recall, we're always testing for
the NFS_LAYOUT_BULK_RECALL flay anyway.
In the case of a file or segment recall, the call to
pnfs_set_layout_stateid() updates the layout_header 'barrier'
sequence id, which triggers the test in pnfs_layoutgets_blocked()
and is less race-prone than NFS4CLNT_LAYOUTRECALL anyway.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
 fs/nfs/pnfs.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index a534216..9c19f92 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -966,8 +966,7 @@ pnfs_update_layout(struct inode *ino,
 	}
 
 	/* Do we even need to bother with this? */
-	if (test_bit(NFS4CLNT_LAYOUTRECALL, &clp->cl_state) ||
-	    test_bit(NFS_LAYOUT_BULK_RECALL, &lo->plh_flags)) {
+	if (test_bit(NFS_LAYOUT_BULK_RECALL, &lo->plh_flags)) {
 		dprintk("%s matches recall, use MDS\n", __func__);
 		goto out_unlock;
 	}
@@ -1048,8 +1047,7 @@ pnfs_layout_process(struct nfs4_layoutget *lgp)
 	}
 
 	spin_lock(&ino->i_lock);
-	if (test_bit(NFS4CLNT_LAYOUTRECALL, &clp->cl_state) ||
-	    test_bit(NFS_LAYOUT_BULK_RECALL, &lo->plh_flags)) {
+	if (test_bit(NFS_LAYOUT_BULK_RECALL, &lo->plh_flags)) {
 		dprintk("%s forget reply due to recall\n", __func__);
 		goto out_forget_reply;
 	}
-- 
1.7.7.6

[PATCH 2/2] NFSv4.1: Get rid of NFS4CLNT_LAYOUTRECALL

[Trond Myklebust]

The NFS4CLNT_LAYOUTRECALL bit is a long-term impediment to scalability. It
basically stops all other recalls by a given server once any layout recall
is requested.

If the recall is for a different file, then we don't care.
If the recall applies to the same file, then we're in one of two situations:
Either we are in the case of a replay of an existing request, in which case
the session is supposed to deal with matters, or we are dealing with a
completely different request, in which case we should just try to process
it.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
 fs/nfs/callback_proc.c |    6 +-----
 fs/nfs/nfs4_fs.h       |    1 -
 2 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index f71978d..0e0865e 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -233,17 +233,13 @@ static u32 initiate_bulk_draining(struct nfs_client *clp,
 static u32 do_callback_layoutrecall(struct nfs_client *clp,
 				    struct cb_layoutrecallargs *args)
 {
-	u32 res = NFS4ERR_DELAY;
+	u32 res;
 
 	dprintk("%s enter, type=%i\n", __func__, args->cbl_recall_type);
-	if (test_and_set_bit(NFS4CLNT_LAYOUTRECALL, &clp->cl_state))
-		goto out;
 	if (args->cbl_recall_type == RETURN_FILE)
 		res = initiate_file_draining(clp, args);
 	else
 		res = initiate_bulk_draining(clp, args);
-	clear_bit(NFS4CLNT_LAYOUTRECALL, &clp->cl_state);
-out:
 	dprintk("%s returning %i\n", __func__, res);
 	return res;
 
diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index b133b50..19079ec 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -20,7 +20,6 @@ enum nfs4_client_state {
 	NFS4CLNT_RECLAIM_REBOOT,
 	NFS4CLNT_RECLAIM_NOGRACE,
 	NFS4CLNT_DELEGRETURN,
-	NFS4CLNT_LAYOUTRECALL,
 	NFS4CLNT_SESSION_RESET,
 	NFS4CLNT_RECALL_SLOT,
 	NFS4CLNT_LEASE_CONFIRM,
-- 
1.7.7.6

Re: [PATCH 1/2] NFSv4.1: Get rid of redundant NFS4CLNT_LAYOUTRECALL tests

[Bryan Schumaker]

On 02/29/2012 06:34 PM, Trond Myklebust wrote:

> The NFS4CLNT_LAYOUTRECALL tests in pnfs_layout_process and
> pnfs_update_layout are redundant.
> 
> In the case of a bulk layout recall, we're always testing for
> the NFS_LAYOUT_BULK_RECALL flay anyway.
> In the case of a file or segment recall, the call to
> pnfs_set_layout_stateid() updates the layout_header 'barrier'
> sequence id, which triggers the test in pnfs_layoutgets_blocked()
> and is less race-prone than NFS4CLNT_LAYOUTRECALL anyway.
> 
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
>  fs/nfs/pnfs.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index a534216..9c19f92 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -966,8 +966,7 @@ pnfs_update_layout(struct inode *ino,
>  	}
>  
>  	/* Do we even need to bother with this? */
> -	if (test_bit(NFS4CLNT_LAYOUTRECALL, &clp->cl_state) ||
> -	    test_bit(NFS_LAYOUT_BULK_RECALL, &lo->plh_flags)) {
> +	if (test_bit(NFS_LAYOUT_BULK_RECALL, &lo->plh_flags)) {
>  		dprintk("%s matches recall, use MDS\n", __func__);
>  		goto out_unlock;
>  	}
> @@ -1048,8 +1047,7 @@ pnfs_layout_process(struct nfs4_layoutget *lgp)
>  	}
>  
>  	spin_lock(&ino->i_lock);
> -	if (test_bit(NFS4CLNT_LAYOUTRECALL, &clp->cl_state) ||
> -	    test_bit(NFS_LAYOUT_BULK_RECALL, &lo->plh_flags)) {


This was the only place that the clp structure was used, so you could probably remove it to avoid this warning:

fs/nfs/pnfs.c: In function 'pnfs_layout_process':
fs/nfs/pnfs.c:1058:21: warning: unused variable 'clp' [-Wunused-variable]

- Bryan

> +	if (test_bit(NFS_LAYOUT_BULK_RECALL, &lo->plh_flags)) {
>  		dprintk("%s forget reply due to recall\n", __func__);
>  		goto out_forget_reply;
>  	}

Re: [PATCH 1/2] NFSv4.1: Get rid of redundant NFS4CLNT_LAYOUTRECALL tests

[Myklebust, Trond]

T24gVGh1LCAyMDEyLTAzLTAxIGF0IDEwOjM5IC0wNTAwLCBCcnlhbiBTY2h1bWFrZXIgd3JvdGU6
DQo+IE9uIDAyLzI5LzIwMTIgMDY6MzQgUE0sIFRyb25kIE15a2xlYnVzdCB3cm90ZToNCj4gDQo+
ID4gVGhlIE5GUzRDTE5UX0xBWU9VVFJFQ0FMTCB0ZXN0cyBpbiBwbmZzX2xheW91dF9wcm9jZXNz
IGFuZA0KPiA+IHBuZnNfdXBkYXRlX2xheW91dCBhcmUgcmVkdW5kYW50Lg0KPiA+IA0KPiA+IElu
IHRoZSBjYXNlIG9mIGEgYnVsayBsYXlvdXQgcmVjYWxsLCB3ZSdyZSBhbHdheXMgdGVzdGluZyBm
b3INCj4gPiB0aGUgTkZTX0xBWU9VVF9CVUxLX1JFQ0FMTCBmbGF5IGFueXdheS4NCj4gPiBJbiB0
aGUgY2FzZSBvZiBhIGZpbGUgb3Igc2VnbWVudCByZWNhbGwsIHRoZSBjYWxsIHRvDQo+ID4gcG5m
c19zZXRfbGF5b3V0X3N0YXRlaWQoKSB1cGRhdGVzIHRoZSBsYXlvdXRfaGVhZGVyICdiYXJyaWVy
Jw0KPiA+IHNlcXVlbmNlIGlkLCB3aGljaCB0cmlnZ2VycyB0aGUgdGVzdCBpbiBwbmZzX2xheW91
dGdldHNfYmxvY2tlZCgpDQo+ID4gYW5kIGlzIGxlc3MgcmFjZS1wcm9uZSB0aGFuIE5GUzRDTE5U
X0xBWU9VVFJFQ0FMTCBhbnl3YXkuDQo+ID4gDQo+ID4gU2lnbmVkLW9mZi1ieTogVHJvbmQgTXlr
bGVidXN0IDxUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbT4NCj4gPiAtLS0NCj4gPiAgZnMvbmZz
L3BuZnMuYyB8ICAgIDYgKystLS0tDQo+ID4gIDEgZmlsZXMgY2hhbmdlZCwgMiBpbnNlcnRpb25z
KCspLCA0IGRlbGV0aW9ucygtKQ0KPiA+IA0KPiA+IGRpZmYgLS1naXQgYS9mcy9uZnMvcG5mcy5j
IGIvZnMvbmZzL3BuZnMuYw0KPiA+IGluZGV4IGE1MzQyMTYuLjljMTlmOTIgMTAwNjQ0DQo+ID4g
LS0tIGEvZnMvbmZzL3BuZnMuYw0KPiA+ICsrKyBiL2ZzL25mcy9wbmZzLmMNCj4gPiBAQCAtOTY2
LDggKzk2Niw3IEBAIHBuZnNfdXBkYXRlX2xheW91dChzdHJ1Y3QgaW5vZGUgKmlubywNCj4gPiAg
CX0NCj4gPiAgDQo+ID4gIAkvKiBEbyB3ZSBldmVuIG5lZWQgdG8gYm90aGVyIHdpdGggdGhpcz8g
Ki8NCj4gPiAtCWlmICh0ZXN0X2JpdChORlM0Q0xOVF9MQVlPVVRSRUNBTEwsICZjbHAtPmNsX3N0
YXRlKSB8fA0KPiA+IC0JICAgIHRlc3RfYml0KE5GU19MQVlPVVRfQlVMS19SRUNBTEwsICZsby0+
cGxoX2ZsYWdzKSkgew0KPiA+ICsJaWYgKHRlc3RfYml0KE5GU19MQVlPVVRfQlVMS19SRUNBTEws
ICZsby0+cGxoX2ZsYWdzKSkgew0KPiA+ICAJCWRwcmludGsoIiVzIG1hdGNoZXMgcmVjYWxsLCB1
c2UgTURTXG4iLCBfX2Z1bmNfXyk7DQo+ID4gIAkJZ290byBvdXRfdW5sb2NrOw0KPiA+ICAJfQ0K
PiA+IEBAIC0xMDQ4LDggKzEwNDcsNyBAQCBwbmZzX2xheW91dF9wcm9jZXNzKHN0cnVjdCBuZnM0
X2xheW91dGdldCAqbGdwKQ0KPiA+ICAJfQ0KPiA+ICANCj4gPiAgCXNwaW5fbG9jaygmaW5vLT5p
X2xvY2spOw0KPiA+IC0JaWYgKHRlc3RfYml0KE5GUzRDTE5UX0xBWU9VVFJFQ0FMTCwgJmNscC0+
Y2xfc3RhdGUpIHx8DQo+ID4gLQkgICAgdGVzdF9iaXQoTkZTX0xBWU9VVF9CVUxLX1JFQ0FMTCwg
JmxvLT5wbGhfZmxhZ3MpKSB7DQo+IA0KPiANCj4gVGhpcyB3YXMgdGhlIG9ubHkgcGxhY2UgdGhh
dCB0aGUgY2xwIHN0cnVjdHVyZSB3YXMgdXNlZCwgc28geW91IGNvdWxkIHByb2JhYmx5IHJlbW92
ZSBpdCB0byBhdm9pZCB0aGlzIHdhcm5pbmc6DQo+IA0KPiBmcy9uZnMvcG5mcy5jOiBJbiBmdW5j
dGlvbiAncG5mc19sYXlvdXRfcHJvY2Vzcyc6DQo+IGZzL25mcy9wbmZzLmM6MTA1ODoyMTogd2Fy
bmluZzogdW51c2VkIHZhcmlhYmxlICdjbHAnIFstV3VudXNlZC12YXJpYWJsZV0NCg0KRG9uZS4g
VGhhbmtzIGZvciBzcG90dGluZyB0aGF0IQ0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXgg
TkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5j
b20NCnd3dy5uZXRhcHAuY29tDQoNCg==