2012-03-08 16:04:15

by Andy Adamson

[permalink] [raw]
Subject: [PATCH 1/1] NFSv4.1 cleanup DS stateid error handling

From: Andy Adamson <[email protected]>

The error handler nfs4_state parameter is never NULL in the pNFS case as
the open_context must carry an nfs_state.

Signed-off-by: Andy Adamson <[email protected]>
---
fs/nfs/nfs4filelayout.c | 9 ++-------
1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
index b2d3bb5..768f6f8 100644
--- a/fs/nfs/nfs4filelayout.c
+++ b/fs/nfs/nfs4filelayout.c
@@ -99,16 +99,12 @@ static int filelayout_async_handle_error(struct rpc_task *task,
case -NFS4ERR_DELEG_REVOKED:
case -NFS4ERR_ADMIN_REVOKED:
case -NFS4ERR_BAD_STATEID:
- if (state != NULL)
- nfs_remove_bad_delegation(state->inode);
+ nfs_remove_bad_delegation(state->inode);
case -NFS4ERR_OPENMODE:
- if (state == NULL)
- break;
nfs4_schedule_stateid_recovery(mds_server, state);
goto wait_on_recovery;
case -NFS4ERR_EXPIRED:
- if (state != NULL)
- nfs4_schedule_stateid_recovery(mds_server, state);
+ nfs4_schedule_stateid_recovery(mds_server, state);
nfs4_schedule_lease_recovery(mds_client);
goto wait_on_recovery;
/* DS session errors */
@@ -145,7 +141,6 @@ wait_on_recovery:
if (test_bit(NFS4CLNT_MANAGER_RUNNING, &mds_client->cl_state) == 0)
rpc_wake_up_queued_task(&mds_client->cl_rpcwaitq, task);
goto out;
-
}

/* NFS_PROTO call done callback routines */
--
1.7.6.4



2012-03-09 22:31:06

by Fred Isaman

[permalink] [raw]
Subject: Re: [PATCH 1/1] NFSv4.1 cleanup DS stateid error handling

On Thu, Mar 8, 2012 at 8:03 AM, <[email protected]> wrote:
> From: Andy Adamson <[email protected]>
>
> The error handler nfs4_state parameter is never NULL in the pNFS case as
> the open_context must carry an nfs_state.
>


Note Trond's argument is that with commit the state is avail for the
caller, so it need not be NULL.
But the caller filelayout_commit_done_cb() needs to be adjusted.
Currently commit to ds oopses
due to that.

Fred

> Signed-off-by: Andy Adamson <[email protected]>
> ---
> ?fs/nfs/nfs4filelayout.c | ? ?9 ++-------
> ?1 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
> index b2d3bb5..768f6f8 100644
> --- a/fs/nfs/nfs4filelayout.c
> +++ b/fs/nfs/nfs4filelayout.c
> @@ -99,16 +99,12 @@ static int filelayout_async_handle_error(struct rpc_task *task,
> ? ? ? ?case -NFS4ERR_DELEG_REVOKED:
> ? ? ? ?case -NFS4ERR_ADMIN_REVOKED:
> ? ? ? ?case -NFS4ERR_BAD_STATEID:
> - ? ? ? ? ? ? ? if (state != NULL)
> - ? ? ? ? ? ? ? ? ? ? ? nfs_remove_bad_delegation(state->inode);
> + ? ? ? ? ? ? ? nfs_remove_bad_delegation(state->inode);
> ? ? ? ?case -NFS4ERR_OPENMODE:
> - ? ? ? ? ? ? ? if (state == NULL)
> - ? ? ? ? ? ? ? ? ? ? ? break;
> ? ? ? ? ? ? ? ?nfs4_schedule_stateid_recovery(mds_server, state);
> ? ? ? ? ? ? ? ?goto wait_on_recovery;
> ? ? ? ?case -NFS4ERR_EXPIRED:
> - ? ? ? ? ? ? ? if (state != NULL)
> - ? ? ? ? ? ? ? ? ? ? ? nfs4_schedule_stateid_recovery(mds_server, state);
> + ? ? ? ? ? ? ? nfs4_schedule_stateid_recovery(mds_server, state);
> ? ? ? ? ? ? ? ?nfs4_schedule_lease_recovery(mds_client);
> ? ? ? ? ? ? ? ?goto wait_on_recovery;
> ? ? ? ?/* DS session errors */
> @@ -145,7 +141,6 @@ wait_on_recovery:
> ? ? ? ?if (test_bit(NFS4CLNT_MANAGER_RUNNING, &mds_client->cl_state) == 0)
> ? ? ? ? ? ? ? ?rpc_wake_up_queued_task(&mds_client->cl_rpcwaitq, task);
> ? ? ? ?goto out;
> -
> ?}
>
> ?/* NFS_PROTO call done callback routines */
> --
> 1.7.6.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html

2012-03-10 02:03:08

by Fred Isaman

[permalink] [raw]
Subject: Re: [PATCH 1/1] NFSv4.1 cleanup DS stateid error handling

On Fri, Mar 9, 2012 at 2:51 PM, Myklebust, Trond
<[email protected]> wrote:
> On Fri, 2012-03-09 at 14:31 -0800, Fred Isaman wrote:
>> On Thu, Mar 8, 2012 at 8:03 AM, ?<[email protected]> wrote:
>> > From: Andy Adamson <[email protected]>
>> >
>> > The error handler nfs4_state parameter is never NULL in the pNFS case as
>> > the open_context must carry an nfs_state.
>> >
>>
>>
>> Note Trond's argument is that with commit the state is avail for the
>> caller, so it need not be NULL.
>> But the caller filelayout_commit_done_cb() needs to be adjusted.
>> Currently commit to ds oopses
>> due to that.
>
> Are you sure? As far as I can see, filelayout_commit_pagelist _does_
> call nfs_init_commit(), which again sets the data->args.context.
> Later, filelayout_commit_done_cb calls filelayout_async_handle_error()
> using the data->args.context->state argument.
>
> How is that failing to provide a valid open state argument?
>
> --

Gah, sorry. A rebase of some of my commit changes silently reverted
it to a NULL argument. I withdraw my complaint.

Fred

2012-03-09 22:52:08

by Myklebust, Trond

[permalink] [raw]
Subject: Re: [PATCH 1/1] NFSv4.1 cleanup DS stateid error handling

T24gRnJpLCAyMDEyLTAzLTA5IGF0IDE0OjMxIC0wODAwLCBGcmVkIElzYW1hbiB3cm90ZToNCj4g
T24gVGh1LCBNYXIgOCwgMjAxMiBhdCA4OjAzIEFNLCAgPGFuZHJvc0BuZXRhcHAuY29tPiB3cm90
ZToNCj4gPiBGcm9tOiBBbmR5IEFkYW1zb24gPGFuZHJvc0BuZXRhcHAuY29tPg0KPiA+DQo+ID4g
VGhlIGVycm9yIGhhbmRsZXIgbmZzNF9zdGF0ZSBwYXJhbWV0ZXIgaXMgbmV2ZXIgTlVMTCBpbiB0
aGUgcE5GUyBjYXNlIGFzDQo+ID4gdGhlIG9wZW5fY29udGV4dCBtdXN0IGNhcnJ5IGFuIG5mc19z
dGF0ZS4NCj4gPg0KPiANCj4gDQo+IE5vdGUgVHJvbmQncyBhcmd1bWVudCBpcyB0aGF0IHdpdGgg
Y29tbWl0IHRoZSBzdGF0ZSBpcyBhdmFpbCBmb3IgdGhlDQo+IGNhbGxlciwgc28gaXQgbmVlZCBu
b3QgYmUgTlVMTC4NCj4gQnV0IHRoZSBjYWxsZXIgZmlsZWxheW91dF9jb21taXRfZG9uZV9jYigp
IG5lZWRzIHRvIGJlIGFkanVzdGVkLg0KPiBDdXJyZW50bHkgY29tbWl0IHRvIGRzIG9vcHNlcw0K
PiBkdWUgdG8gdGhhdC4NCg0KQXJlIHlvdSBzdXJlPyBBcyBmYXIgYXMgSSBjYW4gc2VlLCBmaWxl
bGF5b3V0X2NvbW1pdF9wYWdlbGlzdCBfZG9lc18NCmNhbGwgbmZzX2luaXRfY29tbWl0KCksIHdo
aWNoIGFnYWluIHNldHMgdGhlIGRhdGEtPmFyZ3MuY29udGV4dC4NCkxhdGVyLCBmaWxlbGF5b3V0
X2NvbW1pdF9kb25lX2NiIGNhbGxzIGZpbGVsYXlvdXRfYXN5bmNfaGFuZGxlX2Vycm9yKCkNCnVz
aW5nIHRoZSBkYXRhLT5hcmdzLmNvbnRleHQtPnN0YXRlIGFyZ3VtZW50Lg0KDQpIb3cgaXMgdGhh
dCBmYWlsaW5nIHRvIHByb3ZpZGUgYSB2YWxpZCBvcGVuIHN0YXRlIGFyZ3VtZW50Pw0KDQotLSAN
ClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0K
VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCg==