| Message ID | 20120709154409.1604.59004.stgit@degas.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
T24gTW9uLCAyMDEyLTA3LTA5IGF0IDExOjQ0IC0wNDAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4g QmVmb3JlIGJlZ2lubmluZyBzdGF0ZSByZWNvdmVyeSwgdGhlIHN0YXRlIG1hbmFnZXIgemFwcyBv cGVuIGFuZCBsb2NrDQo+IHN0YXRlIGZvciBlYWNoIG9wZW4gZmlsZSwgdGh1cyB0aGUgInN0YXRl LT5mbGFncyAmIGZsYWdzIiB0ZXN0IGluDQo+IG5mczQxX3tvcGVuLGxvY2t9X2V4cGlyZWQoKSBh bHdheXMgZmFpbHMgZHVyaW5nIHJlY2xhaW0uICBCdXQgb3Blbg0KPiByZWNvdmVyeSBpcyBzdGls bCBuZWVkZWQgZm9yIHRoZXNlIGZpbGVzLg0KPiANCj4gVG8gZm9yY2UgYSBjYWxsIHRvIG5mczRf b3Blbl9leHBpcmVkKCksIGNoYW5nZSB0aGUgZGVmYXVsdCByZXR1cm4NCj4gdmFsdWUgZm9yIG5m czQxX2NoZWNrX2V4cGlyZWRfc3RhdGVpZCgpIHRvIGZvcmNlIG9wZW4gcmVjb3ZlcnksIGFuZA0K PiB0aGUgZGVmYXVsdCByZXR1cm4gdmFsdWUgZm9yIG5mczQxX2NoZWNrX2xvY2tzKCkgdG8gZm9y Y2UgbG9jaw0KPiByZWNvdmVyeSwgaWYgdGhlIHJlcXVlc3RlZCBmbGFncyBhcmUgY2xlYXIuICBG aXggc3VnZ2VzdGVkIGJ5IEJyeWFuDQo+IFNjaHVtYWtlci4NCg0KVGhpcyBtYWtlcyBhYnNvbHV0 ZWx5IG5vIHNlbnNlLi4uDQoNClRoZSBwb2ludCBvZiB0aGVzZSBmdW5jdGlvbiBzaG91bGQgYmUg dG8gdGVzdCBpZiB0aGUgc3RhdGVpZCBpcyBzdGlsbA0KdmFsaWQuIElmIGl0IGlzLCB0aGVuIHdl IG5lZWQgbm8gcmVjb3ZlcnkuDQpJZiB0aGUgc3RhdGVpZCBpcyBfbm90XyB2YWxpZCwgdGhlbiB3 ZSBmcmVlIGl0LCBhbmQgdGhlbiBkZWNpZGUgaWYNCnJlY292ZXJ5IGlzIG5lZWRlZC4gVGhlIG9u bHkgZXhjZXB0aW9uIHRvIHRoYXQgcnVsZSBpcyBpZiBURVNUX1NUQVRFSUQNCnJldHVybnMgTkZT NEVSUl9CQURfU1RBVEVJRCwgKGluIHdoaWNoIGNhc2Ugd2UgZG9uJ3QgbmVlZCB0byBmcmVlIHRo ZQ0Kc3RhdGVpZCBidXQganVzdCByZWNvdmVyIGltbWVkaWF0ZWx5KS4NCg0KSU9XOiBUaGUgc3Rh dGUtPmZsYWdzIGFuZCBsc3AtPmxzX2ZsYWdzIHRlc3RzIHNob3VsZCBqdXN0IGJlIHJlbW92ZWQu DQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXINCg0K TmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20NCg0K -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Jul 9, 2012, at 4:03 PM, Myklebust, Trond wrote: > On Mon, 2012-07-09 at 11:44 -0400, Chuck Lever wrote: >> Before beginning state recovery, the state manager zaps open and lock >> state for each open file, thus the "state->flags & flags" test in >> nfs41_{open,lock}_expired() always fails during reclaim. But open >> recovery is still needed for these files. >> >> To force a call to nfs4_open_expired(), change the default return >> value for nfs41_check_expired_stateid() to force open recovery, and >> the default return value for nfs41_check_locks() to force lock >> recovery, if the requested flags are clear. Fix suggested by Bryan >> Schumaker. > > This makes absolutely no sense... > > The point of these function should be to test if the stateid is still > valid. If it is, then we need no recovery. > If the stateid is _not_ valid, then we free it, and then decide if > recovery is needed. The only exception to that rule is if TEST_STATEID > returns NFS4ERR_BAD_STATEID, (in which case we don't need to free the > stateid but just recover immediately). So you'd like to change the sense of the switch statement in my earlier patch: NFS4_OK: do nothing NFS4ERR_BAD_STATEID: just open default: free the state ID, then open I still don't think we need a FREE_STATEID call for NFS4ERR_EXPIRED, for example. It seems to me the only cases where FREE_STATEID is needed is the _REVOKED cases. > IOW: The state->flags and lsp->ls_flags tests should just be removed. Bryan had a reason to leave those tests in, but I don't remember what it was. > > -- > Trond Myklebust > Linux NFS client maintainer > > NetApp > Trond.Myklebust@netapp.com > www.netapp.com > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Jul 9, 2012, at 4:15 PM, Chuck Lever wrote: > > On Jul 9, 2012, at 4:03 PM, Myklebust, Trond wrote: > >> On Mon, 2012-07-09 at 11:44 -0400, Chuck Lever wrote: >>> Before beginning state recovery, the state manager zaps open and lock >>> state for each open file, thus the "state->flags & flags" test in >>> nfs41_{open,lock}_expired() always fails during reclaim. But open >>> recovery is still needed for these files. >>> >>> To force a call to nfs4_open_expired(), change the default return >>> value for nfs41_check_expired_stateid() to force open recovery, and >>> the default return value for nfs41_check_locks() to force lock >>> recovery, if the requested flags are clear. Fix suggested by Bryan >>> Schumaker. >> >> This makes absolutely no sense... >> >> The point of these function should be to test if the stateid is still >> valid. If it is, then we need no recovery. >> If the stateid is _not_ valid, then we free it, and then decide if >> recovery is needed. The only exception to that rule is if TEST_STATEID >> returns NFS4ERR_BAD_STATEID, (in which case we don't need to free the >> stateid but just recover immediately). > > So you'd like to change the sense of the switch statement in my earlier patch: > > NFS4_OK: > do nothing > NFS4ERR_BAD_STATEID: > just open > default: > free the state ID, then open > > I still don't think we need a FREE_STATEID call for NFS4ERR_EXPIRED, for example. It seems to me the only cases where FREE_STATEID is needed is the _REVOKED cases. > >> IOW: The state->flags and lsp->ls_flags tests should just be removed. > > Bryan had a reason to leave those tests in, but I don't remember what it was. Ah. I think it was that if those flags are set, then the client assumes the server still has that state, thus no recovery actions should be done.
On Mon, 2012-07-09 at 16:19 -0400, Chuck Lever wrote: > On Jul 9, 2012, at 4:15 PM, Chuck Lever wrote: > > > > > On Jul 9, 2012, at 4:03 PM, Myklebust, Trond wrote: > > > >> On Mon, 2012-07-09 at 11:44 -0400, Chuck Lever wrote: > >>> Before beginning state recovery, the state manager zaps open and lock > >>> state for each open file, thus the "state->flags & flags" test in > >>> nfs41_{open,lock}_expired() always fails during reclaim. But open > >>> recovery is still needed for these files. > >>> > >>> To force a call to nfs4_open_expired(), change the default return > >>> value for nfs41_check_expired_stateid() to force open recovery, and > >>> the default return value for nfs41_check_locks() to force lock > >>> recovery, if the requested flags are clear. Fix suggested by Bryan > >>> Schumaker. > >> > >> This makes absolutely no sense... > >> > >> The point of these function should be to test if the stateid is still > >> valid. If it is, then we need no recovery. > >> If the stateid is _not_ valid, then we free it, and then decide if > >> recovery is needed. The only exception to that rule is if TEST_STATEID > >> returns NFS4ERR_BAD_STATEID, (in which case we don't need to free the > >> stateid but just recover immediately). > > > > So you'd like to change the sense of the switch statement in my earlier patch: > > > > NFS4_OK: > > do nothing > > NFS4ERR_BAD_STATEID: > > just open > > default: > > free the state ID, then open > > > > I still don't think we need a FREE_STATEID call for NFS4ERR_EXPIRED, for example. It seems to me the only cases where FREE_STATEID is needed is the _REVOKED cases. > > > >> IOW: The state->flags and lsp->ls_flags tests should just be removed. > > > > Bryan had a reason to leave those tests in, but I don't remember what it was. > > Ah. > > I think it was that if those flags are set, then the client assumes the server still has that state, thus no recovery actions should be done. ??? If the server is still holding the state, then the TEST_STATEID will return '0'. Why do we need extra tests? -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com
On Jul 9, 2012, at 4:35 PM, Myklebust, Trond wrote: > On Mon, 2012-07-09 at 16:19 -0400, Chuck Lever wrote: >> On Jul 9, 2012, at 4:15 PM, Chuck Lever wrote: >> >>> >>> On Jul 9, 2012, at 4:03 PM, Myklebust, Trond wrote: >>> >>>> On Mon, 2012-07-09 at 11:44 -0400, Chuck Lever wrote: >>>>> Before beginning state recovery, the state manager zaps open and lock >>>>> state for each open file, thus the "state->flags & flags" test in >>>>> nfs41_{open,lock}_expired() always fails during reclaim. But open >>>>> recovery is still needed for these files. >>>>> >>>>> To force a call to nfs4_open_expired(), change the default return >>>>> value for nfs41_check_expired_stateid() to force open recovery, and >>>>> the default return value for nfs41_check_locks() to force lock >>>>> recovery, if the requested flags are clear. Fix suggested by Bryan >>>>> Schumaker. >>>> >>>> This makes absolutely no sense... >>>> >>>> The point of these function should be to test if the stateid is still >>>> valid. If it is, then we need no recovery. >>>> If the stateid is _not_ valid, then we free it, and then decide if >>>> recovery is needed. The only exception to that rule is if TEST_STATEID >>>> returns NFS4ERR_BAD_STATEID, (in which case we don't need to free the >>>> stateid but just recover immediately). >>> >>> So you'd like to change the sense of the switch statement in my earlier patch: >>> >>> NFS4_OK: >>> do nothing >>> NFS4ERR_BAD_STATEID: >>> just open >>> default: >>> free the state ID, then open >>> >>> I still don't think we need a FREE_STATEID call for NFS4ERR_EXPIRED, for example. It seems to me the only cases where FREE_STATEID is needed is the _REVOKED cases. >>> >>>> IOW: The state->flags and lsp->ls_flags tests should just be removed. >>> >>> Bryan had a reason to leave those tests in, but I don't remember what it was. >> >> Ah. >> >> I think it was that if those flags are set, then the client assumes the server still has that state, thus no recovery actions should be done. > > ??? If the server is still holding the state, then the TEST_STATEID will > return '0'. Why do we need extra tests? This is Bryan's design. But my understanding is that recovery always visits every state ID, and these flags prevent tests from going to the server for all but the state ID to be recovered. Is that correct, Bryan?
T24gTW9uLCAyMDEyLTA3LTA5IGF0IDE2OjM3IC0wNDAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4g T24gSnVsIDksIDIwMTIsIGF0IDQ6MzUgUE0sIE15a2xlYnVzdCwgVHJvbmQgd3JvdGU6DQo+IA0K PiA+IE9uIE1vbiwgMjAxMi0wNy0wOSBhdCAxNjoxOSAtMDQwMCwgQ2h1Y2sgTGV2ZXIgd3JvdGU6 DQo+ID4+IE9uIEp1bCA5LCAyMDEyLCBhdCA0OjE1IFBNLCBDaHVjayBMZXZlciB3cm90ZToNCj4g Pj4gDQo+ID4+PiANCj4gPj4+IE9uIEp1bCA5LCAyMDEyLCBhdCA0OjAzIFBNLCBNeWtsZWJ1c3Qs IFRyb25kIHdyb3RlOg0KPiA+Pj4gDQo+ID4+Pj4gT24gTW9uLCAyMDEyLTA3LTA5IGF0IDExOjQ0 IC0wNDAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4gPj4+Pj4gQmVmb3JlIGJlZ2lubmluZyBzdGF0 ZSByZWNvdmVyeSwgdGhlIHN0YXRlIG1hbmFnZXIgemFwcyBvcGVuIGFuZCBsb2NrDQo+ID4+Pj4+ IHN0YXRlIGZvciBlYWNoIG9wZW4gZmlsZSwgdGh1cyB0aGUgInN0YXRlLT5mbGFncyAmIGZsYWdz IiB0ZXN0IGluDQo+ID4+Pj4+IG5mczQxX3tvcGVuLGxvY2t9X2V4cGlyZWQoKSBhbHdheXMgZmFp bHMgZHVyaW5nIHJlY2xhaW0uICBCdXQgb3Blbg0KPiA+Pj4+PiByZWNvdmVyeSBpcyBzdGlsbCBu ZWVkZWQgZm9yIHRoZXNlIGZpbGVzLg0KPiA+Pj4+PiANCj4gPj4+Pj4gVG8gZm9yY2UgYSBjYWxs IHRvIG5mczRfb3Blbl9leHBpcmVkKCksIGNoYW5nZSB0aGUgZGVmYXVsdCByZXR1cm4NCj4gPj4+ Pj4gdmFsdWUgZm9yIG5mczQxX2NoZWNrX2V4cGlyZWRfc3RhdGVpZCgpIHRvIGZvcmNlIG9wZW4g cmVjb3ZlcnksIGFuZA0KPiA+Pj4+PiB0aGUgZGVmYXVsdCByZXR1cm4gdmFsdWUgZm9yIG5mczQx X2NoZWNrX2xvY2tzKCkgdG8gZm9yY2UgbG9jaw0KPiA+Pj4+PiByZWNvdmVyeSwgaWYgdGhlIHJl cXVlc3RlZCBmbGFncyBhcmUgY2xlYXIuICBGaXggc3VnZ2VzdGVkIGJ5IEJyeWFuDQo+ID4+Pj4+ IFNjaHVtYWtlci4NCj4gPj4+PiANCj4gPj4+PiBUaGlzIG1ha2VzIGFic29sdXRlbHkgbm8gc2Vu c2UuLi4NCj4gPj4+PiANCj4gPj4+PiBUaGUgcG9pbnQgb2YgdGhlc2UgZnVuY3Rpb24gc2hvdWxk IGJlIHRvIHRlc3QgaWYgdGhlIHN0YXRlaWQgaXMgc3RpbGwNCj4gPj4+PiB2YWxpZC4gSWYgaXQg aXMsIHRoZW4gd2UgbmVlZCBubyByZWNvdmVyeS4NCj4gPj4+PiBJZiB0aGUgc3RhdGVpZCBpcyBf bm90XyB2YWxpZCwgdGhlbiB3ZSBmcmVlIGl0LCBhbmQgdGhlbiBkZWNpZGUgaWYNCj4gPj4+PiBy ZWNvdmVyeSBpcyBuZWVkZWQuIFRoZSBvbmx5IGV4Y2VwdGlvbiB0byB0aGF0IHJ1bGUgaXMgaWYg VEVTVF9TVEFURUlEDQo+ID4+Pj4gcmV0dXJucyBORlM0RVJSX0JBRF9TVEFURUlELCAoaW4gd2hp Y2ggY2FzZSB3ZSBkb24ndCBuZWVkIHRvIGZyZWUgdGhlDQo+ID4+Pj4gc3RhdGVpZCBidXQganVz dCByZWNvdmVyIGltbWVkaWF0ZWx5KS4NCj4gPj4+IA0KPiA+Pj4gU28geW91J2QgbGlrZSB0byBj aGFuZ2UgdGhlIHNlbnNlIG9mIHRoZSBzd2l0Y2ggc3RhdGVtZW50IGluIG15IGVhcmxpZXIgcGF0 Y2g6DQo+ID4+PiANCj4gPj4+ICBORlM0X09LOg0KPiA+Pj4gICAgZG8gbm90aGluZw0KPiA+Pj4g IE5GUzRFUlJfQkFEX1NUQVRFSUQ6DQo+ID4+PiAgICBqdXN0IG9wZW4NCj4gPj4+ICBkZWZhdWx0 Og0KPiA+Pj4gICAgZnJlZSB0aGUgc3RhdGUgSUQsIHRoZW4gb3Blbg0KPiA+Pj4gDQo+ID4+PiBJ IHN0aWxsIGRvbid0IHRoaW5rIHdlIG5lZWQgYSBGUkVFX1NUQVRFSUQgY2FsbCBmb3IgTkZTNEVS Ul9FWFBJUkVELCBmb3IgZXhhbXBsZS4gIEl0IHNlZW1zIHRvIG1lIHRoZSBvbmx5IGNhc2VzIHdo ZXJlIEZSRUVfU1RBVEVJRCBpcyBuZWVkZWQgaXMgdGhlIF9SRVZPS0VEIGNhc2VzLg0KDQpJJ20g bm90IHN1cmUgYWJvdXQgd2hhdCB0aGUgcnVsZXMgYXJlIGZvciBORlM0RVJSX0VYUElSRUQuIEFz IGZhciBhcyBJJ20NCmF3YXJlLCBORlM0RVJSX0VYUElSRUQgaXMgTkZTdjQuMC1zcGVhayBmb3Ig Kl9TVEFURV9SRVZPS0VEOyByZXR1cm5pbmcNCml0IGFzIHBhcnQgb2YgYSBURVNUX1NUQVRFSUQg bWFrZXMgbGl0dGxlIHNlbnNlIHRvIG1lLCBzbyBJJ20gdGFraW5nIHRoZQ0KY29uc2VydmF0aXZl IGFwcHJvYWNoIHRvIGl0Lg0KDQo+ID4+Pj4gSU9XOiBUaGUgc3RhdGUtPmZsYWdzIGFuZCBsc3At PmxzX2ZsYWdzIHRlc3RzIHNob3VsZCBqdXN0IGJlIHJlbW92ZWQuDQo+ID4+PiANCj4gPj4+IEJy eWFuIGhhZCBhIHJlYXNvbiB0byBsZWF2ZSB0aG9zZSB0ZXN0cyBpbiwgYnV0IEkgZG9uJ3QgcmVt ZW1iZXIgd2hhdCBpdCB3YXMuDQo+ID4+IA0KPiA+PiBBaC4NCj4gPj4gDQo+ID4+IEkgdGhpbmsg aXQgd2FzIHRoYXQgaWYgdGhvc2UgZmxhZ3MgYXJlIHNldCwgdGhlbiB0aGUgY2xpZW50IGFzc3Vt ZXMgdGhlIHNlcnZlciBzdGlsbCBoYXMgdGhhdCBzdGF0ZSwgdGh1cyBubyByZWNvdmVyeSBhY3Rp b25zIHNob3VsZCBiZSBkb25lLg0KPiA+IA0KPiA+ID8/PyBJZiB0aGUgc2VydmVyIGlzIHN0aWxs IGhvbGRpbmcgdGhlIHN0YXRlLCB0aGVuIHRoZSBURVNUX1NUQVRFSUQgd2lsbA0KPiA+IHJldHVy biAnMCcuIFdoeSBkbyB3ZSBuZWVkIGV4dHJhIHRlc3RzPw0KPiANCj4gVGhpcyBpcyBCcnlhbidz IGRlc2lnbi4gIEJ1dCBteSB1bmRlcnN0YW5kaW5nIGlzIHRoYXQgcmVjb3ZlcnkgYWx3YXlzIHZp c2l0cyBldmVyeSBzdGF0ZSBJRCwgYW5kIHRoZXNlIGZsYWdzIHByZXZlbnQgdGVzdHMgZnJvbSBn b2luZyB0byB0aGUgc2VydmVyIGZvciBhbGwgYnV0IHRoZSBzdGF0ZSBJRCB0byBiZSByZWNvdmVy ZWQuDQo+IA0KPiBJcyB0aGF0IGNvcnJlY3QsIEJyeWFuPw0KDQpPSy4gSSB0aGluayBJIHVuZGVy c3RhbmQgd2hhdCB0aGUgaXNzdWUgaXMuDQoNCklmIHdlIGNhbGwgbmZzNF9zdGF0ZV9zdGFydF9y ZWNsYWltX25vZ3JhY2UoKSAoZHVlIHRvIGEgbGVhc2UgcHVyZ2Ugb3IgYQ0KTkZTNEVSUl9FWFBJ UkVEIG9uIG5mczRfY2hlY2tfbGVhc2UoKSksIHRoZW4gdGhhdCByZXNldHMgdGhlIHNlcXVlbmNl DQppZHMgYW5kIGRvZXMgYSBmdWxsIHJlc2V0IG9mIGFsbCBzdGF0ZS4gSW4gdGhhdCBjYXNlLCB3 ZSBkbyBpbmRlZWQgbm90DQp3YW50IHRvIGNhbGwgVEVTVF9TVEFURUlELCBzaW5jZSB0aGUgYmVz dCBpdCBjYW4gZG8gaXMgcmV0dXJuDQpORlM0RVJSX09MRF9TVEFURUlELg0KDQpPSy4gVGhpcyBu ZWVkcyBzb21lIGNvbW1lbnRzIGluIHRoZSBjb2RlLi4uDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0 DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RA bmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20NCg0K -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Jul 9, 2012, at 5:31 PM, Myklebust, Trond wrote: > On Mon, 2012-07-09 at 16:37 -0400, Chuck Lever wrote: >> On Jul 9, 2012, at 4:35 PM, Myklebust, Trond wrote: >> >>> On Mon, 2012-07-09 at 16:19 -0400, Chuck Lever wrote: >>>> On Jul 9, 2012, at 4:15 PM, Chuck Lever wrote: >>>> >>>>> >>>>> On Jul 9, 2012, at 4:03 PM, Myklebust, Trond wrote: >>>>> >>>>>> On Mon, 2012-07-09 at 11:44 -0400, Chuck Lever wrote: >>>>>>> Before beginning state recovery, the state manager zaps open and lock >>>>>>> state for each open file, thus the "state->flags & flags" test in >>>>>>> nfs41_{open,lock}_expired() always fails during reclaim. But open >>>>>>> recovery is still needed for these files. >>>>>>> >>>>>>> To force a call to nfs4_open_expired(), change the default return >>>>>>> value for nfs41_check_expired_stateid() to force open recovery, and >>>>>>> the default return value for nfs41_check_locks() to force lock >>>>>>> recovery, if the requested flags are clear. Fix suggested by Bryan >>>>>>> Schumaker. >>>>>> >>>>>> This makes absolutely no sense... >>>>>> >>>>>> The point of these function should be to test if the stateid is still >>>>>> valid. If it is, then we need no recovery. >>>>>> If the stateid is _not_ valid, then we free it, and then decide if >>>>>> recovery is needed. The only exception to that rule is if TEST_STATEID >>>>>> returns NFS4ERR_BAD_STATEID, (in which case we don't need to free the >>>>>> stateid but just recover immediately). >>>>> >>>>> So you'd like to change the sense of the switch statement in my earlier patch: >>>>> >>>>> NFS4_OK: >>>>> do nothing >>>>> NFS4ERR_BAD_STATEID: >>>>> just open >>>>> default: >>>>> free the state ID, then open >>>>> >>>>> I still don't think we need a FREE_STATEID call for NFS4ERR_EXPIRED, for example. It seems to me the only cases where FREE_STATEID is needed is the _REVOKED cases. > > I'm not sure about what the rules are for NFS4ERR_EXPIRED. As far as I'm > aware, NFS4ERR_EXPIRED is NFSv4.0-speak for *_STATE_REVOKED; returning > it as part of a TEST_STATEID makes little sense to me, so I'm taking the > conservative approach to it. Based on your review comments, we have an opportunity to make this error handling more clear, which I'd like to clarify before redriving the patch. I'm not quite sure what "conservative approach" means. Do you want to include NFS4ERR_EXPIRED with the _REVOKED arms, treat it as evidence of a broken server, or ignore it entirely? Do you want to leave the default: arm as it is, or do you have a preference for adding additional explicit error codes in the switch statement? Are there cases which report that something has gone awry versus cases which simply report the state ID is rotten? My feeling when I wrote this was to get the switch statement in there, and then we can introduce more error codes as needed. > >>>>>> IOW: The state->flags and lsp->ls_flags tests should just be removed. >>>>> >>>>> Bryan had a reason to leave those tests in, but I don't remember what it was. >>>> >>>> Ah. >>>> >>>> I think it was that if those flags are set, then the client assumes the server still has that state, thus no recovery actions should be done. >>> >>> ??? If the server is still holding the state, then the TEST_STATEID will >>> return '0'. Why do we need extra tests? >> >> This is Bryan's design. But my understanding is that recovery always visits every state ID, and these flags prevent tests from going to the server for all but the state ID to be recovered. >> >> Is that correct, Bryan? > > OK. I think I understand what the issue is. > > If we call nfs4_state_start_reclaim_nograce() (due to a lease purge or a > NFS4ERR_EXPIRED on nfs4_check_lease()), then that resets the sequence > ids and does a full reset of all state. In that case, we do indeed not > want to call TEST_STATEID, since the best it can do is return > NFS4ERR_OLD_STATEID. > > OK. This needs some comments in the code... I can add comments in both the patch description and in the code itself.
On Mon, 2012-07-09 at 17:45 -0400, Chuck Lever wrote: > On Jul 9, 2012, at 5:31 PM, Myklebust, Trond wrote: > > > On Mon, 2012-07-09 at 16:37 -0400, Chuck Lever wrote: > >> On Jul 9, 2012, at 4:35 PM, Myklebust, Trond wrote: > >> > >>> On Mon, 2012-07-09 at 16:19 -0400, Chuck Lever wrote: > >>>> On Jul 9, 2012, at 4:15 PM, Chuck Lever wrote: > >>>> > >>>>> > >>>>> On Jul 9, 2012, at 4:03 PM, Myklebust, Trond wrote: > >>>>> > >>>>>> On Mon, 2012-07-09 at 11:44 -0400, Chuck Lever wrote: > >>>>>>> Before beginning state recovery, the state manager zaps open and lock > >>>>>>> state for each open file, thus the "state->flags & flags" test in > >>>>>>> nfs41_{open,lock}_expired() always fails during reclaim. But open > >>>>>>> recovery is still needed for these files. > >>>>>>> > >>>>>>> To force a call to nfs4_open_expired(), change the default return > >>>>>>> value for nfs41_check_expired_stateid() to force open recovery, and > >>>>>>> the default return value for nfs41_check_locks() to force lock > >>>>>>> recovery, if the requested flags are clear. Fix suggested by Bryan > >>>>>>> Schumaker. > >>>>>> > >>>>>> This makes absolutely no sense... > >>>>>> > >>>>>> The point of these function should be to test if the stateid is still > >>>>>> valid. If it is, then we need no recovery. > >>>>>> If the stateid is _not_ valid, then we free it, and then decide if > >>>>>> recovery is needed. The only exception to that rule is if TEST_STATEID > >>>>>> returns NFS4ERR_BAD_STATEID, (in which case we don't need to free the > >>>>>> stateid but just recover immediately). > >>>>> > >>>>> So you'd like to change the sense of the switch statement in my earlier patch: > >>>>> > >>>>> NFS4_OK: > >>>>> do nothing > >>>>> NFS4ERR_BAD_STATEID: > >>>>> just open > >>>>> default: > >>>>> free the state ID, then open > >>>>> > >>>>> I still don't think we need a FREE_STATEID call for NFS4ERR_EXPIRED, for example. It seems to me the only cases where FREE_STATEID is needed is the _REVOKED cases. > > > > I'm not sure about what the rules are for NFS4ERR_EXPIRED. As far as I'm > > aware, NFS4ERR_EXPIRED is NFSv4.0-speak for *_STATE_REVOKED; returning > > it as part of a TEST_STATEID makes little sense to me, so I'm taking the > > conservative approach to it. > > Based on your review comments, we have an opportunity to make this error handling more clear, which I'd like to clarify before redriving the patch. > > I'm not quite sure what "conservative approach" means. Do you want to include NFS4ERR_EXPIRED with the _REVOKED arms, treat it as evidence of a broken server, or ignore it entirely? I mean that calling FREE_STATEID in cases where it isn't strictly needed because the stateid is already invalid is safe. Not calling it when it is needed ties up resources unnecessarily on the server. -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 60a320c..4bc21a3 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -1766,8 +1766,8 @@ static int nfs4_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *sta */ static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *stateid, unsigned int flags) { - int status = NFS_OK; struct nfs_server *server = NFS_SERVER(state->inode); + int status = -NFS4ERR_BAD_STATEID; if (state->flags & flags) { status = nfs41_test_stateid(server, stateid); @@ -1789,16 +1789,17 @@ static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *s static int nfs41_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *state) { - int deleg_status, open_status; int deleg_flags = 1 << NFS_DELEGATED_STATE; int open_flags = (1 << NFS_O_RDONLY_STATE) | (1 << NFS_O_WRONLY_STATE) | (1 << NFS_O_RDWR_STATE); + int status; - deleg_status = nfs41_check_expired_stateid(state, &state->stateid, deleg_flags); - open_status = nfs41_check_expired_stateid(state, &state->open_stateid, open_flags); + nfs41_check_expired_stateid(state, &state->stateid, deleg_flags); + status = nfs41_check_expired_stateid(state, &state->open_stateid, + open_flags); - if ((deleg_status == NFS_OK) && (open_status == NFS_OK)) - return NFS_OK; - return nfs4_open_expired(sp, state); + if (status != NFS_OK) + status = nfs4_open_expired(sp, state); + return status; } #endif @@ -4724,7 +4725,7 @@ out: */ static int nfs41_check_expired_locks(struct nfs4_state *state) { - int status, ret = NFS_OK; + int status, ret = -NFS4ERR_BAD_STATEID; struct nfs4_lock_state *lsp; struct nfs_server *server = NFS_SERVER(state->inode); @@ -4756,9 +4757,9 @@ static int nfs41_lock_expired(struct nfs4_state *state, struct file_lock *reques if (test_bit(LK_STATE_IN_USE, &state->flags)) status = nfs41_check_expired_locks(state); - if (status == NFS_OK) - return status; - return nfs4_lock_expired(state, request); + if (status != NFS_OK) + status = nfs4_lock_expired(state, request); + return status; } #endif
Before beginning state recovery, the state manager zaps open and lock state for each open file, thus the "state->flags & flags" test in nfs41_{open,lock}_expired() always fails during reclaim. But open recovery is still needed for these files. To force a call to nfs4_open_expired(), change the default return value for nfs41_check_expired_stateid() to force open recovery, and the default return value for nfs41_check_locks() to force lock recovery, if the requested flags are clear. Fix suggested by Bryan Schumaker. The presence of a delegation state ID must not prevent normal open recovery. The delegation state ID must be cleared if it was revoked, but once cleared I don't think it's presence or absence has any bearing on whether open recovery is still needed. Signed-off-by: Chuck Lever <chuck.lever@oracle.com> --- fs/nfs/nfs4proc.c | 23 ++++++++++++----------- 1 files changed, 12 insertions(+), 11 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html