diff mbox

[3/4] NFSv4 _nfs4_do_setattr use zero stateid on lost lock

Message ID 1393954269-3974-4-git-send-email-andros@netapp.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andy Adamson March 4, 2014, 5:31 p.m. UTC
From: Andy Adamson <andros@netapp.com>

Use the zero stateid if nfs4_select_rw_stateid indicates a lost lock (-EIO)

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/nfs4proc.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Comments

Trond Myklebust March 4, 2014, 6:42 p.m. UTC | #1
On Tue, 2014-03-04 at 12:31 -0500, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> Use the zero stateid if nfs4_select_rw_stateid indicates a lost lock (-EIO)
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfs/nfs4proc.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index de8e7f5..efe940c 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2458,18 +2458,22 @@ static int _nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
>  	truncate = (sattr->ia_valid & ATTR_SIZE) ? true : false;
>  	fmode = truncate ? FMODE_WRITE : FMODE_READ;
>  
> -	if (nfs4_copy_delegation_stateid(&arg.stateid, inode, fmode)) {
> -		/* Use that stateid */
> -	} else if (truncate && state != NULL && nfs4_valid_open_stateid(state)) {
> +	if (nfs4_copy_delegation_stateid(&arg.stateid, inode, fmode))
> +		/* Use delegation stateid */
> +		goto do_call;
> +	if (truncate && state != NULL && nfs4_valid_open_stateid(state)) {
>  		struct nfs_lockowner lockowner = {
>  			.l_owner = current->files,
>  			.l_pid = current->tgid,
>  		};
> -		nfs4_select_rw_stateid(&arg.stateid, state, FMODE_WRITE,
> -				&lockowner);
> -	} else
> -		nfs4_stateid_copy(&arg.stateid, &zero_stateid);
> +		/* Use zero stateid if lock is lost (-EIO fall through) */
> +		if (nfs4_select_rw_stateid(&arg.stateid, state, FMODE_WRITE,
> +						&lockowner) != -EIO)

Shouldn't we fail with EBADF instead?

> +			goto do_call;
> +	}
> +	nfs4_stateid_copy(&arg.stateid, &zero_stateid);
>  
> +do_call:
>  	status = nfs4_call_sync(server->client, server, &msg, &arg.seq_args, &res.seq_res, 1);
>  	if (status == 0 && state != NULL)
>  		renew_lease(server, timestamp);
William A. (Andy) Adamson March 5, 2014, 8:51 a.m. UTC | #2
On Tue, Mar 4, 2014 at 1:42 PM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> On Tue, 2014-03-04 at 12:31 -0500, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> Use the zero stateid if nfs4_select_rw_stateid indicates a lost lock (-EIO)
>>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>>  fs/nfs/nfs4proc.c | 18 +++++++++++-------
>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index de8e7f5..efe940c 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -2458,18 +2458,22 @@ static int _nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
>>       truncate = (sattr->ia_valid & ATTR_SIZE) ? true : false;
>>       fmode = truncate ? FMODE_WRITE : FMODE_READ;
>>
>> -     if (nfs4_copy_delegation_stateid(&arg.stateid, inode, fmode)) {
>> -             /* Use that stateid */
>> -     } else if (truncate && state != NULL && nfs4_valid_open_stateid(state)) {
>> +     if (nfs4_copy_delegation_stateid(&arg.stateid, inode, fmode))
>> +             /* Use delegation stateid */
>> +             goto do_call;
>> +     if (truncate && state != NULL && nfs4_valid_open_stateid(state)) {
>>               struct nfs_lockowner lockowner = {
>>                       .l_owner = current->files,
>>                       .l_pid = current->tgid,
>>               };
>> -             nfs4_select_rw_stateid(&arg.stateid, state, FMODE_WRITE,
>> -                             &lockowner);
>> -     } else
>> -             nfs4_stateid_copy(&arg.stateid, &zero_stateid);
>> +             /* Use zero stateid if lock is lost (-EIO fall through) */
>> +             if (nfs4_select_rw_stateid(&arg.stateid, state, FMODE_WRITE,
>> +                                             &lockowner) != -EIO)
>
> Shouldn't we fail with EBADF instead?

Hmm. -EIO means lost lock, but not lost file, which is why I say we
send it with a zero stateid.

-->Andy

>
>> +                     goto do_call;
>> +     }
>> +     nfs4_stateid_copy(&arg.stateid, &zero_stateid);
>>
>> +do_call:
>>       status = nfs4_call_sync(server->client, server, &msg, &arg.seq_args, &res.seq_res, 1);
>>       if (status == 0 && state != NULL)
>>               renew_lease(server, timestamp);
>
> --
> Trond Myklebust
> Linux NFS client maintainer, PrimaryData
> trond.myklebust@primarydata.com
>
>
> --
> 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
--
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
Trond Myklebust March 5, 2014, 1:04 p.m. UTC | #3
On Mar 5, 2014, at 3:51, Andy Adamson <androsadamson@gmail.com> wrote:

> On Tue, Mar 4, 2014 at 1:42 PM, Trond Myklebust
> <trond.myklebust@primarydata.com> wrote:
>> On Tue, 2014-03-04 at 12:31 -0500, andros@netapp.com wrote:
>>> From: Andy Adamson <andros@netapp.com>
>>> 
>>> Use the zero stateid if nfs4_select_rw_stateid indicates a lost lock (-EIO)
>>> 
>>> Signed-off-by: Andy Adamson <andros@netapp.com>
>>> ---
>>> fs/nfs/nfs4proc.c | 18 +++++++++++-------
>>> 1 file changed, 11 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index de8e7f5..efe940c 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -2458,18 +2458,22 @@ static int _nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
>>>      truncate = (sattr->ia_valid & ATTR_SIZE) ? true : false;
>>>      fmode = truncate ? FMODE_WRITE : FMODE_READ;
>>> 
>>> -     if (nfs4_copy_delegation_stateid(&arg.stateid, inode, fmode)) {
>>> -             /* Use that stateid */
>>> -     } else if (truncate && state != NULL && nfs4_valid_open_stateid(state)) {
>>> +     if (nfs4_copy_delegation_stateid(&arg.stateid, inode, fmode))
>>> +             /* Use delegation stateid */
>>> +             goto do_call;
>>> +     if (truncate && state != NULL && nfs4_valid_open_stateid(state)) {
>>>              struct nfs_lockowner lockowner = {
>>>                      .l_owner = current->files,
>>>                      .l_pid = current->tgid,
>>>              };
>>> -             nfs4_select_rw_stateid(&arg.stateid, state, FMODE_WRITE,
>>> -                             &lockowner);
>>> -     } else
>>> -             nfs4_stateid_copy(&arg.stateid, &zero_stateid);
>>> +             /* Use zero stateid if lock is lost (-EIO fall through) */
>>> +             if (nfs4_select_rw_stateid(&arg.stateid, state, FMODE_WRITE,
>>> +                                             &lockowner) != -EIO)
>> 
>> Shouldn't we fail with EBADF instead?
> 
> Hmm. -EIO means lost lock, but not lost file, which is why I say we
> send it with a zero stateid.

If the application has lost the lock that was protecting the data, then why should we think it is safe to allow it to proceed to modify the file by truncating it?
diff mbox

Patch

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index de8e7f5..efe940c 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2458,18 +2458,22 @@  static int _nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
 	truncate = (sattr->ia_valid & ATTR_SIZE) ? true : false;
 	fmode = truncate ? FMODE_WRITE : FMODE_READ;
 
-	if (nfs4_copy_delegation_stateid(&arg.stateid, inode, fmode)) {
-		/* Use that stateid */
-	} else if (truncate && state != NULL && nfs4_valid_open_stateid(state)) {
+	if (nfs4_copy_delegation_stateid(&arg.stateid, inode, fmode))
+		/* Use delegation stateid */
+		goto do_call;
+	if (truncate && state != NULL && nfs4_valid_open_stateid(state)) {
 		struct nfs_lockowner lockowner = {
 			.l_owner = current->files,
 			.l_pid = current->tgid,
 		};
-		nfs4_select_rw_stateid(&arg.stateid, state, FMODE_WRITE,
-				&lockowner);
-	} else
-		nfs4_stateid_copy(&arg.stateid, &zero_stateid);
+		/* Use zero stateid if lock is lost (-EIO fall through) */
+		if (nfs4_select_rw_stateid(&arg.stateid, state, FMODE_WRITE,
+						&lockowner) != -EIO)
+			goto do_call;
+	}
+	nfs4_stateid_copy(&arg.stateid, &zero_stateid);
 
+do_call:
 	status = nfs4_call_sync(server->client, server, &msg, &arg.seq_args, &res.seq_res, 1);
 	if (status == 0 && state != NULL)
 		renew_lease(server, timestamp);