Uninitialized seq_nr causes sending first SESSION request
with sa_sequenceid = 0.
Signed-off-by: Vitaliy Gusev <[email protected]>
---
fs/nfs/nfs4proc.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index f0c849c..711a812 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5067,6 +5067,7 @@ static int nfs4_init_slot_table(struct nfs4_slot_table *tbl,
int max_slots, int ivalue)
{
struct nfs4_slot *slot;
+ int i;
int ret = -ENOMEM;
BUG_ON(max_slots > NFS4_MAX_SLOT_TABLE);
@@ -5082,6 +5083,8 @@ static int nfs4_init_slot_table(struct nfs4_slot_table *tbl,
tbl->max_slots = max_slots;
tbl->slots = slot;
tbl->highest_used_slotid = -1; /* no slot is currently used */
+ for (i = 0; i < tbl->max_slots; ++i)
+ tbl->slots[i].seq_nr = ivalue;
spin_unlock(&tbl->slot_tbl_lock);
dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
tbl, tbl->slots, tbl->max_slots);
--
1.7.5.4
On Wed, Feb 15, 2012 at 10:29 AM, Myklebust, Trond
<[email protected]> wrote:
> On Wed, 2012-02-15 at 15:01 +0000, Adamson, Andy wrote:
>> On Feb 14, 2012, at 8:50 PM, Myklebust, Trond wrote:
>>
>> > On Tue, 2012-02-14 at 19:53 -0500, J. Bruce Fields wrote:
>> >> On Wed, Feb 15, 2012 at 12:43:08AM +0000, Myklebust, Trond wrote:
>> >>> Then we have a problem. The zero initialisation is already in use out
>> >>> there in both commercial and non-commercial versions of Linux. It is too
>> >>> late to change that now.
>> >>>
>> >>> Furthermore, since none of the servers we've tested against in earlier
>> >>> Bakeathons and Connectathons have complained, I suggest that we rather
>> >>> change the spec with an errata.
>> >>
>> >> Argh.
>> >>
>> >> I just noticed that the server was crashing intermittently and traced it
>> >> to incorrect handling of the case where the client sends a one-op
>> >> SEQUENCE compound with seqid 0. ?I'm not sure why it started popping up
>> >> just in 3.3--perhaps some change in the way the client uses slots made
>> >> that more likely.
>> >>
>> >> The server code actually looks like it did assume initial seqid 1, but
>> >> accepted initial seqid 0 (except in this one case) basically by mistake.
>> >>
>> >> In any case, I think it should be easy enough to teach it just to accept
>> >> any seqid on a previously unused slot, so I'll do that....
>> >
>> > Hang on... Looking at 3.2, I think you're correct. We used to initialise
>> > to 1, and now we're initialising to 0.
>>
>> No, commit aacd5537270a752fe12a9914a207284fc2341c6d initializes and resets the slot tables with the ivalue which is 1 for the forechannel and 0 for the back channel, just as in 3.2.
>> To be clear from aacd5537:
>>
>> -static int nfs4_init_slot_tables(struct nfs4_session *session)
>> +static int nfs4_setup_session_slot_tables(struct nfs4_session *ses)
>> ?{
>> ? ? ? ? struct nfs4_slot_table *tbl;
>> - ? ? ? int status = 0;
>> + ? ? ? int status;
>>
>> - ? ? ? tbl = &session->fc_slot_table;
>> + ? ? ? dprintk("--> %s\n", __func__);
>> + ? ? ? /* Fore channel */
>> + ? ? ? tbl = &ses->fc_slot_table;
>> ? ? ? ? if (tbl->slots == NULL) {
>> - ? ? ? ? ? ? ? status = nfs4_init_slot_table(tbl,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? session->fc_attrs.max_reqs, 1);
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?^^^^^^^
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?ivalue of 1
>>
>> + ? ? ? ? ? ? ? status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ^^^^^
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ivalue of 1
>> + ? ? ? ? ? ? ? if (status) /* -ENOMEM */
>> + ? ? ? ? ? ? ? ? ? ? ? return status;
>> + ? ? ? } else {
>> + ? ? ? ? ? ? ? status = nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ^^^^^
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ivalue of 1
>> ? ? ? ? ? ? ? ? if (status)
>> ? ? ? ? ? ? ? ? ? ? ? ? return status;
>> ? ? ? ? }
>> -
>> - ? ? ? tbl = &session->bc_slot_table;
>> + ? ? ? /* Back channel */
>> + ? ? ? tbl = &ses->bc_slot_table;
>> ? ? ? ? if (tbl->slots == NULL) {
>> - ? ? ? ? ? ? ? status = nfs4_init_slot_table(tbl,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? session->bc_attrs.max_reqs, 0);
>> + ? ? ? ? ? ? ? status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
>> ? ? ? ? ? ? ? ? if (status)
>> - ? ? ? ? ? ? ? ? ? ? ? nfs4_destroy_slot_tables(session);
>> - ? ? ? }
>> -
>> + ? ? ? ? ? ? ? ? ? ? ? /* Fore and back channel share a connection so get
>> + ? ? ? ? ? ? ? ? ? ? ? ?* both slot tables or neither */
>> + ? ? ? ? ? ? ? ? ? ? ? nfs4_destroy_slot_tables(ses);
>> + ? ? ? } else
>> + ? ? ? ? ? ? ? status = nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
>> ? ? ? ? return status;
>> ?}
>
> Yes, and it then _removed_ the previously unconditional call to
> nfs4_reset_slot_tables() from nfs4_proc_create_session(). Which means
> that the tbl->slots == NULL path no longer initialises the slot table
> sequence numbers.
Ah - you are correct. nfs4_setup_session_slot_tables is called and
incorrectly assumed that nfs4_init_slot_tables actually initialized
the slots, which it doesn't and is a bug.
For my 2 cents: I like Vitilay's inthinkitial patch.
-->Andy
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com
>
T24gV2VkLCAyMDEyLTAyLTE1IGF0IDExOjA2IC0wNTAwLCBBbmR5IEFkYW1zb24gd3JvdGU6DQo+
IE9uIFdlZCwgRmViIDE1LCAyMDEyIGF0IDEwOjQ2IEFNLCBNeWtsZWJ1c3QsIFRyb25kDQo+IDxU
cm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbT4gd3JvdGU6DQo+ID4gT24gV2VkLCAyMDEyLTAyLTE1
IGF0IDE1OjA3ICswMDAwLCBBZGFtc29uLCBBbmR5IHdyb3RlOg0KPiA+Pg0KPiA+PiBQbGVhc2Ug
ZG9uJ3QgbWVyZ2UgbmZzNF9yZXNldF9zbG90X3RhYmxlcyBhbmQgbmZzNF9pbml0X3Nsb3RfdGFi
bGVzLiAgSXQgIGFzc3VtZXMgYSBzdGF0aWMgYXJyYXkgKHJlYWxsb2MpIHdoaWNoIGdvZXMgYXdh
eSB3aXRoIHRoZSBkeW5hbWljIHNsb3QgdGFibGUgY29kZS4gSW5zdGVhZCwgdGFrZSBhIGxvb2sg
YXQgdGhlIHBhdGNoIHNldCBJIHNlbnQgb24gRmViIDEyDQo+ID4NCj4gPiBXaHkgbm90PyBUaGVy
ZSBpcyBfbm9fIGZ1bmN0aW9uYWwgZGlmZmVyZW5jZSBiZXR3ZWVuIHdoYXQNCj4gPiBuZnM0X3Jl
c2V0X3Nsb3RfdGFibGVzKCkgYW5kIG5mczRfaW5pdF9zbG90X3RhYmxlcygpIG5lZWQgdG8gZG8u
DQo+ID4gVGhleSBib3RoIG5lZWQgdG8gYWxsb2NhdGUgbmV3IHNsb3RzIChjb25kaXRpb25hbGx5
IGluIHRoZSBjYXNlIG9mDQo+ID4gbmZzNF9yZXNldF9zbG90X3RhYmxlcywgYnV0IHRoZSBjb25k
aXRpb24gaXMgY29tcGF0aWJsZSB3aXRoIHRoZQ0KPiA+IG5mczRfaW5pdF9zbG90X3RhYmxlcyBj
YXNlKSwgYW5kIHRoZXkgYm90aCBuZWVkIHRvIGluaXRpYWxpc2UgdGhvc2UNCj4gPiBzbG90cyB0
byB0aGUgdmFsdWUgJzEnLg0KPiANCj4gVGhlIHJlc2V0IGNhc2UgYWxzbyBuZWVkcyB0byByZWR1
Y2UgdGhlIG51bWJlciBvZiBzbG90cy4gIEluIHRoZQ0KPiBkeW5hbWljIHNsb3QgY2FzZSwgdGhl
IHJlc2V0IGZ1bmN0aW9uIHdpbGwgbm90IHJlcXVpcmUgZHJhaW5pbmcgb2YgdGhlDQo+IHNlc3Np
b24uIFNvIHJlc2V0IHdpbGwsIGluIHJlc3BvbnNlIHRvIGEgY2hhbmdlIGluICB0aGUNCj4gdGFy
Z2V0X2hpZ2hlc3Rfc2xvdGlkIG9yIGEgQ0JfUkVDQUxMX1NMT1QgZnJvbSB0aGUgc2VydmVyLCBh
ZGQgb3INCj4gcmVtb3ZlIHNsb3RzIHdpdGhvdXQgZHJhaW5pbmcgdGhlIHNlc3Npb24uIEluIHRo
aXMgY2FzZSwgcmVzZXQgd2lsbA0KPiBvbmx5IGluaXRpYWxpemUgYW55IG5ldyBzbG90cyBzZXF1
ZW5jZSBudW1iZXJzIHRvIDEsIG5vdCBhbGwgc2xvdHMuDQo+IA0KPiBUaGF0IHNhaWQsIEkgZ3Vl
c3MgaXQgY2FuIGFsbCBiZSBpbiBvbmUgZnVuY3Rpb24uDQo+IA0KPiA+DQo+ID4gQUZBSUNTIFRo
ZXJlIGlzIG5vIHJlYXNvbiBmb3Iga2VlcGluZyB0aG9zZSBhcyAyIHNlcGFyYXRlIGZ1bmN0aW9u
cywgYW5kDQo+ID4gSSBkb24ndCBzZWUgaG93IHRoZSBkeW5hbWljIHNlc3Npb24gcGF0Y2hlcyBj
aGFuZ2UgYW55dGhpbmcgdG8gdGhhdA0KPiA+IGNvbmNsdXNpb24uDQo+ID4NCj4gPj4gW1BBVENI
IFZlcnNpb24gNyAzLzNdIE5GU3Y0LjEgYXZvaWQgZnJlZWluZyBzbG90IHdoZW4gdGFza3MgYXJl
IHdhaXRpbmcNCj4gPj4gW1BBVENIIFZlcnNpb24gMSAyLzNdIE5GU3Y0LjEgcHJlcGFyZSBmb3Ig
ZHlhbWljIHNlc3Npb24gc2xvdHMNCj4gPj4gW1BBVENIIFZlcnNpb24gNyAxLzNdIE5GUzQuMSBj
bGVhbiB1cCBuZnM0X2FsbG9jX3Nlc3Npb24NCj4gPg0KPiA+IEkgbmVlZCBhIGZpeCBmb3IgdGhl
IDMuMyBmaW5hbC4uLg0KPiA+DQo+ID4gVGhvc2UgcGF0Y2hlcyBjYW4gYmUgY2xlYW5lZCB1cCBh
bmQgbWFkZSByZWFkeSBmb3IgMy40IChuZWVkcyB3b3JrIC0NCj4gPiB0aGV5IHdvbid0IGFwcGx5
IHRvIHRoZSAnbmZzLWZvci1uZXh0JyBicmFuY2gpLA0KPiANCj4gSSB3b3JrZWQgYWdhaW5zdCB0
aGUgYnVnZml4IGJyYW5jaC4gSSdsbCB1c2UgdGhlIG5mcy1mb3ItbmV4dCBicmFuY2guDQo+IA0K
PiA+IGJ1dCByaWdodCBub3cgdGhleSdyZQ0KPiA+IG5vdCBzdWZmaWNpZW50bHkgdGVzdGVkIG5v
ciBhcmUgdGhleSBzdWZmaWNpZW50bHkgcmV2aWV3ZWQuDQo+IA0KPiBBZ3JlZWQuIEkgd2FudCB0
byB0ZXN0IHNvbWUgbW9yZSBhdCBjb25uZWN0YXRob24uIEkgZGlkIGlvem9uZSB0ZXN0cw0KPiB0
byBzaG93IHRoYXQgdGhpcyBkeW5hbWljIHNsb3QgaW1wbGVtZW50YXRpb24gcGVyZm9ybXMgYXMg
d2VsbCBhcyB0aGUNCj4gc3RhdGljIGFycmF5LCBidXQgbm90IGVub3VnaCB0ZXN0aW5nIC8gcmV2
aWV3IGZvciBjb2RpbmcgYnVncy4NCj4gDQo+ID4gRm9yDQo+ID4gaW5zdGFuY2UsIEknbSBub3Qg
aGFwcHkgd2l0aCB0aGUgaWRlYSBvZiBhZGRpbmcgYSAndGtfcHJpdmF0ZScgZmllbGQgaW4NCj4g
PiB0aGUgc3RydWN0IHJwY190YXNrLg0KPiANCj4gT0suIERvIHlvdSBoYXZlIGEgc3VnZ2VzdGlv
bj8NCg0KWWVzLiBQbGVhc2Ugc2VlIGhvdyB3ZSBzb2x2ZSB0aGUgbWlub3IgdmVyc2lvbiAwIGVx
dWl2YWxlbnQgcHJvYmxlbSBpbg0KbmZzX3JlbGVhc2Vfc2VxaWQoKS4NCg0KSW4gdGhlIGNhc2Ug
b2YgbWlub3IgdmVyc2lvbiAxLCBJIHN1Z2dlc3Qgc2V0dGluZyB1cCBhIGRlZGljYXRlZCBsaW5r
ZWQNCmxpc3Qgb2YgJ3RoaW5ncyB3YWl0aW5nIGZvciBhIHNsb3QnLCB3aGVyZSBlYWNoIGVudHJ5
IGluIHRoZSBsaW5rZWQgbGlzdA0KY29udGFpbnMgdGhlIG5mczRfc2VxdWVuY2VfYXJncy9uZnM0
X3NlcXVlbmNlX3JlcyB0byBpbml0aWFsaXNlLCBhbmQNCnRoZW4gYSBwb2ludGVyIHRvIHRoZSBy
cGNfdGFzayB0byB3YWtlIHVwLg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNs
aWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3
dy5uZXRhcHAuY29tDQoNCg==
T24gV2VkLCAyMDEyLTAyLTE1IGF0IDE1OjA3ICswMDAwLCBBZGFtc29uLCBBbmR5IHdyb3RlOg0K
PiANCj4gUGxlYXNlIGRvbid0IG1lcmdlIG5mczRfcmVzZXRfc2xvdF90YWJsZXMgYW5kIG5mczRf
aW5pdF9zbG90X3RhYmxlcy4gIEl0ICBhc3N1bWVzIGEgc3RhdGljIGFycmF5IChyZWFsbG9jKSB3
aGljaCBnb2VzIGF3YXkgd2l0aCB0aGUgZHluYW1pYyBzbG90IHRhYmxlIGNvZGUuIEluc3RlYWQs
IHRha2UgYSBsb29rIGF0IHRoZSBwYXRjaCBzZXQgSSBzZW50IG9uIEZlYiAxMg0KDQpXaHkgbm90
PyBUaGVyZSBpcyBfbm9fIGZ1bmN0aW9uYWwgZGlmZmVyZW5jZSBiZXR3ZWVuIHdoYXQNCm5mczRf
cmVzZXRfc2xvdF90YWJsZXMoKSBhbmQgbmZzNF9pbml0X3Nsb3RfdGFibGVzKCkgbmVlZCB0byBk
by4NClRoZXkgYm90aCBuZWVkIHRvIGFsbG9jYXRlIG5ldyBzbG90cyAoY29uZGl0aW9uYWxseSBp
biB0aGUgY2FzZSBvZg0KbmZzNF9yZXNldF9zbG90X3RhYmxlcywgYnV0IHRoZSBjb25kaXRpb24g
aXMgY29tcGF0aWJsZSB3aXRoIHRoZQ0KbmZzNF9pbml0X3Nsb3RfdGFibGVzIGNhc2UpLCBhbmQg
dGhleSBib3RoIG5lZWQgdG8gaW5pdGlhbGlzZSB0aG9zZQ0Kc2xvdHMgdG8gdGhlIHZhbHVlICcx
Jy4NCg0KQUZBSUNTIFRoZXJlIGlzIG5vIHJlYXNvbiBmb3Iga2VlcGluZyB0aG9zZSBhcyAyIHNl
cGFyYXRlIGZ1bmN0aW9ucywgYW5kDQpJIGRvbid0IHNlZSBob3cgdGhlIGR5bmFtaWMgc2Vzc2lv
biBwYXRjaGVzIGNoYW5nZSBhbnl0aGluZyB0byB0aGF0DQpjb25jbHVzaW9uLg0KDQo+IFtQQVRD
SCBWZXJzaW9uIDcgMy8zXSBORlN2NC4xIGF2b2lkIGZyZWVpbmcgc2xvdCB3aGVuIHRhc2tzIGFy
ZSB3YWl0aW5nDQo+IFtQQVRDSCBWZXJzaW9uIDEgMi8zXSBORlN2NC4xIHByZXBhcmUgZm9yIGR5
YW1pYyBzZXNzaW9uIHNsb3RzDQo+IFtQQVRDSCBWZXJzaW9uIDcgMS8zXSBORlM0LjEgY2xlYW4g
dXAgbmZzNF9hbGxvY19zZXNzaW9uDQoNCkkgbmVlZCBhIGZpeCBmb3IgdGhlIDMuMyBmaW5hbC4u
Lg0KDQpUaG9zZSBwYXRjaGVzIGNhbiBiZSBjbGVhbmVkIHVwIGFuZCBtYWRlIHJlYWR5IGZvciAz
LjQgKG5lZWRzIHdvcmsgLQ0KdGhleSB3b24ndCBhcHBseSB0byB0aGUgJ25mcy1mb3ItbmV4dCcg
YnJhbmNoKSwgYnV0IHJpZ2h0IG5vdyB0aGV5J3JlDQpub3Qgc3VmZmljaWVudGx5IHRlc3RlZCBu
b3IgYXJlIHRoZXkgc3VmZmljaWVudGx5IHJldmlld2VkLiBGb3INCmluc3RhbmNlLCBJJ20gbm90
IGhhcHB5IHdpdGggdGhlIGlkZWEgb2YgYWRkaW5nIGEgJ3RrX3ByaXZhdGUnIGZpZWxkIGluDQp0
aGUgc3RydWN0IHJwY190YXNrLg0KDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMg
Y2xpZW50IG1haW50YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbQ0K
d3d3Lm5ldGFwcC5jb20NCg0K
T24gVHVlLCAyMDEyLTAyLTE0IGF0IDE5OjUzIC0wNTAwLCBKLiBCcnVjZSBGaWVsZHMgd3JvdGU6
DQo+IE9uIFdlZCwgRmViIDE1LCAyMDEyIGF0IDEyOjQzOjA4QU0gKzAwMDAsIE15a2xlYnVzdCwg
VHJvbmQgd3JvdGU6DQo+ID4gVGhlbiB3ZSBoYXZlIGEgcHJvYmxlbS4gVGhlIHplcm8gaW5pdGlh
bGlzYXRpb24gaXMgYWxyZWFkeSBpbiB1c2Ugb3V0DQo+ID4gdGhlcmUgaW4gYm90aCBjb21tZXJj
aWFsIGFuZCBub24tY29tbWVyY2lhbCB2ZXJzaW9ucyBvZiBMaW51eC4gSXQgaXMgdG9vDQo+ID4g
bGF0ZSB0byBjaGFuZ2UgdGhhdCBub3cuDQo+ID4gDQo+ID4gRnVydGhlcm1vcmUsIHNpbmNlIG5v
bmUgb2YgdGhlIHNlcnZlcnMgd2UndmUgdGVzdGVkIGFnYWluc3QgaW4gZWFybGllcg0KPiA+IEJh
a2VhdGhvbnMgYW5kIENvbm5lY3RhdGhvbnMgaGF2ZSBjb21wbGFpbmVkLCBJIHN1Z2dlc3QgdGhh
dCB3ZSByYXRoZXINCj4gPiBjaGFuZ2UgdGhlIHNwZWMgd2l0aCBhbiBlcnJhdGEuDQo+IA0KPiBB
cmdoLg0KPiANCj4gSSBqdXN0IG5vdGljZWQgdGhhdCB0aGUgc2VydmVyIHdhcyBjcmFzaGluZyBp
bnRlcm1pdHRlbnRseSBhbmQgdHJhY2VkIGl0DQo+IHRvIGluY29ycmVjdCBoYW5kbGluZyBvZiB0
aGUgY2FzZSB3aGVyZSB0aGUgY2xpZW50IHNlbmRzIGEgb25lLW9wDQo+IFNFUVVFTkNFIGNvbXBv
dW5kIHdpdGggc2VxaWQgMC4gIEknbSBub3Qgc3VyZSB3aHkgaXQgc3RhcnRlZCBwb3BwaW5nIHVw
DQo+IGp1c3QgaW4gMy4zLS1wZXJoYXBzIHNvbWUgY2hhbmdlIGluIHRoZSB3YXkgdGhlIGNsaWVu
dCB1c2VzIHNsb3RzIG1hZGUNCj4gdGhhdCBtb3JlIGxpa2VseS4NCj4gDQo+IFRoZSBzZXJ2ZXIg
Y29kZSBhY3R1YWxseSBsb29rcyBsaWtlIGl0IGRpZCBhc3N1bWUgaW5pdGlhbCBzZXFpZCAxLCBi
dXQNCj4gYWNjZXB0ZWQgaW5pdGlhbCBzZXFpZCAwIChleGNlcHQgaW4gdGhpcyBvbmUgY2FzZSkg
YmFzaWNhbGx5IGJ5IG1pc3Rha2UuDQo+IA0KPiBJbiBhbnkgY2FzZSwgSSB0aGluayBpdCBzaG91
bGQgYmUgZWFzeSBlbm91Z2ggdG8gdGVhY2ggaXQganVzdCB0byBhY2NlcHQNCj4gYW55IHNlcWlk
IG9uIGEgcHJldmlvdXNseSB1bnVzZWQgc2xvdCwgc28gSSdsbCBkbyB0aGF0Li4uLg0KDQpIYW5n
IG9uLi4uIExvb2tpbmcgYXQgMy4yLCBJIHRoaW5rIHlvdSdyZSBjb3JyZWN0LiBXZSB1c2VkIHRv
IGluaXRpYWxpc2UNCnRvIDEsIGFuZCBub3cgd2UncmUgaW5pdGlhbGlzaW5nIHRvIDAuDQoNCkFo
Li4uIEkgc2VlIHdoYXQgaGFwcGVuZWQuDQoNCmNvbW1pdCBhYWNkNTUzNzI3MGE3NTJmZTEyYTk5
MTRhMjA3Mjg0ZmMyMzQxYzZkIChORlN2NC4xOiBjbGVhbnVwIGluaXQNCmFuZCByZXNldCBvZiBz
ZXNzaW9uIHNsb3QgdGFibGVzKSBzZWVtcyB0byBoYXZlIHJlbW92ZWQgdGhlIGNhbGwgdG8NCm5m
czRfcmVzZXRfc2xvdF90YWJsZXMoKSB0aGF0IHdhcyBkb2luZyB0aGF0IGluaXRpYWxpc2F0aW9u
LiBPSy4uLiBTbw0KdGhpcyBpcyBhIExpbnV4IDMuMy1yY1ggb25seSByZWdyZXNzaW9uLCBhbmQg
c2hvdWxkIGJlIHRyZWF0ZWQgYXMNCnN1Y2guLi4NCg0KSG93IGFib3V0IHRoZSBmb2xsb3dpbmcg
aW5zdGVhZCBzbyB0aGF0IHdlIGVuc3VyZSB3ZSBmdWxseSBzaGFyZSB0aGUNCmluaXRpYWxpc2F0
aW9uIGNvZGUgYmV0d2VlbiBuZnM0X3Jlc2V0X3Nsb3RfdGFibGVzIGFuZA0KbmZzNF9pbml0X3Ns
b3RfdGFibGVzPw0KDQo4PC0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0NCkZyb20gOGFlOGQxNzdjYzEwNmVjZGIz
ZTI5YjQ4Yzc4ZWMzN2NmNzZlYTU0OSBNb24gU2VwIDE3IDAwOjAwOjAwIDIwMDENCkZyb206IFRy
b25kIE15a2xlYnVzdCA8VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20+DQpEYXRlOiBUdWUsIDE0
IEZlYiAyMDEyIDIwOjMzOjE5IC0wNTAwDQpTdWJqZWN0OiBbUEFUQ0hdIE5GU3Y0LjE6IEZpeCBh
IE5GU3Y0LjEgc2Vzc2lvbiBpbml0aWFsaXNhdGlvbiByZWdyZXNzaW9uDQoNCkNvbW1pdCBhYWNk
NTUzIChORlN2NC4xOiBjbGVhbnVwIGluaXQgYW5kIHJlc2V0IG9mIHNlc3Npb24gc2xvdCB0YWJs
ZXMpDQppbnRyb2R1Y2VzIGEgcmVncmVzc2lvbiBpbiB0aGUgc2Vzc2lvbiBpbml0aWFsaXNhdGlv
biBjb2RlLiBOZXcgdGFibGVzDQpub3cgZmluZCB0aGVpciBzZXF1ZW5jZSBpZHMgaW5pdGlhbGlz
ZWQgdG8gMCwgcmF0aGVyIHRoYW4gdGhlIG1hbmRhdGVkDQp2YWx1ZSBvZiAxIChzZWUgUkZDNTY2
MSkuDQoNClJlcG9ydGVkLWJ5OiBWaXRhbGl5IEd1c2V2IDxndXNldi52aXRhbGl5QG5leGVudGEu
Y29tPg0KU2lnbmVkLW9mZi1ieTogVHJvbmQgTXlrbGVidXN0IDxUcm9uZC5NeWtsZWJ1c3RAbmV0
YXBwLmNvbT4NCi0tLQ0KIGZzL25mcy9uZnM0cHJvYy5jIHwgICA1OCArKysrKysrKysrKysrKysr
KysrKysrKysrKysrKysrKy0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KIDEgZmlsZXMgY2hhbmdlZCwg
MzUgaW5zZXJ0aW9ucygrKSwgMjMgZGVsZXRpb25zKC0pDQoNCmRpZmYgLS1naXQgYS9mcy9uZnMv
bmZzNHByb2MuYyBiL2ZzL25mcy9uZnM0cHJvYy5jDQppbmRleCBkMjAyZTA0Li45Zjc2Y2U5IDEw
MDY0NA0KLS0tIGEvZnMvbmZzL25mczRwcm9jLmMNCisrKyBiL2ZzL25mcy9uZnM0cHJvYy5jDQpA
QCAtNTAwOCwzNyArNTAwOCw1MyBAQCBpbnQgbmZzNF9wcm9jX2dldF9sZWFzZV90aW1lKHN0cnVj
dCBuZnNfY2xpZW50ICpjbHAsIHN0cnVjdCBuZnNfZnNpbmZvICpmc2luZm8pDQogCXJldHVybiBz
dGF0dXM7DQogfQ0KIA0KK3N0YXRpYyBzdHJ1Y3QgbmZzNF9zbG90ICpuZnM0X3Nlc3Npb25fYWxs
b2Nfc2xvdHModTMyIG1heF9zbG90cywgZ2ZwX3QgZ2ZwX2ZsYWdzKQ0KK3sNCisJcmV0dXJuIGtj
YWxsb2MobWF4X3Nsb3RzLCBzaXplb2Yoc3RydWN0IG5mczRfc2xvdCksIGdmcF9mbGFncyk7DQor
fQ0KKw0KK3N0YXRpYyB2b2lkIG5mczRfc2Vzc2lvbl9hZGRfc2xvdHMoc3RydWN0IG5mczRfc2xv
dF90YWJsZSAqdGJsLA0KKwkJc3RydWN0IG5mczRfc2xvdCAqbmV3LA0KKwkJdTMyIG1heF9zbG90
cywNCisJCXUzMiBpdmFsdWUpDQorew0KKwlzdHJ1Y3QgbmZzNF9zbG90ICpvbGQgPSBOVUxMOw0K
Kwl1MzIgaTsNCisNCisJc3Bpbl9sb2NrKCZ0YmwtPnNsb3RfdGJsX2xvY2spOw0KKwlpZiAobmV3
KSB7DQorCQlvbGQgPSB0YmwtPnNsb3RzOw0KKwkJdGJsLT5zbG90cyA9IG5ldzsNCisJCXRibC0+
bWF4X3Nsb3RzID0gbWF4X3Nsb3RzOw0KKwl9DQorCXRibC0+aGlnaGVzdF91c2VkX3Nsb3RpZCA9
IC0xOwkvKiBubyBzbG90IGlzIGN1cnJlbnRseSB1c2VkICovDQorCWZvciAoaSA9IDA7IGkgPCB0
YmwtPm1heF9zbG90czsgaSsrKQ0KKwkJdGJsLT5zbG90c1tpXS5zZXFfbnIgPSBpdmFsdWU7DQor
CXNwaW5fdW5sb2NrKCZ0YmwtPnNsb3RfdGJsX2xvY2spOw0KKwlrZnJlZShvbGQpOw0KK30NCisN
CiAvKg0KICAqIFJlc2V0IGEgc2xvdCB0YWJsZQ0KICAqLw0KIHN0YXRpYyBpbnQgbmZzNF9yZXNl
dF9zbG90X3RhYmxlKHN0cnVjdCBuZnM0X3Nsb3RfdGFibGUgKnRibCwgdTMyIG1heF9yZXFzLA0K
LQkJCQkgaW50IGl2YWx1ZSkNCisJCQkJIHUzMiBpdmFsdWUpDQogew0KIAlzdHJ1Y3QgbmZzNF9z
bG90ICpuZXcgPSBOVUxMOw0KLQlpbnQgaTsNCi0JaW50IHJldCA9IDA7DQorCWludCByZXQgPSAt
RU5PTUVNOw0KIA0KIAlkcHJpbnRrKCItLT4gJXM6IG1heF9yZXFzPSV1LCB0YmwtPm1heF9zbG90
cyAlZFxuIiwgX19mdW5jX18sDQogCQltYXhfcmVxcywgdGJsLT5tYXhfc2xvdHMpOw0KIA0KIAkv
KiBEb2VzIHRoZSBuZXdseSBuZWdvdGlhdGVkIG1heF9yZXFzIG1hdGNoIHRoZSBleGlzdGluZyBz
bG90IHRhYmxlPyAqLw0KIAlpZiAobWF4X3JlcXMgIT0gdGJsLT5tYXhfc2xvdHMpIHsNCi0JCXJl
dCA9IC1FTk9NRU07DQotCQluZXcgPSBrbWFsbG9jKG1heF9yZXFzICogc2l6ZW9mKHN0cnVjdCBu
ZnM0X3Nsb3QpLA0KLQkJCSAgICAgIEdGUF9OT0ZTKTsNCisJCW5ldyA9IG5mczRfc2Vzc2lvbl9h
bGxvY19zbG90cyhtYXhfcmVxcywgR0ZQX05PRlMpOw0KIAkJaWYgKCFuZXcpDQogCQkJZ290byBv
dXQ7DQotCQlyZXQgPSAwOw0KLQkJa2ZyZWUodGJsLT5zbG90cyk7DQogCX0NCi0Jc3Bpbl9sb2Nr
KCZ0YmwtPnNsb3RfdGJsX2xvY2spOw0KLQlpZiAobmV3KSB7DQotCQl0YmwtPnNsb3RzID0gbmV3
Ow0KLQkJdGJsLT5tYXhfc2xvdHMgPSBtYXhfcmVxczsNCi0JfQ0KLQlmb3IgKGkgPSAwOyBpIDwg
dGJsLT5tYXhfc2xvdHM7ICsraSkNCi0JCXRibC0+c2xvdHNbaV0uc2VxX25yID0gaXZhbHVlOw0K
LQlzcGluX3VubG9jaygmdGJsLT5zbG90X3RibF9sb2NrKTsNCisJcmV0ID0gMDsNCisNCisJbmZz
NF9zZXNzaW9uX2FkZF9zbG90cyh0YmwsIG5ldywgbWF4X3JlcXMsIGl2YWx1ZSk7DQogCWRwcmlu
dGsoIiVzOiB0Ymw9JXAgc2xvdHM9JXAgbWF4X3Nsb3RzPSVkXG4iLCBfX2Z1bmNfXywNCiAJCXRi
bCwgdGJsLT5zbG90cywgdGJsLT5tYXhfc2xvdHMpOw0KIG91dDoNCkBAIC01MDY0LDcgKzUwODAs
NyBAQCBzdGF0aWMgdm9pZCBuZnM0X2Rlc3Ryb3lfc2xvdF90YWJsZXMoc3RydWN0IG5mczRfc2Vz
c2lvbiAqc2Vzc2lvbikNCiAgKiBJbml0aWFsaXplIHNsb3QgdGFibGUNCiAgKi8NCiBzdGF0aWMg
aW50IG5mczRfaW5pdF9zbG90X3RhYmxlKHN0cnVjdCBuZnM0X3Nsb3RfdGFibGUgKnRibCwNCi0J
CWludCBtYXhfc2xvdHMsIGludCBpdmFsdWUpDQorCQl1MzIgbWF4X3Nsb3RzLCB1MzIgaXZhbHVl
KQ0KIHsNCiAJc3RydWN0IG5mczRfc2xvdCAqc2xvdDsNCiAJaW50IHJldCA9IC1FTk9NRU07DQpA
QCAtNTA3MywxNiArNTA4OSwxMiBAQCBzdGF0aWMgaW50IG5mczRfaW5pdF9zbG90X3RhYmxlKHN0
cnVjdCBuZnM0X3Nsb3RfdGFibGUgKnRibCwNCiANCiAJZHByaW50aygiLS0+ICVzOiBtYXhfcmVx
cz0ldVxuIiwgX19mdW5jX18sIG1heF9zbG90cyk7DQogDQotCXNsb3QgPSBrY2FsbG9jKG1heF9z
bG90cywgc2l6ZW9mKHN0cnVjdCBuZnM0X3Nsb3QpLCBHRlBfTk9GUyk7DQorCXNsb3QgPSBuZnM0
X3Nlc3Npb25fYWxsb2Nfc2xvdHMobWF4X3Nsb3RzLCBHRlBfTk9GUyk7DQogCWlmICghc2xvdCkN
CiAJCWdvdG8gb3V0Ow0KIAlyZXQgPSAwOw0KIA0KLQlzcGluX2xvY2soJnRibC0+c2xvdF90Ymxf
bG9jayk7DQotCXRibC0+bWF4X3Nsb3RzID0gbWF4X3Nsb3RzOw0KLQl0YmwtPnNsb3RzID0gc2xv
dDsNCi0JdGJsLT5oaWdoZXN0X3VzZWRfc2xvdGlkID0gLTE7ICAvKiBubyBzbG90IGlzIGN1cnJl
bnRseSB1c2VkICovDQotCXNwaW5fdW5sb2NrKCZ0YmwtPnNsb3RfdGJsX2xvY2spOw0KKwluZnM0
X3Nlc3Npb25fYWRkX3Nsb3RzKHRibCwgc2xvdCwgbWF4X3Nsb3RzLCBpdmFsdWUpOw0KIAlkcHJp
bnRrKCIlczogdGJsPSVwIHNsb3RzPSVwIG1heF9zbG90cz0lZFxuIiwgX19mdW5jX18sDQogCQl0
YmwsIHRibC0+c2xvdHMsIHRibC0+bWF4X3Nsb3RzKTsNCiBvdXQ6DQotLSANCjEuNy43LjYNCg0K
DQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5l
dEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCg==
On 02/15/2012 02:46 AM, Myklebust, Trond wrote:
> Hi Vitaliy.
>
> What's wrong with sa_sequenceid == 0? I can't see anything in the spec
> that states that the sequenceid is supposed to be initialised to 1.
That is good question.
In 2.10.6.1. "Slot Identifiers and Reply Cache":
A slot contains a sequence ID and the cached reply corresponding to
the request sent with that sequence ID. The sequence ID is a 32-bit
unsigned value, and is therefore in the range 0..0xFFFFFFFF (2^32 -
1). The first time a slot is used, the requester MUST specify a
sequence ID of one (Section 18.36).
In 18.36.4.4 "Session creation":
For each slot in the reply cache, the
server sets the sequence ID to zero, and records an entry
^^^^^^^^^
containing a COMPOUND reply with zero operations and the error
NFS4ERR_SEQ_MISORDERED. This way, if the first SEQUENCE request
sent has a sequence ID equal to zero, the server can simply
^^^^^^^^^^^^
return what is in the reply cache: NFS4ERR_SEQ_MISORDERED. The
client initializes its reply cache for receiving callbacks in the
same way, and similarly, the first CB_SEQUENCE operation on a
slot after session creation MUST have a sequence ID of one.
So sending "zero" sa_sequenceid is allowed.
But if Linux NFS server receives first OP_SEQUENCE with "sa_sequenceid
== 0", it replies "NFS4ERR_RETRY_UNCACHED_REP" (for OP_PUTROOTFH) - but
expected SEQ_MISORDERED for OP_SEQUENCE. Illumos NFSv4.1 implementation
returns allowed SEQ_MISORDERED.
So why just do not fix receiving error and correct sending sa_sequenceid
to 1?
On Feb 14, 2012, at 8:50 PM, Myklebust, Trond wrote:
> On Tue, 2012-02-14 at 19:53 -0500, J. Bruce Fields wrote:
>> On Wed, Feb 15, 2012 at 12:43:08AM +0000, Myklebust, Trond wrote:
>>> Then we have a problem. The zero initialisation is already in use out
>>> there in both commercial and non-commercial versions of Linux. It is too
>>> late to change that now.
>>>
>>> Furthermore, since none of the servers we've tested against in earlier
>>> Bakeathons and Connectathons have complained, I suggest that we rather
>>> change the spec with an errata.
>>
>> Argh.
>>
>> I just noticed that the server was crashing intermittently and traced it
>> to incorrect handling of the case where the client sends a one-op
>> SEQUENCE compound with seqid 0. I'm not sure why it started popping up
>> just in 3.3--perhaps some change in the way the client uses slots made
>> that more likely.
>>
>> The server code actually looks like it did assume initial seqid 1, but
>> accepted initial seqid 0 (except in this one case) basically by mistake.
>>
>> In any case, I think it should be easy enough to teach it just to accept
>> any seqid on a previously unused slot, so I'll do that....
>
> Hang on... Looking at 3.2, I think you're correct. We used to initialise
> to 1, and now we're initialising to 0.
No, commit aacd5537270a752fe12a9914a207284fc2341c6d initializes and resets the slot tables with the ivalue which is 1 for the forechannel and 0 for the back channel, just as in 3.2.
To be clear from aacd5537:
-static int nfs4_init_slot_tables(struct nfs4_session *session)
+static int nfs4_setup_session_slot_tables(struct nfs4_session *ses)
{
struct nfs4_slot_table *tbl;
- int status = 0;
+ int status;
- tbl = &session->fc_slot_table;
+ dprintk("--> %s\n", __func__);
+ /* Fore channel */
+ tbl = &ses->fc_slot_table;
if (tbl->slots == NULL) {
- status = nfs4_init_slot_table(tbl,
- session->fc_attrs.max_reqs, 1);
^^^^^^^
ivalue of 1
+ status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
^^^^^
ivalue of 1
+ if (status) /* -ENOMEM */
+ return status;
+ } else {
+ status = nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
^^^^^
ivalue of 1
if (status)
return status;
}
-
- tbl = &session->bc_slot_table;
+ /* Back channel */
+ tbl = &ses->bc_slot_table;
if (tbl->slots == NULL) {
- status = nfs4_init_slot_table(tbl,
- session->bc_attrs.max_reqs, 0);
+ status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
if (status)
- nfs4_destroy_slot_tables(session);
- }
-
+ /* Fore and back channel share a connection so get
+ * both slot tables or neither */
+ nfs4_destroy_slot_tables(ses);
+ } else
+ status = nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
return status;
}
-->Andy
>
> Ah... I see what happened.
>
> commit aacd5537270a752fe12a9914a207284fc2341c6d (NFSv4.1: cleanup init
> and reset of session slot tables) seems to have removed the call to
> nfs4_reset_slot_tables() that was doing that initialisation. OK.
> this is a Linux 3.3-rcX only regression, and should be treated as
> such...
>
> How about the following instead so that we ensure we fully share the
> initialisation code between nfs4_reset_slot_tables and
> nfs4_init_slot_tables?
>
> 8<-------------------------------------------------------------------------
> From 8ae8d177cc106ecdb3e29b48c78ec37cf76ea549 Mon Sep 17 00:00:00 2001
> From: Trond Myklebust <[email protected]>
> Date: Tue, 14 Feb 2012 20:33:19 -0500
> Subject: [PATCH] NFSv4.1: Fix a NFSv4.1 session initialisation regression
>
> Commit aacd553 (NFSv4.1: cleanup init and reset of session slot tables)
> introduces a regression in the session initialisation code. New tables
> now find their sequence ids initialised to 0, rather than the mandated
> value of 1 (see RFC5661).
>
> Reported-by: Vitaliy Gusev <[email protected]>
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> fs/nfs/nfs4proc.c | 58 ++++++++++++++++++++++++++++++++---------------------
> 1 files changed, 35 insertions(+), 23 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index d202e04..9f76ce9 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5008,37 +5008,53 @@ int nfs4_proc_get_lease_time(struct nfs_client *clp, struct nfs_fsinfo *fsinfo)
> return status;
> }
>
> +static struct nfs4_slot *nfs4_session_alloc_slots(u32 max_slots, gfp_t gfp_flags)
> +{
> + return kcalloc(max_slots, sizeof(struct nfs4_slot), gfp_flags);
> +}
> +
> +static void nfs4_session_add_slots(struct nfs4_slot_table *tbl,
> + struct nfs4_slot *new,
> + u32 max_slots,
> + u32 ivalue)
> +{
> + struct nfs4_slot *old = NULL;
> + u32 i;
> +
> + spin_lock(&tbl->slot_tbl_lock);
> + if (new) {
> + old = tbl->slots;
> + tbl->slots = new;
> + tbl->max_slots = max_slots;
> + }
> + tbl->highest_used_slotid = -1; /* no slot is currently used */
> + for (i = 0; i < tbl->max_slots; i++)
> + tbl->slots[i].seq_nr = ivalue;
> + spin_unlock(&tbl->slot_tbl_lock);
> + kfree(old);
> +}
> +
> /*
> * Reset a slot table
> */
> static int nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 max_reqs,
> - int ivalue)
> + u32 ivalue)
> {
> struct nfs4_slot *new = NULL;
> - int i;
> - int ret = 0;
> + int ret = -ENOMEM;
>
> dprintk("--> %s: max_reqs=%u, tbl->max_slots %d\n", __func__,
> max_reqs, tbl->max_slots);
>
> /* Does the newly negotiated max_reqs match the existing slot table? */
> if (max_reqs != tbl->max_slots) {
> - ret = -ENOMEM;
> - new = kmalloc(max_reqs * sizeof(struct nfs4_slot),
> - GFP_NOFS);
> + new = nfs4_session_alloc_slots(max_reqs, GFP_NOFS);
> if (!new)
> goto out;
> - ret = 0;
> - kfree(tbl->slots);
> }
> - spin_lock(&tbl->slot_tbl_lock);
> - if (new) {
> - tbl->slots = new;
> - tbl->max_slots = max_reqs;
> - }
> - for (i = 0; i < tbl->max_slots; ++i)
> - tbl->slots[i].seq_nr = ivalue;
> - spin_unlock(&tbl->slot_tbl_lock);
> + ret = 0;
> +
> + nfs4_session_add_slots(tbl, new, max_reqs, ivalue);
> dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
> tbl, tbl->slots, tbl->max_slots);
> out:
> @@ -5064,7 +5080,7 @@ static void nfs4_destroy_slot_tables(struct nfs4_session *session)
> * Initialize slot table
> */
> static int nfs4_init_slot_table(struct nfs4_slot_table *tbl,
> - int max_slots, int ivalue)
> + u32 max_slots, u32 ivalue)
> {
> struct nfs4_slot *slot;
> int ret = -ENOMEM;
> @@ -5073,16 +5089,12 @@ static int nfs4_init_slot_table(struct nfs4_slot_table *tbl,
>
> dprintk("--> %s: max_reqs=%u\n", __func__, max_slots);
>
> - slot = kcalloc(max_slots, sizeof(struct nfs4_slot), GFP_NOFS);
> + slot = nfs4_session_alloc_slots(max_slots, GFP_NOFS);
> if (!slot)
> goto out;
> ret = 0;
>
> - spin_lock(&tbl->slot_tbl_lock);
> - tbl->max_slots = max_slots;
> - tbl->slots = slot;
> - tbl->highest_used_slotid = -1; /* no slot is currently used */
> - spin_unlock(&tbl->slot_tbl_lock);
> + nfs4_session_add_slots(tbl, slot, max_slots, ivalue);
> dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
> tbl, tbl->slots, tbl->max_slots);
> out:
> --
> 1.7.7.6
>
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com
>
T24gV2VkLCAyMDEyLTAyLTE1IGF0IDAxOjQ4ICswNDAwLCBWaXRhbGl5IEd1c2V2IHdyb3RlOg0K
PiBVbmluaXRpYWxpemVkIHNlcV9uciBjYXVzZXMgc2VuZGluZyBmaXJzdCBTRVNTSU9OIHJlcXVl
c3QNCj4gd2l0aCBzYV9zZXF1ZW5jZWlkID0gMC4NCj4gDQo+IFNpZ25lZC1vZmYtYnk6IFZpdGFs
aXkgR3VzZXYgPGd1c2V2LnZpdGFsaXlAbmV4ZW50YS5jb20+DQoNCkhpIFZpdGFsaXkuDQoNCldo
YXQncyB3cm9uZyB3aXRoIHNhX3NlcXVlbmNlaWQgPT0gMD8gSSBjYW4ndCBzZWUgYW55dGhpbmcg
aW4gdGhlIHNwZWMNCnRoYXQgc3RhdGVzIHRoYXQgdGhlIHNlcXVlbmNlaWQgaXMgc3VwcG9zZWQg
dG8gYmUgaW5pdGlhbGlzZWQgdG8gMS4NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5G
UyBjbGllbnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29t
DQp3d3cubmV0YXBwLmNvbQ0KDQo=
VGhlbiB3ZSBoYXZlIGEgcHJvYmxlbS4gVGhlIHplcm8gaW5pdGlhbGlzYXRpb24gaXMgYWxyZWFk
eSBpbiB1c2Ugb3V0DQp0aGVyZSBpbiBib3RoIGNvbW1lcmNpYWwgYW5kIG5vbi1jb21tZXJjaWFs
IHZlcnNpb25zIG9mIExpbnV4LiBJdCBpcyB0b28NCmxhdGUgdG8gY2hhbmdlIHRoYXQgbm93Lg0K
DQpGdXJ0aGVybW9yZSwgc2luY2Ugbm9uZSBvZiB0aGUgc2VydmVycyB3ZSd2ZSB0ZXN0ZWQgYWdh
aW5zdCBpbiBlYXJsaWVyDQpCYWtlYXRob25zIGFuZCBDb25uZWN0YXRob25zIGhhdmUgY29tcGxh
aW5lZCwgSSBzdWdnZXN0IHRoYXQgd2UgcmF0aGVyDQpjaGFuZ2UgdGhlIHNwZWMgd2l0aCBhbiBl
cnJhdGEuDQoNClRyb25kDQoNCk9uIFdlZCwgMjAxMi0wMi0xNSBhdCAwNDozNCArMDQwMCwgVml0
YWxpeSBHdXNldiB3cm90ZToNCj4gT24gMDIvMTUvMjAxMiAwMjo0NiBBTSwgTXlrbGVidXN0LCBU
cm9uZCB3cm90ZToNCj4gPiBIaSBWaXRhbGl5Lg0KPiA+DQo+ID4gV2hhdCdzIHdyb25nIHdpdGgg
c2Ffc2VxdWVuY2VpZCA9PSAwPyBJIGNhbid0IHNlZSBhbnl0aGluZyBpbiB0aGUgc3BlYw0KPiA+
IHRoYXQgc3RhdGVzIHRoYXQgdGhlIHNlcXVlbmNlaWQgaXMgc3VwcG9zZWQgdG8gYmUgaW5pdGlh
bGlzZWQgdG8gMS4NCj4gDQo+IFRoYXQgaXMgZ29vZCBxdWVzdGlvbi4NCj4gDQo+ICAgSW4gMi4x
MC42LjEuICAiU2xvdCBJZGVudGlmaWVycyBhbmQgUmVwbHkgQ2FjaGUiOg0KPiANCj4gDQo+ICAg
ICBBIHNsb3QgY29udGFpbnMgYSBzZXF1ZW5jZSBJRCBhbmQgdGhlIGNhY2hlZCByZXBseSBjb3Jy
ZXNwb25kaW5nIHRvDQo+ICAgICB0aGUgcmVxdWVzdCBzZW50IHdpdGggdGhhdCBzZXF1ZW5jZSBJ
RC4gIFRoZSBzZXF1ZW5jZSBJRCBpcyBhIDMyLWJpdA0KPiAgICAgdW5zaWduZWQgdmFsdWUsIGFu
ZCBpcyB0aGVyZWZvcmUgaW4gdGhlIHJhbmdlIDAuLjB4RkZGRkZGRkYgKDJeMzIgLQ0KPiAgICAg
MSkuICBUaGUgZmlyc3QgdGltZSBhIHNsb3QgaXMgdXNlZCwgdGhlIHJlcXVlc3RlciBNVVNUIHNw
ZWNpZnkgYQ0KPiAgICAgc2VxdWVuY2UgSUQgb2Ygb25lIChTZWN0aW9uIDE4LjM2KS4NCj4gDQo+
IA0KPiAgIEluIDE4LjM2LjQuNCAiU2Vzc2lvbiBjcmVhdGlvbiI6DQo+IA0KPiAgICAgICAgIEZv
ciBlYWNoIHNsb3QgaW4gdGhlIHJlcGx5IGNhY2hlLCB0aGUNCj4gICAgICAgICBzZXJ2ZXIgc2V0
cyB0aGUgc2VxdWVuY2UgSUQgdG8gemVybywgYW5kIHJlY29yZHMgYW4gZW50cnkNCj4gICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgXl5eXl5eXl5eDQo+IA0KPiAgICAgICAgY29u
dGFpbmluZyBhIENPTVBPVU5EIHJlcGx5IHdpdGggemVybyBvcGVyYXRpb25zIGFuZCB0aGUgZXJy
b3INCj4gICAgICAgICBORlM0RVJSX1NFUV9NSVNPUkRFUkVELiAgVGhpcyB3YXksIGlmIHRoZSBm
aXJzdCBTRVFVRU5DRSByZXF1ZXN0DQo+ICAgICAgICAgc2VudCBoYXMgYSBzZXF1ZW5jZSBJRCBl
cXVhbCB0byB6ZXJvLCB0aGUgc2VydmVyIGNhbiBzaW1wbHkNCj4gICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgIF5eXl5eXl5eXl5eXg0KPiANCj4gICAgICAgICByZXR1cm4gd2hhdCBp
cyBpbiB0aGUgcmVwbHkgY2FjaGU6IE5GUzRFUlJfU0VRX01JU09SREVSRUQuICBUaGUNCj4gICAg
ICAgICBjbGllbnQgaW5pdGlhbGl6ZXMgaXRzIHJlcGx5IGNhY2hlIGZvciByZWNlaXZpbmcgY2Fs
bGJhY2tzIGluIHRoZQ0KPiAgICAgICAgIHNhbWUgd2F5LCBhbmQgc2ltaWxhcmx5LCB0aGUgZmly
c3QgQ0JfU0VRVUVOQ0Ugb3BlcmF0aW9uIG9uIGENCj4gICAgICAgICBzbG90IGFmdGVyIHNlc3Np
b24gY3JlYXRpb24gTVVTVCBoYXZlIGEgc2VxdWVuY2UgSUQgb2Ygb25lLg0KPiANCj4gDQo+IFNv
IHNlbmRpbmcgInplcm8iIHNhX3NlcXVlbmNlaWQgaXMgYWxsb3dlZC4NCj4gQnV0IGlmIExpbnV4
IE5GUyBzZXJ2ZXIgcmVjZWl2ZXMgZmlyc3QgT1BfU0VRVUVOQ0Ugd2l0aCAic2Ffc2VxdWVuY2Vp
ZCANCj4gPT0gMCIsIGl0IHJlcGxpZXMgIk5GUzRFUlJfUkVUUllfVU5DQUNIRURfUkVQIiAgKGZv
ciBPUF9QVVRST09URkgpIC0gYnV0IA0KPiBleHBlY3RlZCBTRVFfTUlTT1JERVJFRCBmb3IgT1Bf
U0VRVUVOQ0UuICBJbGx1bW9zIE5GU3Y0LjEgaW1wbGVtZW50YXRpb24gDQo+IHJldHVybnMgYWxs
b3dlZCBTRVFfTUlTT1JERVJFRC4NCj4gDQo+IFNvIHdoeSBqdXN0IGRvIG5vdCBmaXggcmVjZWl2
aW5nIGVycm9yIGFuZCBjb3JyZWN0IHNlbmRpbmcgc2Ffc2VxdWVuY2VpZCANCj4gdG8gMT8NCg0K
LS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lcg0KDQpOZXRB
cHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNvbQ0KDQo=
On Wed, Feb 15, 2012 at 10:06 AM, Vitaliy Gusev
<[email protected]> wrote:
> On 02/15/2012 07:01 PM, Adamson, Andy wrote:
>>
>>
>> No, commit aacd5537270a752fe12a9914a207284fc2341c6d initializes and resets
>> the slot tables with the ivalue
>> which is 1 for the forechannel and 0 for the back channel, just as in 3.2.
>
>
>
> nfs4_init_slot_table() doesn't use ivalue at all.
Yep - that is the bug - which is in v2.6.39, and v3.2 ....
-->Andy
>
>
>
>>
>> To be clear from aacd5537:
>>
>> -static int nfs4_init_slot_tables(struct nfs4_session *session)
>> +static int nfs4_setup_session_slot_tables(struct nfs4_session *ses)
>> ?{
>> ? ? ? ? struct nfs4_slot_table *tbl;
>> - ? ? ? int status = 0;
>> + ? ? ? int status;
>>
>> - ? ? ? tbl =&session->fc_slot_table;
>>
>> + ? ? ? dprintk("--> ?%s\n", __func__);
>> + ? ? ? /* Fore channel */
>> + ? ? ? tbl =&ses->fc_slot_table;
>>
>> ? ? ? ? if (tbl->slots == NULL) {
>> - ? ? ? ? ? ? ? status = nfs4_init_slot_table(tbl,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? session->fc_attrs.max_reqs, 1);
>>
>> ? ?^^^^^^^
>>
>> ? ?ivalue of 1
>>
> --
> 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
This looks correct to me. This same bug - where the ivalue is ignored,
is in v2.6.39 and v3.2.
-->Andy
On Tue, Feb 14, 2012 at 4:48 PM, Vitaliy Gusev <[email protected]> wrote:
> Uninitialized seq_nr causes sending first SESSION request
> with sa_sequenceid = 0.
>
> Signed-off-by: Vitaliy Gusev <[email protected]>
> ---
> ?fs/nfs/nfs4proc.c | ? ?3 +++
> ?1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index f0c849c..711a812 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5067,6 +5067,7 @@ static int nfs4_init_slot_table(struct nfs4_slot_table *tbl,
> ? ? ? ? ? ? ? ?int max_slots, int ivalue)
> ?{
> ? ? ? ?struct nfs4_slot *slot;
> + ? ? ? int i;
> ? ? ? ?int ret = -ENOMEM;
>
> ? ? ? ?BUG_ON(max_slots > NFS4_MAX_SLOT_TABLE);
> @@ -5082,6 +5083,8 @@ static int nfs4_init_slot_table(struct nfs4_slot_table *tbl,
> ? ? ? ?tbl->max_slots = max_slots;
> ? ? ? ?tbl->slots = slot;
> ? ? ? ?tbl->highest_used_slotid = -1; ?/* no slot is currently used */
> + ? ? ? for (i = 0; i < tbl->max_slots; ++i)
> + ? ? ? ? ? ? ? tbl->slots[i].seq_nr = ivalue;
> ? ? ? ?spin_unlock(&tbl->slot_tbl_lock);
> ? ? ? ?dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
> ? ? ? ? ? ? ? ?tbl, tbl->slots, tbl->max_slots);
> --
> 1.7.5.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
T24gV2VkLCAyMDEyLTAyLTE1IGF0IDE1OjAxICswMDAwLCBBZGFtc29uLCBBbmR5IHdyb3RlOg0K
PiBPbiBGZWIgMTQsIDIwMTIsIGF0IDg6NTAgUE0sIE15a2xlYnVzdCwgVHJvbmQgd3JvdGU6DQo+
IA0KPiA+IE9uIFR1ZSwgMjAxMi0wMi0xNCBhdCAxOTo1MyAtMDUwMCwgSi4gQnJ1Y2UgRmllbGRz
IHdyb3RlOg0KPiA+PiBPbiBXZWQsIEZlYiAxNSwgMjAxMiBhdCAxMjo0MzowOEFNICswMDAwLCBN
eWtsZWJ1c3QsIFRyb25kIHdyb3RlOg0KPiA+Pj4gVGhlbiB3ZSBoYXZlIGEgcHJvYmxlbS4gVGhl
IHplcm8gaW5pdGlhbGlzYXRpb24gaXMgYWxyZWFkeSBpbiB1c2Ugb3V0DQo+ID4+PiB0aGVyZSBp
biBib3RoIGNvbW1lcmNpYWwgYW5kIG5vbi1jb21tZXJjaWFsIHZlcnNpb25zIG9mIExpbnV4LiBJ
dCBpcyB0b28NCj4gPj4+IGxhdGUgdG8gY2hhbmdlIHRoYXQgbm93Lg0KPiA+Pj4gDQo+ID4+PiBG
dXJ0aGVybW9yZSwgc2luY2Ugbm9uZSBvZiB0aGUgc2VydmVycyB3ZSd2ZSB0ZXN0ZWQgYWdhaW5z
dCBpbiBlYXJsaWVyDQo+ID4+PiBCYWtlYXRob25zIGFuZCBDb25uZWN0YXRob25zIGhhdmUgY29t
cGxhaW5lZCwgSSBzdWdnZXN0IHRoYXQgd2UgcmF0aGVyDQo+ID4+PiBjaGFuZ2UgdGhlIHNwZWMg
d2l0aCBhbiBlcnJhdGEuDQo+ID4+IA0KPiA+PiBBcmdoLg0KPiA+PiANCj4gPj4gSSBqdXN0IG5v
dGljZWQgdGhhdCB0aGUgc2VydmVyIHdhcyBjcmFzaGluZyBpbnRlcm1pdHRlbnRseSBhbmQgdHJh
Y2VkIGl0DQo+ID4+IHRvIGluY29ycmVjdCBoYW5kbGluZyBvZiB0aGUgY2FzZSB3aGVyZSB0aGUg
Y2xpZW50IHNlbmRzIGEgb25lLW9wDQo+ID4+IFNFUVVFTkNFIGNvbXBvdW5kIHdpdGggc2VxaWQg
MC4gIEknbSBub3Qgc3VyZSB3aHkgaXQgc3RhcnRlZCBwb3BwaW5nIHVwDQo+ID4+IGp1c3QgaW4g
My4zLS1wZXJoYXBzIHNvbWUgY2hhbmdlIGluIHRoZSB3YXkgdGhlIGNsaWVudCB1c2VzIHNsb3Rz
IG1hZGUNCj4gPj4gdGhhdCBtb3JlIGxpa2VseS4NCj4gPj4gDQo+ID4+IFRoZSBzZXJ2ZXIgY29k
ZSBhY3R1YWxseSBsb29rcyBsaWtlIGl0IGRpZCBhc3N1bWUgaW5pdGlhbCBzZXFpZCAxLCBidXQN
Cj4gPj4gYWNjZXB0ZWQgaW5pdGlhbCBzZXFpZCAwIChleGNlcHQgaW4gdGhpcyBvbmUgY2FzZSkg
YmFzaWNhbGx5IGJ5IG1pc3Rha2UuDQo+ID4+IA0KPiA+PiBJbiBhbnkgY2FzZSwgSSB0aGluayBp
dCBzaG91bGQgYmUgZWFzeSBlbm91Z2ggdG8gdGVhY2ggaXQganVzdCB0byBhY2NlcHQNCj4gPj4g
YW55IHNlcWlkIG9uIGEgcHJldmlvdXNseSB1bnVzZWQgc2xvdCwgc28gSSdsbCBkbyB0aGF0Li4u
Lg0KPiA+IA0KPiA+IEhhbmcgb24uLi4gTG9va2luZyBhdCAzLjIsIEkgdGhpbmsgeW91J3JlIGNv
cnJlY3QuIFdlIHVzZWQgdG8gaW5pdGlhbGlzZQ0KPiA+IHRvIDEsIGFuZCBub3cgd2UncmUgaW5p
dGlhbGlzaW5nIHRvIDAuDQo+IA0KPiBObywgY29tbWl0IGFhY2Q1NTM3MjcwYTc1MmZlMTJhOTkx
NGEyMDcyODRmYzIzNDFjNmQgaW5pdGlhbGl6ZXMgYW5kIHJlc2V0cyB0aGUgc2xvdCB0YWJsZXMg
d2l0aCB0aGUgaXZhbHVlIHdoaWNoIGlzIDEgZm9yIHRoZSBmb3JlY2hhbm5lbCBhbmQgMCBmb3Ig
dGhlIGJhY2sgY2hhbm5lbCwganVzdCBhcyBpbiAzLjIuIA0KPiBUbyBiZSBjbGVhciBmcm9tIGFh
Y2Q1NTM3Og0KPiANCj4gLXN0YXRpYyBpbnQgbmZzNF9pbml0X3Nsb3RfdGFibGVzKHN0cnVjdCBu
ZnM0X3Nlc3Npb24gKnNlc3Npb24pDQo+ICtzdGF0aWMgaW50IG5mczRfc2V0dXBfc2Vzc2lvbl9z
bG90X3RhYmxlcyhzdHJ1Y3QgbmZzNF9zZXNzaW9uICpzZXMpDQo+ICB7DQo+ICAgICAgICAgc3Ry
dWN0IG5mczRfc2xvdF90YWJsZSAqdGJsOw0KPiAtICAgICAgIGludCBzdGF0dXMgPSAwOw0KPiAr
ICAgICAgIGludCBzdGF0dXM7DQo+ICANCj4gLSAgICAgICB0YmwgPSAmc2Vzc2lvbi0+ZmNfc2xv
dF90YWJsZTsNCj4gKyAgICAgICBkcHJpbnRrKCItLT4gJXNcbiIsIF9fZnVuY19fKTsNCj4gKyAg
ICAgICAvKiBGb3JlIGNoYW5uZWwgKi8NCj4gKyAgICAgICB0YmwgPSAmc2VzLT5mY19zbG90X3Rh
YmxlOw0KPiAgICAgICAgIGlmICh0YmwtPnNsb3RzID09IE5VTEwpIHsNCj4gLSAgICAgICAgICAg
ICAgIHN0YXR1cyA9IG5mczRfaW5pdF9zbG90X3RhYmxlKHRibCwNCj4gLSAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICBzZXNzaW9uLT5mY19hdHRycy5tYXhfcmVxcywgMSk7DQo+ICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgXl5eXl5eXg0KPiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGl2YWx1ZSBvZiAx
DQo+IA0KPiArICAgICAgICAgICAgICAgc3RhdHVzID0gbmZzNF9pbml0X3Nsb3RfdGFibGUodGJs
LCBzZXMtPmZjX2F0dHJzLm1heF9yZXFzLCAxKTsNCj4gICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBeXl5eXg0KPiAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBpdmFsdWUgb2YgMQ0K
PiArICAgICAgICAgICAgICAgaWYgKHN0YXR1cykgLyogLUVOT01FTSAqLw0KPiArICAgICAgICAg
ICAgICAgICAgICAgICByZXR1cm4gc3RhdHVzOw0KPiArICAgICAgIH0gZWxzZSB7DQo+ICsgICAg
ICAgICAgICAgICBzdGF0dXMgPSBuZnM0X3Jlc2V0X3Nsb3RfdGFibGUodGJsLCBzZXMtPmZjX2F0
dHJzLm1heF9yZXFzLCAxKTsNCj4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICBeXl5eXg0KPiAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBpdmFsdWUgb2YgMQ0KPiAgICAgICAgICAg
ICAgICAgaWYgKHN0YXR1cykNCj4gICAgICAgICAgICAgICAgICAgICAgICAgcmV0dXJuIHN0YXR1
czsNCj4gICAgICAgICB9DQo+IC0NCj4gLSAgICAgICB0YmwgPSAmc2Vzc2lvbi0+YmNfc2xvdF90
YWJsZTsNCj4gKyAgICAgICAvKiBCYWNrIGNoYW5uZWwgKi8NCj4gKyAgICAgICB0YmwgPSAmc2Vz
LT5iY19zbG90X3RhYmxlOw0KPiAgICAgICAgIGlmICh0YmwtPnNsb3RzID09IE5VTEwpIHsNCj4g
LSAgICAgICAgICAgICAgIHN0YXR1cyA9IG5mczRfaW5pdF9zbG90X3RhYmxlKHRibCwNCj4gLSAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICBzZXNzaW9uLT5iY19hdHRycy5tYXhfcmVxcywg
MCk7DQo+ICsgICAgICAgICAgICAgICBzdGF0dXMgPSBuZnM0X2luaXRfc2xvdF90YWJsZSh0Ymws
IHNlcy0+YmNfYXR0cnMubWF4X3JlcXMsIDApOw0KPiAgICAgICAgICAgICAgICAgaWYgKHN0YXR1
cykNCj4gLSAgICAgICAgICAgICAgICAgICAgICAgbmZzNF9kZXN0cm95X3Nsb3RfdGFibGVzKHNl
c3Npb24pOw0KPiAtICAgICAgIH0NCj4gLQ0KPiArICAgICAgICAgICAgICAgICAgICAgICAvKiBG
b3JlIGFuZCBiYWNrIGNoYW5uZWwgc2hhcmUgYSBjb25uZWN0aW9uIHNvIGdldA0KPiArICAgICAg
ICAgICAgICAgICAgICAgICAgKiBib3RoIHNsb3QgdGFibGVzIG9yIG5laXRoZXIgKi8NCj4gKyAg
ICAgICAgICAgICAgICAgICAgICAgbmZzNF9kZXN0cm95X3Nsb3RfdGFibGVzKHNlcyk7DQo+ICsg
ICAgICAgfSBlbHNlDQo+ICsgICAgICAgICAgICAgICBzdGF0dXMgPSBuZnM0X3Jlc2V0X3Nsb3Rf
dGFibGUodGJsLCBzZXMtPmJjX2F0dHJzLm1heF9yZXFzLCAwKTsNCj4gICAgICAgICByZXR1cm4g
c3RhdHVzOw0KPiAgfQ0KDQpZZXMsIGFuZCBpdCB0aGVuIF9yZW1vdmVkXyB0aGUgcHJldmlvdXNs
eSB1bmNvbmRpdGlvbmFsIGNhbGwgdG8NCm5mczRfcmVzZXRfc2xvdF90YWJsZXMoKSBmcm9tIG5m
czRfcHJvY19jcmVhdGVfc2Vzc2lvbigpLiBXaGljaCBtZWFucw0KdGhhdCB0aGUgdGJsLT5zbG90
cyA9PSBOVUxMIHBhdGggbm8gbG9uZ2VyIGluaXRpYWxpc2VzIHRoZSBzbG90IHRhYmxlDQpzZXF1
ZW5jZSBudW1iZXJzLg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBt
YWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRh
cHAuY29tDQoNCg==
Please don't merge nfs4_reset_slot_tables and nfs4_init_slot_tables. It assumes a static array (realloc) which goes away with the dynamic slot table code. Instead, take a look at the patch set I sent on Feb 12
[PATCH Version 7 3/3] NFSv4.1 avoid freeing slot when tasks are waiting
[PATCH Version 1 2/3] NFSv4.1 prepare for dyamic session slots
[PATCH Version 7 1/3] NFS4.1 clean up nfs4_alloc_session
-->Andy
On Feb 14, 2012, at 9:30 PM, Myklebust, Trond wrote:
> On Tue, 2012-02-14 at 20:50 -0500, Trond Myklebust wrote:
>> Hang on... Looking at 3.2, I think you're correct. We used to initialise
>> to 1, and now we're initialising to 0.
>>
>> Ah... I see what happened.
>>
>> commit aacd5537270a752fe12a9914a207284fc2341c6d (NFSv4.1: cleanup init
>> and reset of session slot tables) seems to have removed the call to
>> nfs4_reset_slot_tables() that was doing that initialisation. OK... So
>> this is a Linux 3.3-rcX only regression, and should be treated as
>> such...
>>
>> How about the following instead so that we ensure we fully share the
>> initialisation code between nfs4_reset_slot_tables and
>> nfs4_init_slot_tables?
>>
>
> Here's a version 2 that does a few more cleanups by merging the
> nfs4_init_slot_table() and nfs4_reset_slot_table() into a single
> function.
>
> 8<-------------------------------------------------------------------------
> From dea20ac15be192d0b4c4e367b7b61740ceee8dc4 Mon Sep 17 00:00:00 2001
> From: Trond Myklebust <[email protected]>
> Date: Tue, 14 Feb 2012 20:33:19 -0500
> Subject: [PATCH v2] NFSv4.1: Fix a NFSv4.1 session initialisation regression
>
> Commit aacd553 (NFSv4.1: cleanup init and reset of session slot tables)
> introduces a regression in the session initialisation code. New tables
> now find their sequence ids initialised to 0, rather than the mandated
> value of 1 (see RFC5661).
>
> Fix the problem by merging nfs4_reset_slot_table() and nfs4_init_slot_table().
> Since the tbl->max_slots is initialised to 0, the test in
> nfs4_reset_slot_table for max_reqs != tbl->max_slots will automatically
> pass for an empty table.
>
> Reported-by: Vitaliy Gusev <[email protected]>
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> fs/nfs/nfs4proc.c | 109 ++++++++++++++++++++--------------------------------
> 1 files changed, 42 insertions(+), 67 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index d202e04..1684c9b 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5008,37 +5008,53 @@ int nfs4_proc_get_lease_time(struct nfs_client *clp, struct nfs_fsinfo *fsinfo)
> return status;
> }
>
> +static struct nfs4_slot *nfs4_alloc_slots(u32 max_slots, gfp_t gfp_flags)
> +{
> + return kcalloc(max_slots, sizeof(struct nfs4_slot), gfp_flags);
> +}
> +
> +static void nfs4_add_and_init_slots(struct nfs4_slot_table *tbl,
> + struct nfs4_slot *new,
> + u32 max_slots,
> + u32 ivalue)
> +{
> + struct nfs4_slot *old = NULL;
> + u32 i;
> +
> + spin_lock(&tbl->slot_tbl_lock);
> + if (new) {
> + old = tbl->slots;
> + tbl->slots = new;
> + tbl->max_slots = max_slots;
> + }
> + tbl->highest_used_slotid = -1; /* no slot is currently used */
> + for (i = 0; i < tbl->max_slots; i++)
> + tbl->slots[i].seq_nr = ivalue;
> + spin_unlock(&tbl->slot_tbl_lock);
> + kfree(old);
> +}
> +
> /*
> - * Reset a slot table
> + * (re)Initialise a slot table
> */
> -static int nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 max_reqs,
> - int ivalue)
> +static int nfs4_realloc_slot_table(struct nfs4_slot_table *tbl, u32 max_reqs,
> + u32 ivalue)
> {
> struct nfs4_slot *new = NULL;
> - int i;
> - int ret = 0;
> + int ret = -ENOMEM;
>
> dprintk("--> %s: max_reqs=%u, tbl->max_slots %d\n", __func__,
> max_reqs, tbl->max_slots);
>
> /* Does the newly negotiated max_reqs match the existing slot table? */
> if (max_reqs != tbl->max_slots) {
> - ret = -ENOMEM;
> - new = kmalloc(max_reqs * sizeof(struct nfs4_slot),
> - GFP_NOFS);
> + new = nfs4_alloc_slots(max_reqs, GFP_NOFS);
> if (!new)
> goto out;
> - ret = 0;
> - kfree(tbl->slots);
> - }
> - spin_lock(&tbl->slot_tbl_lock);
> - if (new) {
> - tbl->slots = new;
> - tbl->max_slots = max_reqs;
> }
> - for (i = 0; i < tbl->max_slots; ++i)
> - tbl->slots[i].seq_nr = ivalue;
> - spin_unlock(&tbl->slot_tbl_lock);
> + ret = 0;
> +
> + nfs4_add_and_init_slots(tbl, new, max_reqs, ivalue);
> dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
> tbl, tbl->slots, tbl->max_slots);
> out:
> @@ -5061,36 +5077,6 @@ static void nfs4_destroy_slot_tables(struct nfs4_session *session)
> }
>
> /*
> - * Initialize slot table
> - */
> -static int nfs4_init_slot_table(struct nfs4_slot_table *tbl,
> - int max_slots, int ivalue)
> -{
> - struct nfs4_slot *slot;
> - int ret = -ENOMEM;
> -
> - BUG_ON(max_slots > NFS4_MAX_SLOT_TABLE);
> -
> - dprintk("--> %s: max_reqs=%u\n", __func__, max_slots);
> -
> - slot = kcalloc(max_slots, sizeof(struct nfs4_slot), GFP_NOFS);
> - if (!slot)
> - goto out;
> - ret = 0;
> -
> - spin_lock(&tbl->slot_tbl_lock);
> - tbl->max_slots = max_slots;
> - tbl->slots = slot;
> - tbl->highest_used_slotid = -1; /* no slot is currently used */
> - spin_unlock(&tbl->slot_tbl_lock);
> - dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
> - tbl, tbl->slots, tbl->max_slots);
> -out:
> - dprintk("<-- %s: return %d\n", __func__, ret);
> - return ret;
> -}
> -
> -/*
> * Initialize or reset the forechannel and backchannel tables
> */
> static int nfs4_setup_session_slot_tables(struct nfs4_session *ses)
> @@ -5101,25 +5087,16 @@ static int nfs4_setup_session_slot_tables(struct nfs4_session *ses)
> dprintk("--> %s\n", __func__);
> /* Fore channel */
> tbl = &ses->fc_slot_table;
> - if (tbl->slots == NULL) {
> - status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
> - if (status) /* -ENOMEM */
> - return status;
> - } else {
> - status = nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
> - if (status)
> - return status;
> - }
> + status = nfs4_realloc_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
> + if (status) /* -ENOMEM */
> + return status;
> /* Back channel */
> tbl = &ses->bc_slot_table;
> - if (tbl->slots == NULL) {
> - status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
> - if (status)
> - /* Fore and back channel share a connection so get
> - * both slot tables or neither */
> - nfs4_destroy_slot_tables(ses);
> - } else
> - status = nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
> + status = nfs4_realloc_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
> + if (status && tbl->slots == NULL)
> + /* Fore and back channel share a connection so get
> + * both slot tables or neither */
> + nfs4_destroy_slot_tables(ses);
> return status;
> }
>
> @@ -5133,13 +5110,11 @@ struct nfs4_session *nfs4_alloc_session(struct nfs_client *clp)
> return NULL;
>
> tbl = &session->fc_slot_table;
> - tbl->highest_used_slotid = -1;
> spin_lock_init(&tbl->slot_tbl_lock);
> rpc_init_priority_wait_queue(&tbl->slot_tbl_waitq, "ForeChannel Slot table");
> init_completion(&tbl->complete);
>
> tbl = &session->bc_slot_table;
> - tbl->highest_used_slotid = -1;
> spin_lock_init(&tbl->slot_tbl_lock);
> rpc_init_wait_queue(&tbl->slot_tbl_waitq, "BackChannel Slot table");
> init_completion(&tbl->complete);
> --
> 1.7.7.6
>
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com
>
On Wed, Feb 15, 2012 at 10:46 AM, Myklebust, Trond
<[email protected]> wrote:
> On Wed, 2012-02-15 at 15:07 +0000, Adamson, Andy wrote:
>>
>> Please don't merge nfs4_reset_slot_tables and nfs4_init_slot_tables. ?It ?assumes a static array (realloc) which goes away with the dynamic slot table code. Instead, take a look at the patch set I sent on Feb 12
>
> Why not? There is _no_ functional difference between what
> nfs4_reset_slot_tables() and nfs4_init_slot_tables() need to do.
> They both need to allocate new slots (conditionally in the case of
> nfs4_reset_slot_tables, but the condition is compatible with the
> nfs4_init_slot_tables case), and they both need to initialise those
> slots to the value '1'.
The reset case also needs to reduce the number of slots. In the
dynamic slot case, the reset function will not require draining of the
session. So reset will, in response to a change in the
target_highest_slotid or a CB_RECALL_SLOT from the server, add or
remove slots without draining the session. In this case, reset will
only initialize any new slots sequence numbers to 1, not all slots.
That said, I guess it can all be in one function.
>
> AFAICS There is no reason for keeping those as 2 separate functions, and
> I don't see how the dynamic session patches change anything to that
> conclusion.
>
>> [PATCH Version 7 3/3] NFSv4.1 avoid freeing slot when tasks are waiting
>> [PATCH Version 1 2/3] NFSv4.1 prepare for dyamic session slots
>> [PATCH Version 7 1/3] NFS4.1 clean up nfs4_alloc_session
>
> I need a fix for the 3.3 final...
>
> Those patches can be cleaned up and made ready for 3.4 (needs work -
> they won't apply to the 'nfs-for-next' branch),
I worked against the bugfix branch. I'll use the nfs-for-next branch.
> but right now they're
> not sufficiently tested nor are they sufficiently reviewed.
Agreed. I want to test some more at connectathon. I did iozone tests
to show that this dynamic slot implementation performs as well as the
static array, but not enough testing / review for coding bugs.
> For
> instance, I'm not happy with the idea of adding a 'tk_private' field in
> the struct rpc_task.
OK. Do you have a suggestion?
Thanks
-->Andy
>
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com
>
On Feb 15, 2012, at 1:37 PM, Myklebust, Trond wrote:
> On Wed, 2012-02-15 at 11:06 -0500, Andy Adamson wrote:
>> On Wed, Feb 15, 2012 at 10:46 AM, Myklebust, Trond
>> <[email protected]> wrote:
>>> On Wed, 2012-02-15 at 15:07 +0000, Adamson, Andy wrote:
>>>>
>>>> Please don't merge nfs4_reset_slot_tables and nfs4_init_slot_tables. It assumes a static array (realloc) which goes away with the dynamic slot table code. Instead, take a look at the patch set I sent on Feb 12
>>>
>>> Why not? There is _no_ functional difference between what
>>> nfs4_reset_slot_tables() and nfs4_init_slot_tables() need to do.
>>> They both need to allocate new slots (conditionally in the case of
>>> nfs4_reset_slot_tables, but the condition is compatible with the
>>> nfs4_init_slot_tables case), and they both need to initialise those
>>> slots to the value '1'.
>>
>> The reset case also needs to reduce the number of slots. In the
>> dynamic slot case, the reset function will not require draining of the
>> session. So reset will, in response to a change in the
>> target_highest_slotid or a CB_RECALL_SLOT from the server, add or
>> remove slots without draining the session. In this case, reset will
>> only initialize any new slots sequence numbers to 1, not all slots.
>>
>> That said, I guess it can all be in one function.
>>
>>>
>>> AFAICS There is no reason for keeping those as 2 separate functions, and
>>> I don't see how the dynamic session patches change anything to that
>>> conclusion.
>>>
>>>> [PATCH Version 7 3/3] NFSv4.1 avoid freeing slot when tasks are waiting
>>>> [PATCH Version 1 2/3] NFSv4.1 prepare for dyamic session slots
>>>> [PATCH Version 7 1/3] NFS4.1 clean up nfs4_alloc_session
>>>
>>> I need a fix for the 3.3 final...
>>>
>>> Those patches can be cleaned up and made ready for 3.4 (needs work -
>>> they won't apply to the 'nfs-for-next' branch),
>>
>> I worked against the bugfix branch. I'll use the nfs-for-next branch.
>>
>>> but right now they're
>>> not sufficiently tested nor are they sufficiently reviewed.
>>
>> Agreed. I want to test some more at connectathon. I did iozone tests
>> to show that this dynamic slot implementation performs as well as the
>> static array, but not enough testing / review for coding bugs.
>>
>>> For
>>> instance, I'm not happy with the idea of adding a 'tk_private' field in
>>> the struct rpc_task.
>>
>> OK. Do you have a suggestion?
>
> Yes. Please see how we solve the minor version 0 equivalent problem in
> nfs_release_seqid().
>
> In the case of minor version 1, I suggest setting up a dedicated linked
> list of 'things waiting for a slot', where each entry in the linked list
> contains the nfs4_sequence_args/nfs4_sequence_res to initialise, and
> then a pointer to the rpc_task to wake up.
OK - thanks for the advice.
-->Andy
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com
>
On 02/15/2012 07:01 PM, Adamson, Andy wrote:
>
> No, commit aacd5537270a752fe12a9914a207284fc2341c6d initializes and resets the slot tables with the ivalue
> which is 1 for the forechannel and 0 for the back channel, just as in 3.2.
nfs4_init_slot_table() doesn't use ivalue at all.
>
> To be clear from aacd5537:
>
> -static int nfs4_init_slot_tables(struct nfs4_session *session)
> +static int nfs4_setup_session_slot_tables(struct nfs4_session *ses)
> {
> struct nfs4_slot_table *tbl;
> - int status = 0;
> + int status;
>
> - tbl =&session->fc_slot_table;
> + dprintk("--> %s\n", __func__);
> + /* Fore channel */
> + tbl =&ses->fc_slot_table;
> if (tbl->slots == NULL) {
> - status = nfs4_init_slot_table(tbl,
> - session->fc_attrs.max_reqs, 1);
> ^^^^^^^
> ivalue of 1
>
On Wed, Feb 15, 2012 at 12:43:08AM +0000, Myklebust, Trond wrote:
> Then we have a problem. The zero initialisation is already in use out
> there in both commercial and non-commercial versions of Linux. It is too
> late to change that now.
>
> Furthermore, since none of the servers we've tested against in earlier
> Bakeathons and Connectathons have complained, I suggest that we rather
> change the spec with an errata.
Argh.
I just noticed that the server was crashing intermittently and traced it
to incorrect handling of the case where the client sends a one-op
SEQUENCE compound with seqid 0. I'm not sure why it started popping up
just in 3.3--perhaps some change in the way the client uses slots made
that more likely.
The server code actually looks like it did assume initial seqid 1, but
accepted initial seqid 0 (except in this one case) basically by mistake.
In any case, I think it should be easy enough to teach it just to accept
any seqid on a previously unused slot, so I'll do that....
--b.
T24gVHVlLCAyMDEyLTAyLTE0IGF0IDIwOjUwIC0wNTAwLCBUcm9uZCBNeWtsZWJ1c3Qgd3JvdGU6
DQo+IEhhbmcgb24uLi4gTG9va2luZyBhdCAzLjIsIEkgdGhpbmsgeW91J3JlIGNvcnJlY3QuIFdl
IHVzZWQgdG8gaW5pdGlhbGlzZQ0KPiB0byAxLCBhbmQgbm93IHdlJ3JlIGluaXRpYWxpc2luZyB0
byAwLg0KPiANCj4gQWguLi4gSSBzZWUgd2hhdCBoYXBwZW5lZC4NCj4gDQo+IGNvbW1pdCBhYWNk
NTUzNzI3MGE3NTJmZTEyYTk5MTRhMjA3Mjg0ZmMyMzQxYzZkIChORlN2NC4xOiBjbGVhbnVwIGlu
aXQNCj4gYW5kIHJlc2V0IG9mIHNlc3Npb24gc2xvdCB0YWJsZXMpIHNlZW1zIHRvIGhhdmUgcmVt
b3ZlZCB0aGUgY2FsbCB0bw0KPiBuZnM0X3Jlc2V0X3Nsb3RfdGFibGVzKCkgdGhhdCB3YXMgZG9p
bmcgdGhhdCBpbml0aWFsaXNhdGlvbi4gT0suLi4gU28NCj4gdGhpcyBpcyBhIExpbnV4IDMuMy1y
Y1ggb25seSByZWdyZXNzaW9uLCBhbmQgc2hvdWxkIGJlIHRyZWF0ZWQgYXMNCj4gc3VjaC4uLg0K
PiANCj4gSG93IGFib3V0IHRoZSBmb2xsb3dpbmcgaW5zdGVhZCBzbyB0aGF0IHdlIGVuc3VyZSB3
ZSBmdWxseSBzaGFyZSB0aGUNCj4gaW5pdGlhbGlzYXRpb24gY29kZSBiZXR3ZWVuIG5mczRfcmVz
ZXRfc2xvdF90YWJsZXMgYW5kDQo+IG5mczRfaW5pdF9zbG90X3RhYmxlcz8NCj4gDQoNCkhlcmUn
cyBhIHZlcnNpb24gMiB0aGF0IGRvZXMgYSBmZXcgbW9yZSBjbGVhbnVwcyBieSBtZXJnaW5nIHRo
ZQ0KbmZzNF9pbml0X3Nsb3RfdGFibGUoKSBhbmQgbmZzNF9yZXNldF9zbG90X3RhYmxlKCkgaW50
byBhIHNpbmdsZQ0KZnVuY3Rpb24uDQoNCjg8LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KRnJvbSBkZWEyMGFj
MTViZTE5MmQwYjRjNGUzNjdiN2I2MTc0MGNlZWU4ZGM0IE1vbiBTZXAgMTcgMDA6MDA6MDAgMjAw
MQ0KRnJvbTogVHJvbmQgTXlrbGVidXN0IDxUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbT4NCkRh
dGU6IFR1ZSwgMTQgRmViIDIwMTIgMjA6MzM6MTkgLTA1MDANClN1YmplY3Q6IFtQQVRDSCB2Ml0g
TkZTdjQuMTogRml4IGEgTkZTdjQuMSBzZXNzaW9uIGluaXRpYWxpc2F0aW9uIHJlZ3Jlc3Npb24N
Cg0KQ29tbWl0IGFhY2Q1NTMgKE5GU3Y0LjE6IGNsZWFudXAgaW5pdCBhbmQgcmVzZXQgb2Ygc2Vz
c2lvbiBzbG90IHRhYmxlcykNCmludHJvZHVjZXMgYSByZWdyZXNzaW9uIGluIHRoZSBzZXNzaW9u
IGluaXRpYWxpc2F0aW9uIGNvZGUuIE5ldyB0YWJsZXMNCm5vdyBmaW5kIHRoZWlyIHNlcXVlbmNl
IGlkcyBpbml0aWFsaXNlZCB0byAwLCByYXRoZXIgdGhhbiB0aGUgbWFuZGF0ZWQNCnZhbHVlIG9m
IDEgKHNlZSBSRkM1NjYxKS4NCg0KRml4IHRoZSBwcm9ibGVtIGJ5IG1lcmdpbmcgbmZzNF9yZXNl
dF9zbG90X3RhYmxlKCkgYW5kIG5mczRfaW5pdF9zbG90X3RhYmxlKCkuDQpTaW5jZSB0aGUgdGJs
LT5tYXhfc2xvdHMgaXMgaW5pdGlhbGlzZWQgdG8gMCwgdGhlIHRlc3QgaW4NCm5mczRfcmVzZXRf
c2xvdF90YWJsZSBmb3IgbWF4X3JlcXMgIT0gdGJsLT5tYXhfc2xvdHMgd2lsbCBhdXRvbWF0aWNh
bGx5DQpwYXNzIGZvciBhbiBlbXB0eSB0YWJsZS4NCg0KUmVwb3J0ZWQtYnk6IFZpdGFsaXkgR3Vz
ZXYgPGd1c2V2LnZpdGFsaXlAbmV4ZW50YS5jb20+DQpTaWduZWQtb2ZmLWJ5OiBUcm9uZCBNeWts
ZWJ1c3QgPFRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tPg0KLS0tDQogZnMvbmZzL25mczRwcm9j
LmMgfCAgMTA5ICsrKysrKysrKysrKysrKysrKysrLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0NCiAxIGZpbGVzIGNoYW5nZWQsIDQyIGluc2VydGlvbnMoKyksIDY3IGRlbGV0aW9ucygt
KQ0KDQpkaWZmIC0tZ2l0IGEvZnMvbmZzL25mczRwcm9jLmMgYi9mcy9uZnMvbmZzNHByb2MuYw0K
aW5kZXggZDIwMmUwNC4uMTY4NGM5YiAxMDA2NDQNCi0tLSBhL2ZzL25mcy9uZnM0cHJvYy5jDQor
KysgYi9mcy9uZnMvbmZzNHByb2MuYw0KQEAgLTUwMDgsMzcgKzUwMDgsNTMgQEAgaW50IG5mczRf
cHJvY19nZXRfbGVhc2VfdGltZShzdHJ1Y3QgbmZzX2NsaWVudCAqY2xwLCBzdHJ1Y3QgbmZzX2Zz
aW5mbyAqZnNpbmZvKQ0KIAlyZXR1cm4gc3RhdHVzOw0KIH0NCiANCitzdGF0aWMgc3RydWN0IG5m
czRfc2xvdCAqbmZzNF9hbGxvY19zbG90cyh1MzIgbWF4X3Nsb3RzLCBnZnBfdCBnZnBfZmxhZ3Mp
DQorew0KKwlyZXR1cm4ga2NhbGxvYyhtYXhfc2xvdHMsIHNpemVvZihzdHJ1Y3QgbmZzNF9zbG90
KSwgZ2ZwX2ZsYWdzKTsNCit9DQorDQorc3RhdGljIHZvaWQgbmZzNF9hZGRfYW5kX2luaXRfc2xv
dHMoc3RydWN0IG5mczRfc2xvdF90YWJsZSAqdGJsLA0KKwkJc3RydWN0IG5mczRfc2xvdCAqbmV3
LA0KKwkJdTMyIG1heF9zbG90cywNCisJCXUzMiBpdmFsdWUpDQorew0KKwlzdHJ1Y3QgbmZzNF9z
bG90ICpvbGQgPSBOVUxMOw0KKwl1MzIgaTsNCisNCisJc3Bpbl9sb2NrKCZ0YmwtPnNsb3RfdGJs
X2xvY2spOw0KKwlpZiAobmV3KSB7DQorCQlvbGQgPSB0YmwtPnNsb3RzOw0KKwkJdGJsLT5zbG90
cyA9IG5ldzsNCisJCXRibC0+bWF4X3Nsb3RzID0gbWF4X3Nsb3RzOw0KKwl9DQorCXRibC0+aGln
aGVzdF91c2VkX3Nsb3RpZCA9IC0xOwkvKiBubyBzbG90IGlzIGN1cnJlbnRseSB1c2VkICovDQor
CWZvciAoaSA9IDA7IGkgPCB0YmwtPm1heF9zbG90czsgaSsrKQ0KKwkJdGJsLT5zbG90c1tpXS5z
ZXFfbnIgPSBpdmFsdWU7DQorCXNwaW5fdW5sb2NrKCZ0YmwtPnNsb3RfdGJsX2xvY2spOw0KKwlr
ZnJlZShvbGQpOw0KK30NCisNCiAvKg0KLSAqIFJlc2V0IGEgc2xvdCB0YWJsZQ0KKyAqIChyZSlJ
bml0aWFsaXNlIGEgc2xvdCB0YWJsZQ0KICAqLw0KLXN0YXRpYyBpbnQgbmZzNF9yZXNldF9zbG90
X3RhYmxlKHN0cnVjdCBuZnM0X3Nsb3RfdGFibGUgKnRibCwgdTMyIG1heF9yZXFzLA0KLQkJCQkg
aW50IGl2YWx1ZSkNCitzdGF0aWMgaW50IG5mczRfcmVhbGxvY19zbG90X3RhYmxlKHN0cnVjdCBu
ZnM0X3Nsb3RfdGFibGUgKnRibCwgdTMyIG1heF9yZXFzLA0KKwkJCQkgdTMyIGl2YWx1ZSkNCiB7
DQogCXN0cnVjdCBuZnM0X3Nsb3QgKm5ldyA9IE5VTEw7DQotCWludCBpOw0KLQlpbnQgcmV0ID0g
MDsNCisJaW50IHJldCA9IC1FTk9NRU07DQogDQogCWRwcmludGsoIi0tPiAlczogbWF4X3JlcXM9
JXUsIHRibC0+bWF4X3Nsb3RzICVkXG4iLCBfX2Z1bmNfXywNCiAJCW1heF9yZXFzLCB0YmwtPm1h
eF9zbG90cyk7DQogDQogCS8qIERvZXMgdGhlIG5ld2x5IG5lZ290aWF0ZWQgbWF4X3JlcXMgbWF0
Y2ggdGhlIGV4aXN0aW5nIHNsb3QgdGFibGU/ICovDQogCWlmIChtYXhfcmVxcyAhPSB0YmwtPm1h
eF9zbG90cykgew0KLQkJcmV0ID0gLUVOT01FTTsNCi0JCW5ldyA9IGttYWxsb2MobWF4X3JlcXMg
KiBzaXplb2Yoc3RydWN0IG5mczRfc2xvdCksDQotCQkJICAgICAgR0ZQX05PRlMpOw0KKwkJbmV3
ID0gbmZzNF9hbGxvY19zbG90cyhtYXhfcmVxcywgR0ZQX05PRlMpOw0KIAkJaWYgKCFuZXcpDQog
CQkJZ290byBvdXQ7DQotCQlyZXQgPSAwOw0KLQkJa2ZyZWUodGJsLT5zbG90cyk7DQotCX0NCi0J
c3Bpbl9sb2NrKCZ0YmwtPnNsb3RfdGJsX2xvY2spOw0KLQlpZiAobmV3KSB7DQotCQl0YmwtPnNs
b3RzID0gbmV3Ow0KLQkJdGJsLT5tYXhfc2xvdHMgPSBtYXhfcmVxczsNCiAJfQ0KLQlmb3IgKGkg
PSAwOyBpIDwgdGJsLT5tYXhfc2xvdHM7ICsraSkNCi0JCXRibC0+c2xvdHNbaV0uc2VxX25yID0g
aXZhbHVlOw0KLQlzcGluX3VubG9jaygmdGJsLT5zbG90X3RibF9sb2NrKTsNCisJcmV0ID0gMDsN
CisNCisJbmZzNF9hZGRfYW5kX2luaXRfc2xvdHModGJsLCBuZXcsIG1heF9yZXFzLCBpdmFsdWUp
Ow0KIAlkcHJpbnRrKCIlczogdGJsPSVwIHNsb3RzPSVwIG1heF9zbG90cz0lZFxuIiwgX19mdW5j
X18sDQogCQl0YmwsIHRibC0+c2xvdHMsIHRibC0+bWF4X3Nsb3RzKTsNCiBvdXQ6DQpAQCAtNTA2
MSwzNiArNTA3Nyw2IEBAIHN0YXRpYyB2b2lkIG5mczRfZGVzdHJveV9zbG90X3RhYmxlcyhzdHJ1
Y3QgbmZzNF9zZXNzaW9uICpzZXNzaW9uKQ0KIH0NCiANCiAvKg0KLSAqIEluaXRpYWxpemUgc2xv
dCB0YWJsZQ0KLSAqLw0KLXN0YXRpYyBpbnQgbmZzNF9pbml0X3Nsb3RfdGFibGUoc3RydWN0IG5m
czRfc2xvdF90YWJsZSAqdGJsLA0KLQkJaW50IG1heF9zbG90cywgaW50IGl2YWx1ZSkNCi17DQot
CXN0cnVjdCBuZnM0X3Nsb3QgKnNsb3Q7DQotCWludCByZXQgPSAtRU5PTUVNOw0KLQ0KLQlCVUdf
T04obWF4X3Nsb3RzID4gTkZTNF9NQVhfU0xPVF9UQUJMRSk7DQotDQotCWRwcmludGsoIi0tPiAl
czogbWF4X3JlcXM9JXVcbiIsIF9fZnVuY19fLCBtYXhfc2xvdHMpOw0KLQ0KLQlzbG90ID0ga2Nh
bGxvYyhtYXhfc2xvdHMsIHNpemVvZihzdHJ1Y3QgbmZzNF9zbG90KSwgR0ZQX05PRlMpOw0KLQlp
ZiAoIXNsb3QpDQotCQlnb3RvIG91dDsNCi0JcmV0ID0gMDsNCi0NCi0Jc3Bpbl9sb2NrKCZ0Ymwt
PnNsb3RfdGJsX2xvY2spOw0KLQl0YmwtPm1heF9zbG90cyA9IG1heF9zbG90czsNCi0JdGJsLT5z
bG90cyA9IHNsb3Q7DQotCXRibC0+aGlnaGVzdF91c2VkX3Nsb3RpZCA9IC0xOyAgLyogbm8gc2xv
dCBpcyBjdXJyZW50bHkgdXNlZCAqLw0KLQlzcGluX3VubG9jaygmdGJsLT5zbG90X3RibF9sb2Nr
KTsNCi0JZHByaW50aygiJXM6IHRibD0lcCBzbG90cz0lcCBtYXhfc2xvdHM9JWRcbiIsIF9fZnVu
Y19fLA0KLQkJdGJsLCB0YmwtPnNsb3RzLCB0YmwtPm1heF9zbG90cyk7DQotb3V0Og0KLQlkcHJp
bnRrKCI8LS0gJXM6IHJldHVybiAlZFxuIiwgX19mdW5jX18sIHJldCk7DQotCXJldHVybiByZXQ7
DQotfQ0KLQ0KLS8qDQogICogSW5pdGlhbGl6ZSBvciByZXNldCB0aGUgZm9yZWNoYW5uZWwgYW5k
IGJhY2tjaGFubmVsIHRhYmxlcw0KICAqLw0KIHN0YXRpYyBpbnQgbmZzNF9zZXR1cF9zZXNzaW9u
X3Nsb3RfdGFibGVzKHN0cnVjdCBuZnM0X3Nlc3Npb24gKnNlcykNCkBAIC01MTAxLDI1ICs1MDg3
LDE2IEBAIHN0YXRpYyBpbnQgbmZzNF9zZXR1cF9zZXNzaW9uX3Nsb3RfdGFibGVzKHN0cnVjdCBu
ZnM0X3Nlc3Npb24gKnNlcykNCiAJZHByaW50aygiLS0+ICVzXG4iLCBfX2Z1bmNfXyk7DQogCS8q
IEZvcmUgY2hhbm5lbCAqLw0KIAl0YmwgPSAmc2VzLT5mY19zbG90X3RhYmxlOw0KLQlpZiAodGJs
LT5zbG90cyA9PSBOVUxMKSB7DQotCQlzdGF0dXMgPSBuZnM0X2luaXRfc2xvdF90YWJsZSh0Ymws
IHNlcy0+ZmNfYXR0cnMubWF4X3JlcXMsIDEpOw0KLQkJaWYgKHN0YXR1cykgLyogLUVOT01FTSAq
Lw0KLQkJCXJldHVybiBzdGF0dXM7DQotCX0gZWxzZSB7DQotCQlzdGF0dXMgPSBuZnM0X3Jlc2V0
X3Nsb3RfdGFibGUodGJsLCBzZXMtPmZjX2F0dHJzLm1heF9yZXFzLCAxKTsNCi0JCWlmIChzdGF0
dXMpDQotCQkJcmV0dXJuIHN0YXR1czsNCi0JfQ0KKwlzdGF0dXMgPSBuZnM0X3JlYWxsb2Nfc2xv
dF90YWJsZSh0YmwsIHNlcy0+ZmNfYXR0cnMubWF4X3JlcXMsIDEpOw0KKwlpZiAoc3RhdHVzKSAv
KiAtRU5PTUVNICovDQorCQlyZXR1cm4gc3RhdHVzOw0KIAkvKiBCYWNrIGNoYW5uZWwgKi8NCiAJ
dGJsID0gJnNlcy0+YmNfc2xvdF90YWJsZTsNCi0JaWYgKHRibC0+c2xvdHMgPT0gTlVMTCkgew0K
LQkJc3RhdHVzID0gbmZzNF9pbml0X3Nsb3RfdGFibGUodGJsLCBzZXMtPmJjX2F0dHJzLm1heF9y
ZXFzLCAwKTsNCi0JCWlmIChzdGF0dXMpDQotCQkJLyogRm9yZSBhbmQgYmFjayBjaGFubmVsIHNo
YXJlIGEgY29ubmVjdGlvbiBzbyBnZXQNCi0JCQkgKiBib3RoIHNsb3QgdGFibGVzIG9yIG5laXRo
ZXIgKi8NCi0JCQluZnM0X2Rlc3Ryb3lfc2xvdF90YWJsZXMoc2VzKTsNCi0JfSBlbHNlDQotCQlz
dGF0dXMgPSBuZnM0X3Jlc2V0X3Nsb3RfdGFibGUodGJsLCBzZXMtPmJjX2F0dHJzLm1heF9yZXFz
LCAwKTsNCisJc3RhdHVzID0gbmZzNF9yZWFsbG9jX3Nsb3RfdGFibGUodGJsLCBzZXMtPmJjX2F0
dHJzLm1heF9yZXFzLCAwKTsNCisJaWYgKHN0YXR1cyAmJiB0YmwtPnNsb3RzID09IE5VTEwpDQor
CQkvKiBGb3JlIGFuZCBiYWNrIGNoYW5uZWwgc2hhcmUgYSBjb25uZWN0aW9uIHNvIGdldA0KKwkJ
ICogYm90aCBzbG90IHRhYmxlcyBvciBuZWl0aGVyICovDQorCQluZnM0X2Rlc3Ryb3lfc2xvdF90
YWJsZXMoc2VzKTsNCiAJcmV0dXJuIHN0YXR1czsNCiB9DQogDQpAQCAtNTEzMywxMyArNTExMCwx
MSBAQCBzdHJ1Y3QgbmZzNF9zZXNzaW9uICpuZnM0X2FsbG9jX3Nlc3Npb24oc3RydWN0IG5mc19j
bGllbnQgKmNscCkNCiAJCXJldHVybiBOVUxMOw0KIA0KIAl0YmwgPSAmc2Vzc2lvbi0+ZmNfc2xv
dF90YWJsZTsNCi0JdGJsLT5oaWdoZXN0X3VzZWRfc2xvdGlkID0gLTE7DQogCXNwaW5fbG9ja19p
bml0KCZ0YmwtPnNsb3RfdGJsX2xvY2spOw0KIAlycGNfaW5pdF9wcmlvcml0eV93YWl0X3F1ZXVl
KCZ0YmwtPnNsb3RfdGJsX3dhaXRxLCAiRm9yZUNoYW5uZWwgU2xvdCB0YWJsZSIpOw0KIAlpbml0
X2NvbXBsZXRpb24oJnRibC0+Y29tcGxldGUpOw0KIA0KIAl0YmwgPSAmc2Vzc2lvbi0+YmNfc2xv
dF90YWJsZTsNCi0JdGJsLT5oaWdoZXN0X3VzZWRfc2xvdGlkID0gLTE7DQogCXNwaW5fbG9ja19p
bml0KCZ0YmwtPnNsb3RfdGJsX2xvY2spOw0KIAlycGNfaW5pdF93YWl0X3F1ZXVlKCZ0YmwtPnNs
b3RfdGJsX3dhaXRxLCAiQmFja0NoYW5uZWwgU2xvdCB0YWJsZSIpOw0KIAlpbml0X2NvbXBsZXRp
b24oJnRibC0+Y29tcGxldGUpOw0KLS0gDQoxLjcuNy42DQoNCg0KLS0gDQpUcm9uZCBNeWtsZWJ1
c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVz
dEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNvbQ0KDQo=
On Wed, Feb 15, 2012 at 01:50:28AM +0000, Myklebust, Trond wrote:
> On Tue, 2012-02-14 at 19:53 -0500, J. Bruce Fields wrote:
> > On Wed, Feb 15, 2012 at 12:43:08AM +0000, Myklebust, Trond wrote:
> > > Then we have a problem. The zero initialisation is already in use out
> > > there in both commercial and non-commercial versions of Linux. It is too
> > > late to change that now.
> > >
> > > Furthermore, since none of the servers we've tested against in earlier
> > > Bakeathons and Connectathons have complained, I suggest that we rather
> > > change the spec with an errata.
> >
> > Argh.
> >
> > I just noticed that the server was crashing intermittently and traced it
> > to incorrect handling of the case where the client sends a one-op
> > SEQUENCE compound with seqid 0. I'm not sure why it started popping up
> > just in 3.3--perhaps some change in the way the client uses slots made
> > that more likely.
> >
> > The server code actually looks like it did assume initial seqid 1, but
> > accepted initial seqid 0 (except in this one case) basically by mistake.
> >
> > In any case, I think it should be easy enough to teach it just to accept
> > any seqid on a previously unused slot, so I'll do that....
>
> Hang on... Looking at 3.2, I think you're correct. We used to initialise
> to 1, and now we're initialising to 0.
OK. I'll continue with my current fix, in that case, so using initial
sequenceid 0 will result in a MISORDERED error.
--b.
Looks good.
However here MUST be checking on big value of ses->fc_attrs.max_reqs and
ses->bc_attrs.max_reqs. I'll send it soon.
On 02/15/2012 06:30 AM, Myklebust, Trond wrote:
>
> Here's a version 2 that does a few more cleanups by merging the
> nfs4_init_slot_table() and nfs4_reset_slot_table() into a single
> function.
>
On 02/15/2012 07:16 PM, Andy Adamson wrote:
>>> No, commit aacd5537270a752fe12a9914a207284fc2341c6d initializes and resets
>>> the slot tables with the ivalue
>>> which is 1 for the forechannel and 0 for the back channel, just as in 3.2.
>>
>>
>>
>> nfs4_init_slot_table() doesn't use ivalue at all.
>
> Yep - that is the bug - which is in v2.6.39, and v3.2 ....
This bug was introduced by commit aacd5537270