2013-09-11 13:13:55

by Myklebust, Trond

[permalink] [raw]
Subject: Re: [PATCH 3/4] NFSv4.1: sp4_mach_cred: no need to ref count creds

T24gV2VkLCAyMDEzLTA5LTExIGF0IDA4OjU5IC0wNDAwLCBBbm5hIFNjaHVtYWtlciB3cm90ZToN
Cj4gRG8gd2UgbmVlZCB0byBkbyBhIGdldF9ycGNjcmVkKCkgYWZ0ZXIgbG9va2luZyB1cCB0aGUg
bWFjaGluZSBjcmVkIGluDQo+IG5mc19hbGxvY19jbGllbnQoKT8gIFdoZXJlIGRvZXMgb3VyIGd1
YXJhbnRlZWQgcmVmZXJlbmNlIGNvbWUgZnJvbT8NCg0KU2VlIG15IGNvbW1lbnQgdG8gRHJvcycg
Zmlyc3QgcGF0Y2hzZXQgeWVzdGVyZGF5LiBJbiB0aGUgY2FzZSBvZg0KU1A0X01BQ0hfQ1JFRCwg
d2UgY2Fubm90IGNoYW5nZSB0aGUgbWFjaGluZSBjcmVkZW50aWFsLCBzaW5jZSB0aGUgc3RhdGUN
CnByb3RlY3Rpb24gaXMgY29tcGxldGVseSB0aWVkIHRvIHRoYXQgY3JlZGVudGlhbC4gSW4gZmFj
dCBpZiB5b3UgbG9vayBhdA0KdGhlIGN1cnJlbnQgY29kZSwgd2Ugb25seSBhY3R1YWxseSBmcmVl
IHRoZSBjbF9tYWNoaW5lX2NyZWQgaW4NCm5mc19mcmVlX2NsaWVudCgpLg0KDQpGb3IgdGhhdCBy
ZWFzb24sIGl0IGlzIHNhZmUgdG8gYXNzdW1lIHRoYXQgaXQgaXMgYXZhaWxhYmxlIGZvciB0aGUN
CmR1cmF0aW9uIG9mIHRoZSBSUEMgY2FsbC4gT2YgY291cnNlLCBycGNfdGFza19zZXRfcnBjX21l
c3NhZ2UoKSB3aWxsDQphbHNvIHRha2UgYSByZWZlcmVuY2UsIGJ1dCB0aGF0IHRvbyBpcyByZWR1
bmRhbnQgaW4gdGhpcyBwYXJ0aWN1bGFyDQpjYXNlLg0KDQpDaGVlcnMNCiAgVHJvbmQNCg0KPiBB
bm5hDQo+IA0KPiANCj4gDQo+IE9uIFR1ZSwgU2VwIDEwLCAyMDEzIGF0IDY6NDQgUE0sIFdlc3Rv
biBBbmRyb3MgQWRhbXNvbg0KPiA8ZHJvc0BuZXRhcHAuY29tPiB3cm90ZToNCj4gICAgICAgICBU
aGUgY2xfbWFjaGluZV9jcmVkIGRvZXNuJ3QgbmVlZCB0byBiZSByZWZlcmVuY2UgY291bnRlZCBo
ZXJlDQo+ICAgICAgICAgLQ0KPiAgICAgICAgIGEgcmVmZXJlbmNlIGlzIGhlbGQgaXMgZm9yIHRo
ZSBsaWZldGltZSBvZiB0aGUgc3RydWN0DQo+ICAgICAgICAgbmZzX2NsaWVudC4NCj4gICAgICAg
ICBBbHNvLCBubyBuZWVkIHRvIHB1dF9ycGNjcmVkIHRoZSBycGNfbWVzc2FnZS5ycGNfY3JlZC4N
Cj4gICAgICAgICANCj4gICAgICAgICBTaWduZWQtb2ZmLWJ5OiBXZXN0b24gQW5kcm9zIEFkYW1z
b24gPGRyb3NAbmV0YXBwLmNvbT4NCj4gICAgICAgICAtLS0NCj4gICAgICAgICAgZnMvbmZzL25m
czRfZnMuaCB8IDYgKysrLS0tDQo+ICAgICAgICAgIDEgZmlsZSBjaGFuZ2VkLCAzIGluc2VydGlv
bnMoKyksIDMgZGVsZXRpb25zKC0pDQo+ICAgICAgICAgDQo+ICAgICAgICAgZGlmZiAtLWdpdCBh
L2ZzL25mcy9uZnM0X2ZzLmggYi9mcy9uZnMvbmZzNF9mcy5oDQo+ICAgICAgICAgaW5kZXggZjUy
MGExMS4uMDdhOGFhOSAxMDA2NDQNCj4gICAgICAgICAtLS0gYS9mcy9uZnMvbmZzNF9mcy5oDQo+
ICAgICAgICAgKysrIGIvZnMvbmZzL25mczRfZnMuaA0KPiAgICAgICAgIEBAIC0yNzksMTAgKzI3
OSwxMCBAQCBfbmZzNF9zdGF0ZV9wcm90ZWN0KHN0cnVjdCBuZnNfY2xpZW50DQo+ICAgICAgICAg
KmNscCwgdW5zaWduZWQgbG9uZyBzcDRfbW9kZSwNCj4gICAgICAgICAgICAgICAgIGlmICh0ZXN0
X2JpdChzcDRfbW9kZSwgJmNscC0+Y2xfc3A0X2ZsYWdzKSkgew0KPiAgICAgICAgICAgICAgICAg
ICAgICAgICBzcGluX2xvY2soJmNscC0+Y2xfbG9jayk7DQo+ICAgICAgICAgICAgICAgICAgICAg
ICAgIGlmIChjbHAtPmNsX21hY2hpbmVfY3JlZCAhPSBOVUxMKQ0KPiAgICAgICAgIC0gICAgICAg
ICAgICAgICAgICAgICAgIG5ld2NyZWQgPQ0KPiAgICAgICAgIGdldF9ycGNjcmVkKGNscC0+Y2xf
bWFjaGluZV9jcmVkKTsNCj4gICAgICAgICArICAgICAgICAgICAgICAgICAgICAgICAvKiBkb24n
dCBjYWxsIGdldF9ycGNjcmVkIG9uIHRoZQ0KPiAgICAgICAgIG1hY2hpbmUgY3JlZCAtDQo+ICAg
ICAgICAgKyAgICAgICAgICAgICAgICAgICAgICAgICogYSByZWZlcmVuY2Ugd2lsbCBiZSBoZWxk
IGZvciBsaWZlDQo+ICAgICAgICAgb2YgY2xwICovDQo+ICAgICAgICAgKyAgICAgICAgICAgICAg
ICAgICAgICAgbmV3Y3JlZCA9IGNscC0+Y2xfbWFjaGluZV9jcmVkOw0KPiAgICAgICAgICAgICAg
ICAgICAgICAgICBzcGluX3VubG9jaygmY2xwLT5jbF9sb2NrKTsNCj4gICAgICAgICAtICAgICAg
ICAgICAgICAgaWYgKG1zZy0+cnBjX2NyZWQpDQo+ICAgICAgICAgLSAgICAgICAgICAgICAgICAg
ICAgICAgcHV0X3JwY2NyZWQobXNnLT5ycGNfY3JlZCk7DQo+ICAgICAgICAgICAgICAgICAgICAg
ICAgIG1zZy0+cnBjX2NyZWQgPSBuZXdjcmVkOw0KPiAgICAgICAgIA0KPiAgICAgICAgICAgICAg
ICAgICAgICAgICBmbGF2b3IgPQ0KPiAgICAgICAgIGNscC0+Y2xfcnBjY2xpZW50LT5jbF9hdXRo
LT5hdV9mbGF2b3I7DQo+ICAgICAgICAgLS0NCj4gICAgICAgICAxLjcuMTIuNCAoQXBwbGUgR2l0
LTM3KQ0KPiAgICAgICAgIA0KPiAgICAgICAgIC0tDQo+ICAgICAgICAgVG8gdW5zdWJzY3JpYmUg
ZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2NyaWJlDQo+ICAgICAgICAgbGlu
dXgtbmZzIiBpbg0KPiAgICAgICAgIHRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9A
dmdlci5rZXJuZWwub3JnDQo+ICAgICAgICAgTW9yZSBtYWpvcmRvbW8gaW5mbyBhdA0KPiAgICAg
ICAgICBodHRwOi8vdmdlci5rZXJuZWwub3JnL21ham9yZG9tby1pbmZvLmh0bWwNCj4gDQo+IA0K
DQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5l
dEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQo=


2013-09-11 13:27:29

by Anna Schumaker

[permalink] [raw]
Subject: Re: [PATCH 3/4] NFSv4.1: sp4_mach_cred: no need to ref count creds

Okay, thanks! I think I missed that explanation yesterday.

On Wed, Sep 11, 2013 at 9:13 AM, Myklebust, Trond
<[email protected]> wrote:
> On Wed, 2013-09-11 at 08:59 -0400, Anna Schumaker wrote:
>> Do we need to do a get_rpccred() after looking up the machine cred in
>> nfs_alloc_client()? Where does our guaranteed reference come from?
>
> See my comment to Dros' first patchset yesterday. In the case of
> SP4_MACH_CRED, we cannot change the machine credential, since the state
> protection is completely tied to that credential. In fact if you look at
> the current code, we only actually free the cl_machine_cred in
> nfs_free_client().
>
> For that reason, it is safe to assume that it is available for the
> duration of the RPC call. Of course, rpc_task_set_rpc_message() will
> also take a reference, but that too is redundant in this particular
> case.
>
> Cheers
> Trond
>
>> Anna
>>
>>
>>
>> On Tue, Sep 10, 2013 at 6:44 PM, Weston Andros Adamson
>> <[email protected]> wrote:
>> The cl_machine_cred doesn't need to be reference counted here
>> -
>> a reference is held is for the lifetime of the struct
>> nfs_client.
>> Also, no need to put_rpccred the rpc_message.rpc_cred.
>>
>> Signed-off-by: Weston Andros Adamson <[email protected]>
>> ---
>> fs/nfs/nfs4_fs.h | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
>> index f520a11..07a8aa9 100644
>> --- a/fs/nfs/nfs4_fs.h
>> +++ b/fs/nfs/nfs4_fs.h
>> @@ -279,10 +279,10 @@ _nfs4_state_protect(struct nfs_client
>> *clp, unsigned long sp4_mode,
>> if (test_bit(sp4_mode, &clp->cl_sp4_flags)) {
>> spin_lock(&clp->cl_lock);
>> if (clp->cl_machine_cred != NULL)
>> - newcred =
>> get_rpccred(clp->cl_machine_cred);
>> + /* don't call get_rpccred on the
>> machine cred -
>> + * a reference will be held for life
>> of clp */
>> + newcred = clp->cl_machine_cred;
>> spin_unlock(&clp->cl_lock);
>> - if (msg->rpc_cred)
>> - put_rpccred(msg->rpc_cred);
>> msg->rpc_cred = newcred;
>>
>> flavor =
>> clp->cl_rpcclient->cl_auth->au_flavor;
>> --
>> 1.7.12.4 (Apple Git-37)
>>
>> --
>> 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