Trond Myklebust (2):
NFSv4: Fix CLOSE races with OPEN
NFSv4: Don't call close if the open stateid has already been cleared
fs/nfs/nfs4_fs.h | 7 +++++++
fs/nfs/nfs4proc.c | 15 ++++++++-------
2 files changed, 15 insertions(+), 7 deletions(-)
--
2.7.4
If the reply to a successful CLOSE call races with an OPEN to the same
file, we can end up scribbling over the stateid that represents the
new open state.
The race looks like:
Client Server
====== ======
CLOSE stateid A on file "foo"
CLOSE stateid A, return stateid C
OPEN file "foo"
OPEN "foo", return stateid B
Receive reply to OPEN
Reset open state for "foo"
Associate stateid B to "foo"
Receive CLOSE for A
Reset open state for "foo"
Replace stateid B with C
The fix is to examine the argument of the CLOSE, and check for a match
with the current stateid "other" field. If the two do not match, then
the above race occurred, and we should just ignore the CLOSE.
Reported-by: Benjamin Coddington <[email protected]>
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs4_fs.h | 7 +++++++
fs/nfs/nfs4proc.c | 12 ++++++------
2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index 9b3a82abab07..1452177c822d 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -542,6 +542,13 @@ static inline bool nfs4_valid_open_stateid(const struct nfs4_state *state)
return test_bit(NFS_STATE_RECOVERY_FAILED, &state->flags) == 0;
}
+static inline bool nfs4_state_match_open_stateid_other(const struct nfs4_state *state,
+ const nfs4_stateid *stateid)
+{
+ return test_bit(NFS_OPEN_STATE, &state->flags) &&
+ nfs4_stateid_match_other(&state->open_stateid, stateid);
+}
+
#else
#define nfs4_close_state(a, b) do { } while (0)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index f550ac69ffa0..b7b0080977c0 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1458,7 +1458,6 @@ static void nfs_resync_open_stateid_locked(struct nfs4_state *state)
}
static void nfs_clear_open_stateid_locked(struct nfs4_state *state,
- nfs4_stateid *arg_stateid,
nfs4_stateid *stateid, fmode_t fmode)
{
clear_bit(NFS_O_RDWR_STATE, &state->flags);
@@ -1476,10 +1475,9 @@ static void nfs_clear_open_stateid_locked(struct nfs4_state *state,
}
if (stateid == NULL)
return;
- /* Handle races with OPEN */
- if (!nfs4_stateid_match_other(arg_stateid, &state->open_stateid) ||
- (nfs4_stateid_match_other(stateid, &state->open_stateid) &&
- !nfs4_stateid_is_newer(stateid, &state->open_stateid))) {
+ /* Handle OPEN+OPEN_DOWNGRADE races */
+ if (nfs4_stateid_match_other(stateid, &state->open_stateid) &&
+ !nfs4_stateid_is_newer(stateid, &state->open_stateid)) {
nfs_resync_open_stateid_locked(state);
return;
}
@@ -1493,7 +1491,9 @@ static void nfs_clear_open_stateid(struct nfs4_state *state,
nfs4_stateid *stateid, fmode_t fmode)
{
write_seqlock(&state->seqlock);
- nfs_clear_open_stateid_locked(state, arg_stateid, stateid, fmode);
+ /* Ignore, if the CLOSE argment doesn't match the current stateid */
+ if (nfs4_state_match_open_stateid_other(state, arg_stateid))
+ nfs_clear_open_stateid_locked(state, stateid, fmode);
write_sequnlock(&state->seqlock);
if (test_bit(NFS_STATE_RECLAIM_NOGRACE, &state->flags))
nfs4_schedule_state_manager(state->owner->so_server->nfs_client);
--
2.7.4
Ensure we test to see if the open stateid is actually set, before we
send a CLOSE.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs4proc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index b7b0080977c0..b801040c9585 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3129,7 +3129,8 @@ static void nfs4_close_prepare(struct rpc_task *task, void *data)
} else if (is_rdwr)
calldata->arg.fmode |= FMODE_READ|FMODE_WRITE;
- if (!nfs4_valid_open_stateid(state))
+ if (!nfs4_valid_open_stateid(state) ||
+ test_bit(NFS_OPEN_STATE, &state->flags) == 0)
call_close = 0;
spin_unlock(&state->owner->so_lock);
--
2.7.4
On Mon, 2016-11-14 at 11:19 -0500, Trond Myklebust wrote:
> If the reply to a successful CLOSE call races with an OPEN to the same
> file, we can end up scribbling over the stateid that represents the
> new open state.
> The race looks like:
>
> Client Server
> ====== ======
>
> CLOSE stateid A on file "foo"
> CLOSE stateid A, return stateid C
> OPEN file "foo"
> OPEN "foo", return stateid B
> Receive reply to OPEN
> Reset open state for "foo"
> Associate stateid B to "foo"
>
> Receive CLOSE for A
> Reset open state for "foo"
> Replace stateid B with C
>
> The fix is to examine the argument of the CLOSE, and check for a match
> with the current stateid "other" field. If the two do not match, then
> the above race occurred, and we should just ignore the CLOSE.
>
> Reported-by: Benjamin Coddington <[email protected]>
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> fs/nfs/nfs4_fs.h | 7 +++++++
> fs/nfs/nfs4proc.c | 12 ++++++------
> 2 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> index 9b3a82abab07..1452177c822d 100644
> --- a/fs/nfs/nfs4_fs.h
> +++ b/fs/nfs/nfs4_fs.h
> @@ -542,6 +542,13 @@ static inline bool nfs4_valid_open_stateid(const struct nfs4_state *state)
> return test_bit(NFS_STATE_RECOVERY_FAILED, &state->flags) == 0;
> }
>
> +static inline bool nfs4_state_match_open_stateid_other(const struct nfs4_state *state,
> + const nfs4_stateid *stateid)
> +{
> + return test_bit(NFS_OPEN_STATE, &state->flags) &&
> + nfs4_stateid_match_other(&state->open_stateid, stateid);
> +}
> +
> #else
>
> #define nfs4_close_state(a, b) do { } while (0)
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index f550ac69ffa0..b7b0080977c0 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -1458,7 +1458,6 @@ static void nfs_resync_open_stateid_locked(struct nfs4_state *state)
> }
>
> static void nfs_clear_open_stateid_locked(struct nfs4_state *state,
> - nfs4_stateid *arg_stateid,
> nfs4_stateid *stateid, fmode_t fmode)
> {
> clear_bit(NFS_O_RDWR_STATE, &state->flags);
> @@ -1476,10 +1475,9 @@ static void nfs_clear_open_stateid_locked(struct nfs4_state *state,
> }
> if (stateid == NULL)
> return;
> - /* Handle races with OPEN */
> - if (!nfs4_stateid_match_other(arg_stateid, &state->open_stateid) ||
> - (nfs4_stateid_match_other(stateid, &state->open_stateid) &&
> - !nfs4_stateid_is_newer(stateid, &state->open_stateid))) {
> + /* Handle OPEN+OPEN_DOWNGRADE races */
> + if (nfs4_stateid_match_other(stateid, &state->open_stateid) &&
> + !nfs4_stateid_is_newer(stateid, &state->open_stateid)) {
> nfs_resync_open_stateid_locked(state);
> return;
> }
> @@ -1493,7 +1491,9 @@ static void nfs_clear_open_stateid(struct nfs4_state *state,
> nfs4_stateid *stateid, fmode_t fmode)
> {
> write_seqlock(&state->seqlock);
> - nfs_clear_open_stateid_locked(state, arg_stateid, stateid, fmode);
> + /* Ignore, if the CLOSE argment doesn't match the current stateid */
> + if (nfs4_state_match_open_stateid_other(state, arg_stateid))
> + nfs_clear_open_stateid_locked(state, stateid, fmode);
> write_sequnlock(&state->seqlock);
> if (test_bit(NFS_STATE_RECLAIM_NOGRACE, &state->flags))
> nfs4_schedule_state_manager(state->owner->so_server->nfs_client);
I still don't quite get it. What's the point of paying any attention at
all to the stateid returned by the server in a CLOSE response? It's
either:
a) completely bogus, if the server is following the SHOULD in RFC5661,
section 18.2.4
...or...
b) refers to a now-defunct stateid -- probably a later version of the
one sent in the request, but the spec doesn't really spell that out,
AFAICT.
In either case, I don't think it ought to be trusted. Why not just use
the arg_stateid universally here?
--
Jeff Layton <[email protected]>
T24gTW9uLCAyMDE2LTExLTE0IGF0IDExOjQwIC0wNTAwLCBKZWZmIExheXRvbiB3cm90ZToNCj4g
T24gTW9uLCAyMDE2LTExLTE0IGF0IDExOjE5IC0wNTAwLCBUcm9uZCBNeWtsZWJ1c3Qgd3JvdGU6
DQo+ID4gDQo+ID4gSWYgdGhlIHJlcGx5IHRvIGEgc3VjY2Vzc2Z1bCBDTE9TRSBjYWxsIHJhY2Vz
IHdpdGggYW4gT1BFTiB0byB0aGUNCj4gPiBzYW1lDQo+ID4gZmlsZSwgd2UgY2FuIGVuZCB1cCBz
Y3JpYmJsaW5nIG92ZXIgdGhlIHN0YXRlaWQgdGhhdCByZXByZXNlbnRzIHRoZQ0KPiA+IG5ldyBv
cGVuIHN0YXRlLg0KPiA+IFRoZSByYWNlIGxvb2tzIGxpa2U6DQo+ID4gDQo+ID4gwqAgQ2xpZW50
CQkJCVNlcnZlcg0KPiA+IMKgID09PT09PQkJCQk9PT09PT0NCj4gPiANCj4gPiDCoCBDTE9TRSBz
dGF0ZWlkIEEgb24gZmlsZSAiZm9vIg0KPiA+IAkJCQkJQ0xPU0Ugc3RhdGVpZCBBLCByZXR1cm4g
c3RhdGVpZA0KPiA+IEMNCj4gPiDCoCBPUEVOIGZpbGUgImZvbyINCj4gPiAJCQkJCU9QRU4gImZv
byIsIHJldHVybiBzdGF0ZWlkIEINCj4gPiDCoCBSZWNlaXZlIHJlcGx5IHRvIE9QRU4NCj4gPiDC
oCBSZXNldCBvcGVuIHN0YXRlIGZvciAiZm9vIg0KPiA+IMKgIEFzc29jaWF0ZSBzdGF0ZWlkIEIg
dG8gImZvbyINCj4gPiANCj4gPiDCoCBSZWNlaXZlIENMT1NFIGZvciBBDQo+ID4gwqAgUmVzZXQg
b3BlbiBzdGF0ZSBmb3IgImZvbyINCj4gPiDCoCBSZXBsYWNlIHN0YXRlaWQgQiB3aXRoIEMNCj4g
PiANCj4gPiBUaGUgZml4IGlzIHRvIGV4YW1pbmUgdGhlIGFyZ3VtZW50IG9mIHRoZSBDTE9TRSwg
YW5kIGNoZWNrIGZvciBhDQo+ID4gbWF0Y2gNCj4gPiB3aXRoIHRoZSBjdXJyZW50IHN0YXRlaWQg
Im90aGVyIiBmaWVsZC4gSWYgdGhlIHR3byBkbyBub3QgbWF0Y2gsDQo+ID4gdGhlbg0KPiA+IHRo
ZSBhYm92ZSByYWNlIG9jY3VycmVkLCBhbmQgd2Ugc2hvdWxkIGp1c3QgaWdub3JlIHRoZSBDTE9T
RS4NCj4gPiANCj4gPiBSZXBvcnRlZC1ieTogQmVuamFtaW4gQ29kZGluZ3RvbiA8YmNvZGRpbmdA
cmVkaGF0LmNvbT4NCj4gPiBTaWduZWQtb2ZmLWJ5OiBUcm9uZCBNeWtsZWJ1c3QgPHRyb25kLm15
a2xlYnVzdEBwcmltYXJ5ZGF0YS5jb20+DQo+ID4gLS0tDQo+ID4gwqBmcy9uZnMvbmZzNF9mcy5o
wqDCoHzCoMKgNyArKysrKysrDQo+ID4gwqBmcy9uZnMvbmZzNHByb2MuYyB8IDEyICsrKysrKy0t
LS0tLQ0KPiA+IMKgMiBmaWxlcyBjaGFuZ2VkLCAxMyBpbnNlcnRpb25zKCspLCA2IGRlbGV0aW9u
cygtKQ0KPiA+IA0KPiA+IGRpZmYgLS1naXQgYS9mcy9uZnMvbmZzNF9mcy5oIGIvZnMvbmZzL25m
czRfZnMuaA0KPiA+IGluZGV4IDliM2E4MmFiYWIwNy4uMTQ1MjE3N2M4MjJkIDEwMDY0NA0KPiA+
IC0tLSBhL2ZzL25mcy9uZnM0X2ZzLmgNCj4gPiArKysgYi9mcy9uZnMvbmZzNF9mcy5oDQo+ID4g
QEAgLTU0Miw2ICs1NDIsMTMgQEAgc3RhdGljIGlubGluZSBib29sDQo+ID4gbmZzNF92YWxpZF9v
cGVuX3N0YXRlaWQoY29uc3Qgc3RydWN0IG5mczRfc3RhdGUgKnN0YXRlKQ0KPiA+IMKgCXJldHVy
biB0ZXN0X2JpdChORlNfU1RBVEVfUkVDT1ZFUllfRkFJTEVELCAmc3RhdGUtPmZsYWdzKQ0KPiA+
ID09IDA7DQo+ID4gwqB9DQo+ID4gwqANCj4gPiArc3RhdGljIGlubGluZSBib29sIG5mczRfc3Rh
dGVfbWF0Y2hfb3Blbl9zdGF0ZWlkX290aGVyKGNvbnN0DQo+ID4gc3RydWN0IG5mczRfc3RhdGUg
KnN0YXRlLA0KPiA+ICsJCWNvbnN0IG5mczRfc3RhdGVpZCAqc3RhdGVpZCkNCj4gPiArew0KPiA+
ICsJcmV0dXJuIHRlc3RfYml0KE5GU19PUEVOX1NUQVRFLCAmc3RhdGUtPmZsYWdzKSAmJg0KPiA+
ICsJCW5mczRfc3RhdGVpZF9tYXRjaF9vdGhlcigmc3RhdGUtPm9wZW5fc3RhdGVpZCwNCj4gPiBz
dGF0ZWlkKTsNCj4gPiArfQ0KPiA+ICsNCj4gPiDCoCNlbHNlDQo+ID4gwqANCj4gPiDCoCNkZWZp
bmUgbmZzNF9jbG9zZV9zdGF0ZShhLCBiKSBkbyB7IH0gd2hpbGUgKDApDQo+ID4gZGlmZiAtLWdp
dCBhL2ZzL25mcy9uZnM0cHJvYy5jIGIvZnMvbmZzL25mczRwcm9jLmMNCj4gPiBpbmRleCBmNTUw
YWM2OWZmYTAuLmI3YjAwODA5NzdjMCAxMDA2NDQNCj4gPiAtLS0gYS9mcy9uZnMvbmZzNHByb2Mu
Yw0KPiA+ICsrKyBiL2ZzL25mcy9uZnM0cHJvYy5jDQo+ID4gQEAgLTE0NTgsNyArMTQ1OCw2IEBA
IHN0YXRpYyB2b2lkDQo+ID4gbmZzX3Jlc3luY19vcGVuX3N0YXRlaWRfbG9ja2VkKHN0cnVjdCBu
ZnM0X3N0YXRlICpzdGF0ZSkNCj4gPiDCoH0NCj4gPiDCoA0KPiA+IMKgc3RhdGljIHZvaWQgbmZz
X2NsZWFyX29wZW5fc3RhdGVpZF9sb2NrZWQoc3RydWN0IG5mczRfc3RhdGUNCj4gPiAqc3RhdGUs
DQo+ID4gLQkJbmZzNF9zdGF0ZWlkICphcmdfc3RhdGVpZCwNCj4gPiDCoAkJbmZzNF9zdGF0ZWlk
ICpzdGF0ZWlkLCBmbW9kZV90IGZtb2RlKQ0KPiA+IMKgew0KPiA+IMKgCWNsZWFyX2JpdChORlNf
T19SRFdSX1NUQVRFLCAmc3RhdGUtPmZsYWdzKTsNCj4gPiBAQCAtMTQ3NiwxMCArMTQ3NSw5IEBA
IHN0YXRpYyB2b2lkDQo+ID4gbmZzX2NsZWFyX29wZW5fc3RhdGVpZF9sb2NrZWQoc3RydWN0IG5m
czRfc3RhdGUgKnN0YXRlLA0KPiA+IMKgCX0NCj4gPiDCoAlpZiAoc3RhdGVpZCA9PSBOVUxMKQ0K
PiA+IMKgCQlyZXR1cm47DQo+ID4gLQkvKiBIYW5kbGUgcmFjZXMgd2l0aCBPUEVOICovDQo+ID4g
LQlpZiAoIW5mczRfc3RhdGVpZF9tYXRjaF9vdGhlcihhcmdfc3RhdGVpZCwgJnN0YXRlLQ0KPiA+
ID5vcGVuX3N0YXRlaWQpIHx8DQo+ID4gLQnCoMKgwqDCoChuZnM0X3N0YXRlaWRfbWF0Y2hfb3Ro
ZXIoc3RhdGVpZCwgJnN0YXRlLQ0KPiA+ID5vcGVuX3N0YXRlaWQpICYmDQo+ID4gLQnCoMKgwqDC
oCFuZnM0X3N0YXRlaWRfaXNfbmV3ZXIoc3RhdGVpZCwgJnN0YXRlLQ0KPiA+ID5vcGVuX3N0YXRl
aWQpKSkgew0KPiA+ICsJLyogSGFuZGxlIE9QRU4rT1BFTl9ET1dOR1JBREUgcmFjZXMgKi8NCj4g
PiArCWlmIChuZnM0X3N0YXRlaWRfbWF0Y2hfb3RoZXIoc3RhdGVpZCwgJnN0YXRlLQ0KPiA+ID5v
cGVuX3N0YXRlaWQpICYmDQo+ID4gKwnCoMKgwqDCoCFuZnM0X3N0YXRlaWRfaXNfbmV3ZXIoc3Rh
dGVpZCwgJnN0YXRlLT5vcGVuX3N0YXRlaWQpKSANCj4gPiB7DQo+ID4gwqAJCW5mc19yZXN5bmNf
b3Blbl9zdGF0ZWlkX2xvY2tlZChzdGF0ZSk7DQo+ID4gwqAJCXJldHVybjsNCj4gPiDCoAl9DQo+
ID4gQEAgLTE0OTMsNyArMTQ5MSw5IEBAIHN0YXRpYyB2b2lkIG5mc19jbGVhcl9vcGVuX3N0YXRl
aWQoc3RydWN0DQo+ID4gbmZzNF9zdGF0ZSAqc3RhdGUsDQo+ID4gwqAJbmZzNF9zdGF0ZWlkICpz
dGF0ZWlkLCBmbW9kZV90IGZtb2RlKQ0KPiA+IMKgew0KPiA+IMKgCXdyaXRlX3NlcWxvY2soJnN0
YXRlLT5zZXFsb2NrKTsNCj4gPiAtCW5mc19jbGVhcl9vcGVuX3N0YXRlaWRfbG9ja2VkKHN0YXRl
LCBhcmdfc3RhdGVpZCwgc3RhdGVpZCwNCj4gPiBmbW9kZSk7DQo+ID4gKwkvKiBJZ25vcmUsIGlm
IHRoZSBDTE9TRSBhcmdtZW50IGRvZXNuJ3QgbWF0Y2ggdGhlIGN1cnJlbnQNCj4gPiBzdGF0ZWlk
ICovDQo+ID4gKwlpZiAobmZzNF9zdGF0ZV9tYXRjaF9vcGVuX3N0YXRlaWRfb3RoZXIoc3RhdGUs
DQo+ID4gYXJnX3N0YXRlaWQpKQ0KPiA+ICsJCW5mc19jbGVhcl9vcGVuX3N0YXRlaWRfbG9ja2Vk
KHN0YXRlLCBzdGF0ZWlkLA0KPiA+IGZtb2RlKTsNCj4gPiDCoAl3cml0ZV9zZXF1bmxvY2soJnN0
YXRlLT5zZXFsb2NrKTsNCj4gPiDCoAlpZiAodGVzdF9iaXQoTkZTX1NUQVRFX1JFQ0xBSU1fTk9H
UkFDRSwgJnN0YXRlLT5mbGFncykpDQo+ID4gwqAJCW5mczRfc2NoZWR1bGVfc3RhdGVfbWFuYWdl
cihzdGF0ZS0+b3duZXItDQo+ID4gPnNvX3NlcnZlci0+bmZzX2NsaWVudCk7DQo+IA0KPiBJIHN0
aWxsIGRvbid0IHF1aXRlIGdldCBpdC4gV2hhdCdzIHRoZSBwb2ludCBvZiBwYXlpbmcgYW55IGF0
dGVudGlvbg0KPiBhdA0KPiBhbGwgdG8gdGhlIHN0YXRlaWQgcmV0dXJuZWQgYnkgdGhlIHNlcnZl
ciBpbiBhIENMT1NFIHJlc3BvbnNlPyBJdCdzDQo+IGVpdGhlcjoNCj4gDQo+IGEpIGNvbXBsZXRl
bHkgYm9ndXMsIGlmIHRoZSBzZXJ2ZXIgaXMgZm9sbG93aW5nIHRoZSBTSE9VTEQgaW4NCj4gUkZD
NTY2MSwNCj4gc2VjdGlvbiAxOC4yLjQNCj4gDQo+IC4uLm9yLi4uDQo+IA0KPiBiKSByZWZlcnMg
dG8gYSBub3ctZGVmdW5jdCBzdGF0ZWlkIC0tIHByb2JhYmx5IGEgbGF0ZXIgdmVyc2lvbiBvZiB0
aGUNCj4gb25lIHNlbnQgaW4gdGhlIHJlcXVlc3QsIGJ1dCB0aGUgc3BlYyBkb2Vzbid0IHJlYWxs
eSBzcGVsbCB0aGF0IG91dCwNCj4gQUZBSUNULg0KPiANCj4gSW4gZWl0aGVyIGNhc2UsIEkgZG9u
J3QgdGhpbmsgaXQgb3VnaHQgdG8gYmUgdHJ1c3RlZC4gV2h5IG5vdCBqdXN0DQo+IHVzZQ0KPiB0
aGUgYXJnX3N0YXRlaWQgdW5pdmVyc2FsbHkgaGVyZT8NCj4gDQoNClRoZSBjb2RlIHBhdGggYWxz
byBoYXMgdG8gaGFuZGxlIE9QRU5fRE9XTkdSQURFLg==
On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust
<[email protected]> wrote:
> If the reply to a successful CLOSE call races with an OPEN to the same
> file, we can end up scribbling over the stateid that represents the
> new open state.
> The race looks like:
>
> Client Server
> ====== ======
>
> CLOSE stateid A on file "foo"
> CLOSE stateid A, return stateid C
Hi folks,
I'd like to understand this particular issue. Specifically I don't
understand how can server return stateid C to the close with stateid
A.
As per RFC 7530 or 5661. It says that state is returned by the close
shouldn't be used.
Even though CLOSE returns a stateid, this stateid is not useful to
the client and should be treated as deprecated. CLOSE "shuts down"
the state associated with all OPENs for the file by a single
open-owner. As noted above, CLOSE will either release all file
locking state or return an error. Therefore, the stateid returned by
CLOSE is not useful for the operations that follow.
Is this because the spec says "should" and not a "must"?
Linux server increments a state's sequenceid on CLOSE. Ontap server
does not. I'm not sure what other servers do. Are all these
implementations equality correct?
> OPEN file "foo"
> OPEN "foo", return stateid B
> Receive reply to OPEN
> Reset open state for "foo"
> Associate stateid B to "foo"
>
> Receive CLOSE for A
> Reset open state for "foo"
> Replace stateid B with C
>
> The fix is to examine the argument of the CLOSE, and check for a match
> with the current stateid "other" field. If the two do not match, then
> the above race occurred, and we should just ignore the CLOSE.
>
> Reported-by: Benjamin Coddington <[email protected]>
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> fs/nfs/nfs4_fs.h | 7 +++++++
> fs/nfs/nfs4proc.c | 12 ++++++------
> 2 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> index 9b3a82abab07..1452177c822d 100644
> --- a/fs/nfs/nfs4_fs.h
> +++ b/fs/nfs/nfs4_fs.h
> @@ -542,6 +542,13 @@ static inline bool nfs4_valid_open_stateid(const struct nfs4_state *state)
> return test_bit(NFS_STATE_RECOVERY_FAILED, &state->flags) == 0;
> }
>
> +static inline bool nfs4_state_match_open_stateid_other(const struct nfs4_state *state,
> + const nfs4_stateid *stateid)
> +{
> + return test_bit(NFS_OPEN_STATE, &state->flags) &&
> + nfs4_stateid_match_other(&state->open_stateid, stateid);
> +}
> +
> #else
>
> #define nfs4_close_state(a, b) do { } while (0)
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index f550ac69ffa0..b7b0080977c0 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -1458,7 +1458,6 @@ static void nfs_resync_open_stateid_locked(struct nfs4_state *state)
> }
>
> static void nfs_clear_open_stateid_locked(struct nfs4_state *state,
> - nfs4_stateid *arg_stateid,
> nfs4_stateid *stateid, fmode_t fmode)
> {
> clear_bit(NFS_O_RDWR_STATE, &state->flags);
> @@ -1476,10 +1475,9 @@ static void nfs_clear_open_stateid_locked(struct nfs4_state *state,
> }
> if (stateid == NULL)
> return;
> - /* Handle races with OPEN */
> - if (!nfs4_stateid_match_other(arg_stateid, &state->open_stateid) ||
> - (nfs4_stateid_match_other(stateid, &state->open_stateid) &&
> - !nfs4_stateid_is_newer(stateid, &state->open_stateid))) {
> + /* Handle OPEN+OPEN_DOWNGRADE races */
> + if (nfs4_stateid_match_other(stateid, &state->open_stateid) &&
> + !nfs4_stateid_is_newer(stateid, &state->open_stateid)) {
> nfs_resync_open_stateid_locked(state);
> return;
> }
> @@ -1493,7 +1491,9 @@ static void nfs_clear_open_stateid(struct nfs4_state *state,
> nfs4_stateid *stateid, fmode_t fmode)
> {
> write_seqlock(&state->seqlock);
> - nfs_clear_open_stateid_locked(state, arg_stateid, stateid, fmode);
> + /* Ignore, if the CLOSE argment doesn't match the current stateid */
> + if (nfs4_state_match_open_stateid_other(state, arg_stateid))
> + nfs_clear_open_stateid_locked(state, stateid, fmode);
> write_sequnlock(&state->seqlock);
> if (test_bit(NFS_STATE_RECLAIM_NOGRACE, &state->flags))
> nfs4_schedule_state_manager(state->owner->so_server->nfs_client);
> --
> 2.7.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
Hi Olga,
On 13 Feb 2018, at 15:08, Olga Kornievskaia wrote:
> On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust
> <[email protected]> wrote:
>> If the reply to a successful CLOSE call races with an OPEN to the same
>> file, we can end up scribbling over the stateid that represents the
>> new open state.
>> The race looks like:
>>
>> Client Server
>> ====== ======
>>
>> CLOSE stateid A on file "foo"
>> CLOSE stateid A, return stateid C
>
> Hi folks,
>
> I'd like to understand this particular issue. Specifically I don't
> understand how can server return stateid C to the close with stateid
> A.
I think in this explanation of the race, stateid C is an incremented version
of A -- the stateid's "other" fields match -- OR it is the invalid special
stateid according to RFC 5661, Section 18.2.4.
> As per RFC 7530 or 5661. It says that state is returned by the close
> shouldn't be used.
>
> Even though CLOSE returns a stateid, this stateid is not useful to
> the client and should be treated as deprecated. CLOSE "shuts down"
> the state associated with all OPENs for the file by a single
> open-owner. As noted above, CLOSE will either release all file
> locking state or return an error. Therefore, the stateid returned by
> CLOSE is not useful for the operations that follow.
>
> Is this because the spec says "should" and not a "must"?
I don't understand - is what because? The state returned from CLOSE is
useful to be used by the client for housekeeping, but it won't be re-used in
the protocol.
> Linux server increments a state's sequenceid on CLOSE. Ontap server
> does not. I'm not sure what other servers do. Are all these
> implementations equality correct?
Ah, good question there.. Even though the stateid is not useful for
operations that follow, I think the sequenceid should be incremented because
the CLOSE is an operation that modifies the set of locks or state:
In RFC 7530, Section 9.1.4.2.:
...
When such a set of locks is first created, the server returns a
stateid with a seqid value of one. On subsequent operations that
modify the set of locks, the server is required to advance the
seqid field by one whenever it returns a stateid for the same
state-owner/file/type combination and the operation is one that might
make some change in the set of locks actually designated. In this
case, the server will return a stateid with an "other" field the same
as previously used for that state-owner/file/type combination, with
an incremented seqid field.
But, in RFC 5661, the CLOSE response SHOULD be the invalid special stateid.
I don't recall, does the linux server increment the existing stateid or send
back the invalid special stateid on v4.1?
Ben
On Wed, 2018-02-14 at 10:05 -0500, Benjamin Coddington wrote:
> Hi Olga,
>
> On 13 Feb 2018, at 15:08, Olga Kornievskaia wrote:
>
> > On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust
> > <[email protected]> wrote:
> > > If the reply to a successful CLOSE call races with an OPEN to the same
> > > file, we can end up scribbling over the stateid that represents the
> > > new open state.
> > > The race looks like:
> > >
> > > Client Server
> > > ====== ======
> > >
> > > CLOSE stateid A on file "foo"
> > > CLOSE stateid A, return stateid C
> >
> > Hi folks,
> >
> > I'd like to understand this particular issue. Specifically I don't
> > understand how can server return stateid C to the close with stateid
> > A.
>
> I think in this explanation of the race, stateid C is an incremented version
> of A -- the stateid's "other" fields match -- OR it is the invalid special
> stateid according to RFC 5661, Section 18.2.4.
>
> > As per RFC 7530 or 5661. It says that state is returned by the close
> > shouldn't be used.
> >
> > Even though CLOSE returns a stateid, this stateid is not useful to
> > the client and should be treated as deprecated. CLOSE "shuts down"
> > the state associated with all OPENs for the file by a single
> > open-owner. As noted above, CLOSE will either release all file
> > locking state or return an error. Therefore, the stateid returned by
> > CLOSE is not useful for the operations that follow.
> >
> > Is this because the spec says "should" and not a "must"?
>
> I don't understand - is what because? The state returned from CLOSE is
> useful to be used by the client for housekeeping, but it won't be re-used in
> the protocol.
>
> > Linux server increments a state's sequenceid on CLOSE. Ontap server
> > does not. I'm not sure what other servers do. Are all these
> > implementations equality correct?
>
> Ah, good question there.. Even though the stateid is not useful for
> operations that follow, I think the sequenceid should be incremented because
> the CLOSE is an operation that modifies the set of locks or state:
>
It doesn't matter.
> In RFC 7530, Section 9.1.4.2.:
> ...
> When such a set of locks is first created, the server returns a
> stateid with a seqid value of one. On subsequent operations that
> modify the set of locks, the server is required to advance the
> seqid field by one whenever it returns a stateid for the same
> state-owner/file/type combination and the operation is one that might
> make some change in the set of locks actually designated. In this
> case, the server will return a stateid with an "other" field the same
> as previously used for that state-owner/file/type combination, with
> an incremented seqid field.
>
> But, in RFC 5661, the CLOSE response SHOULD be the invalid special stateid.
> I don't recall, does the linux server increment the existing stateid or send
> back the invalid special stateid on v4.1?
>
A CLOSE, by definition, is destroying the old stateid, so what does it
matter what you return on success? It's bogus either way.
If knfsd is sending back a "real" stateid there, then it's likely only
because that's what the v4.0 spec said to do ~10 years ago. It's
probably fine to fix it to just return the invalid special stateid and
call it a day. All clients should just ignore it.
--
Jeff Layton <[email protected]>
T24gV2VkLCAyMDE4LTAyLTE0IGF0IDEwOjIxIC0wNTAwLCBKZWZmIExheXRvbiB3cm90ZToNCj4g
T24gV2VkLCAyMDE4LTAyLTE0IGF0IDEwOjA1IC0wNTAwLCBCZW5qYW1pbiBDb2RkaW5ndG9uIHdy
b3RlOg0KPiA+IEhpIE9sZ2EsDQo+ID4gDQo+ID4gT24gMTMgRmViIDIwMTgsIGF0IDE1OjA4LCBP
bGdhIEtvcm5pZXZza2FpYSB3cm90ZToNCj4gPiANCj4gPiA+IE9uIE1vbiwgTm92IDE0LCAyMDE2
IGF0IDExOjE5IEFNLCBUcm9uZCBNeWtsZWJ1c3QNCj4gPiA+IDx0cm9uZC5teWtsZWJ1c3RAcHJp
bWFyeWRhdGEuY29tPiB3cm90ZToNCj4gPiA+ID4gSWYgdGhlIHJlcGx5IHRvIGEgc3VjY2Vzc2Z1
bCBDTE9TRSBjYWxsIHJhY2VzIHdpdGggYW4gT1BFTiB0bw0KPiA+ID4gPiB0aGUgc2FtZQ0KPiA+
ID4gPiBmaWxlLCB3ZSBjYW4gZW5kIHVwIHNjcmliYmxpbmcgb3ZlciB0aGUgc3RhdGVpZCB0aGF0
IHJlcHJlc2VudHMNCj4gPiA+ID4gdGhlDQo+ID4gPiA+IG5ldyBvcGVuIHN0YXRlLg0KPiA+ID4g
PiBUaGUgcmFjZSBsb29rcyBsaWtlOg0KPiA+ID4gPiANCj4gPiA+ID4gICBDbGllbnQgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgIFNlcnZlcg0KPiA+ID4gPiAgID09PT09PSAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgPT09PT09DQo+ID4gPiA+IA0KPiA+ID4gPiAgIENMT1NF
IHN0YXRlaWQgQSBvbiBmaWxlICJmb28iDQo+ID4gPiA+ICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICBDTE9TRSBzdGF0ZWlkIEEsIHJldHVybg0KPiA+ID4gPiBzdGF0ZWlk
IEMNCj4gPiA+IA0KPiA+ID4gSGkgZm9sa3MsDQo+ID4gPiANCj4gPiA+IEknZCBsaWtlIHRvIHVu
ZGVyc3RhbmQgdGhpcyBwYXJ0aWN1bGFyIGlzc3VlLiBTcGVjaWZpY2FsbHkgSQ0KPiA+ID4gZG9u
J3QNCj4gPiA+IHVuZGVyc3RhbmQgaG93IGNhbiBzZXJ2ZXIgcmV0dXJuIHN0YXRlaWQgQyB0byB0
aGUgY2xvc2Ugd2l0aA0KPiA+ID4gc3RhdGVpZA0KPiA+ID4gQS4NCj4gPiANCj4gPiBJIHRoaW5r
IGluIHRoaXMgZXhwbGFuYXRpb24gb2YgdGhlIHJhY2UsIHN0YXRlaWQgQyBpcyBhbg0KPiA+IGlu
Y3JlbWVudGVkIHZlcnNpb24NCj4gPiBvZiBBIC0tIHRoZSBzdGF0ZWlkJ3MgIm90aGVyIiBmaWVs
ZHMgbWF0Y2ggLS0gT1IgaXQgaXMgdGhlIGludmFsaWQNCj4gPiBzcGVjaWFsDQo+ID4gc3RhdGVp
ZCBhY2NvcmRpbmcgdG8gUkZDIDU2NjEsIFNlY3Rpb24gMTguMi40Lg0KPiA+IA0KPiA+ID4gQXMg
cGVyIFJGQyA3NTMwIG9yIDU2NjEuIEl0IHNheXMgdGhhdCBzdGF0ZSBpcyByZXR1cm5lZCBieSB0
aGUNCj4gPiA+IGNsb3NlDQo+ID4gPiBzaG91bGRuJ3QgYmUgdXNlZC4NCj4gPiA+IA0KPiA+ID4g
RXZlbiB0aG91Z2ggQ0xPU0UgcmV0dXJucyBhIHN0YXRlaWQsIHRoaXMgc3RhdGVpZCBpcyBub3Qg
dXNlZnVsDQo+ID4gPiB0bw0KPiA+ID4gICAgdGhlIGNsaWVudCBhbmQgc2hvdWxkIGJlIHRyZWF0
ZWQgYXMgZGVwcmVjYXRlZC4gIENMT1NFICJzaHV0cw0KPiA+ID4gZG93biINCj4gPiA+ICAgIHRo
ZSBzdGF0ZSBhc3NvY2lhdGVkIHdpdGggYWxsIE9QRU5zIGZvciB0aGUgZmlsZSBieSBhIHNpbmds
ZQ0KPiA+ID4gICAgb3Blbi1vd25lci4gIEFzIG5vdGVkIGFib3ZlLCBDTE9TRSB3aWxsIGVpdGhl
ciByZWxlYXNlIGFsbA0KPiA+ID4gZmlsZQ0KPiA+ID4gICAgbG9ja2luZyBzdGF0ZSBvciByZXR1
cm4gYW4gZXJyb3IuICBUaGVyZWZvcmUsIHRoZSBzdGF0ZWlkDQo+ID4gPiByZXR1cm5lZCBieQ0K
PiA+ID4gICAgQ0xPU0UgaXMgbm90IHVzZWZ1bCBmb3IgdGhlIG9wZXJhdGlvbnMgdGhhdCBmb2xs
b3cuDQo+ID4gPiANCj4gPiA+IElzIHRoaXMgYmVjYXVzZSB0aGUgc3BlYyBzYXlzICJzaG91bGQi
IGFuZCBub3QgYSAibXVzdCI/DQo+ID4gDQo+ID4gSSBkb24ndCB1bmRlcnN0YW5kIC0gaXMgd2hh
dCBiZWNhdXNlPyAgVGhlIHN0YXRlIHJldHVybmVkIGZyb20NCj4gPiBDTE9TRSBpcw0KPiA+IHVz
ZWZ1bCB0byBiZSB1c2VkIGJ5IHRoZSBjbGllbnQgZm9yIGhvdXNla2VlcGluZywgYnV0IGl0IHdv
bid0IGJlDQo+ID4gcmUtdXNlZCBpbg0KPiA+IHRoZSBwcm90b2NvbC4NCj4gPiANCj4gPiA+IExp
bnV4IHNlcnZlciBpbmNyZW1lbnRzIGEgc3RhdGUncyBzZXF1ZW5jZWlkIG9uIENMT1NFLiBPbnRh
cA0KPiA+ID4gc2VydmVyDQo+ID4gPiBkb2VzIG5vdC4gSSdtIG5vdCBzdXJlIHdoYXQgb3RoZXIg
c2VydmVycyBkby4gQXJlIGFsbCB0aGVzZQ0KPiA+ID4gaW1wbGVtZW50YXRpb25zIGVxdWFsaXR5
IGNvcnJlY3Q/DQo+ID4gDQo+ID4gQWgsIGdvb2QgcXVlc3Rpb24gdGhlcmUuLiAgRXZlbiB0aG91
Z2ggdGhlIHN0YXRlaWQgaXMgbm90IHVzZWZ1bA0KPiA+IGZvcg0KPiA+IG9wZXJhdGlvbnMgdGhh
dCBmb2xsb3csIEkgdGhpbmsgdGhlIHNlcXVlbmNlaWQgc2hvdWxkIGJlDQo+ID4gaW5jcmVtZW50
ZWQgYmVjYXVzZQ0KPiA+IHRoZSBDTE9TRSBpcyBhbiBvcGVyYXRpb24gdGhhdCBtb2RpZmllcyB0
aGUgc2V0IG9mIGxvY2tzIG9yIHN0YXRlOg0KPiA+IA0KPiANCj4gSXQgZG9lc24ndCBtYXR0ZXIu
DQo+IA0KPiA+IEluIFJGQyA3NTMwLCBTZWN0aW9uIDkuMS40LjIuOg0KPiA+IC4uLg0KPiA+ICAg
IFdoZW4gc3VjaCBhIHNldCBvZiBsb2NrcyBpcyBmaXJzdCBjcmVhdGVkLCB0aGUgc2VydmVyIHJl
dHVybnMgYQ0KPiA+ICAgIHN0YXRlaWQgd2l0aCBhIHNlcWlkIHZhbHVlIG9mIG9uZS4gIE9uIHN1
YnNlcXVlbnQgb3BlcmF0aW9ucw0KPiA+IHRoYXQNCj4gPiAgICBtb2RpZnkgdGhlIHNldCBvZiBs
b2NrcywgdGhlIHNlcnZlciBpcyByZXF1aXJlZCB0byBhZHZhbmNlIHRoZQ0KPiA+ICAgIHNlcWlk
IGZpZWxkIGJ5IG9uZSB3aGVuZXZlciBpdCByZXR1cm5zIGEgc3RhdGVpZCBmb3IgdGhlIHNhbWUN
Cj4gPiAgICBzdGF0ZS1vd25lci9maWxlL3R5cGUgY29tYmluYXRpb24gYW5kIHRoZSBvcGVyYXRp
b24gaXMgb25lIHRoYXQNCj4gPiBtaWdodA0KPiA+ICAgIG1ha2Ugc29tZSBjaGFuZ2UgaW4gdGhl
IHNldCBvZiBsb2NrcyBhY3R1YWxseSBkZXNpZ25hdGVkLiAgSW4NCj4gPiB0aGlzDQo+ID4gICAg
Y2FzZSwgdGhlIHNlcnZlciB3aWxsIHJldHVybiBhIHN0YXRlaWQgd2l0aCBhbiAib3RoZXIiIGZp
ZWxkIHRoZQ0KPiA+IHNhbWUNCj4gPiAgICBhcyBwcmV2aW91c2x5IHVzZWQgZm9yIHRoYXQgc3Rh
dGUtb3duZXIvZmlsZS90eXBlIGNvbWJpbmF0aW9uLA0KPiA+IHdpdGgNCj4gPiAgICBhbiBpbmNy
ZW1lbnRlZCBzZXFpZCBmaWVsZC4NCj4gPiANCj4gPiBCdXQsIGluIFJGQyA1NjYxLCB0aGUgQ0xP
U0UgcmVzcG9uc2UgU0hPVUxEIGJlIHRoZSBpbnZhbGlkIHNwZWNpYWwNCj4gPiBzdGF0ZWlkLg0K
PiA+IEkgZG9uJ3QgcmVjYWxsLCBkb2VzIHRoZSBsaW51eCBzZXJ2ZXIgaW5jcmVtZW50IHRoZSBl
eGlzdGluZw0KPiA+IHN0YXRlaWQgb3Igc2VuZA0KPiA+IGJhY2sgdGhlIGludmFsaWQgc3BlY2lh
bCBzdGF0ZWlkIG9uIHY0LjE/DQo+ID4gDQo+IA0KPiBBIENMT1NFLCBieSBkZWZpbml0aW9uLCBp
cyBkZXN0cm95aW5nIHRoZSBvbGQgc3RhdGVpZCwgc28gd2hhdCBkb2VzDQo+IGl0DQo+IG1hdHRl
ciB3aGF0IHlvdSByZXR1cm4gb24gc3VjY2Vzcz8gSXQncyBib2d1cyBlaXRoZXIgd2F5Lg0KPiAN
Cj4gSWYga25mc2QgaXMgc2VuZGluZyBiYWNrIGEgInJlYWwiIHN0YXRlaWQgdGhlcmUsIHRoZW4g
aXQncyBsaWtlbHkNCj4gb25seQ0KPiBiZWNhdXNlIHRoYXQncyB3aGF0IHRoZSB2NC4wIHNwZWMg
c2FpZCB0byBkbyB+MTAgeWVhcnMgYWdvLiBJdCdzDQo+IHByb2JhYmx5IGZpbmUgdG8gZml4IGl0
IHRvIGp1c3QgcmV0dXJuIHRoZSBpbnZhbGlkIHNwZWNpYWwgc3RhdGVpZA0KPiBhbmQNCj4gY2Fs
bCBpdCBhIGRheS4gQWxsIGNsaWVudHMgc2hvdWxkIGp1c3QgaWdub3JlIGl0Lg0KPiANCg0KSXMg
dGhlcmUgYSBwcm9wb3NhbCB0byBjaGFuZ2UgdGhlIGN1cnJlbnQgY2xpZW50IGJlaGF2aW91ciBo
ZXJlPyBBcyBmYXINCmFzIEkgY2FuIHRlbGwsIHRoZSBhbnN3ZXIgaXMgIm5vIi4gQW0gSSBtaXNz
aW5nIHNvbWV0aGluZz8NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQg
bWFpbnRhaW5lciwgUHJpbWFyeURhdGENCnRyb25kLm15a2xlYnVzdEBwcmltYXJ5ZGF0YS5jb20N
Cg==
On 14 Feb 2018, at 10:29, Trond Myklebust wrote:
> On Wed, 2018-02-14 at 10:21 -0500, Jeff Layton wrote:
>> On Wed, 2018-02-14 at 10:05 -0500, Benjamin Coddington wrote:
>>> Hi Olga,
>>>
>>> On 13 Feb 2018, at 15:08, Olga Kornievskaia wrote:
>>>
>>>> On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust
>>>> <[email protected]> wrote:
>>>> ...
>>> Ah, good question there.. Even though the stateid is not useful
>>> for
>>> operations that follow, I think the sequenceid should be
>>> incremented because
>>> the CLOSE is an operation that modifies the set of locks or state:
>>>
>>
>> It doesn't matter.
Yes, good condensed point. It doesn't matter.
>>> ...
> Is there a proposal to change the current client behaviour here? As far
> as I can tell, the answer is "no". Am I missing something?
Not that I can see. I think we're just comparing linux and netapp server
behaviors..
Ben
On Wed, Feb 14, 2018 at 10:42 AM, Benjamin Coddington
<[email protected]> wrote:
> On 14 Feb 2018, at 10:29, Trond Myklebust wrote:
>> On Wed, 2018-02-14 at 10:21 -0500, Jeff Layton wrote:
>>> On Wed, 2018-02-14 at 10:05 -0500, Benjamin Coddington wrote:
>>>> Hi Olga,
>>>>
>>>> On 13 Feb 2018, at 15:08, Olga Kornievskaia wrote:
>>>>
>>>>> On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust
>>>>> <[email protected]> wrote:
>>>>> ...
>>>> Ah, good question there.. Even though the stateid is not useful
>>>> for
>>>> operations that follow, I think the sequenceid should be
>>>> incremented because
>>>> the CLOSE is an operation that modifies the set of locks or state:
>>>>
>>>
>>> It doesn't matter.
>
> Yes, good condensed point. It doesn't matter.
>
>>>> ...
>> Is there a proposal to change the current client behaviour here? As far
>> as I can tell, the answer is "no". Am I missing something?
>
> Not that I can see. I think we're just comparing linux and netapp server
> behaviors..
>
> Ben
I just found very surprising that in nfs4_close_done() which calls
eventually calls nfs_clear_open_stateid_locked() we change the state
based on the stateid received from the CLOSE.
nfs_clear_open_stateid_locked() is only called from nfs4_close_done()
and no other function.
I guess I'm not wondering if we had this problem described in this
patch of the delayed CLOSE, if we didn't update the state after
receiving the close... (so yes this is a weak proposal).
T24gV2VkLCAyMDE4LTAyLTE0IGF0IDExOjA2IC0wNTAwLCBPbGdhIEtvcm5pZXZza2FpYSB3cm90
ZToNCj4gT24gV2VkLCBGZWIgMTQsIDIwMTggYXQgMTA6NDIgQU0sIEJlbmphbWluIENvZGRpbmd0
b24NCj4gPGJjb2RkaW5nQHJlZGhhdC5jb20+IHdyb3RlOg0KPiA+IE9uIDE0IEZlYiAyMDE4LCBh
dCAxMDoyOSwgVHJvbmQgTXlrbGVidXN0IHdyb3RlOg0KPiA+ID4gT24gV2VkLCAyMDE4LTAyLTE0
IGF0IDEwOjIxIC0wNTAwLCBKZWZmIExheXRvbiB3cm90ZToNCj4gPiA+ID4gT24gV2VkLCAyMDE4
LTAyLTE0IGF0IDEwOjA1IC0wNTAwLCBCZW5qYW1pbiBDb2RkaW5ndG9uIHdyb3RlOg0KPiA+ID4g
PiA+IEhpIE9sZ2EsDQo+ID4gPiA+ID4gDQo+ID4gPiA+ID4gT24gMTMgRmViIDIwMTgsIGF0IDE1
OjA4LCBPbGdhIEtvcm5pZXZza2FpYSB3cm90ZToNCj4gPiA+ID4gPiANCj4gPiA+ID4gPiA+IE9u
IE1vbiwgTm92IDE0LCAyMDE2IGF0IDExOjE5IEFNLCBUcm9uZCBNeWtsZWJ1c3QNCj4gPiA+ID4g
PiA+IDx0cm9uZC5teWtsZWJ1c3RAcHJpbWFyeWRhdGEuY29tPiB3cm90ZToNCj4gPiA+ID4gPiA+
IC4uLg0KPiA+ID4gPiA+IA0KPiA+ID4gPiA+IEFoLCBnb29kIHF1ZXN0aW9uIHRoZXJlLi4gIEV2
ZW4gdGhvdWdoIHRoZSBzdGF0ZWlkIGlzIG5vdA0KPiA+ID4gPiA+IHVzZWZ1bA0KPiA+ID4gPiA+
IGZvcg0KPiA+ID4gPiA+IG9wZXJhdGlvbnMgdGhhdCBmb2xsb3csIEkgdGhpbmsgdGhlIHNlcXVl
bmNlaWQgc2hvdWxkIGJlDQo+ID4gPiA+ID4gaW5jcmVtZW50ZWQgYmVjYXVzZQ0KPiA+ID4gPiA+
IHRoZSBDTE9TRSBpcyBhbiBvcGVyYXRpb24gdGhhdCBtb2RpZmllcyB0aGUgc2V0IG9mIGxvY2tz
IG9yDQo+ID4gPiA+ID4gc3RhdGU6DQo+ID4gPiA+ID4gDQo+ID4gPiA+IA0KPiA+ID4gPiBJdCBk
b2Vzbid0IG1hdHRlci4NCj4gPiANCj4gPiBZZXMsIGdvb2QgY29uZGVuc2VkIHBvaW50LiAgSXQg
ZG9lc24ndCBtYXR0ZXIuDQo+ID4gDQo+ID4gPiA+ID4gLi4uDQo+ID4gPiANCj4gPiA+IElzIHRo
ZXJlIGEgcHJvcG9zYWwgdG8gY2hhbmdlIHRoZSBjdXJyZW50IGNsaWVudCBiZWhhdmlvdXIgaGVy
ZT8NCj4gPiA+IEFzIGZhcg0KPiA+ID4gYXMgSSBjYW4gdGVsbCwgdGhlIGFuc3dlciBpcyAibm8i
LiBBbSBJIG1pc3Npbmcgc29tZXRoaW5nPw0KPiA+IA0KPiA+IE5vdCB0aGF0IEkgY2FuIHNlZS4g
IEkgdGhpbmsgd2UncmUganVzdCBjb21wYXJpbmcgbGludXggYW5kIG5ldGFwcA0KPiA+IHNlcnZl
cg0KPiA+IGJlaGF2aW9ycy4uDQo+ID4gDQo+ID4gQmVuDQo+IA0KPiBJIGp1c3QgZm91bmQgdmVy
eSBzdXJwcmlzaW5nIHRoYXQgaW4gbmZzNF9jbG9zZV9kb25lKCkgd2hpY2ggY2FsbHMNCj4gZXZl
bnR1YWxseSBjYWxscyBuZnNfY2xlYXJfb3Blbl9zdGF0ZWlkX2xvY2tlZCgpIHdlIGNoYW5nZSB0
aGUgc3RhdGUNCj4gYmFzZWQgb24gdGhlIHN0YXRlaWQgcmVjZWl2ZWQgZnJvbSB0aGUgQ0xPU0Uu
DQo+IG5mc19jbGVhcl9vcGVuX3N0YXRlaWRfbG9ja2VkKCkgaXMgb25seSBjYWxsZWQgZnJvbSBu
ZnM0X2Nsb3NlX2RvbmUoKQ0KPiBhbmQgbm8gb3RoZXIgZnVuY3Rpb24uDQo+IA0KPiBJIGd1ZXNz
IEknbSBub3Qgd29uZGVyaW5nIGlmIHdlIGhhZCB0aGlzIHByb2JsZW0gZGVzY3JpYmVkIGluIHRo
aXMNCj4gcGF0Y2ggb2YgdGhlIGRlbGF5ZWQgQ0xPU0UsIGlmIHdlIGRpZG4ndCB1cGRhdGUgdGhl
IHN0YXRlIGFmdGVyDQo+IHJlY2VpdmluZyB0aGUgY2xvc2UuLi4gKHNvIHllcyB0aGlzIGlzIGEg
d2VhayBwcm9wb3NhbCkuDQo+IA0KDQpuZnM0X2Nsb3NlX2RvbmUoKSBkb2Vzbid0IG9ubHkgZGVh
bCB3aXRoIENMT1NFLiBJdCBhbHNvIGhhcyB0byBoYW5kbGUNCk9QRU5fRE9XTkdSQURFLg0KDQot
LSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyLCBQcmltYXJ5
RGF0YQ0KdHJvbmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNvbQ0K
On Wed, Feb 14, 2018 at 11:59 AM, Trond Myklebust
<[email protected]> wrote:
> On Wed, 2018-02-14 at 11:06 -0500, Olga Kornievskaia wrote:
>> On Wed, Feb 14, 2018 at 10:42 AM, Benjamin Coddington
>> <[email protected]> wrote:
>> > On 14 Feb 2018, at 10:29, Trond Myklebust wrote:
>> > > On Wed, 2018-02-14 at 10:21 -0500, Jeff Layton wrote:
>> > > > On Wed, 2018-02-14 at 10:05 -0500, Benjamin Coddington wrote:
>> > > > > Hi Olga,
>> > > > >
>> > > > > On 13 Feb 2018, at 15:08, Olga Kornievskaia wrote:
>> > > > >
>> > > > > > On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust
>> > > > > > <[email protected]> wrote:
>> > > > > > ...
>> > > > >
>> > > > > Ah, good question there.. Even though the stateid is not
>> > > > > useful
>> > > > > for
>> > > > > operations that follow, I think the sequenceid should be
>> > > > > incremented because
>> > > > > the CLOSE is an operation that modifies the set of locks or
>> > > > > state:
>> > > > >
>> > > >
>> > > > It doesn't matter.
>> >
>> > Yes, good condensed point. It doesn't matter.
>> >
>> > > > > ...
>> > >
>> > > Is there a proposal to change the current client behaviour here?
>> > > As far
>> > > as I can tell, the answer is "no". Am I missing something?
>> >
>> > Not that I can see. I think we're just comparing linux and netapp
>> > server
>> > behaviors..
>> >
>> > Ben
>>
>> I just found very surprising that in nfs4_close_done() which calls
>> eventually calls nfs_clear_open_stateid_locked() we change the state
>> based on the stateid received from the CLOSE.
>> nfs_clear_open_stateid_locked() is only called from nfs4_close_done()
>> and no other function.
>>
>> I guess I'm not wondering if we had this problem described in this
>> patch of the delayed CLOSE, if we didn't update the state after
>> receiving the close... (so yes this is a weak proposal).
>>
>
> nfs4_close_done() doesn't only deal with CLOSE. It also has to handle
> OPEN_DOWNGRADE.
What about doing an update for OPEN_DOWNGRADE but not for the CLOSE (untested):
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index f8a2b226..5868a6a 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1472,7 +1472,7 @@ static void nfs_clear_open_stateid_locked(struct
nfs4_state *state,
if (stateid == NULL)
return;
/* Handle OPEN+OPEN_DOWNGRADE races */
- if (nfs4_stateid_match_other(stateid, &state->open_stateid) &&
+ if (fmode && nfs4_stateid_match_other(stateid, &state->open_stateid) &&
!nfs4_stateid_is_newer(stateid, &state->open_stateid)) {
nfs_resync_open_stateid_locked(state);
goto out;
>
> --
> Trond Myklebust
> Linux NFS client maintainer, PrimaryData
> [email protected]
----- Original Message -----
> From: "Olga Kornievskaia" <[email protected]>
> To: "Trond Myklebust" <[email protected]>
> Cc: "linux-nfs" <[email protected]>, "Benjamin Coddington" <[email protected]>, "Jeff Layton"
> <[email protected]>
> Sent: Tuesday, February 13, 2018 9:08:01 PM
> Subject: Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN
> On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust
> <[email protected]> wrote:
>> If the reply to a successful CLOSE call races with an OPEN to the same
>> file, we can end up scribbling over the stateid that represents the
>> new open state.
>> The race looks like:
>>
>> Client Server
>> ====== ======
>>
>> CLOSE stateid A on file "foo"
>> CLOSE stateid A, return stateid C
>
> Hi folks,
>
> I'd like to understand this particular issue. Specifically I don't
> understand how can server return stateid C to the close with stateid
> A.
>
> As per RFC 7530 or 5661. It says that state is returned by the close
> shouldn't be used.
>
> Even though CLOSE returns a stateid, this stateid is not useful to
> the client and should be treated as deprecated. CLOSE "shuts down"
> the state associated with all OPENs for the file by a single
> open-owner. As noted above, CLOSE will either release all file
> locking state or return an error. Therefore, the stateid returned by
> CLOSE is not useful for the operations that follow.
>
> Is this because the spec says "should" and not a "must"?
>
> Linux server increments a state's sequenceid on CLOSE. Ontap server
> does not. I'm not sure what other servers do. Are all these
Our server sends back invalid state id for v4.1 and v4.0.
Tigran.
> implementations equality correct?
>
>> OPEN file "foo"
>> OPEN "foo", return stateid B
>> Receive reply to OPEN
>> Reset open state for "foo"
>> Associate stateid B to "foo"
>>
>> Receive CLOSE for A
>> Reset open state for "foo"
>> Replace stateid B with C
>>
>> The fix is to examine the argument of the CLOSE, and check for a match
>> with the current stateid "other" field. If the two do not match, then
>> the above race occurred, and we should just ignore the CLOSE.
>>
>> Reported-by: Benjamin Coddington <[email protected]>
>> Signed-off-by: Trond Myklebust <[email protected]>
>> ---
>> fs/nfs/nfs4_fs.h | 7 +++++++
>> fs/nfs/nfs4proc.c | 12 ++++++------
>> 2 files changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
>> index 9b3a82abab07..1452177c822d 100644
>> --- a/fs/nfs/nfs4_fs.h
>> +++ b/fs/nfs/nfs4_fs.h
>> @@ -542,6 +542,13 @@ static inline bool nfs4_valid_open_stateid(const struct
>> nfs4_state *state)
>> return test_bit(NFS_STATE_RECOVERY_FAILED, &state->flags) == 0;
>> }
>>
>> +static inline bool nfs4_state_match_open_stateid_other(const struct nfs4_state
>> *state,
>> + const nfs4_stateid *stateid)
>> +{
>> + return test_bit(NFS_OPEN_STATE, &state->flags) &&
>> + nfs4_stateid_match_other(&state->open_stateid, stateid);
>> +}
>> +
>> #else
>>
>> #define nfs4_close_state(a, b) do { } while (0)
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index f550ac69ffa0..b7b0080977c0 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -1458,7 +1458,6 @@ static void nfs_resync_open_stateid_locked(struct
>> nfs4_state *state)
>> }
>>
>> static void nfs_clear_open_stateid_locked(struct nfs4_state *state,
>> - nfs4_stateid *arg_stateid,
>> nfs4_stateid *stateid, fmode_t fmode)
>> {
>> clear_bit(NFS_O_RDWR_STATE, &state->flags);
>> @@ -1476,10 +1475,9 @@ static void nfs_clear_open_stateid_locked(struct
>> nfs4_state *state,
>> }
>> if (stateid == NULL)
>> return;
>> - /* Handle races with OPEN */
>> - if (!nfs4_stateid_match_other(arg_stateid, &state->open_stateid) ||
>> - (nfs4_stateid_match_other(stateid, &state->open_stateid) &&
>> - !nfs4_stateid_is_newer(stateid, &state->open_stateid))) {
>> + /* Handle OPEN+OPEN_DOWNGRADE races */
>> + if (nfs4_stateid_match_other(stateid, &state->open_stateid) &&
>> + !nfs4_stateid_is_newer(stateid, &state->open_stateid)) {
>> nfs_resync_open_stateid_locked(state);
>> return;
>> }
>> @@ -1493,7 +1491,9 @@ static void nfs_clear_open_stateid(struct nfs4_state
>> *state,
>> nfs4_stateid *stateid, fmode_t fmode)
>> {
>> write_seqlock(&state->seqlock);
>> - nfs_clear_open_stateid_locked(state, arg_stateid, stateid, fmode);
>> + /* Ignore, if the CLOSE argment doesn't match the current stateid */
>> + if (nfs4_state_match_open_stateid_other(state, arg_stateid))
>> + nfs_clear_open_stateid_locked(state, stateid, fmode);
>> write_sequnlock(&state->seqlock);
>> if (test_bit(NFS_STATE_RECLAIM_NOGRACE, &state->flags))
>> nfs4_schedule_state_manager(state->owner->so_server->nfs_client);
>> --
>> 2.7.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
> --
> 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
On Thu, 2018-02-15 at 13:19 +0100, Mkrtchyan, Tigran wrote:
>
> ----- Original Message -----
> > From: "Olga Kornievskaia" <[email protected]>
> > To: "Trond Myklebust" <[email protected]>
> > Cc: "linux-nfs" <[email protected]>, "Benjamin Coddington" <[email protected]>, "Jeff Layton"
> > <[email protected]>
> > Sent: Tuesday, February 13, 2018 9:08:01 PM
> > Subject: Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN
> > On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust
> > <[email protected]> wrote:
> > > If the reply to a successful CLOSE call races with an OPEN to the same
> > > file, we can end up scribbling over the stateid that represents the
> > > new open state.
> > > The race looks like:
> > >
> > > Client Server
> > > ====== ======
> > >
> > > CLOSE stateid A on file "foo"
> > > CLOSE stateid A, return stateid C
> >
> > Hi folks,
> >
> > I'd like to understand this particular issue. Specifically I don't
> > understand how can server return stateid C to the close with stateid
> > A.
> >
> > As per RFC 7530 or 5661. It says that state is returned by the close
> > shouldn't be used.
> >
> > Even though CLOSE returns a stateid, this stateid is not useful to
> > the client and should be treated as deprecated. CLOSE "shuts down"
> > the state associated with all OPENs for the file by a single
> > open-owner. As noted above, CLOSE will either release all file
> > locking state or return an error. Therefore, the stateid returned by
> > CLOSE is not useful for the operations that follow.
> >
> > Is this because the spec says "should" and not a "must"?
> >
> > Linux server increments a state's sequenceid on CLOSE. Ontap server
> > does not. I'm not sure what other servers do. Are all these
>
>
> Our server sends back invalid state id for v4.1 and v4.0.
>
> Tigran.
>
That's probably the best thing to do, and we should probably do the same
for v4.0 in knfsd. Doing that might cause problems for clients that are
not ignoring that field like they should, but they're buggy already.
> > implementations equality correct?
> >
> > > OPEN file "foo"
> > > OPEN "foo", return stateid B
> > > Receive reply to OPEN
> > > Reset open state for "foo"
> > > Associate stateid B to "foo"
> > >
> > > Receive CLOSE for A
> > > Reset open state for "foo"
> > > Replace stateid B with C
> > >
> > > The fix is to examine the argument of the CLOSE, and check for a match
> > > with the current stateid "other" field. If the two do not match, then
> > > the above race occurred, and we should just ignore the CLOSE.
> > >
> > > Reported-by: Benjamin Coddington <[email protected]>
> > > Signed-off-by: Trond Myklebust <[email protected]>
> > > ---
> > > fs/nfs/nfs4_fs.h | 7 +++++++
> > > fs/nfs/nfs4proc.c | 12 ++++++------
> > > 2 files changed, 13 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> > > index 9b3a82abab07..1452177c822d 100644
> > > --- a/fs/nfs/nfs4_fs.h
> > > +++ b/fs/nfs/nfs4_fs.h
> > > @@ -542,6 +542,13 @@ static inline bool nfs4_valid_open_stateid(const struct
> > > nfs4_state *state)
> > > return test_bit(NFS_STATE_RECOVERY_FAILED, &state->flags) == 0;
> > > }
> > >
> > > +static inline bool nfs4_state_match_open_stateid_other(const struct nfs4_state
> > > *state,
> > > + const nfs4_stateid *stateid)
> > > +{
> > > + return test_bit(NFS_OPEN_STATE, &state->flags) &&
> > > + nfs4_stateid_match_other(&state->open_stateid, stateid);
> > > +}
> > > +
> > > #else
> > >
> > > #define nfs4_close_state(a, b) do { } while (0)
> > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > > index f550ac69ffa0..b7b0080977c0 100644
> > > --- a/fs/nfs/nfs4proc.c
> > > +++ b/fs/nfs/nfs4proc.c
> > > @@ -1458,7 +1458,6 @@ static void nfs_resync_open_stateid_locked(struct
> > > nfs4_state *state)
> > > }
> > >
> > > static void nfs_clear_open_stateid_locked(struct nfs4_state *state,
> > > - nfs4_stateid *arg_stateid,
> > > nfs4_stateid *stateid, fmode_t fmode)
> > > {
> > > clear_bit(NFS_O_RDWR_STATE, &state->flags);
> > > @@ -1476,10 +1475,9 @@ static void nfs_clear_open_stateid_locked(struct
> > > nfs4_state *state,
> > > }
> > > if (stateid == NULL)
> > > return;
> > > - /* Handle races with OPEN */
> > > - if (!nfs4_stateid_match_other(arg_stateid, &state->open_stateid) ||
> > > - (nfs4_stateid_match_other(stateid, &state->open_stateid) &&
> > > - !nfs4_stateid_is_newer(stateid, &state->open_stateid))) {
> > > + /* Handle OPEN+OPEN_DOWNGRADE races */
> > > + if (nfs4_stateid_match_other(stateid, &state->open_stateid) &&
> > > + !nfs4_stateid_is_newer(stateid, &state->open_stateid)) {
> > > nfs_resync_open_stateid_locked(state);
> > > return;
> > > }
> > > @@ -1493,7 +1491,9 @@ static void nfs_clear_open_stateid(struct nfs4_state
> > > *state,
> > > nfs4_stateid *stateid, fmode_t fmode)
> > > {
> > > write_seqlock(&state->seqlock);
> > > - nfs_clear_open_stateid_locked(state, arg_stateid, stateid, fmode);
> > > + /* Ignore, if the CLOSE argment doesn't match the current stateid */
> > > + if (nfs4_state_match_open_stateid_other(state, arg_stateid))
> > > + nfs_clear_open_stateid_locked(state, stateid, fmode);
> > > write_sequnlock(&state->seqlock);
> > > if (test_bit(NFS_STATE_RECLAIM_NOGRACE, &state->flags))
> > > nfs4_schedule_state_manager(state->owner->so_server->nfs_client);
> > > --
> > > 2.7.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
> >
> > --
> > 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
--
Jeff Layton <[email protected]>
On Thu, Feb 15, 2018 at 07:23:00AM -0500, Jeff Layton wrote:
> On Thu, 2018-02-15 at 13:19 +0100, Mkrtchyan, Tigran wrote:
> >
> > ----- Original Message -----
> > > From: "Olga Kornievskaia" <[email protected]>
> > > To: "Trond Myklebust" <[email protected]>
> > > Cc: "linux-nfs" <[email protected]>, "Benjamin Coddington" <[email protected]>, "Jeff Layton"
> > > <[email protected]>
> > > Sent: Tuesday, February 13, 2018 9:08:01 PM
> > > Subject: Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN
> > > On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust
> > > <[email protected]> wrote:
> > > > If the reply to a successful CLOSE call races with an OPEN to the same
> > > > file, we can end up scribbling over the stateid that represents the
> > > > new open state.
> > > > The race looks like:
> > > >
> > > > Client Server
> > > > ====== ======
> > > >
> > > > CLOSE stateid A on file "foo"
> > > > CLOSE stateid A, return stateid C
> > >
> > > Hi folks,
> > >
> > > I'd like to understand this particular issue. Specifically I don't
> > > understand how can server return stateid C to the close with stateid
> > > A.
> > >
> > > As per RFC 7530 or 5661. It says that state is returned by the close
> > > shouldn't be used.
> > >
> > > Even though CLOSE returns a stateid, this stateid is not useful to
> > > the client and should be treated as deprecated. CLOSE "shuts down"
> > > the state associated with all OPENs for the file by a single
> > > open-owner. As noted above, CLOSE will either release all file
> > > locking state or return an error. Therefore, the stateid returned by
> > > CLOSE is not useful for the operations that follow.
> > >
> > > Is this because the spec says "should" and not a "must"?
> > >
> > > Linux server increments a state's sequenceid on CLOSE. Ontap server
> > > does not. I'm not sure what other servers do. Are all these
> >
> >
> > Our server sends back invalid state id for v4.1 and v4.0.
> >
> > Tigran.
> >
>
> That's probably the best thing to do, and we should probably do the same
> for v4.0 in knfsd. Doing that might cause problems for clients that are
> not ignoring that field like they should, but they're buggy already.
Not only buggy in theory, but actually failing in practice, it sounds
like. So, a pretty safe change?
Returning an all-zeroes stateid would be simple and make the situation
really obvious.
--b.
T24gVGh1LCAyMDE4LTAyLTE1IGF0IDEwOjI5IC0wNTAwLCBCcnVjZSBGaWVsZHMgd3JvdGU6DQo+
IE9uIFRodSwgRmViIDE1LCAyMDE4IGF0IDA3OjIzOjAwQU0gLTA1MDAsIEplZmYgTGF5dG9uIHdy
b3RlOg0KPiA+IE9uIFRodSwgMjAxOC0wMi0xNSBhdCAxMzoxOSArMDEwMCwgTWtydGNoeWFuLCBU
aWdyYW4gd3JvdGU6DQo+ID4gPiANCj4gPiA+IC0tLS0tIE9yaWdpbmFsIE1lc3NhZ2UgLS0tLS0N
Cj4gPiA+ID4gRnJvbTogIk9sZ2EgS29ybmlldnNrYWlhIiA8YWdsb0B1bWljaC5lZHU+DQo+ID4g
PiA+IFRvOiAiVHJvbmQgTXlrbGVidXN0IiA8dHJvbmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNv
bT4NCj4gPiA+ID4gQ2M6ICJsaW51eC1uZnMiIDxsaW51eC1uZnNAdmdlci5rZXJuZWwub3JnPiwg
IkJlbmphbWluDQo+ID4gPiA+IENvZGRpbmd0b24iIDxiY29kZGluZ0ByZWRoYXQuY29tPiwgIkpl
ZmYgTGF5dG9uIg0KPiA+ID4gPiA8amxheXRvbkByZWRoYXQuY29tPg0KPiA+ID4gPiBTZW50OiBU
dWVzZGF5LCBGZWJydWFyeSAxMywgMjAxOCA5OjA4OjAxIFBNDQo+ID4gPiA+IFN1YmplY3Q6IFJl
OiBbUEFUQ0ggMS8yXSBORlN2NDogRml4IENMT1NFIHJhY2VzIHdpdGggT1BFTg0KPiA+ID4gPiBP
biBNb24sIE5vdiAxNCwgMjAxNiBhdCAxMToxOSBBTSwgVHJvbmQgTXlrbGVidXN0DQo+ID4gPiA+
IDx0cm9uZC5teWtsZWJ1c3RAcHJpbWFyeWRhdGEuY29tPiB3cm90ZToNCj4gPiA+ID4gPiBJZiB0
aGUgcmVwbHkgdG8gYSBzdWNjZXNzZnVsIENMT1NFIGNhbGwgcmFjZXMgd2l0aCBhbiBPUEVOIHRv
DQo+ID4gPiA+ID4gdGhlIHNhbWUNCj4gPiA+ID4gPiBmaWxlLCB3ZSBjYW4gZW5kIHVwIHNjcmli
Ymxpbmcgb3ZlciB0aGUgc3RhdGVpZCB0aGF0DQo+ID4gPiA+ID4gcmVwcmVzZW50cyB0aGUNCj4g
PiA+ID4gPiBuZXcgb3BlbiBzdGF0ZS4NCj4gPiA+ID4gPiBUaGUgcmFjZSBsb29rcyBsaWtlOg0K
PiA+ID4gPiA+IA0KPiA+ID4gPiA+ICAgQ2xpZW50ICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICBTZXJ2ZXINCj4gPiA+ID4gPiAgID09PT09PSAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgPT09PT09DQo+ID4gPiA+ID4gDQo+ID4gPiA+ID4gICBDTE9TRSBzdGF0ZWlkIEEgb24g
ZmlsZSAiZm9vIg0KPiA+ID4gPiA+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICBDTE9TRSBzdGF0ZWlkIEEsDQo+ID4gPiA+ID4gcmV0dXJuIHN0YXRlaWQgQw0KPiA+ID4g
PiANCj4gPiA+ID4gSGkgZm9sa3MsDQo+ID4gPiA+IA0KPiA+ID4gPiBJJ2QgbGlrZSB0byB1bmRl
cnN0YW5kIHRoaXMgcGFydGljdWxhciBpc3N1ZS4gU3BlY2lmaWNhbGx5IEkNCj4gPiA+ID4gZG9u
J3QNCj4gPiA+ID4gdW5kZXJzdGFuZCBob3cgY2FuIHNlcnZlciByZXR1cm4gc3RhdGVpZCBDIHRv
IHRoZSBjbG9zZSB3aXRoDQo+ID4gPiA+IHN0YXRlaWQNCj4gPiA+ID4gQS4NCj4gPiA+ID4gDQo+
ID4gPiA+IEFzIHBlciBSRkMgNzUzMCBvciA1NjYxLiBJdCBzYXlzIHRoYXQgc3RhdGUgaXMgcmV0
dXJuZWQgYnkgdGhlDQo+ID4gPiA+IGNsb3NlDQo+ID4gPiA+IHNob3VsZG4ndCBiZSB1c2VkLg0K
PiA+ID4gPiANCj4gPiA+ID4gRXZlbiB0aG91Z2ggQ0xPU0UgcmV0dXJucyBhIHN0YXRlaWQsIHRo
aXMgc3RhdGVpZCBpcyBub3QgdXNlZnVsDQo+ID4gPiA+IHRvDQo+ID4gPiA+ICAgdGhlIGNsaWVu
dCBhbmQgc2hvdWxkIGJlIHRyZWF0ZWQgYXMgZGVwcmVjYXRlZC4gIENMT1NFICJzaHV0cw0KPiA+
ID4gPiBkb3duIg0KPiA+ID4gPiAgIHRoZSBzdGF0ZSBhc3NvY2lhdGVkIHdpdGggYWxsIE9QRU5z
IGZvciB0aGUgZmlsZSBieSBhIHNpbmdsZQ0KPiA+ID4gPiAgIG9wZW4tb3duZXIuICBBcyBub3Rl
ZCBhYm92ZSwgQ0xPU0Ugd2lsbCBlaXRoZXIgcmVsZWFzZSBhbGwNCj4gPiA+ID4gZmlsZQ0KPiA+
ID4gPiAgIGxvY2tpbmcgc3RhdGUgb3IgcmV0dXJuIGFuIGVycm9yLiAgVGhlcmVmb3JlLCB0aGUg
c3RhdGVpZA0KPiA+ID4gPiByZXR1cm5lZCBieQ0KPiA+ID4gPiAgIENMT1NFIGlzIG5vdCB1c2Vm
dWwgZm9yIHRoZSBvcGVyYXRpb25zIHRoYXQgZm9sbG93Lg0KPiA+ID4gPiANCj4gPiA+ID4gSXMg
dGhpcyBiZWNhdXNlIHRoZSBzcGVjIHNheXMgInNob3VsZCIgYW5kIG5vdCBhICJtdXN0Ij8NCj4g
PiA+ID4gDQo+ID4gPiA+IExpbnV4IHNlcnZlciBpbmNyZW1lbnRzIGEgc3RhdGUncyBzZXF1ZW5j
ZWlkIG9uIENMT1NFLiBPbnRhcA0KPiA+ID4gPiBzZXJ2ZXINCj4gPiA+ID4gZG9lcyBub3QuIEkn
bSBub3Qgc3VyZSB3aGF0IG90aGVyIHNlcnZlcnMgZG8uIEFyZSBhbGwgdGhlc2UNCj4gPiA+IA0K
PiA+ID4gDQo+ID4gPiBPdXIgc2VydmVyIHNlbmRzIGJhY2sgaW52YWxpZCBzdGF0ZSBpZCBmb3Ig
djQuMSBhbmQgdjQuMC4NCj4gPiA+IA0KPiA+ID4gVGlncmFuLg0KPiA+ID4gDQo+ID4gDQo+ID4g
VGhhdCdzIHByb2JhYmx5IHRoZSBiZXN0IHRoaW5nIHRvIGRvLCBhbmQgd2Ugc2hvdWxkIHByb2Jh
Ymx5IGRvIHRoZQ0KPiA+IHNhbWUNCj4gPiAgZm9yIHY0LjAgaW4ga25mc2QuIERvaW5nIHRoYXQg
bWlnaHQgY2F1c2UgcHJvYmxlbXMgZm9yIGNsaWVudHMNCj4gPiB0aGF0IGFyZQ0KPiA+IG5vdCBp
Z25vcmluZyB0aGF0IGZpZWxkIGxpa2UgdGhleSBzaG91bGQsIGJ1dCB0aGV5J3JlIGJ1Z2d5DQo+
ID4gYWxyZWFkeS4NCj4gDQo+IE5vdCBvbmx5IGJ1Z2d5IGluIHRoZW9yeSwgYnV0IGFjdHVhbGx5
IGZhaWxpbmcgaW4gcHJhY3RpY2UsIGl0IHNvdW5kcw0KPiBsaWtlLiAgU28sIGEgcHJldHR5IHNh
ZmUgY2hhbmdlPw0KPiANCj4gUmV0dXJuaW5nIGFuIGFsbC16ZXJvZXMgc3RhdGVpZCB3b3VsZCBi
ZSBzaW1wbGUgYW5kIG1ha2UgdGhlDQo+IHNpdHVhdGlvbg0KPiByZWFsbHkgb2J2aW91cy4NCj4g
DQoNCklmIHlvdSdyZSBnb2luZyB0byBkbyB0aGF0LCB0aGVuIHdoeSBub3QganVzdCBleHBhbmQg
ZmI1MDBhN2NmZWU3IHRvDQphcHBseSB0byBORlN2NC4wIHRvbz8gSWYgeW91J3JlIGdvaW5nIHRv
IHZpb2xhdGUgdGhlIHY0LjAgc3BlYywgdGhlbg0KeW91IG1pZ2h0IGFzIHdlbGwgZG8gaXQgaW4g
YSB3YXkgdGhhdCBpcyBzYW5jdGlvbmVkIGJ5IHY0LjEsIHNvIHRoYXQNCnBlb3BsZSBjYW4gdXNl
IHRoZSBzYW1lIHdpcmVzaGFyayBmaWx0ZXJzIHdoZW4gZGVidWdnaW5nLg0KDQotLSANClRyb25k
IE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyLCBQcmltYXJ5RGF0YQ0KdHJv
bmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNvbQ0K