[PATCH] NFSv4: Don't add a new lock on an interrupted wait for LOCK

[PATCH] NFSv4: Don't add a new lock on an interrupted wait for LOCK

[Benjamin Coddington]

If the wait for a LOCK operation is interrupted, and then the file is
closed, the locks cleanup code will assume that no new locks will be added
to the inode after it has completed.  We already have a mechanism to detect
if there was signal, so let's use that to avoid recreating the local lock
once the RPC completes.  Also skip re-sending the LOCK operation for the
various error cases if we were signaled.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfs/nfs4proc.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 47f3c273245e..1aba009a5ef8 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6345,32 +6345,36 @@ static void nfs4_lock_done(struct rpc_task *task, void *calldata)
 	case 0:
 		renew_lease(NFS_SERVER(d_inode(data->ctx->dentry)),
 				data->timestamp);
-		if (data->arg.new_lock) {
+		if (data->arg.new_lock && !data->cancelled) {
 			data->fl.fl_flags &= ~(FL_SLEEP | FL_ACCESS);
-			if (locks_lock_inode_wait(lsp->ls_state->inode, &data->fl) < 0) {
-				rpc_restart_call_prepare(task);
+			if (locks_lock_inode_wait(lsp->ls_state->inode, &data->fl) > 0)
 				break;
-			}
 		}
+
 		if (data->arg.new_lock_owner != 0) {
 			nfs_confirm_seqid(&lsp->ls_seqid, 0);
 			nfs4_stateid_copy(&lsp->ls_stateid, &data->res.stateid);
 			set_bit(NFS_LOCK_INITIALIZED, &lsp->ls_flags);
-		} else if (!nfs4_update_lock_stateid(lsp, &data->res.stateid))
-			rpc_restart_call_prepare(task);
+			goto out_done;
+		} else if (nfs4_update_lock_stateid(lsp, &data->res.stateid))
+			goto out_done;
+
 		break;
 	case -NFS4ERR_BAD_STATEID:
 	case -NFS4ERR_OLD_STATEID:
 	case -NFS4ERR_STALE_STATEID:
 	case -NFS4ERR_EXPIRED:
 		if (data->arg.new_lock_owner != 0) {
-			if (!nfs4_stateid_match(&data->arg.open_stateid,
+			if (nfs4_stateid_match(&data->arg.open_stateid,
 						&lsp->ls_state->open_stateid))
-				rpc_restart_call_prepare(task);
-		} else if (!nfs4_stateid_match(&data->arg.lock_stateid,
+				goto out_done;
+		} else if (nfs4_stateid_match(&data->arg.lock_stateid,
 						&lsp->ls_stateid))
-				rpc_restart_call_prepare(task);
+				goto out_done;
 	}
+	if (!data->cancelled)
+		rpc_restart_call_prepare(task);
+out_done:
 	dprintk("%s: done, ret = %d!\n", __func__, data->rpc_status);
 }
 
-- 
2.14.3

Re: [PATCH] NFSv4: Don't add a new lock on an interrupted wait for LOCK

[Trond Myklebust]

T24gVGh1LCAyMDE4LTA1LTAzIGF0IDA3OjEyIC0wNDAwLCBCZW5qYW1pbiBDb2RkaW5ndG9uIHdy
b3RlOg0KPiBJZiB0aGUgd2FpdCBmb3IgYSBMT0NLIG9wZXJhdGlvbiBpcyBpbnRlcnJ1cHRlZCwg
YW5kIHRoZW4gdGhlIGZpbGUgaXMNCj4gY2xvc2VkLCB0aGUgbG9ja3MgY2xlYW51cCBjb2RlIHdp
bGwgYXNzdW1lIHRoYXQgbm8gbmV3IGxvY2tzIHdpbGwgYmUNCj4gYWRkZWQNCj4gdG8gdGhlIGlu
b2RlIGFmdGVyIGl0IGhhcyBjb21wbGV0ZWQuICBXZSBhbHJlYWR5IGhhdmUgYSBtZWNoYW5pc20g
dG8NCj4gZGV0ZWN0DQo+IGlmIHRoZXJlIHdhcyBzaWduYWwsIHNvIGxldCdzIHVzZSB0aGF0IHRv
IGF2b2lkIHJlY3JlYXRpbmcgdGhlIGxvY2FsDQo+IGxvY2sNCj4gb25jZSB0aGUgUlBDIGNvbXBs
ZXRlcy4gIEFsc28gc2tpcCByZS1zZW5kaW5nIHRoZSBMT0NLIG9wZXJhdGlvbiBmb3INCj4gdGhl
DQo+IHZhcmlvdXMgZXJyb3IgY2FzZXMgaWYgd2Ugd2VyZSBzaWduYWxlZC4NCj4gDQo+IFNpZ25l
ZC1vZmYtYnk6IEJlbmphbWluIENvZGRpbmd0b24gPGJjb2RkaW5nQHJlZGhhdC5jb20+DQo+IC0t
LQ0KPiAgZnMvbmZzL25mczRwcm9jLmMgfCAyNCArKysrKysrKysrKysrKy0tLS0tLS0tLS0NCj4g
IDEgZmlsZSBjaGFuZ2VkLCAxNCBpbnNlcnRpb25zKCspLCAxMCBkZWxldGlvbnMoLSkNCj4gDQo+
IGRpZmYgLS1naXQgYS9mcy9uZnMvbmZzNHByb2MuYyBiL2ZzL25mcy9uZnM0cHJvYy5jDQo+IGlu
ZGV4IDQ3ZjNjMjczMjQ1ZS4uMWFiYTAwOWE1ZWY4IDEwMDY0NA0KPiAtLS0gYS9mcy9uZnMvbmZz
NHByb2MuYw0KPiArKysgYi9mcy9uZnMvbmZzNHByb2MuYw0KPiBAQCAtNjM0NSwzMiArNjM0NSwz
NiBAQCBzdGF0aWMgdm9pZCBuZnM0X2xvY2tfZG9uZShzdHJ1Y3QgcnBjX3Rhc2sNCj4gKnRhc2ss
IHZvaWQgKmNhbGxkYXRhKQ0KPiAgCWNhc2UgMDoNCj4gIAkJcmVuZXdfbGVhc2UoTkZTX1NFUlZF
UihkX2lub2RlKGRhdGEtPmN0eC0+ZGVudHJ5KSksDQo+ICAJCQkJZGF0YS0+dGltZXN0YW1wKTsN
Cj4gLQkJaWYgKGRhdGEtPmFyZy5uZXdfbG9jaykgew0KPiArCQlpZiAoZGF0YS0+YXJnLm5ld19s
b2NrICYmICFkYXRhLT5jYW5jZWxsZWQpIHsNCj4gIAkJCWRhdGEtPmZsLmZsX2ZsYWdzICY9IH4o
RkxfU0xFRVAgfA0KPiBGTF9BQ0NFU1MpOw0KPiAtCQkJaWYgKGxvY2tzX2xvY2tfaW5vZGVfd2Fp
dChsc3AtPmxzX3N0YXRlLQ0KPiA+aW5vZGUsICZkYXRhLT5mbCkgPCAwKSB7DQo+IC0JCQkJcnBj
X3Jlc3RhcnRfY2FsbF9wcmVwYXJlKHRhc2spOw0KPiArCQkJaWYgKGxvY2tzX2xvY2tfaW5vZGVf
d2FpdChsc3AtPmxzX3N0YXRlLQ0KPiA+aW5vZGUsICZkYXRhLT5mbCkgPiAwKQ0KDQpBRkFJQ1Mg
dGhpcyB3aWxsIG5ldmVyIGJlIHRydWU7IEl0IGxvb2tzIHRvIG1lIGFzIGlmDQpsb2Nrc19sb2Nr
X2lub2RlX3dhaXQoKSBhbHdheXMgcmV0dXJucyAnMCcgb3IgYSBuZWdhdGl2ZSBlcnJvciB2YWx1
ZS4NCkFtIEkgbWlzc2luZyBzb21ldGhpbmc/DQoNCj4gIAkJCQlicmVhazsNCj4gLQkJCX0NCj4g
IAkJfQ0KPiArDQo+ICAJCWlmIChkYXRhLT5hcmcubmV3X2xvY2tfb3duZXIgIT0gMCkgew0KPiAg
CQkJbmZzX2NvbmZpcm1fc2VxaWQoJmxzcC0+bHNfc2VxaWQsIDApOw0KPiAgCQkJbmZzNF9zdGF0
ZWlkX2NvcHkoJmxzcC0+bHNfc3RhdGVpZCwgJmRhdGEtDQo+ID5yZXMuc3RhdGVpZCk7DQo+ICAJ
CQlzZXRfYml0KE5GU19MT0NLX0lOSVRJQUxJWkVELCAmbHNwLQ0KPiA+bHNfZmxhZ3MpOw0KPiAt
CQl9IGVsc2UgaWYgKCFuZnM0X3VwZGF0ZV9sb2NrX3N0YXRlaWQobHNwLCAmZGF0YS0NCj4gPnJl
cy5zdGF0ZWlkKSkNCj4gLQkJCXJwY19yZXN0YXJ0X2NhbGxfcHJlcGFyZSh0YXNrKTsNCj4gKwkJ
CWdvdG8gb3V0X2RvbmU7DQo+ICsJCX0gZWxzZSBpZiAobmZzNF91cGRhdGVfbG9ja19zdGF0ZWlk
KGxzcCwgJmRhdGEtDQo+ID5yZXMuc3RhdGVpZCkpDQo+ICsJCQlnb3RvIG91dF9kb25lOw0KPiAr
DQo+ICAJCWJyZWFrOw0KPiAgCWNhc2UgLU5GUzRFUlJfQkFEX1NUQVRFSUQ6DQo+ICAJY2FzZSAt
TkZTNEVSUl9PTERfU1RBVEVJRDoNCj4gIAljYXNlIC1ORlM0RVJSX1NUQUxFX1NUQVRFSUQ6DQo+
ICAJY2FzZSAtTkZTNEVSUl9FWFBJUkVEOg0KPiAgCQlpZiAoZGF0YS0+YXJnLm5ld19sb2NrX293
bmVyICE9IDApIHsNCj4gLQkJCWlmICghbmZzNF9zdGF0ZWlkX21hdGNoKCZkYXRhLQ0KPiA+YXJn
Lm9wZW5fc3RhdGVpZCwNCj4gKwkJCWlmIChuZnM0X3N0YXRlaWRfbWF0Y2goJmRhdGEtDQo+ID5h
cmcub3Blbl9zdGF0ZWlkLA0KPiAgCQkJCQkJJmxzcC0+bHNfc3RhdGUtDQo+ID5vcGVuX3N0YXRl
aWQpKQ0KPiAtCQkJCXJwY19yZXN0YXJ0X2NhbGxfcHJlcGFyZSh0YXNrKTsNCj4gLQkJfSBlbHNl
IGlmICghbmZzNF9zdGF0ZWlkX21hdGNoKCZkYXRhLQ0KPiA+YXJnLmxvY2tfc3RhdGVpZCwNCj4g
KwkJCQlnb3RvIG91dF9kb25lOw0KPiArCQl9IGVsc2UgaWYgKG5mczRfc3RhdGVpZF9tYXRjaCgm
ZGF0YS0NCj4gPmFyZy5sb2NrX3N0YXRlaWQsDQo+ICAJCQkJCQkmbHNwLT5sc19zdGF0ZWlkKSkN
Cj4gLQkJCQlycGNfcmVzdGFydF9jYWxsX3ByZXBhcmUodGFzayk7DQo+ICsJCQkJZ290byBvdXRf
ZG9uZTsNCj4gIAl9DQo+ICsJaWYgKCFkYXRhLT5jYW5jZWxsZWQpDQo+ICsJCXJwY19yZXN0YXJ0
X2NhbGxfcHJlcGFyZSh0YXNrKTsNCj4gK291dF9kb25lOg0KPiAgCWRwcmludGsoIiVzOiBkb25l
LCByZXQgPSAlZCFcbiIsIF9fZnVuY19fLCBkYXRhLQ0KPiA+cnBjX3N0YXR1cyk7DQo+ICB9DQo+
ICANCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXIsIEhh
bW1lcnNwYWNlDQp0cm9uZC5teWtsZWJ1c3RAaGFtbWVyc3BhY2UuY29tDQoNCg==

Re: [PATCH] NFSv4: Don't add a new lock on an interrupted wait for LOCK

[Benjamin Coddington]

On 29 May 2018, at 10:02, Trond Myklebust wrote:

> On Thu, 2018-05-03 at 07:12 -0400, Benjamin Coddington wrote:
>> If the wait for a LOCK operation is interrupted, and then the file is
>> closed, the locks cleanup code will assume that no new locks will be
>> added
>> to the inode after it has completed.  We already have a mechanism to
>> detect
>> if there was signal, so let's use that to avoid recreating the local
>> lock
>> once the RPC completes.  Also skip re-sending the LOCK operation for
>> the
>> various error cases if we were signaled.
>>
>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>> ---
>>  fs/nfs/nfs4proc.c | 24 ++++++++++++++----------
>>  1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 47f3c273245e..1aba009a5ef8 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -6345,32 +6345,36 @@ static void nfs4_lock_done(struct rpc_task
>> *task, void *calldata)
>>  	case 0:
>>  		renew_lease(NFS_SERVER(d_inode(data->ctx->dentry)),
>>  				data->timestamp);
>> -		if (data->arg.new_lock) {
>> +		if (data->arg.new_lock && !data->cancelled) {
>>  			data->fl.fl_flags &= ~(FL_SLEEP |
>> FL_ACCESS);
>> -			if (locks_lock_inode_wait(lsp->ls_state-
>>> inode, &data->fl) < 0) {
>> -				rpc_restart_call_prepare(task);
>> +			if (locks_lock_inode_wait(lsp->ls_state-
>>> inode, &data->fl) > 0)
>
> AFAICS this will never be true; It looks to me as if
> locks_lock_inode_wait() always returns '0' or a negative error value.
> Am I missing something?

No you're not missing anything, you're catching a typo, it should be:

 +			if (locks_lock_inode_wait(lsp->ls_state-> inode, &data->fl) < 0)

We want to break out of the switch and restart the rpc call if
locks_lock_inode_wait returns an error.  Should I send another version or
can you fix it up?

Ben

Re: [PATCH] NFSv4: Don't add a new lock on an interrupted wait for LOCK

[Jeff Layton]

On Thu, 2018-05-03 at 07:12 -0400, Benjamin Coddington wrote:
> If the wait for a LOCK operation is interrupted, and then the file is
> closed, the locks cleanup code will assume that no new locks will be added
> to the inode after it has completed.  We already have a mechanism to detect
> if there was signal, so let's use that to avoid recreating the local lock
> once the RPC completes.  Also skip re-sending the LOCK operation for the
> various error cases if we were signaled.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/nfs4proc.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 47f3c273245e..1aba009a5ef8 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -6345,32 +6345,36 @@ static void nfs4_lock_done(struct rpc_task *task, void *calldata)
>  	case 0:
>  		renew_lease(NFS_SERVER(d_inode(data->ctx->dentry)),
>  				data->timestamp);
> -		if (data->arg.new_lock) {
> +		if (data->arg.new_lock && !data->cancelled) {
>  			data->fl.fl_flags &= ~(FL_SLEEP | FL_ACCESS);
> -			if (locks_lock_inode_wait(lsp->ls_state->inode, &data->fl) < 0) {
> -				rpc_restart_call_prepare(task);
> +			if (locks_lock_inode_wait(lsp->ls_state->inode, &data->fl) > 0)
>  				break;

It turns out that this patch is causing a problem with non-blocking lock
requests. The issue is that it doesn't handle NFS4ERR_DENIED correctly,
so we just end up requeueing the request over and over again on a non-
blocking lock.

As Trond had pointed out, the sense of the if statement is wrong here,
but there's a different issue as well. In the event that we do race in
the way you're concerned about, we'll end up with a lock set on the
server and no local record of that lock.

What's going to release that lock on the server at that point? AFAICT,
the assumption we've always had is that closing the file will end up
releasing them (via locks_remove_file) in the case where the process is
signaled, but this commit will change that.

If the concern is locks being set on the inode after the filp has
already been closed, then I think we'll need to come up with some other
way to synchronize this.

> -			}
>  		}
> +
>  		if (data->arg.new_lock_owner != 0) {
>  			nfs_confirm_seqid(&lsp->ls_seqid, 0);
>  			nfs4_stateid_copy(&lsp->ls_stateid, &data->res.stateid);
>  			set_bit(NFS_LOCK_INITIALIZED, &lsp->ls_flags);
> -		} else if (!nfs4_update_lock_stateid(lsp, &data->res.stateid))
> -			rpc_restart_call_prepare(task);
> +			goto out_done;
> +		} else if (nfs4_update_lock_stateid(lsp, &data->res.stateid))
> +			goto out_done;
> +
>  		break;
>  	case -NFS4ERR_BAD_STATEID:
>  	case -NFS4ERR_OLD_STATEID:
>  	case -NFS4ERR_STALE_STATEID:
>  	case -NFS4ERR_EXPIRED:
>  		if (data->arg.new_lock_owner != 0) {
> -			if (!nfs4_stateid_match(&data->arg.open_stateid,
> +			if (nfs4_stateid_match(&data->arg.open_stateid,
>  						&lsp->ls_state->open_stateid))
> -				rpc_restart_call_prepare(task);
> -		} else if (!nfs4_stateid_match(&data->arg.lock_stateid,
> +				goto out_done;
> +		} else if (nfs4_stateid_match(&data->arg.lock_stateid,
>  						&lsp->ls_stateid))
> -				rpc_restart_call_prepare(task);
> +				goto out_done;
>  	}
> +	if (!data->cancelled)
> +		rpc_restart_call_prepare(task);


Restarting the task should only be done when we want to poll again for
the lock immediately. In the case of something like NFS4ERR_DENIED, we
don't want to do that right away -- we'd rather the upper layer redrive
a new RPC after it has finished waiting.

> +out_done:
>  	dprintk("%s: done, ret = %d!\n", __func__, data->rpc_status);
>  }
>  

-- 
Jeff Layton <jlayton@kernel.org>

[PATCH] NFSv4: Don't add a new lock on an interrupted wait for LOCK

[Benjamin Coddington]

If the wait for a LOCK operation is interrupted, and then the file is
closed, the locks cleanup code will assume that no new locks will be added
to the inode after it has completed.  We already have a mechanism to detect
if there was an interrupt, so let's use that to avoid recreating the local
lock once the RPC completes.

Signed-off-by: Benjamin Coddington <bcodding@redhat.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 dbfa18900e25..5256f429c268 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6100,7 +6100,7 @@ static void nfs4_lock_done(struct rpc_task *task, void *calldata)
 	case 0:
 		renew_lease(NFS_SERVER(d_inode(data->ctx->dentry)),
 				data->timestamp);
-		if (data->arg.new_lock) {
+		if (data->arg.new_lock && !data->cancelled) {
 			data->fl.fl_flags &= ~(FL_SLEEP | FL_ACCESS);
 			if (locks_lock_inode_wait(lsp->ls_state->inode, &data->fl) < 0) {
 				rpc_restart_call_prepare(task);
-- 
2.9.3

Re: [PATCH] NFSv4: Don't add a new lock on an interrupted wait for LOCK

[Jeff Layton]

On Fri, 2017-07-21 at 13:38 -0400, Benjamin Coddington wrote:
> If the wait for a LOCK operation is interrupted, and then the file is
> closed, the locks cleanup code will assume that no new locks will be added
> to the inode after it has completed.  We already have a mechanism to detect
> if there was an interrupt, so let's use that to avoid recreating the local
> lock once the RPC completes.
> 

nit: "if there was a signal"

> Signed-off-by: Benjamin Coddington <bcodding@redhat.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 dbfa18900e25..5256f429c268 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -6100,7 +6100,7 @@ static void nfs4_lock_done(struct rpc_task *task, void *calldata)
>  	case 0:
>  		renew_lease(NFS_SERVER(d_inode(data->ctx->dentry)),
>  				data->timestamp);
> -		if (data->arg.new_lock) {
> +		if (data->arg.new_lock && !data->cancelled) {
>  			data->fl.fl_flags &= ~(FL_SLEEP | FL_ACCESS);
>  			if (locks_lock_inode_wait(lsp->ls_state->inode, &data->fl) < 0) {
>  				rpc_restart_call_prepare(task);

Patch looks fine though:

Reviewed-by: Jeff Layton <jlayton@redhat.com>