Re: [ 131/149] NFSv4.1: Fix layoutcommit error handling

[Myklebust, Trond]

On Apr 1, 2012, at 2:23 PM, Ben Hutchings wrote:

> On Fri, 2012-03-30 at 12:50 -0700, Greg KH wrote:
>> 3.2-stable review patch.  If anyone has any objections, please let me know.
>> 
>> ------------------
>> 
>> From: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
>> 
>> commit e59d27e05a6435f8c04d5ad843f37fa795f2eaaa upstream.
>> 
>> Firstly, task->tk_status will always return negative error values,
>> so the current tests for 'NFS4ERR_DELEG_REVOKED' etc. are all being
>> ignored.
>> Secondly, clean up the code so that we only need to test
>> task->tk_status once!
>> 
>> Signed-off-by: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> 
>> ---
>> fs/nfs/nfs4proc.c |   25 +++++++++++++------------
>> 1 file changed, 13 insertions(+), 12 deletions(-)
>> 
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -5983,21 +5983,22 @@ nfs4_layoutcommit_done(struct rpc_task *
>> 		return;
>> 
>> 	switch (task->tk_status) { /* Just ignore these failures */
>> -	case NFS4ERR_DELEG_REVOKED: /* layout was recalled */
>> -	case NFS4ERR_BADIOMODE:     /* no IOMODE_RW layout for range */
>> -	case NFS4ERR_BADLAYOUT:     /* no layout */
>> -	case NFS4ERR_GRACE:	    /* loca_recalim always false */
>> +	case -NFS4ERR_DELEG_REVOKED: /* layout was recalled */
>> +	case -NFS4ERR_BADIOMODE:     /* no IOMODE_RW layout for range */
>> +	case -NFS4ERR_BADLAYOUT:     /* no layout */
>> +	case -NFS4ERR_GRACE:	    /* loca_recalim always false */
>> 		task->tk_status = 0;
>> -	}
>> -
>> -	if (nfs4_async_handle_error(task, server, NULL) == -EAGAIN) {
>> -		rpc_restart_call_prepare(task);
>> -		return;
>> -	}
>> -
>> -	if (task->tk_status == 0)
>> +		break;
> 
> It loooks like the previous intent was that for all those specific error
> codes we would end up calling nfs_post_op_update_inode_force_wcc().
> That didn't happen because of the incorrectly signed error codes.  But
> it still won't happen now, because of this break.  Do we really want to
> break here or fall through past 'case 0'?
> 

The 'break' there is deliberate. If the LAYOUTCOMMIT gets an error, then the server will never get to process the post-op GETATTR.

Trond

> Ben.
> 
>> +	case 0:
>> 		nfs_post_op_update_inode_force_wcc(data->args.inode,
>> 						   data->res.fattr);
>> +		break;
>> +	default:
>> +		if (nfs4_async_handle_error(task, server, NULL) == -EAGAIN) {
>> +			rpc_restart_call_prepare(task);
>> +			return;
>> +		}
>> +	}
>> }
>> 
>> static void nfs4_layoutcommit_release(void *calldata)
> 
> -- 
> Ben Hutchings
> I'm always amazed by the number of people who take up solipsism because
> they heard someone else explain it. - E*Borg on alt.fan.pratchett

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/