We can currently loop forever in nfs4_lookup_root() and in
nfs41_proc_secinfo_no_name(), if the first iteration returns a
NFS4ERR_DELAY or something else that causes exception.retry to get
set.
Reported-by: Dros Adamson <[email protected]>
Signed-off-by: Trond Myklebust <[email protected]>
Cc: [email protected]
---
fs/nfs/nfs4proc.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 45df7d4..ee9ca19 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2290,11 +2290,12 @@ static int nfs4_lookup_root(struct nfs_server *server, struct nfs_fh *fhandle,
switch (err) {
case 0:
case -NFS4ERR_WRONGSEC:
- break;
+ goto out;
default:
err = nfs4_handle_exception(server, err, &exception);
}
} while (exception.retry);
+out:
return err;
}
@@ -6229,11 +6230,12 @@ nfs41_proc_secinfo_no_name(struct nfs_server *server, struct nfs_fh *fhandle,
case 0:
case -NFS4ERR_WRONGSEC:
case -NFS4ERR_NOTSUPP:
- break;
+ goto out;
default:
err = nfs4_handle_exception(server, err, &exception);
}
} while (exception.retry);
+out:
return err;
}
--
1.7.7.6
T24gV2VkLCAyMDEyLTAzLTI4IGF0IDE1OjA3ICswMDAwLCBBZGFtc29uLCBEcm9zIHdyb3RlOg0K
PiBUaGlzIHNob3VsZCBwcm9iYWJseSBiZSBzZW50IHRvIHN0YWJsZS4NCg0KU2VlIGJlbG93LiA6
LSkNCg0KPiAtZHJvcw0KPiANCj4gT24gTWFyIDI3LCAyMDEyLCBhdCA2OjM1IFBNLCBUcm9uZCBN
eWtsZWJ1c3Qgd3JvdGU6DQo+IA0KPiA+IFdlIGNhbiBjdXJyZW50bHkgbG9vcCBmb3JldmVyIGlu
IG5mczRfbG9va3VwX3Jvb3QoKSBhbmQgaW4NCj4gPiBuZnM0MV9wcm9jX3NlY2luZm9fbm9fbmFt
ZSgpLCBpZiB0aGUgZmlyc3QgaXRlcmF0aW9uIHJldHVybnMgYQ0KPiA+IE5GUzRFUlJfREVMQVkg
b3Igc29tZXRoaW5nIGVsc2UgdGhhdCBjYXVzZXMgZXhjZXB0aW9uLnJldHJ5IHRvIGdldA0KPiA+
IHNldC4NCj4gPiANCj4gPiBSZXBvcnRlZC1ieTogRHJvcyBBZGFtc29uIDxXZXN0b24uQWRhbXNv
bkBuZXRhcHAuY29tPg0KPiA+IFNpZ25lZC1vZmYtYnk6IFRyb25kIE15a2xlYnVzdCA8VHJvbmQu
TXlrbGVidXN0QG5ldGFwcC5jb20+DQo+ID4gQ2M6IHN0YWJsZUB2Z2VyLmtlcm5lbC5vcmcNCj4g
PiAtLS0NCj4gPiBmcy9uZnMvbmZzNHByb2MuYyB8ICAgIDYgKysrKy0tDQo+ID4gMSBmaWxlcyBj
aGFuZ2VkLCA0IGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pDQo+ID4gDQo+ID4gZGlmZiAt
LWdpdCBhL2ZzL25mcy9uZnM0cHJvYy5jIGIvZnMvbmZzL25mczRwcm9jLmMNCj4gPiBpbmRleCA0
NWRmN2Q0Li5lZTljYTE5IDEwMDY0NA0KPiA+IC0tLSBhL2ZzL25mcy9uZnM0cHJvYy5jDQo+ID4g
KysrIGIvZnMvbmZzL25mczRwcm9jLmMNCj4gPiBAQCAtMjI5MCwxMSArMjI5MCwxMiBAQCBzdGF0
aWMgaW50IG5mczRfbG9va3VwX3Jvb3Qoc3RydWN0IG5mc19zZXJ2ZXIgKnNlcnZlciwgc3RydWN0
IG5mc19maCAqZmhhbmRsZSwNCj4gPiAJCXN3aXRjaCAoZXJyKSB7DQo+ID4gCQljYXNlIDA6DQo+
ID4gCQljYXNlIC1ORlM0RVJSX1dST05HU0VDOg0KPiA+IC0JCQlicmVhazsNCj4gPiArCQkJZ290
byBvdXQ7DQo+ID4gCQlkZWZhdWx0Og0KPiA+IAkJCWVyciA9IG5mczRfaGFuZGxlX2V4Y2VwdGlv
bihzZXJ2ZXIsIGVyciwgJmV4Y2VwdGlvbik7DQo+ID4gCQl9DQo+ID4gCX0gd2hpbGUgKGV4Y2Vw
dGlvbi5yZXRyeSk7DQo+ID4gK291dDoNCj4gPiAJcmV0dXJuIGVycjsNCj4gPiB9DQo+ID4gDQo+
ID4gQEAgLTYyMjksMTEgKzYyMzAsMTIgQEAgbmZzNDFfcHJvY19zZWNpbmZvX25vX25hbWUoc3Ry
dWN0IG5mc19zZXJ2ZXIgKnNlcnZlciwgc3RydWN0IG5mc19maCAqZmhhbmRsZSwNCj4gPiAJCWNh
c2UgMDoNCj4gPiAJCWNhc2UgLU5GUzRFUlJfV1JPTkdTRUM6DQo+ID4gCQljYXNlIC1ORlM0RVJS
X05PVFNVUFA6DQo+ID4gLQkJCWJyZWFrOw0KPiA+ICsJCQlnb3RvIG91dDsNCj4gPiAJCWRlZmF1
bHQ6DQo+ID4gCQkJZXJyID0gbmZzNF9oYW5kbGVfZXhjZXB0aW9uKHNlcnZlciwgZXJyLCAmZXhj
ZXB0aW9uKTsNCj4gPiAJCX0NCj4gPiAJfSB3aGlsZSAoZXhjZXB0aW9uLnJldHJ5KTsNCj4gPiAr
b3V0Og0KPiA+IAlyZXR1cm4gZXJyOw0KPiA+IH0NCj4gPiANCj4gPiAtLSANCj4gPiAxLjcuNy42
DQo+ID4gDQo+ID4gLS0NCj4gPiBUbyB1bnN1YnNjcmliZSBmcm9tIHRoaXMgbGlzdDogc2VuZCB0
aGUgbGluZSAidW5zdWJzY3JpYmUgbGludXgtbmZzIiBpbg0KPiA+IHRoZSBib2R5IG9mIGEgbWVz
c2FnZSB0byBtYWpvcmRvbW9Admdlci5rZXJuZWwub3JnDQo+ID4gTW9yZSBtYWpvcmRvbW8gaW5m
byBhdCAgaHR0cDovL3ZnZXIua2VybmVsLm9yZy9tYWpvcmRvbW8taW5mby5odG1sDQo+IA0KDQot
LSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFw
cA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCg==
OK, I need more coffee :)
-dros
On Mar 28, 2012, at 11:08 AM, Myklebust, Trond wrote:
> On Wed, 2012-03-28 at 15:07 +0000, Adamson, Dros wrote:
>> This should probably be sent to stable.
>
> See below. :-)
>
>> -dros
>>
>> On Mar 27, 2012, at 6:35 PM, Trond Myklebust wrote:
>>
>>> We can currently loop forever in nfs4_lookup_root() and in
>>> nfs41_proc_secinfo_no_name(), if the first iteration returns a
>>> NFS4ERR_DELAY or something else that causes exception.retry to get
>>> set.
>>>
>>> Reported-by: Dros Adamson <[email protected]>
>>> Signed-off-by: Trond Myklebust <[email protected]>
>>> Cc: [email protected]
>>> ---
>>> fs/nfs/nfs4proc.c | 6 ++++--
>>> 1 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index 45df7d4..ee9ca19 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -2290,11 +2290,12 @@ static int nfs4_lookup_root(struct nfs_server *server, struct nfs_fh *fhandle,
>>> switch (err) {
>>> case 0:
>>> case -NFS4ERR_WRONGSEC:
>>> - break;
>>> + goto out;
>>> default:
>>> err = nfs4_handle_exception(server, err, &exception);
>>> }
>>> } while (exception.retry);
>>> +out:
>>> return err;
>>> }
>>>
>>> @@ -6229,11 +6230,12 @@ nfs41_proc_secinfo_no_name(struct nfs_server *server, struct nfs_fh *fhandle,
>>> case 0:
>>> case -NFS4ERR_WRONGSEC:
>>> case -NFS4ERR_NOTSUPP:
>>> - break;
>>> + goto out;
>>> default:
>>> err = nfs4_handle_exception(server, err, &exception);
>>> }
>>> } while (exception.retry);
>>> +out:
>>> return err;
>>> }
>>>
>>> --
>>> 1.7.7.6
>>>
>>> --
>>> 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
>>
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com
>
This should probably be sent to stable.
-dros
On Mar 27, 2012, at 6:35 PM, Trond Myklebust wrote:
> We can currently loop forever in nfs4_lookup_root() and in
> nfs41_proc_secinfo_no_name(), if the first iteration returns a
> NFS4ERR_DELAY or something else that causes exception.retry to get
> set.
>
> Reported-by: Dros Adamson <[email protected]>
> Signed-off-by: Trond Myklebust <[email protected]>
> Cc: [email protected]
> ---
> fs/nfs/nfs4proc.c | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 45df7d4..ee9ca19 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2290,11 +2290,12 @@ static int nfs4_lookup_root(struct nfs_server *server, struct nfs_fh *fhandle,
> switch (err) {
> case 0:
> case -NFS4ERR_WRONGSEC:
> - break;
> + goto out;
> default:
> err = nfs4_handle_exception(server, err, &exception);
> }
> } while (exception.retry);
> +out:
> return err;
> }
>
> @@ -6229,11 +6230,12 @@ nfs41_proc_secinfo_no_name(struct nfs_server *server, struct nfs_fh *fhandle,
> case 0:
> case -NFS4ERR_WRONGSEC:
> case -NFS4ERR_NOTSUPP:
> - break;
> + goto out;
> default:
> err = nfs4_handle_exception(server, err, &exception);
> }
> } while (exception.retry);
> +out:
> return err;
> }
>
> --
> 1.7.7.6
>
> --
> 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
T24gVGh1LCAyMDEyLTAzLTI5IGF0IDA4OjU1ICswOTAwLCBOYW1qYWUgSmVvbiB3cm90ZToNCj4g
SGkuIFRyb25kLg0KPiANCj4gTm9ybWFsbHkgZ290byBzdGF0ZW1lbnQgaXMgbm90IHJlY29tbWVu
ZGVkKHBpcGVsaW5lIGFuZCBicmFuY2gNCj4gcHJlZGljdGlvbikgaXQgaXMgb25seSByZWNvbW1l
bmRlZCBhcyBhIGxhc3QgcmVzb3J0IGlmIHRoZXJlIGlzIG5vDQo+IG1ldGhvZCB0byBlc2NhcGUg
dGhpcyBsb29wLg0KDQo/Pz8/Pz8/Pz8/Pz8/IEEgZ290byBpcyBhbiB1bmNvbmRpdGlvbmFsIGJy
YW5jaDsgdGhlcmUgaXMgbm8gZnVydGhlcg0KYnJhbmNoIHByZWRpY3Rpb24gaW52b2x2ZWQgb25j
ZSBvbmNlIHRoZSBwcm9jZXNzb3IgZ2V0cyB0aGUgdmFsdWUgb2YNCidlcnInIGluIHRoYXQgc3dp
dGNoIHN0YXRlbWVudCBjb3JyZWN0LiBFeGFjdGx5IGhvdyB3b3VsZCBhZGRpbmcgYW4NCmV4dHJh
IHNldCt0ZXN0IG9mIGV4Y2VwdGlvbi5yZXRyeSBhZnRlciB0aGUgc3dpdGNoKCkgaGVscCB0aGUg
YnJhbmNoDQpwcmVkaWN0b3IgaW4gdGhpcyBzaXR1YXRpb24/DQoNCi0tIA0KVHJvbmQgTXlrbGVi
dXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1
c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20NCg0K
Hi. Trond.
Normally goto statement is not recommended(pipeline and branch
prediction) it is only recommended as a last resort if there is no
method to escape this loop.
It is just my opinion. And when I think more,, looks not bad about
forward goto statement.
Thanks~
2012/3/29 Adamson, Dros <[email protected]>:
> OK, I need more coffee :)
>
> -dros
>
> On Mar 28, 2012, at 11:08 AM, Myklebust, Trond wrote:
>
>> On Wed, 2012-03-28 at 15:07 +0000, Adamson, Dros wrote:
>>> This should probably be sent to stable.
>>
>> See below. :-)
>>
>>> -dros
>>>
>>> On Mar 27, 2012, at 6:35 PM, Trond Myklebust wrote:
>>>
>>>> We can currently loop forever in nfs4_lookup_root() and in
>>>> nfs41_proc_secinfo_no_name(), if the first iteration returns a
>>>> NFS4ERR_DELAY or something else that causes exception.retry to get
>>>> set.
>>>>
>>>> Reported-by: Dros Adamson <[email protected]>
>>>> Signed-off-by: Trond Myklebust <[email protected]>
>>>> Cc: [email protected]
>>>> ---
>>>> fs/nfs/nfs4proc.c | 6 ++++--
>>>> 1 files changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>> index 45df7d4..ee9ca19 100644
>>>> --- a/fs/nfs/nfs4proc.c
>>>> +++ b/fs/nfs/nfs4proc.c
>>>> @@ -2290,11 +2290,12 @@ static int nfs4_lookup_root(struct nfs_server *server, struct nfs_fh *fhandle,
>>>> switch (err) {
>>>> case 0:
>>>> case -NFS4ERR_WRONGSEC:
>>>> - break;
>>>> + goto out;
>>>> default:
>>>> err = nfs4_handle_exception(server, err, &exception);
>>>> }
>>>> } while (exception.retry);
>>>> +out:
>>>> return err;
>>>> }
>>>>
>>>> @@ -6229,11 +6230,12 @@ nfs41_proc_secinfo_no_name(struct nfs_server *server, struct nfs_fh *fhandle,
>>>> case 0:
>>>> case -NFS4ERR_WRONGSEC:
>>>> case -NFS4ERR_NOTSUPP:
>>>> - break;
>>>> + goto out;
>>>> default:
>>>> err = nfs4_handle_exception(server, err, &exception);
>>>> }
>>>> } while (exception.retry);
>>>> +out:
>>>> return err;
>>>> }
>>>>
>>>> --
>>>> 1.7.7.6
>>>>
>>>> --
>>>> 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
>>>
>>
>> --
>> Trond Myklebust
>> Linux NFS client maintainer
>>
>> NetApp
>> [email protected]
>> http://www.netapp.com
>>
>
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs4proc.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 1933e67..f82bde0 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -270,7 +270,7 @@ static int nfs4_handle_exception(struct nfs_server *server, int errorcode, struc
case 0:
return 0;
case -NFS4ERR_OPENMODE:
- if (nfs_have_delegation(inode, FMODE_READ)) {
+ if (inode && nfs_have_delegation(inode, FMODE_READ)) {
nfs_inode_return_delegation(inode);
exception->retry = 1;
return 0;
@@ -282,10 +282,9 @@ static int nfs4_handle_exception(struct nfs_server *server, int errorcode, struc
case -NFS4ERR_DELEG_REVOKED:
case -NFS4ERR_ADMIN_REVOKED:
case -NFS4ERR_BAD_STATEID:
- if (state != NULL)
- nfs_remove_bad_delegation(state->inode);
if (state == NULL)
break;
+ nfs_remove_bad_delegation(state->inode);
nfs4_schedule_stateid_recovery(server, state);
goto wait_on_recovery;
case -NFS4ERR_EXPIRED:
@@ -3825,8 +3824,9 @@ nfs4_async_handle_error(struct rpc_task *task, const struct nfs_server *server,
case -NFS4ERR_DELEG_REVOKED:
case -NFS4ERR_ADMIN_REVOKED:
case -NFS4ERR_BAD_STATEID:
- if (state != NULL)
- nfs_remove_bad_delegation(state->inode);
+ if (state == NULL)
+ break;
+ nfs_remove_bad_delegation(state->inode);
case -NFS4ERR_OPENMODE:
if (state == NULL)
break;
--
1.7.7.6
T24gV2VkLCAyMDEyLTAzLTI4IGF0IDIxOjQ5ICswOTAwLCBOYW1qYWUgSmVvbiB3cm90ZToNCj4g
SGkuIFRyb25kLg0KPiANCj4gSXMgaXQgYmV0dGVyIGlmIGV4Y2VwdGlvbi5yZXRyeSBzZXQgdG8g
emVybyBpbnN0ZWFkIGdvdG8gPw0KDQpXaGF0IG1ha2VzIHlvdSB0aGluayB0aGF0IHdvdWxkIGJl
IGJldHRlcj8gVGhhdCBhZGRzIGFuIHVubmVjZXNzYXJ5DQpleHRyYSBzZXQrdGVzdCBvZiBleGNl
cHRpb24ucmV0cnkgZXhjZXB0IGlmIGdjYyBtYW5hZ2VzIHRvIG9wdGltaXNlIHRoYXQNCmF3YXkg
dG8gdGhlIHNhbWUgZ290byB3ZSd2ZSBub3cgZG9jdW1lbnRlZCBpbiB0aGUgY29kZS4NCg0KPiBU
aGFua3MuDQo+IA0KPiAyMDEyLzMvMjggVHJvbmQgTXlrbGVidXN0IDxUcm9uZC5NeWtsZWJ1c3RA
bmV0YXBwLmNvbT46DQo+ID4gV2UgY2FuIGN1cnJlbnRseSBsb29wIGZvcmV2ZXIgaW4gbmZzNF9s
b29rdXBfcm9vdCgpIGFuZCBpbg0KPiA+IG5mczQxX3Byb2Nfc2VjaW5mb19ub19uYW1lKCksIGlm
IHRoZSBmaXJzdCBpdGVyYXRpb24gcmV0dXJucyBhDQo+ID4gTkZTNEVSUl9ERUxBWSBvciBzb21l
dGhpbmcgZWxzZSB0aGF0IGNhdXNlcyBleGNlcHRpb24ucmV0cnkgdG8gZ2V0DQo+ID4gc2V0Lg0K
PiA+DQo+ID4gUmVwb3J0ZWQtYnk6IERyb3MgQWRhbXNvbiA8V2VzdG9uLkFkYW1zb25AbmV0YXBw
LmNvbT4NCj4gPiBTaWduZWQtb2ZmLWJ5OiBUcm9uZCBNeWtsZWJ1c3QgPFRyb25kLk15a2xlYnVz
dEBuZXRhcHAuY29tPg0KPiA+IENjOiBzdGFibGVAdmdlci5rZXJuZWwub3JnDQo+ID4gLS0tDQo+
ID4gIGZzL25mcy9uZnM0cHJvYy5jIHwgICAgNiArKysrLS0NCj4gPiAgMSBmaWxlcyBjaGFuZ2Vk
LCA0IGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pDQo+ID4NCj4gPiBkaWZmIC0tZ2l0IGEv
ZnMvbmZzL25mczRwcm9jLmMgYi9mcy9uZnMvbmZzNHByb2MuYw0KPiA+IGluZGV4IDQ1ZGY3ZDQu
LmVlOWNhMTkgMTAwNjQ0DQo+ID4gLS0tIGEvZnMvbmZzL25mczRwcm9jLmMNCj4gPiArKysgYi9m
cy9uZnMvbmZzNHByb2MuYw0KPiA+IEBAIC0yMjkwLDExICsyMjkwLDEyIEBAIHN0YXRpYyBpbnQg
bmZzNF9sb29rdXBfcm9vdChzdHJ1Y3QgbmZzX3NlcnZlciAqc2VydmVyLCBzdHJ1Y3QgbmZzX2Zo
ICpmaGFuZGxlLA0KPiA+ICAgICAgICAgICAgICAgIHN3aXRjaCAoZXJyKSB7DQo+ID4gICAgICAg
ICAgICAgICAgY2FzZSAwOg0KPiA+ICAgICAgICAgICAgICAgIGNhc2UgLU5GUzRFUlJfV1JPTkdT
RUM6DQo+ID4gLSAgICAgICAgICAgICAgICAgICAgICAgYnJlYWs7DQo+ID4gKyAgICAgICAgICAg
ICAgICAgICAgICAgZ290byBvdXQ7DQo+ID4gICAgICAgICAgICAgICAgZGVmYXVsdDoNCj4gPiAg
ICAgICAgICAgICAgICAgICAgICAgIGVyciA9IG5mczRfaGFuZGxlX2V4Y2VwdGlvbihzZXJ2ZXIs
IGVyciwgJmV4Y2VwdGlvbik7DQo+ID4gICAgICAgICAgICAgICAgfQ0KPiA+ICAgICAgICB9IHdo
aWxlIChleGNlcHRpb24ucmV0cnkpOw0KPiA+ICtvdXQ6DQo+ID4gICAgICAgIHJldHVybiBlcnI7
DQo+ID4gIH0NCj4gPg0KPiA+IEBAIC02MjI5LDExICs2MjMwLDEyIEBAIG5mczQxX3Byb2Nfc2Vj
aW5mb19ub19uYW1lKHN0cnVjdCBuZnNfc2VydmVyICpzZXJ2ZXIsIHN0cnVjdCBuZnNfZmggKmZo
YW5kbGUsDQo+ID4gICAgICAgICAgICAgICAgY2FzZSAwOg0KPiA+ICAgICAgICAgICAgICAgIGNh
c2UgLU5GUzRFUlJfV1JPTkdTRUM6DQo+ID4gICAgICAgICAgICAgICAgY2FzZSAtTkZTNEVSUl9O
T1RTVVBQOg0KPiA+IC0gICAgICAgICAgICAgICAgICAgICAgIGJyZWFrOw0KPiA+ICsgICAgICAg
ICAgICAgICAgICAgICAgIGdvdG8gb3V0Ow0KPiA+ICAgICAgICAgICAgICAgIGRlZmF1bHQ6DQo+
ID4gICAgICAgICAgICAgICAgICAgICAgICBlcnIgPSBuZnM0X2hhbmRsZV9leGNlcHRpb24oc2Vy
dmVyLCBlcnIsICZleGNlcHRpb24pOw0KPiA+ICAgICAgICAgICAgICAgIH0NCj4gPiAgICAgICAg
fSB3aGlsZSAoZXhjZXB0aW9uLnJldHJ5KTsNCj4gPiArb3V0Og0KPiA+ICAgICAgICByZXR1cm4g
ZXJyOw0KPiA+ICB9DQo+ID4NCj4gPiAtLQ0KPiA+IDEuNy43LjYNCj4gPg0KPiA+IC0tDQo+ID4g
VG8gdW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2NyaWJl
IGxpbnV4LW5mcyIgaW4NCj4gPiB0aGUgYm9keSBvZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZn
ZXIua2VybmVsLm9yZw0KPiA+IE1vcmUgbWFqb3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtl
cm5lbC5vcmcvbWFqb3Jkb21vLWluZm8uaHRtbA0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGlu
dXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFw
cC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCg==
Hi. Trond.
First sorry for noise.
if goto is encountered by branch predictor with switch condition, CPU
try to fetch instruction code after label to cpu cache.
And Cpu is idiling and CPU clock is watsted. if branch prediction is
fail, this cost is more big.
but this patch is good as I said before.
Thanks.
2012/3/29 Myklebust, Trond <[email protected]>:
> On Thu, 2012-03-29 at 08:55 +0900, Namjae Jeon wrote:
>> Hi. Trond.
>>
>> Normally goto statement is not recommended(pipeline and branch
>> prediction) it is only recommended as a last resort if there is no
>> method to escape this loop.
>
> ????????????? A goto is an unconditional branch; there is no further
> branch prediction involved once once the processor gets the value of
> 'err' in that switch statement correct. Exactly how would adding an
> extra set+test of exception.retry after the switch() help the branch
> predictor in this situation?
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com
>
2012/3/28 Trond Myklebust <[email protected]>:
> 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 <[email protected]>
Reviewed-by: Namjae Jeon <[email protected]>
This patch makes good sense.
Thanks~
> Cc: [email protected]
> ---
Myklebust, Trond wrote:
On Thu, 2012-03-29 at 08:55 +0900, Namjae Jeon wrote:
> Hi. Trond.
>
> Normally goto statement is not recommended(pipeline and branch
> prediction) it is only recommended as a last resort if there is no
> method to escape this loop.
????????????? A goto is an unconditional branch; there is no further
branch prediction involved once once the processor gets the value of
'err' in that switch statement correct. Exactly how would adding an
extra set+test of exception.retry after the switch() help the branch
predictor in this situation?
Besides which I see little point in optimizing this error path.
Hi. Trond.
Is it better if exception.retry set to zero instead goto ?
Thanks.
2012/3/28 Trond Myklebust <[email protected]>:
> We can currently loop forever in nfs4_lookup_root() and in
> nfs41_proc_secinfo_no_name(), if the first iteration returns a
> NFS4ERR_DELAY or something else that causes exception.retry to get
> set.
>
> Reported-by: Dros Adamson <[email protected]>
> Signed-off-by: Trond Myklebust <[email protected]>
> Cc: [email protected]
> ---
> fs/nfs/nfs4proc.c | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 45df7d4..ee9ca19 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2290,11 +2290,12 @@ static int nfs4_lookup_root(struct nfs_server *server, struct nfs_fh *fhandle,
> switch (err) {
> case 0:
> case -NFS4ERR_WRONGSEC:
> - break;
> + goto out;
> default:
> err = nfs4_handle_exception(server, err, &exception);
> }
> } while (exception.retry);
> +out:
> return err;
> }
>
> @@ -6229,11 +6230,12 @@ nfs41_proc_secinfo_no_name(struct nfs_server *server, struct nfs_fh *fhandle,
> case 0:
> case -NFS4ERR_WRONGSEC:
> case -NFS4ERR_NOTSUPP:
> - break;
> + goto out;
> default:
> err = nfs4_handle_exception(server, err, &exception);
> }
> } while (exception.retry);
> +out:
> return err;
> }
>
> --
> 1.7.7.6
>
> --
> 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
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 <[email protected]>
Cc: [email protected]
---
fs/nfs/nfs4proc.c | 25 +++++++++++++------------
1 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ee9ca19..1933e67 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6112,21 +6112,22 @@ nfs4_layoutcommit_done(struct rpc_task *task, void *calldata)
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;
+ 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)
--
1.7.7.6
On Tue, 27 Mar 2012 18:35:44 -0400
Trond Myklebust <[email protected]> wrote:
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> fs/nfs/nfs4proc.c | 10 +++++-----
> 1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 1933e67..f82bde0 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -270,7 +270,7 @@ static int nfs4_handle_exception(struct nfs_server *server, int errorcode, struc
> case 0:
> return 0;
> case -NFS4ERR_OPENMODE:
> - if (nfs_have_delegation(inode, FMODE_READ)) {
> + if (inode && nfs_have_delegation(inode, FMODE_READ)) {
> nfs_inode_return_delegation(inode);
> exception->retry = 1;
> return 0;
> @@ -282,10 +282,9 @@ static int nfs4_handle_exception(struct nfs_server *server, int errorcode, struc
> case -NFS4ERR_DELEG_REVOKED:
> case -NFS4ERR_ADMIN_REVOKED:
> case -NFS4ERR_BAD_STATEID:
> - if (state != NULL)
> - nfs_remove_bad_delegation(state->inode);
> if (state == NULL)
> break;
> + nfs_remove_bad_delegation(state->inode);
> nfs4_schedule_stateid_recovery(server, state);
> goto wait_on_recovery;
> case -NFS4ERR_EXPIRED:
> @@ -3825,8 +3824,9 @@ nfs4_async_handle_error(struct rpc_task *task, const struct nfs_server *server,
> case -NFS4ERR_DELEG_REVOKED:
> case -NFS4ERR_ADMIN_REVOKED:
> case -NFS4ERR_BAD_STATEID:
> - if (state != NULL)
> - nfs_remove_bad_delegation(state->inode);
> + if (state == NULL)
> + break;
> + nfs_remove_bad_delegation(state->inode);
> case -NFS4ERR_OPENMODE:
> if (state == NULL)
> break;
Hi Trond,
We got several reports of oopses in Fedora that look like they would be
fixed by this patch. Here's the bug that's tracking them:
https://bugzilla.redhat.com/show_bug.cgi?id=811138
The problem seems to be regression introduced by 3114ea7a. An inode
field was added to the nfs4_exception struct, but most of the callers
of nfs4_handle_exception set it to NULL.
Would it be reasonable to push this to stable too?
--
Jeff Layton <[email protected]>