[PATCH 1/1] Recover from stateid-type error on SETATTR

[PATCH 1/1] Recover from stateid-type error on SETATTR

[Olga Kornievskaia]

Client can receives stateid-type error (eg., BAD_STATEID) on SETATTR when
delegation stateid was used. When no open state exists, in case of application
calling truncate() on the file, client has no state to recover and fails with
EIO.

Instead, upon such error, return the bad delegation and then resend the
SETATTR with a zero stateid.

Signed-off: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfs/nfs4proc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ad7cf7e..2a85af3 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -360,8 +360,14 @@ static int nfs4_handle_exception(struct nfs_server *server, int errorcode, struc
 		case -NFS4ERR_DELEG_REVOKED:
 		case -NFS4ERR_ADMIN_REVOKED:
 		case -NFS4ERR_BAD_STATEID:
-			if (state == NULL)
+			if (state == NULL) {
+				if (inode && nfs4_have_delegation(inode, 
+						FMODE_READ)) {
+					nfs4_inode_return_delegation(inode);
+					exception->retry = 1;
+				}
 				break;
+			}
 			ret = nfs4_schedule_stateid_recovery(server, state);
 			if (ret < 0)
 				break;
-- 
1.8.3.1

Re: [PATCH 1/1] Recover from stateid-type error on SETATTR

[Trond Myklebust]

On Fri, Jun 12, 2015 at 4:53 PM, Olga Kornievskaia <kolga@netapp.com> wrote:
> Client can receives stateid-type error (eg., BAD_STATEID) on SETATTR when
> delegation stateid was used. When no open state exists, in case of application
> calling truncate() on the file, client has no state to recover and fails with
> EIO.
>
> Instead, upon such error, return the bad delegation and then resend the
> SETATTR with a zero stateid.
>
> Signed-off: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfs/nfs4proc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index ad7cf7e..2a85af3 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -360,8 +360,14 @@ static int nfs4_handle_exception(struct nfs_server *server, int errorcode, struc
>                 case -NFS4ERR_DELEG_REVOKED:
>                 case -NFS4ERR_ADMIN_REVOKED:
>                 case -NFS4ERR_BAD_STATEID:
> -                       if (state == NULL)
> +                       if (state == NULL) {
> +                               if (inode && nfs4_have_delegation(inode,
> +                                               FMODE_READ)) {
> +                                       nfs4_inode_return_delegation(inode);
> +                                       exception->retry = 1;
> +                               }
>                                 break;
> +                       }
>                         ret = nfs4_schedule_stateid_recovery(server, state);
>                         if (ret < 0)
>                                 break;

My remaining question here is: We don't seem to be communicating the
_REVOKED state of the delegation to nfs4_schedule_stateid_recovery(),
so won't we continue to loop if state != NULL?

IOW: is there any reason not to do the same thing here that we already
do for NFS4ERR_OPENMODE? At the time when we wrote commit
3114ea7a24d32, there was a difference between
nfs_inode_return_delegation() and nfs_remove_bad_delegation(), but now
the former handles both cases...

Re: [PATCH 1/1] Recover from stateid-type error on SETATTR

[Kornievskaia, Olga]

DQo+IE9uIEp1biAxMiwgMjAxNSwgYXQgNToyMSBQTSwgVHJvbmQgTXlrbGVidXN0IDx0cm9uZC5t
eWtsZWJ1c3RAcHJpbWFyeWRhdGEuY29tPiB3cm90ZToNCj4gDQo+IE9uIEZyaSwgSnVuIDEyLCAy
MDE1IGF0IDQ6NTMgUE0sIE9sZ2EgS29ybmlldnNrYWlhIDxrb2xnYUBuZXRhcHAuY29tPiB3cm90
ZToNCj4+IENsaWVudCBjYW4gcmVjZWl2ZXMgc3RhdGVpZC10eXBlIGVycm9yIChlZy4sIEJBRF9T
VEFURUlEKSBvbiBTRVRBVFRSIHdoZW4NCj4+IGRlbGVnYXRpb24gc3RhdGVpZCB3YXMgdXNlZC4g
V2hlbiBubyBvcGVuIHN0YXRlIGV4aXN0cywgaW4gY2FzZSBvZiBhcHBsaWNhdGlvbg0KPj4gY2Fs
bGluZyB0cnVuY2F0ZSgpIG9uIHRoZSBmaWxlLCBjbGllbnQgaGFzIG5vIHN0YXRlIHRvIHJlY292
ZXIgYW5kIGZhaWxzIHdpdGgNCj4+IEVJTy4NCj4+IA0KPj4gSW5zdGVhZCwgdXBvbiBzdWNoIGVy
cm9yLCByZXR1cm4gdGhlIGJhZCBkZWxlZ2F0aW9uIGFuZCB0aGVuIHJlc2VuZCB0aGUNCj4+IFNF
VEFUVFIgd2l0aCBhIHplcm8gc3RhdGVpZC4NCj4+IA0KPj4gU2lnbmVkLW9mZjogT2xnYSBLb3Ju
aWV2c2thaWEgPGtvbGdhQG5ldGFwcC5jb20+DQo+PiAtLS0NCj4+IGZzL25mcy9uZnM0cHJvYy5j
IHwgOCArKysrKysrLQ0KPj4gMSBmaWxlIGNoYW5nZWQsIDcgaW5zZXJ0aW9ucygrKSwgMSBkZWxl
dGlvbigtKQ0KPj4gDQo+PiBkaWZmIC0tZ2l0IGEvZnMvbmZzL25mczRwcm9jLmMgYi9mcy9uZnMv
bmZzNHByb2MuYw0KPj4gaW5kZXggYWQ3Y2Y3ZS4uMmE4NWFmMyAxMDA2NDQNCj4+IC0tLSBhL2Zz
L25mcy9uZnM0cHJvYy5jDQo+PiArKysgYi9mcy9uZnMvbmZzNHByb2MuYw0KPj4gQEAgLTM2MCw4
ICszNjAsMTQgQEAgc3RhdGljIGludCBuZnM0X2hhbmRsZV9leGNlcHRpb24oc3RydWN0IG5mc19z
ZXJ2ZXIgKnNlcnZlciwgaW50IGVycm9yY29kZSwgc3RydWMNCj4+ICAgICAgICAgICAgICAgIGNh
c2UgLU5GUzRFUlJfREVMRUdfUkVWT0tFRDoNCj4+ICAgICAgICAgICAgICAgIGNhc2UgLU5GUzRF
UlJfQURNSU5fUkVWT0tFRDoNCj4+ICAgICAgICAgICAgICAgIGNhc2UgLU5GUzRFUlJfQkFEX1NU
QVRFSUQ6DQo+PiAtICAgICAgICAgICAgICAgICAgICAgICBpZiAoc3RhdGUgPT0gTlVMTCkNCj4+
ICsgICAgICAgICAgICAgICAgICAgICAgIGlmIChzdGF0ZSA9PSBOVUxMKSB7DQo+PiArICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgIGlmIChpbm9kZSAmJiBuZnM0X2hhdmVfZGVsZWdhdGlv
bihpbm9kZSwNCj4+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgIEZNT0RFX1JFQUQpKSB7DQo+PiArICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgbmZzNF9pbm9kZV9yZXR1cm5fZGVsZWdhdGlvbihpbm9kZSk7DQo+PiArICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgZXhjZXB0aW9uLT5yZXRyeSA9IDE7DQo+PiAr
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIH0NCj4+ICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICBicmVhazsNCj4+ICsgICAgICAgICAgICAgICAgICAgICAgIH0NCj4+ICAgICAg
ICAgICAgICAgICAgICAgICAgcmV0ID0gbmZzNF9zY2hlZHVsZV9zdGF0ZWlkX3JlY292ZXJ5KHNl
cnZlciwgc3RhdGUpOw0KPj4gICAgICAgICAgICAgICAgICAgICAgICBpZiAocmV0IDwgMCkNCj4+
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBicmVhazsNCj4gDQo+IE15IHJlbWFpbmlu
ZyBxdWVzdGlvbiBoZXJlIGlzOiBXZSBkb24ndCBzZWVtIHRvIGJlIGNvbW11bmljYXRpbmcgdGhl
DQo+IF9SRVZPS0VEIHN0YXRlIG9mIHRoZSBkZWxlZ2F0aW9uIHRvIG5mczRfc2NoZWR1bGVfc3Rh
dGVpZF9yZWNvdmVyeSgpLA0KPiBzbyB3b24ndCB3ZSBjb250aW51ZSB0byBsb29wIGlmIHN0YXRl
ICE9IE5VTEw/DQoNCknigJltIG5vdCBmb2xsb3dpbmcuIFRoaXMgcGF0Y2ggaXMgdG8gaGFuZGxl
IHRoZSBjYXNlIHdoZW4gc3RhdGUgaXMgTlVMTCBhbmQgdGhlcmVmb3JlIHdlIGNhbuKAmXQgaW5p
dGlhdGUgcmVjb3ZlcnkuIEJlZm9yZSB0aGlzIHBhdGNoLCB0aGUgcHJvYmxlbSB3YXMgdGhhdCB3
aGVuIHN0YXRlIHdhcyBOVUxMIHdlIGRpZG7igJl0IChhKSBjbGVhbiB1cCBieSBkb2luZyBkZWxl
Z3JldHVybiBhbmQgKGIpIHdlIHdlcmVu4oCZdCByZXRyeWluZyB3aGVuIHdlIGNhbiDigJQgaS5l
LiB3aXRoIGEgc3BlY2lhbCBzdGF0ZWlkIGZvciBTRVRBVFRSLg0KDQpJZiBzdGF0ZSBpcyBub3Qg
bnVsbCwgSeKAmW0gYXNzdW1pbmcgdGhlIGNvZGUgaXMgaGFuZGxpbmcgY29ycmVjdGx5IGJ5IGNh
bGxpbmcgbmZzNF9zY2hlZHVsZV9zdGF0ZWlkX3JlY292ZXJ5LiBUaGUgY29ycmVjdG5lc3Mgb2Yg
dGhhdCBjb2RlIGlzIGJleW9uZCB0aGUgc2NvcGUgb2YgdGhlIHByb2JsZW0gaeKAmW0gcHJvcG9z
aW5nIHRvIHNvbHZlIGhlcmUuDQoNCg0KPiBJT1c6IGlzIHRoZXJlIGFueSByZWFzb24gbm90IHRv
IGRvIHRoZSBzYW1lIHRoaW5nIGhlcmUgdGhhdCB3ZSBhbHJlYWR5DQo+IGRvIGZvciBORlM0RVJS
X09QRU5NT0RFPw0KDQpUaGUgT1BFTk1PREUgZXJyb3IgbW9kZSBjb2RlIHNlZW1zIHRvIGhhdmUg
c2xpZ2h0bHkgZGlmZmVyZW50IGxvZ2ljLiBJdCB3YW50cyB0byByZXR1cm4gIHRoZSBkZWxlZ2F0
aW9uIHJlZ2FyZGxlc3Mgb2Ygd2hldGhlciBvciBub3Qgc3RhdGUgaXMgTlVMTC4gDQoNCg0KPiBB
dCB0aGUgdGltZSB3aGVuIHdlIHdyb3RlIGNvbW1pdA0KPiAzMTE0ZWE3YTI0ZDMyLCB0aGVyZSB3
YXMgYSBkaWZmZXJlbmNlIGJldHdlZW4NCj4gbmZzX2lub2RlX3JldHVybl9kZWxlZ2F0aW9uKCkg
YW5kIG5mc19yZW1vdmVfYmFkX2RlbGVnYXRpb24oKSwgYnV0IG5vdw0KPiB0aGUgZm9ybWVyIGhh
bmRsZXMgYm90aCBjYXNlcy4uLg0KDQo=

Re: [PATCH 1/1] Recover from stateid-type error on SETATTR

[Trond Myklebust]

On Fri, Jun 12, 2015 at 5:37 PM, Kornievskaia, Olga
<Olga.Kornievskaia@netapp.com> wrote:
>
>> On Jun 12, 2015, at 5:21 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:
>>
>> On Fri, Jun 12, 2015 at 4:53 PM, Olga Kornievskaia <kolga@netapp.com> wrote:
>>> Client can receives stateid-type error (eg., BAD_STATEID) on SETATTR when
>>> delegation stateid was used. When no open state exists, in case of application
>>> calling truncate() on the file, client has no state to recover and fails with
>>> EIO.
>>>
>>> Instead, upon such error, return the bad delegation and then resend the
>>> SETATTR with a zero stateid.
>>>
>>> Signed-off: Olga Kornievskaia <kolga@netapp.com>
>>> ---
>>> fs/nfs/nfs4proc.c | 8 +++++++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index ad7cf7e..2a85af3 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -360,8 +360,14 @@ static int nfs4_handle_exception(struct nfs_server *server, int errorcode, struc
>>>                case -NFS4ERR_DELEG_REVOKED:
>>>                case -NFS4ERR_ADMIN_REVOKED:
>>>                case -NFS4ERR_BAD_STATEID:
>>> -                       if (state == NULL)
>>> +                       if (state == NULL) {
>>> +                               if (inode && nfs4_have_delegation(inode,
>>> +                                               FMODE_READ)) {
>>> +                                       nfs4_inode_return_delegation(inode);
>>> +                                       exception->retry = 1;
>>> +                               }
>>>                                break;
>>> +                       }
>>>                        ret = nfs4_schedule_stateid_recovery(server, state);
>>>                        if (ret < 0)
>>>                                break;
>>
>> My remaining question here is: We don't seem to be communicating the
>> _REVOKED state of the delegation to nfs4_schedule_stateid_recovery(),
>> so won't we continue to loop if state != NULL?
>
> I’m not following. This patch is to handle the case when state is NULL and therefore we can’t initiate recovery. Before this patch, the problem was that when state was NULL we didn’t (a) clean up by doing delegreturn and (b) we weren’t retrying when we can — i.e. with a special stateid for SETATTR.
>
> If state is not null, I’m assuming the code is handling correctly by calling nfs4_schedule_stateid_recovery. The correctness of that code is beyond the scope of the problem i’m proposing to solve here.

That's my point. I'm not seeing that the code is handling that case,
and so I'm not seeing why we should treat state == NULL as being
special.

>
>
>> IOW: is there any reason not to do the same thing here that we already
>> do for NFS4ERR_OPENMODE?
>
> The OPENMODE error mode code seems to have slightly different logic. It wants to return  the delegation regardless of whether or not state is NULL.

See above.

Re: [PATCH 1/1] Recover from stateid-type error on SETATTR

[Kornievskaia, Olga]

DQo+IE9uIEp1biAxMiwgMjAxNSwgYXQgNTo0MiBQTSwgVHJvbmQgTXlrbGVidXN0IDx0cm9uZC5t
eWtsZWJ1c3RAcHJpbWFyeWRhdGEuY29tPiB3cm90ZToNCj4gDQo+IE9uIEZyaSwgSnVuIDEyLCAy
MDE1IGF0IDU6MzcgUE0sIEtvcm5pZXZza2FpYSwgT2xnYQ0KPiA8T2xnYS5Lb3JuaWV2c2thaWFA
bmV0YXBwLmNvbT4gd3JvdGU6DQo+PiANCj4+PiBPbiBKdW4gMTIsIDIwMTUsIGF0IDU6MjEgUE0s
IFRyb25kIE15a2xlYnVzdCA8dHJvbmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNvbT4gd3JvdGU6
DQo+Pj4gDQo+Pj4gT24gRnJpLCBKdW4gMTIsIDIwMTUgYXQgNDo1MyBQTSwgT2xnYSBLb3JuaWV2
c2thaWEgPGtvbGdhQG5ldGFwcC5jb20+IHdyb3RlOg0KPj4+PiBDbGllbnQgY2FuIHJlY2VpdmVz
IHN0YXRlaWQtdHlwZSBlcnJvciAoZWcuLCBCQURfU1RBVEVJRCkgb24gU0VUQVRUUiB3aGVuDQo+
Pj4+IGRlbGVnYXRpb24gc3RhdGVpZCB3YXMgdXNlZC4gV2hlbiBubyBvcGVuIHN0YXRlIGV4aXN0
cywgaW4gY2FzZSBvZiBhcHBsaWNhdGlvbg0KPj4+PiBjYWxsaW5nIHRydW5jYXRlKCkgb24gdGhl
IGZpbGUsIGNsaWVudCBoYXMgbm8gc3RhdGUgdG8gcmVjb3ZlciBhbmQgZmFpbHMgd2l0aA0KPj4+
PiBFSU8uDQo+Pj4+IA0KPj4+PiBJbnN0ZWFkLCB1cG9uIHN1Y2ggZXJyb3IsIHJldHVybiB0aGUg
YmFkIGRlbGVnYXRpb24gYW5kIHRoZW4gcmVzZW5kIHRoZQ0KPj4+PiBTRVRBVFRSIHdpdGggYSB6
ZXJvIHN0YXRlaWQuDQo+Pj4+IA0KPj4+PiBTaWduZWQtb2ZmOiBPbGdhIEtvcm5pZXZza2FpYSA8
a29sZ2FAbmV0YXBwLmNvbT4NCj4+Pj4gLS0tDQo+Pj4+IGZzL25mcy9uZnM0cHJvYy5jIHwgOCAr
KysrKysrLQ0KPj4+PiAxIGZpbGUgY2hhbmdlZCwgNyBpbnNlcnRpb25zKCspLCAxIGRlbGV0aW9u
KC0pDQo+Pj4+IA0KPj4+PiBkaWZmIC0tZ2l0IGEvZnMvbmZzL25mczRwcm9jLmMgYi9mcy9uZnMv
bmZzNHByb2MuYw0KPj4+PiBpbmRleCBhZDdjZjdlLi4yYTg1YWYzIDEwMDY0NA0KPj4+PiAtLS0g
YS9mcy9uZnMvbmZzNHByb2MuYw0KPj4+PiArKysgYi9mcy9uZnMvbmZzNHByb2MuYw0KPj4+PiBA
QCAtMzYwLDggKzM2MCwxNCBAQCBzdGF0aWMgaW50IG5mczRfaGFuZGxlX2V4Y2VwdGlvbihzdHJ1
Y3QgbmZzX3NlcnZlciAqc2VydmVyLCBpbnQgZXJyb3Jjb2RlLCBzdHJ1Yw0KPj4+PiAgICAgICAg
ICAgICAgIGNhc2UgLU5GUzRFUlJfREVMRUdfUkVWT0tFRDoNCj4+Pj4gICAgICAgICAgICAgICBj
YXNlIC1ORlM0RVJSX0FETUlOX1JFVk9LRUQ6DQo+Pj4+ICAgICAgICAgICAgICAgY2FzZSAtTkZT
NEVSUl9CQURfU1RBVEVJRDoNCj4+Pj4gLSAgICAgICAgICAgICAgICAgICAgICAgaWYgKHN0YXRl
ID09IE5VTEwpDQo+Pj4+ICsgICAgICAgICAgICAgICAgICAgICAgIGlmIChzdGF0ZSA9PSBOVUxM
KSB7DQo+Pj4+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgaWYgKGlub2RlICYmIG5m
czRfaGF2ZV9kZWxlZ2F0aW9uKGlub2RlLA0KPj4+PiArICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICBGTU9ERV9SRUFEKSkgew0KPj4+PiArICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgbmZzNF9pbm9kZV9yZXR1cm5fZGVsZWdhdGlvbihp
bm9kZSk7DQo+Pj4+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBleGNl
cHRpb24tPnJldHJ5ID0gMTsNCj4+Pj4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICB9
DQo+Pj4+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGJyZWFrOw0KPj4+PiArICAgICAg
ICAgICAgICAgICAgICAgICB9DQo+Pj4+ICAgICAgICAgICAgICAgICAgICAgICByZXQgPSBuZnM0
X3NjaGVkdWxlX3N0YXRlaWRfcmVjb3Zlcnkoc2VydmVyLCBzdGF0ZSk7DQo+Pj4+ICAgICAgICAg
ICAgICAgICAgICAgICBpZiAocmV0IDwgMCkNCj4+Pj4gICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgYnJlYWs7DQo+Pj4gDQo+Pj4gTXkgcmVtYWluaW5nIHF1ZXN0aW9uIGhlcmUgaXM6IFdl
IGRvbid0IHNlZW0gdG8gYmUgY29tbXVuaWNhdGluZyB0aGUNCj4+PiBfUkVWT0tFRCBzdGF0ZSBv
ZiB0aGUgZGVsZWdhdGlvbiB0byBuZnM0X3NjaGVkdWxlX3N0YXRlaWRfcmVjb3ZlcnkoKSwNCj4+
PiBzbyB3b24ndCB3ZSBjb250aW51ZSB0byBsb29wIGlmIHN0YXRlICE9IE5VTEw/DQo+PiANCj4+
IEnigJltIG5vdCBmb2xsb3dpbmcuIFRoaXMgcGF0Y2ggaXMgdG8gaGFuZGxlIHRoZSBjYXNlIHdo
ZW4gc3RhdGUgaXMgTlVMTCBhbmQgdGhlcmVmb3JlIHdlIGNhbuKAmXQgaW5pdGlhdGUgcmVjb3Zl
cnkuIEJlZm9yZSB0aGlzIHBhdGNoLCB0aGUgcHJvYmxlbSB3YXMgdGhhdCB3aGVuIHN0YXRlIHdh
cyBOVUxMIHdlIGRpZG7igJl0IChhKSBjbGVhbiB1cCBieSBkb2luZyBkZWxlZ3JldHVybiBhbmQg
KGIpIHdlIHdlcmVu4oCZdCByZXRyeWluZyB3aGVuIHdlIGNhbiDigJQgaS5lLiB3aXRoIGEgc3Bl
Y2lhbCBzdGF0ZWlkIGZvciBTRVRBVFRSLg0KPj4gDQo+PiBJZiBzdGF0ZSBpcyBub3QgbnVsbCwg
SeKAmW0gYXNzdW1pbmcgdGhlIGNvZGUgaXMgaGFuZGxpbmcgY29ycmVjdGx5IGJ5IGNhbGxpbmcg
bmZzNF9zY2hlZHVsZV9zdGF0ZWlkX3JlY292ZXJ5LiBUaGUgY29ycmVjdG5lc3Mgb2YgdGhhdCBj
b2RlIGlzIGJleW9uZCB0aGUgc2NvcGUgb2YgdGhlIHByb2JsZW0gaeKAmW0gcHJvcG9zaW5nIHRv
IHNvbHZlIGhlcmUuDQo+IA0KPiBUaGF0J3MgbXkgcG9pbnQuIEknbSBub3Qgc2VlaW5nIHRoYXQg
dGhlIGNvZGUgaXMgaGFuZGxpbmcgdGhhdCBjYXNlLA0KPiBhbmQgc28gSSdtIG5vdCBzZWVpbmcg
d2h5IHdlIHNob3VsZCB0cmVhdCBzdGF0ZSA9PSBOVUxMIGFzIGJlaW5nDQo+IHNwZWNpYWwuDQo+
IA0KDQpUaGUgY2FzZSDigJxzdGF0ZT09bnVsbOKAnSBpcyBzcGVjaWFsIGJlY2F1c2Ugc3RhdGUg
aXMgbnVsbCBzbyB3ZSBjYW7igJl0IGluaXRpYXRlIHN0YXRlIHJlY292ZXJ5LiBXaGlsZSBjYWxs
aW5nIG5mczRfaW5vZGVfcmV0dXJuX2RlbGVnYXRpb24gaXMganVzdCB0byBkbyBpbnRlcm5hbCBj
bGVhbiB1cCwgdGhlIGltcG9ydGFudCBwaWVjZSBpbiB0aGlzIGNhc2UgaXMgdG8gcmV0cnkgYmVj
YXVzZSBpbiBTRVRBVFRSIGNhc2Ugd2UgaGF2ZSBhIHNwZWNpYWwgc3RhdGVpZCB0byB0cnkuIFdo
ZW4gc3RhdGUgaXNu4oCZdCBudWxsLCBpbml0aWF0aW5nIHJlY292ZXJ5IGFuZCB0cnlpbmcgdG8g
cmVjbGFpbSBvcGVuIHN0YXRlIGJlZm9yZSByZXR1cmluZyB0aGUgZGVsZWdhdGlvbiBzdGF0ZSB0
aGF04oCZcyB0aGUgc2VydmVyIGlzIGNvbXBsYWluaW5nIGFib3V0IGlzIG5lZWRlZCBiZWZvcmUg
cmV0dXJuaW5nIHRoZSBkZWxlZ2F0aW9uLiBUaGVyZWZvcmUgY2FsbGluZyBuZnM0X2lub2RlX3Jl
dHVybl9kZWxlZ2F0aW9uKCkgd291bGQgbm90IGJlIGFwcHJvcHJpYXRlIGxpa2UgaXTigJlzIGRv
bmUgaW4gT1BFTk1PREUgY2FzZS4NCg0KR2l2ZW4gY3VycmVudCB0ZXN0aW5nLCBJIGRvbuKAmXQg
c2VlIGFuIGlzc3VlIG9mIHJlY292ZXJ5IGZyb20gcmVjZWl2aW5nIEJBRF9TVEFURUlEIG9uIGEg
c3luY2hyb25vdXMgb3BlcmF0aW9uIHdoZW4gc3RhdGUgaXMgbm90IE5VTEwuIFlldCwgSSBoYXZl
IGEgc3BlY2lhbCB0ZXN0IGZhaWx1cmUgY2FzZSBmb3Igd2hlbiBzdGF0ZSBpcyBOVUxMLiBUaHVz
IEnigJltIHRyeWluZyB0byBnZXQgdGhpcyBmaXggaW4uIElmIGFub3RoZXIgcHJvYmxlbSBpcyB1
bmNvdmVyZWQgZm9yIHdoZW4gc3RhdGUgaXMgbm90IG51bGwsIEkgdGhpbmsgdGhhdCB3b3VsZCBi
ZSB0aGUgdGltZSB0byBmaXggdGhhdCBwcm9ibGVtPw0KDQoNCj4+IA0KPj4gDQo+Pj4gSU9XOiBp
cyB0aGVyZSBhbnkgcmVhc29uIG5vdCB0byBkbyB0aGUgc2FtZSB0aGluZyBoZXJlIHRoYXQgd2Ug
YWxyZWFkeQ0KPj4+IGRvIGZvciBORlM0RVJSX09QRU5NT0RFPw0KPj4gDQo+PiBUaGUgT1BFTk1P
REUgZXJyb3IgbW9kZSBjb2RlIHNlZW1zIHRvIGhhdmUgc2xpZ2h0bHkgZGlmZmVyZW50IGxvZ2lj
LiBJdCB3YW50cyB0byByZXR1cm4gIHRoZSBkZWxlZ2F0aW9uIHJlZ2FyZGxlc3Mgb2Ygd2hldGhl
ciBvciBub3Qgc3RhdGUgaXMgTlVMTC4NCj4gDQo+IFNlZSBhYm92ZS4NCg0K

Re: [PATCH 1/1] Recover from stateid-type error on SETATTR

[Trond Myklebust]

On Fri, Jun 12, 2015 at 2:48 PM, Kornievskaia, Olga
<Olga.Kornievskaia@netapp.com> wrote:
>
>> On Jun 12, 2015, at 2:37 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:
>>
>> Hi Olga,
>>
>> On Fri, Jun 12, 2015 at 2:30 PM, Olga Kornievskaia <kolga@netapp.com> wrote:
>>> Client can receives stateid-type error (eg., BAD_STATEID) on SETATTR when
>>> delegation stateid was used. When no open state exists, in case of application
>>> calling truncate() on the file, client has no state to recover and fails with
>>> EIO.
>>>
>>> Instead, upon such error, return the bad delegation and then resend the
>>> SETATTR with a zero stateid.
>>>
>>> Signed-off: Olga Kornievskaia <kolga@netapp.com>
>>> ---
>>> fs/nfs/delegation.h | 6 ++++++
>>> fs/nfs/nfs4proc.c   | 7 ++++++-
>>> 2 files changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
>>> index e3c20a3..e37165f 100644
>>> --- a/fs/nfs/delegation.h
>>> +++ b/fs/nfs/delegation.h
>>> @@ -62,6 +62,12 @@ void nfs_mark_delegation_referenced(struct nfs_delegation *delegation);
>>> int nfs4_have_delegation(struct inode *inode, fmode_t flags);
>>> int nfs4_check_delegation(struct inode *inode, fmode_t flags);
>>>
>>> +static inline int nfs4_have_any_delegation(struct inode *inode)
>>> +{
>>> +       struct nfs_inode *nfsi = NFS_I(inode);
>>> +       return rcu_access_pointer(nfsi->delegation) ? 1 : 0;
>>> +}
>>
>> What would this do that isn't already covered by
>> nfs4_have_delegation(inode, FMODE_READ)?
>
> We need to return delegation regardless of whether it’s read or write.

Yes, and the test for nfs4_have_delegation(inode, FMODE_READ) is
intentionally satisfied when you hold a write delegation, because it
guarantees a superset of the caching guarantees that a read delegation
would offer.

Cheers
  Trond

Re: [PATCH 1/1] Recover from stateid-type error on SETATTR

[Anna Schumaker]

Hey Trond,

On 06/12/2015 02:37 PM, Trond Myklebust wrote:
> Hi Olga,
> 
> On Fri, Jun 12, 2015 at 2:30 PM, Olga Kornievskaia <kolga@netapp.com> wrote:
>> Client can receives stateid-type error (eg., BAD_STATEID) on SETATTR when
>> delegation stateid was used. When no open state exists, in case of application
>> calling truncate() on the file, client has no state to recover and fails with
>> EIO.
>>
>> Instead, upon such error, return the bad delegation and then resend the
>> SETATTR with a zero stateid.
>>
>> Signed-off: Olga Kornievskaia <kolga@netapp.com>
>> ---
>>  fs/nfs/delegation.h | 6 ++++++
>>  fs/nfs/nfs4proc.c   | 7 ++++++-
>>  2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
>> index e3c20a3..e37165f 100644
>> --- a/fs/nfs/delegation.h
>> +++ b/fs/nfs/delegation.h
>> @@ -62,6 +62,12 @@ void nfs_mark_delegation_referenced(struct nfs_delegation *delegation);
>>  int nfs4_have_delegation(struct inode *inode, fmode_t flags);
>>  int nfs4_check_delegation(struct inode *inode, fmode_t flags);
>>
>> +static inline int nfs4_have_any_delegation(struct inode *inode)
>> +{
>> +       struct nfs_inode *nfsi = NFS_I(inode);
>> +       return rcu_access_pointer(nfsi->delegation) ? 1 : 0;
>> +}
> 
> What would this do that isn't already covered by
> nfs4_have_delegation(inode, FMODE_READ)?

It looks like _nfs4_do_settattr() can select between FMODE_READ and FMODE_WRITE, so wouldn't we otherwise need to call nfs4_have_delegation() twice?  Or am I missing something?

Anna

> 
> Cheers
>   Trond
> 

Re: [PATCH 1/1] Recover from stateid-type error on SETATTR

[Kornievskaia, Olga]

DQo+IE9uIEp1biAxMiwgMjAxNSwgYXQgMjozNyBQTSwgVHJvbmQgTXlrbGVidXN0IDx0cm9uZC5t
eWtsZWJ1c3RAcHJpbWFyeWRhdGEuY29tPiB3cm90ZToNCj4gDQo+IEhpIE9sZ2EsDQo+IA0KPiBP
biBGcmksIEp1biAxMiwgMjAxNSBhdCAyOjMwIFBNLCBPbGdhIEtvcm5pZXZza2FpYSA8a29sZ2FA
bmV0YXBwLmNvbT4gd3JvdGU6DQo+PiBDbGllbnQgY2FuIHJlY2VpdmVzIHN0YXRlaWQtdHlwZSBl
cnJvciAoZWcuLCBCQURfU1RBVEVJRCkgb24gU0VUQVRUUiB3aGVuDQo+PiBkZWxlZ2F0aW9uIHN0
YXRlaWQgd2FzIHVzZWQuIFdoZW4gbm8gb3BlbiBzdGF0ZSBleGlzdHMsIGluIGNhc2Ugb2YgYXBw
bGljYXRpb24NCj4+IGNhbGxpbmcgdHJ1bmNhdGUoKSBvbiB0aGUgZmlsZSwgY2xpZW50IGhhcyBu
byBzdGF0ZSB0byByZWNvdmVyIGFuZCBmYWlscyB3aXRoDQo+PiBFSU8uDQo+PiANCj4+IEluc3Rl
YWQsIHVwb24gc3VjaCBlcnJvciwgcmV0dXJuIHRoZSBiYWQgZGVsZWdhdGlvbiBhbmQgdGhlbiBy
ZXNlbmQgdGhlDQo+PiBTRVRBVFRSIHdpdGggYSB6ZXJvIHN0YXRlaWQuDQo+PiANCj4+IFNpZ25l
ZC1vZmY6IE9sZ2EgS29ybmlldnNrYWlhIDxrb2xnYUBuZXRhcHAuY29tPg0KPj4gLS0tDQo+PiBm
cy9uZnMvZGVsZWdhdGlvbi5oIHwgNiArKysrKysNCj4+IGZzL25mcy9uZnM0cHJvYy5jICAgfCA3
ICsrKysrKy0NCj4+IDIgZmlsZXMgY2hhbmdlZCwgMTIgaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlv
bigtKQ0KPj4gDQo+PiBkaWZmIC0tZ2l0IGEvZnMvbmZzL2RlbGVnYXRpb24uaCBiL2ZzL25mcy9k
ZWxlZ2F0aW9uLmgNCj4+IGluZGV4IGUzYzIwYTMuLmUzNzE2NWYgMTAwNjQ0DQo+PiAtLS0gYS9m
cy9uZnMvZGVsZWdhdGlvbi5oDQo+PiArKysgYi9mcy9uZnMvZGVsZWdhdGlvbi5oDQo+PiBAQCAt
NjIsNiArNjIsMTIgQEAgdm9pZCBuZnNfbWFya19kZWxlZ2F0aW9uX3JlZmVyZW5jZWQoc3RydWN0
IG5mc19kZWxlZ2F0aW9uICpkZWxlZ2F0aW9uKTsNCj4+IGludCBuZnM0X2hhdmVfZGVsZWdhdGlv
bihzdHJ1Y3QgaW5vZGUgKmlub2RlLCBmbW9kZV90IGZsYWdzKTsNCj4+IGludCBuZnM0X2NoZWNr
X2RlbGVnYXRpb24oc3RydWN0IGlub2RlICppbm9kZSwgZm1vZGVfdCBmbGFncyk7DQo+PiANCj4+
ICtzdGF0aWMgaW5saW5lIGludCBuZnM0X2hhdmVfYW55X2RlbGVnYXRpb24oc3RydWN0IGlub2Rl
ICppbm9kZSkNCj4+ICt7DQo+PiArICAgICAgIHN0cnVjdCBuZnNfaW5vZGUgKm5mc2kgPSBORlNf
SShpbm9kZSk7DQo+PiArICAgICAgIHJldHVybiByY3VfYWNjZXNzX3BvaW50ZXIobmZzaS0+ZGVs
ZWdhdGlvbikgPyAxIDogMDsNCj4+ICt9DQo+IA0KPiBXaGF0IHdvdWxkIHRoaXMgZG8gdGhhdCBp
c24ndCBhbHJlYWR5IGNvdmVyZWQgYnkNCj4gbmZzNF9oYXZlX2RlbGVnYXRpb24oaW5vZGUsIEZN
T0RFX1JFQUQpPw0KDQpXZSBuZWVkIHRvIHJldHVybiBkZWxlZ2F0aW9uIHJlZ2FyZGxlc3Mgb2Yg
d2hldGhlciBpdOKAmXMgcmVhZCBvciB3cml0ZS4gDQoNCj4gDQo+IENoZWVycw0KPiAgVHJvbmQN
Cg0K

Re: [PATCH 1/1] Recover from stateid-type error on SETATTR

[Trond Myklebust]

Hi Olga,

On Fri, Jun 12, 2015 at 2:30 PM, Olga Kornievskaia <kolga@netapp.com> wrote:
> Client can receives stateid-type error (eg., BAD_STATEID) on SETATTR when
> delegation stateid was used. When no open state exists, in case of application
> calling truncate() on the file, client has no state to recover and fails with
> EIO.
>
> Instead, upon such error, return the bad delegation and then resend the
> SETATTR with a zero stateid.
>
> Signed-off: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfs/delegation.h | 6 ++++++
>  fs/nfs/nfs4proc.c   | 7 ++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
> index e3c20a3..e37165f 100644
> --- a/fs/nfs/delegation.h
> +++ b/fs/nfs/delegation.h
> @@ -62,6 +62,12 @@ void nfs_mark_delegation_referenced(struct nfs_delegation *delegation);
>  int nfs4_have_delegation(struct inode *inode, fmode_t flags);
>  int nfs4_check_delegation(struct inode *inode, fmode_t flags);
>
> +static inline int nfs4_have_any_delegation(struct inode *inode)
> +{
> +       struct nfs_inode *nfsi = NFS_I(inode);
> +       return rcu_access_pointer(nfsi->delegation) ? 1 : 0;
> +}

What would this do that isn't already covered by
nfs4_have_delegation(inode, FMODE_READ)?

Cheers
  Trond

[PATCH 1/1] Recover from stateid-type error on SETATTR

[Olga Kornievskaia]

Client can receives stateid-type error (eg., BAD_STATEID) on SETATTR when
delegation stateid was used. When no open state exists, in case of application
calling truncate() on the file, client has no state to recover and fails with
EIO.

Instead, upon such error, return the bad delegation and then resend the
SETATTR with a zero stateid.

Signed-off: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfs/delegation.h | 6 ++++++
 fs/nfs/nfs4proc.c   | 7 ++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
index e3c20a3..e37165f 100644
--- a/fs/nfs/delegation.h
+++ b/fs/nfs/delegation.h
@@ -62,6 +62,12 @@ void nfs_mark_delegation_referenced(struct nfs_delegation *delegation);
 int nfs4_have_delegation(struct inode *inode, fmode_t flags);
 int nfs4_check_delegation(struct inode *inode, fmode_t flags);
 
+static inline int nfs4_have_any_delegation(struct inode *inode)
+{
+	struct nfs_inode *nfsi = NFS_I(inode);
+	return rcu_access_pointer(nfsi->delegation) ? 1 : 0;
+}
+
 #endif
 
 static inline int nfs_have_delegated_attributes(struct inode *inode)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ad7cf7e..63b3581 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -360,8 +360,13 @@ static int nfs4_handle_exception(struct nfs_server *server, int errorcode, struc
 		case -NFS4ERR_DELEG_REVOKED:
 		case -NFS4ERR_ADMIN_REVOKED:
 		case -NFS4ERR_BAD_STATEID:
-			if (state == NULL)
+			if (state == NULL) {
+				if (nfs4_have_any_delegation(inode)) {
+					nfs4_inode_return_delegation(inode);
+					exception->retry = 1;
+				}
 				break;
+			}
 			ret = nfs4_schedule_stateid_recovery(server, state);
 			if (ret < 0)
 				break;
-- 
1.8.3.1

Re: [PATCH 1/1] Recover from stateid-type error on SETATTR

[Olga Kornievskaia]

Trond,

Can you provide comments on this patch? Thanks.

On Mon, Apr 27, 2015 at 1:48 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> Any comments on this patch?
>
> On Tue, Apr 21, 2015 at 6:36 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>> Client can receives stateid-type error (eg., BAD_STATEID) on SETATTR
>> when delegation stateid was used. When no open state exists, in case
>> of application calling truncate() on the file, client has no state to
>> recover and fails with EIO.
>>
>> Instead, upon such error, return the bad delegation and then resend the
>> SETATTR with a zero stateid. In general, when something calls
>> nfs4_handle_exception() with a null state, the operation should be
>> retried after bad delegation is removed.
>>
>> Signed-off: Olga Kornievskaia <kolga@netapp.com>
>> ---
>>  fs/nfs/nfs4proc.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index ad7cf7e..fbde292 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -360,8 +360,11 @@ static int nfs4_handle_exception(struct
>> nfs_server *server, int errorcode, struc
>>          case -NFS4ERR_DELEG_REVOKED:
>>          case -NFS4ERR_ADMIN_REVOKED:
>>          case -NFS4ERR_BAD_STATEID:
>> -            if (state == NULL)
>> +            if (state == NULL) {
>> +                nfs4_inode_return_delegation(inode);
>> +                exception->retry = 1;
>>                  break;
>> +            }
>>              ret = nfs4_schedule_stateid_recovery(server, state);
>>              if (ret < 0)
>>                  break;
>> --

Re: [PATCH 1/1] Recover from stateid-type error on SETATTR

[Olga Kornievskaia]

Any comments on this patch?

On Tue, Apr 21, 2015 at 6:36 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> Client can receives stateid-type error (eg., BAD_STATEID) on SETATTR
> when delegation stateid was used. When no open state exists, in case
> of application calling truncate() on the file, client has no state to
> recover and fails with EIO.
>
> Instead, upon such error, return the bad delegation and then resend the
> SETATTR with a zero stateid. In general, when something calls
> nfs4_handle_exception() with a null state, the operation should be
> retried after bad delegation is removed.
>
> Signed-off: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfs/nfs4proc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index ad7cf7e..fbde292 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -360,8 +360,11 @@ static int nfs4_handle_exception(struct
> nfs_server *server, int errorcode, struc
>          case -NFS4ERR_DELEG_REVOKED:
>          case -NFS4ERR_ADMIN_REVOKED:
>          case -NFS4ERR_BAD_STATEID:
> -            if (state == NULL)
> +            if (state == NULL) {
> +                nfs4_inode_return_delegation(inode);
> +                exception->retry = 1;
>                  break;
> +            }
>              ret = nfs4_schedule_stateid_recovery(server, state);
>              if (ret < 0)
>                  break;
> --

[PATCH 1/1] Recover from stateid-type error on SETATTR

[Olga Kornievskaia]

Client can receives stateid-type error (eg., BAD_STATEID) on SETATTR
when delegation stateid was used. When no open state exists, in case
of application calling truncate() on the file, client has no state to
recover and fails with EIO.

Instead, upon such error, return the bad delegation and then resend the
SETATTR with a zero stateid. In general, when something calls
nfs4_handle_exception() with a null state, the operation should be
retried after bad delegation is removed.

Signed-off: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfs/nfs4proc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ad7cf7e..fbde292 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -360,8 +360,11 @@ static int nfs4_handle_exception(struct
nfs_server *server, int errorcode, struc
         case -NFS4ERR_DELEG_REVOKED:
         case -NFS4ERR_ADMIN_REVOKED:
         case -NFS4ERR_BAD_STATEID:
-            if (state == NULL)
+            if (state == NULL) {
+                nfs4_inode_return_delegation(inode);
+                exception->retry = 1;
                 break;
+            }
             ret = nfs4_schedule_stateid_recovery(server, state);
             if (ret < 0)
                 break;
--