Discussion:
how to properly handle failures during delegation recall process
Olga Kornievskaia
2014-10-13 18:51:56 UTC
Permalink
I'd like to hear community's thought about how to properly handle
failures during delegation recall process and/or thoughts about a
proposed fixed listed at the end.

There are two problems seen during the following situation:
A client get a cb_call for a delegation it currently holds. Consider
the case where the client has a delegated lock for this open. Callback
thread will send an open with delegation_cur, followed by a lock
operation, and finally delegreturn.

Problem#1: the client will send a lock operation regardless of whether
or not the open succeeded. This is a new_owner lock and in nfs4xdr.c,
the lock operation will choose to use the open_stateid. However, when
the open failed, the stateid is 0. Thus, we send an erroneous stateid
of 0.

Problem#2: if the open fails with admin_revoked, bad_stateid errors,
it leads to an infinite loop of sending an open with deleg_cur and
getting a bad_stateid error back.

It seems to me that we shouldn't even be trying to recover if we get a
bad_stateid-type of errors on open with deleg_cur because they are
unrecoverable.

Furthermore, I propose that if we get an error in
nfs4_open_delegation_recall() then we mark any delegation locks as
lost and in nfs4_lock_delegation_recall() don't attempt to recover
lost lock.

I have tested to following code as a fix:

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 5aa55c1..523fae0 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1682,6 +1682,22 @@ int nfs4_open_delegation_recall(struct
nfs_open_context *ctx, struct nfs4_state
nfs4_stateid_copy(&opendata->o_arg.u.delegation, stateid);
err = nfs4_open_recover(opendata, state);
nfs4_opendata_put(opendata);
+ switch(err) {
+ case -NFS4ERR_DELEG_REVOKED:
+ case -NFS4ERR_ADMIN_REVOKED:
+ case -NFS4ERR_BAD_STATEID:
+ case -NFS4ERR_OPENMODE: {
+ struct nfs4_lock_state *lock;
+ /* go through open locks and mark them lost */
+ spin_lock(&state->state_lock);
+ list_for_each_entry(lock, &state->lock_states,
ls_locks) {
+ if (!test_bit(NFS_LOCK_INITIALIZED,
&lock->ls_flags))
+ set_bit(NFS_LOCK_LOST, &lock->ls_flags);
+ }
+ spin_unlock(&state->state_lock);
+ return 0;
+ }
+ }
return nfs4_handle_delegation_recall_error(server, state, stateid, err);
}

@@ -5957,6 +5973,10 @@ int nfs4_lock_delegation_recall(struct
file_lock *fl, struct nfs4_state *state,
err = nfs4_set_lock_state(state, fl);
if (err != 0)
return err;
+ if (test_bit(NFS_LOCK_LOST, &fl->fl_u.nfs4_fl.owner->ls_flags)) {
+ pr_warn_ratelimited("NFS: %s: Lock reclaim failed!\n",
__func__);
+ return -EIO;
+ }
err = _nfs4_do_setlk(state, F_SETLK, fl, NFS_LOCK_NEW);
return nfs4_handle_delegation_recall_error(server, state, stateid, err);
}
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Trond Myklebust
2014-10-13 19:53:36 UTC
Permalink
Hi Olga,
Post by Olga Kornievskaia
I'd like to hear community's thought about how to properly handle
failures during delegation recall process and/or thoughts about a
proposed fixed listed at the end.
A client get a cb_call for a delegation it currently holds. Consider
the case where the client has a delegated lock for this open. Callback
thread will send an open with delegation_cur, followed by a lock
operation, and finally delegreturn.
Problem#1: the client will send a lock operation regardless of whether
or not the open succeeded. This is a new_owner lock and in nfs4xdr.c,
the lock operation will choose to use the open_stateid. However, when
the open failed, the stateid is 0. Thus, we send an erroneous stateid
of 0.
Problem#2: if the open fails with admin_revoked, bad_stateid errors,
it leads to an infinite loop of sending an open with deleg_cur and
getting a bad_stateid error back.
It seems to me that we shouldn't even be trying to recover if we get a
bad_stateid-type of errors on open with deleg_cur because they are
unrecoverable.
Furthermore, I propose that if we get an error in
nfs4_open_delegation_recall() then we mark any delegation locks as
lost and in nfs4_lock_delegation_recall() don't attempt to recover
lost lock.
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 5aa55c1..523fae0 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1682,6 +1682,22 @@ int nfs4_open_delegation_recall(struct
nfs_open_context *ctx, struct nfs4_state
nfs4_stateid_copy(&opendata->o_arg.u.delegation, stateid);
err = nfs4_open_recover(opendata, state);
nfs4_opendata_put(opendata);
+ switch(err) {
+ case -NFS4ERR_OPENMODE: {
+ struct nfs4_lock_state *lock;
+ /* go through open locks and mark them lost */
+ spin_lock(&state->state_lock);
+ list_for_each_entry(lock, &state->lock_states,
ls_locks) {
+ if (!test_bit(NFS_LOCK_INITIALIZED,
&lock->ls_flags))
+ set_bit(NFS_LOCK_LOST, &lock->ls_flags);
+ }
+ spin_unlock(&state->state_lock);
+ return 0;
If the open stated is marked for "nograce" recovery by
nfs4_handle_delegation_recall_errror(), then nfs4_lock_expired()
should do the above for you automatically. Are you reproducing this
bug with a 3.17 kernel?
Post by Olga Kornievskaia
+ }
+ }
return nfs4_handle_delegation_recall_error(server, state, stateid, err);
}
@@ -5957,6 +5973,10 @@ int nfs4_lock_delegation_recall(struct
file_lock *fl, struct nfs4_state *state,
err = nfs4_set_lock_state(state, fl);
if (err != 0)
return err;
+ if (test_bit(NFS_LOCK_LOST, &fl->fl_u.nfs4_fl.owner->ls_flags)) {
+ pr_warn_ratelimited("NFS: %s: Lock reclaim failed!\n",
__func__);
+ return -EIO;
+ }
err = _nfs4_do_setlk(state, F_SETLK, fl, NFS_LOCK_NEW);
Note that there is a potential race here if the server is playing with
NFS4ERR_DELEG_REVOKED or NFS4ERR_ADMIN_REVOKED. Since we do not
present the delegation as part of the LOCK request, we have no way of
knowing if the delegation is still in effect. I guess we can check the
return value of DELEGRETURN, but if it returns NFS4ERR_DELEG_REVOKED
or NFS4ERR_ADMIN_REVOKED, then we still do not know whether or not the
LOCK is safe.
Post by Olga Kornievskaia
return nfs4_handle_delegation_recall_error(server, state, stateid, err);
}
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust-7I+n7zu2hftEKMMhf/***@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Olga Kornievskaia
2014-10-13 20:23:24 UTC
Permalink
On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
Post by Trond Myklebust
Hi Olga,
Post by Olga Kornievskaia
I'd like to hear community's thought about how to properly handle
failures during delegation recall process and/or thoughts about a
proposed fixed listed at the end.
A client get a cb_call for a delegation it currently holds. Consider
the case where the client has a delegated lock for this open. Callback
thread will send an open with delegation_cur, followed by a lock
operation, and finally delegreturn.
Problem#1: the client will send a lock operation regardless of whether
or not the open succeeded. This is a new_owner lock and in nfs4xdr.c,
the lock operation will choose to use the open_stateid. However, when
the open failed, the stateid is 0. Thus, we send an erroneous stateid
of 0.
Problem#2: if the open fails with admin_revoked, bad_stateid errors,
it leads to an infinite loop of sending an open with deleg_cur and
getting a bad_stateid error back.
It seems to me that we shouldn't even be trying to recover if we get a
bad_stateid-type of errors on open with deleg_cur because they are
unrecoverable.
Furthermore, I propose that if we get an error in
nfs4_open_delegation_recall() then we mark any delegation locks as
lost and in nfs4_lock_delegation_recall() don't attempt to recover
lost lock.
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 5aa55c1..523fae0 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1682,6 +1682,22 @@ int nfs4_open_delegation_recall(struct
nfs_open_context *ctx, struct nfs4_state
nfs4_stateid_copy(&opendata->o_arg.u.delegation, stateid);
err = nfs4_open_recover(opendata, state);
nfs4_opendata_put(opendata);
+ switch(err) {
+ case -NFS4ERR_OPENMODE: {
+ struct nfs4_lock_state *lock;
+ /* go through open locks and mark them lost */
+ spin_lock(&state->state_lock);
+ list_for_each_entry(lock, &state->lock_states,
ls_locks) {
+ if (!test_bit(NFS_LOCK_INITIALIZED,
&lock->ls_flags))
+ set_bit(NFS_LOCK_LOST, &lock->ls_flags);
+ }
+ spin_unlock(&state->state_lock);
+ return 0;
If the open stated is marked for "nograce" recovery by
nfs4_handle_delegation_recall_errror(), then nfs4_lock_expired()
should do the above for you automatically. Are you reproducing this
bug with a 3.17 kernel?
Yes, the bug is with the 3.17 kernel.

Yes, the nfs4_lock_expired() will mark it. However, the state_manager
thread (most frequently) doesn't get to run to do that before
nfs4_open_delegation_recall() returns 0 and calls the
nfs_delegation_claim_locks(). Therefore, I found the code needed
before returning from nfs4_open_delegation_recall().
Post by Trond Myklebust
Post by Olga Kornievskaia
+ }
+ }
return nfs4_handle_delegation_recall_error(server, state, stateid, err);
}
@@ -5957,6 +5973,10 @@ int nfs4_lock_delegation_recall(struct
file_lock *fl, struct nfs4_state *state,
err = nfs4_set_lock_state(state, fl);
if (err != 0)
return err;
+ if (test_bit(NFS_LOCK_LOST, &fl->fl_u.nfs4_fl.owner->ls_flags)) {
+ pr_warn_ratelimited("NFS: %s: Lock reclaim failed!\n",
__func__);
+ return -EIO;
+ }
err = _nfs4_do_setlk(state, F_SETLK, fl, NFS_LOCK_NEW);
Note that there is a potential race here if the server is playing with
NFS4ERR_DELEG_REVOKED or NFS4ERR_ADMIN_REVOKED. Since we do not
present the delegation as part of the LOCK request, we have no way of
knowing if the delegation is still in effect. I guess we can check the
return value of DELEGRETURN, but if it returns NFS4ERR_DELEG_REVOKED
or NFS4ERR_ADMIN_REVOKED, then we still do not know whether or not the
LOCK is safe.
I'm not following you. We send LOCK before we send DELEGRETURN? Also,
we normally send in LOCK the open_stateid returned by the open with
cur so do we know that delegation is still in effect.
Post by Trond Myklebust
Post by Olga Kornievskaia
return nfs4_handle_delegation_recall_error(server, state, stateid, err);
}
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Trond Myklebust
2014-10-13 21:12:19 UTC
Permalink
Post by Olga Kornievskaia
On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
Post by Trond Myklebust
Hi Olga,
Post by Olga Kornievskaia
I'd like to hear community's thought about how to properly handle
failures during delegation recall process and/or thoughts about a
proposed fixed listed at the end.
A client get a cb_call for a delegation it currently holds. Consider
the case where the client has a delegated lock for this open. Callback
thread will send an open with delegation_cur, followed by a lock
operation, and finally delegreturn.
Problem#1: the client will send a lock operation regardless of whether
or not the open succeeded. This is a new_owner lock and in nfs4xdr.c,
the lock operation will choose to use the open_stateid. However, when
the open failed, the stateid is 0. Thus, we send an erroneous stateid
of 0.
Problem#2: if the open fails with admin_revoked, bad_stateid errors,
it leads to an infinite loop of sending an open with deleg_cur and
getting a bad_stateid error back.
It seems to me that we shouldn't even be trying to recover if we get a
bad_stateid-type of errors on open with deleg_cur because they are
unrecoverable.
Furthermore, I propose that if we get an error in
nfs4_open_delegation_recall() then we mark any delegation locks as
lost and in nfs4_lock_delegation_recall() don't attempt to recover
lost lock.
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 5aa55c1..523fae0 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1682,6 +1682,22 @@ int nfs4_open_delegation_recall(struct
nfs_open_context *ctx, struct nfs4_state
nfs4_stateid_copy(&opendata->o_arg.u.delegation, stateid);
err = nfs4_open_recover(opendata, state);
nfs4_opendata_put(opendata);
+ switch(err) {
+ case -NFS4ERR_OPENMODE: {
+ struct nfs4_lock_state *lock;
+ /* go through open locks and mark them lost */
+ spin_lock(&state->state_lock);
+ list_for_each_entry(lock, &state->lock_states,
ls_locks) {
+ if (!test_bit(NFS_LOCK_INITIALIZED,
&lock->ls_flags))
+ set_bit(NFS_LOCK_LOST, &lock->ls_flags);
+ }
+ spin_unlock(&state->state_lock);
+ return 0;
If the open stated is marked for "nograce" recovery by
nfs4_handle_delegation_recall_errror(), then nfs4_lock_expired()
should do the above for you automatically. Are you reproducing this
bug with a 3.17 kernel?
Yes, the bug is with the 3.17 kernel.
Yes, the nfs4_lock_expired() will mark it. However, the state_manager
thread (most frequently) doesn't get to run to do that before
nfs4_open_delegation_recall() returns 0 and calls the
nfs_delegation_claim_locks(). Therefore, I found the code needed
before returning from nfs4_open_delegation_recall().
Right. We probably should not be returning a value of 0 in that case.
If the delegation has been revoked, then we want
nfs4_open_delegation_recall() to just return immediately, and then we
want nfs_end_delegation_return() to detach the delegation and free it
without calling delegreturn.
Post by Olga Kornievskaia
Post by Trond Myklebust
Post by Olga Kornievskaia
+ }
+ }
return nfs4_handle_delegation_recall_error(server, state, stateid, err);
}
@@ -5957,6 +5973,10 @@ int nfs4_lock_delegation_recall(struct
file_lock *fl, struct nfs4_state *state,
err = nfs4_set_lock_state(state, fl);
if (err != 0)
return err;
+ if (test_bit(NFS_LOCK_LOST, &fl->fl_u.nfs4_fl.owner->ls_flags)) {
+ pr_warn_ratelimited("NFS: %s: Lock reclaim failed!\n",
__func__);
+ return -EIO;
+ }
err = _nfs4_do_setlk(state, F_SETLK, fl, NFS_LOCK_NEW);
Note that there is a potential race here if the server is playing with
NFS4ERR_DELEG_REVOKED or NFS4ERR_ADMIN_REVOKED. Since we do not
present the delegation as part of the LOCK request, we have no way of
knowing if the delegation is still in effect. I guess we can check the
return value of DELEGRETURN, but if it returns NFS4ERR_DELEG_REVOKED
or NFS4ERR_ADMIN_REVOKED, then we still do not know whether or not the
LOCK is safe.
I'm not following you. We send LOCK before we send DELEGRETURN? Also,
we normally send in LOCK the open_stateid returned by the open with
cur so do we know that delegation is still in effect.
How so? The open stateid doesn't tell you that the delegation is still
in effect. If the DELEGRETURN returns NFS4ERR_DELEG_REVOKED, how can
you tell if the delegation was revoked before or after the LOCK
request was handled?
Post by Olga Kornievskaia
Post by Trond Myklebust
Post by Olga Kornievskaia
return nfs4_handle_delegation_recall_error(server, state, stateid, err);
}
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
--
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust-7I+n7zu2hftEKMMhf/***@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Trond Myklebust
2014-10-13 21:29:08 UTC
Permalink
On Mon, Oct 13, 2014 at 5:12 PM, Trond Myklebust
Post by Trond Myklebust
Post by Olga Kornievskaia
On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
Post by Trond Myklebust
Post by Olga Kornievskaia
+ }
+ }
return nfs4_handle_delegation_recall_error(server, state, stateid, err);
}
@@ -5957,6 +5973,10 @@ int nfs4_lock_delegation_recall(struct
file_lock *fl, struct nfs4_state *state,
err = nfs4_set_lock_state(state, fl);
if (err != 0)
return err;
+ if (test_bit(NFS_LOCK_LOST, &fl->fl_u.nfs4_fl.owner->ls_flags)) {
+ pr_warn_ratelimited("NFS: %s: Lock reclaim failed!\n",
__func__);
+ return -EIO;
+ }
err = _nfs4_do_setlk(state, F_SETLK, fl, NFS_LOCK_NEW);
Note that there is a potential race here if the server is playing with
NFS4ERR_DELEG_REVOKED or NFS4ERR_ADMIN_REVOKED. Since we do not
present the delegation as part of the LOCK request, we have no way of
knowing if the delegation is still in effect. I guess we can check the
return value of DELEGRETURN, but if it returns NFS4ERR_DELEG_REVOKED
or NFS4ERR_ADMIN_REVOKED, then we still do not know whether or not the
LOCK is safe.
I'm not following you. We send LOCK before we send DELEGRETURN? Also,
we normally send in LOCK the open_stateid returned by the open with
cur so do we know that delegation is still in effect.
How so? The open stateid doesn't tell you that the delegation is still
in effect. If the DELEGRETURN returns NFS4ERR_DELEG_REVOKED, how can
you tell if the delegation was revoked before or after the LOCK
request was handled?
Actually, let me answer that myself. You can sort of figure things out
in NFSv4.1 if you look at the SEQ4_STATUS_RECALLABLE_STATE_REVOKED
flag. If it is set, you should probably distrust the lock stateid that
you just attempted to recover, since you now know that at least one
delegation was just revoked.

In that case, we probably also have to do a TEST_STATEID+FREE_STATEID
on the delegation stateid.
--
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust-7I+n7zu2hftEKMMhf/***@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Olga Kornievskaia
2014-10-13 22:13:05 UTC
Permalink
On Mon, Oct 13, 2014 at 5:29 PM, Trond Myklebust
Post by Trond Myklebust
On Mon, Oct 13, 2014 at 5:12 PM, Trond Myklebust
Post by Trond Myklebust
Post by Olga Kornievskaia
On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
Post by Trond Myklebust
Post by Olga Kornievskaia
+ }
+ }
return nfs4_handle_delegation_recall_error(server, state, stateid, err);
}
@@ -5957,6 +5973,10 @@ int nfs4_lock_delegation_recall(struct
file_lock *fl, struct nfs4_state *state,
err = nfs4_set_lock_state(state, fl);
if (err != 0)
return err;
+ if (test_bit(NFS_LOCK_LOST, &fl->fl_u.nfs4_fl.owner->ls_flags)) {
+ pr_warn_ratelimited("NFS: %s: Lock reclaim failed!\n",
__func__);
+ return -EIO;
+ }
err = _nfs4_do_setlk(state, F_SETLK, fl, NFS_LOCK_NEW);
Note that there is a potential race here if the server is playing with
NFS4ERR_DELEG_REVOKED or NFS4ERR_ADMIN_REVOKED. Since we do not
present the delegation as part of the LOCK request, we have no way of
knowing if the delegation is still in effect. I guess we can check the
return value of DELEGRETURN, but if it returns NFS4ERR_DELEG_REVOKED
or NFS4ERR_ADMIN_REVOKED, then we still do not know whether or not the
LOCK is safe.
I'm not following you. We send LOCK before we send DELEGRETURN? Also,
we normally send in LOCK the open_stateid returned by the open with
cur so do we know that delegation is still in effect.
How so? The open stateid doesn't tell you that the delegation is still
in effect. If the DELEGRETURN returns NFS4ERR_DELEG_REVOKED, how can
you tell if the delegation was revoked before or after the LOCK
request was handled?
Actually, let me answer that myself. You can sort of figure things out
in NFSv4.1 if you look at the SEQ4_STATUS_RECALLABLE_STATE_REVOKED
flag. If it is set, you should probably distrust the lock stateid that
you just attempted to recover, since you now know that at least one
delegation was just revoked.
In that case, we probably also have to do a TEST_STATEID+FREE_STATEID
on the delegation stateid.
I think we are mis-communicating here by talking about different
nuances. I agree with you that when an operation is sent there is no
way of knowing if in the mean while the server has decided to revoke
the delegation. However, this is not what I'm confused about regarding
your comment. I'm noticing that in the flow of operations, we send (1)
open with cur, then (2) lock, then (3) delegreturn. So I was confused
about how can we check return of delegreturn, step 3, if we are in
step 2.

I think the LOCK is safe if the reply to the LOCK is successful.

Let me just step back from this to note that your solution to "lost
locks during delegation" is to recognize the open with cure failure
and skip locking and delegreturn. I can work on a patch for that.

Do you agree that the state recovery should not be initiated in case
we get those errors?
Post by Trond Myklebust
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Trond Myklebust
2014-10-13 22:24:21 UTC
Permalink
Post by Olga Kornievskaia
On Mon, Oct 13, 2014 at 5:29 PM, Trond Myklebust
Post by Trond Myklebust
On Mon, Oct 13, 2014 at 5:12 PM, Trond Myklebust
Post by Trond Myklebust
Post by Olga Kornievskaia
On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
Post by Trond Myklebust
Post by Olga Kornievskaia
+ }
+ }
return nfs4_handle_delegation_recall_error(server, state, stateid, err);
}
@@ -5957,6 +5973,10 @@ int nfs4_lock_delegation_recall(struct
file_lock *fl, struct nfs4_state *state,
err = nfs4_set_lock_state(state, fl);
if (err != 0)
return err;
+ if (test_bit(NFS_LOCK_LOST, &fl->fl_u.nfs4_fl.owner->ls_flags)) {
+ pr_warn_ratelimited("NFS: %s: Lock reclaim failed!\n",
__func__);
+ return -EIO;
+ }
err = _nfs4_do_setlk(state, F_SETLK, fl, NFS_LOCK_NEW);
Note that there is a potential race here if the server is playing with
NFS4ERR_DELEG_REVOKED or NFS4ERR_ADMIN_REVOKED. Since we do not
present the delegation as part of the LOCK request, we have no way of
knowing if the delegation is still in effect. I guess we can check the
return value of DELEGRETURN, but if it returns NFS4ERR_DELEG_REVOKED
or NFS4ERR_ADMIN_REVOKED, then we still do not know whether or not the
LOCK is safe.
I'm not following you. We send LOCK before we send DELEGRETURN? Also,
we normally send in LOCK the open_stateid returned by the open with
cur so do we know that delegation is still in effect.
How so? The open stateid doesn't tell you that the delegation is still
in effect. If the DELEGRETURN returns NFS4ERR_DELEG_REVOKED, how can
you tell if the delegation was revoked before or after the LOCK
request was handled?
Actually, let me answer that myself. You can sort of figure things out
in NFSv4.1 if you look at the SEQ4_STATUS_RECALLABLE_STATE_REVOKED
flag. If it is set, you should probably distrust the lock stateid that
you just attempted to recover, since you now know that at least one
delegation was just revoked.
In that case, we probably also have to do a TEST_STATEID+FREE_STATEID
on the delegation stateid.
I think we are mis-communicating here by talking about different
nuances. I agree with you that when an operation is sent there is no
way of knowing if in the mean while the server has decided to revoke
the delegation. However, this is not what I'm confused about regarding
your comment. I'm noticing that in the flow of operations, we send (1)
open with cur, then (2) lock, then (3) delegreturn. So I was confused
about how can we check return of delegreturn, step 3, if we are in
step 2.
I think the LOCK is safe if the reply to the LOCK is successful.
It needs to be concurrent with the delegation as well, otherwise
general lock atomicity is broken.
Post by Olga Kornievskaia
Let me just step back from this to note that your solution to "lost
locks during delegation" is to recognize the open with cure failure
and skip locking and delegreturn. I can work on a patch for that.
Do you agree that the state recovery should not be initiated in case
we get those errors?
State recovery _should_ be initiated so that we do reclaim opens (I
don't care about share lock atomicity on Linux). However
nfs_delegation_claim_locks() _should_not_ be called.

I'll give some more thought as to how we can ensure the missing lock atomicity.
--
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust-7I+n7zu2hftEKMMhf/***@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Olga Kornievskaia
2014-10-14 15:48:33 UTC
Permalink
On Mon, Oct 13, 2014 at 6:24 PM, Trond Myklebust
Post by Trond Myklebust
Post by Olga Kornievskaia
On Mon, Oct 13, 2014 at 5:29 PM, Trond Myklebust
Post by Trond Myklebust
On Mon, Oct 13, 2014 at 5:12 PM, Trond Myklebust
Post by Trond Myklebust
Post by Olga Kornievskaia
On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
Post by Trond Myklebust
Post by Olga Kornievskaia
+ }
+ }
return nfs4_handle_delegation_recall_error(server, state, stateid, err);
}
@@ -5957,6 +5973,10 @@ int nfs4_lock_delegation_recall(struct
file_lock *fl, struct nfs4_state *state,
err = nfs4_set_lock_state(state, fl);
if (err != 0)
return err;
+ if (test_bit(NFS_LOCK_LOST, &fl->fl_u.nfs4_fl.owner->ls_flags)) {
+ pr_warn_ratelimited("NFS: %s: Lock reclaim failed!\n",
__func__);
+ return -EIO;
+ }
err = _nfs4_do_setlk(state, F_SETLK, fl, NFS_LOCK_NEW);
Note that there is a potential race here if the server is playing with
NFS4ERR_DELEG_REVOKED or NFS4ERR_ADMIN_REVOKED. Since we do not
present the delegation as part of the LOCK request, we have no way of
knowing if the delegation is still in effect. I guess we can check the
return value of DELEGRETURN, but if it returns NFS4ERR_DELEG_REVOKED
or NFS4ERR_ADMIN_REVOKED, then we still do not know whether or not the
LOCK is safe.
I'm not following you. We send LOCK before we send DELEGRETURN? Also,
we normally send in LOCK the open_stateid returned by the open with
cur so do we know that delegation is still in effect.
How so? The open stateid doesn't tell you that the delegation is still
in effect. If the DELEGRETURN returns NFS4ERR_DELEG_REVOKED, how can
you tell if the delegation was revoked before or after the LOCK
request was handled?
Actually, let me answer that myself. You can sort of figure things out
in NFSv4.1 if you look at the SEQ4_STATUS_RECALLABLE_STATE_REVOKED
flag. If it is set, you should probably distrust the lock stateid that
you just attempted to recover, since you now know that at least one
delegation was just revoked.
In that case, we probably also have to do a TEST_STATEID+FREE_STATEID
on the delegation stateid.
I think we are mis-communicating here by talking about different
nuances. I agree with you that when an operation is sent there is no
way of knowing if in the mean while the server has decided to revoke
the delegation. However, this is not what I'm confused about regarding
your comment. I'm noticing that in the flow of operations, we send (1)
open with cur, then (2) lock, then (3) delegreturn. So I was confused
about how can we check return of delegreturn, step 3, if we are in
step 2.
I think the LOCK is safe if the reply to the LOCK is successful.
It needs to be concurrent with the delegation as well, otherwise
general lock atomicity is broken.
Post by Olga Kornievskaia
Let me just step back from this to note that your solution to "lost
locks during delegation" is to recognize the open with cure failure
and skip locking and delegreturn. I can work on a patch for that.
Do you agree that the state recovery should not be initiated in case
we get those errors?
State recovery _should_ be initiated so that we do reclaim opens (I
don't care about share lock atomicity on Linux). However
nfs_delegation_claim_locks() _should_not_ be called.
I'll give some more thought as to how we can ensure the missing lock atomicity.
If state recover is initiated, it will go into an infinite loop. There
is no way to stop it once it's initiated. A "recovery" open will get a
BAD_STATEID which will again initiated state recovery.

Are proposing to add smarts to the state manager when it should not
recover from a BAD_STATEID?
Post by Trond Myklebust
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Olga Kornievskaia
2014-10-16 18:43:48 UTC
Permalink
Post by Olga Kornievskaia
On Mon, Oct 13, 2014 at 6:24 PM, Trond Myklebust
Post by Trond Myklebust
Post by Olga Kornievskaia
On Mon, Oct 13, 2014 at 5:29 PM, Trond Myklebust
Post by Trond Myklebust
On Mon, Oct 13, 2014 at 5:12 PM, Trond Myklebust
Post by Trond Myklebust
Post by Olga Kornievskaia
On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
Post by Trond Myklebust
Post by Olga Kornievskaia
+ }
+ }
return nfs4_handle_delegation_recall_error(server, state, stateid, err);
}
@@ -5957,6 +5973,10 @@ int nfs4_lock_delegation_recall(struct
file_lock *fl, struct nfs4_state *state,
err = nfs4_set_lock_state(state, fl);
if (err != 0)
return err;
+ if (test_bit(NFS_LOCK_LOST, &fl->fl_u.nfs4_fl.owner->ls_flags)) {
+ pr_warn_ratelimited("NFS: %s: Lock reclaim failed!\n",
__func__);
+ return -EIO;
+ }
err = _nfs4_do_setlk(state, F_SETLK, fl, NFS_LOCK_NEW);
Note that there is a potential race here if the server is playing with
NFS4ERR_DELEG_REVOKED or NFS4ERR_ADMIN_REVOKED. Since we do not
present the delegation as part of the LOCK request, we have no way of
knowing if the delegation is still in effect. I guess we can check the
return value of DELEGRETURN, but if it returns NFS4ERR_DELEG_REVOKED
or NFS4ERR_ADMIN_REVOKED, then we still do not know whether or not the
LOCK is safe.
I'm not following you. We send LOCK before we send DELEGRETURN? Also,
we normally send in LOCK the open_stateid returned by the open with
cur so do we know that delegation is still in effect.
How so? The open stateid doesn't tell you that the delegation is still
in effect. If the DELEGRETURN returns NFS4ERR_DELEG_REVOKED, how can
you tell if the delegation was revoked before or after the LOCK
request was handled?
Actually, let me answer that myself. You can sort of figure things out
in NFSv4.1 if you look at the SEQ4_STATUS_RECALLABLE_STATE_REVOKED
flag. If it is set, you should probably distrust the lock stateid that
you just attempted to recover, since you now know that at least one
delegation was just revoked.
In that case, we probably also have to do a TEST_STATEID+FREE_STATEID
on the delegation stateid.
I think we are mis-communicating here by talking about different
nuances. I agree with you that when an operation is sent there is no
way of knowing if in the mean while the server has decided to revoke
the delegation. However, this is not what I'm confused about regarding
your comment. I'm noticing that in the flow of operations, we send (1)
open with cur, then (2) lock, then (3) delegreturn. So I was confused
about how can we check return of delegreturn, step 3, if we are in
step 2.
I think the LOCK is safe if the reply to the LOCK is successful.
It needs to be concurrent with the delegation as well, otherwise
general lock atomicity is broken.
Post by Olga Kornievskaia
Let me just step back from this to note that your solution to "lost
locks during delegation" is to recognize the open with cure failure
and skip locking and delegreturn. I can work on a patch for that.
Do you agree that the state recovery should not be initiated in case
we get those errors?
State recovery _should_ be initiated so that we do reclaim opens (I
don't care about share lock atomicity on Linux). However
nfs_delegation_claim_locks() _should_not_ be called.
I'll give some more thought as to how we can ensure the missing lock atomicity.
If state recover is initiated, it will go into an infinite loop. There
is no way to stop it once it's initiated. A "recovery" open will get a
BAD_STATEID which will again initiated state recovery.
Are proposing to add smarts to the state manager when it should not
recover from a BAD_STATEID?
Ok. How about something like this?

[PATCH 1/1] Fixing infinite state recovery loop due to failed delegation return

If we get a bad-stateid-type of error when we send OPEN with delegate_cur
to return currently held delegation, we shouldn't be trying to reclaim locks
associated with that delegation state_id because we don't have an
open_stateid to be used for the LOCK operation. Thus, we should
return an error from the nfs4_open_delegation_recall() in that case.

Furthermore, if an error occurs the delegation code will call
nfs_abort_delegation_return() which sets again the NFS4CLNT_DELEGRETURN
flags in the state and it leads the state manager to into an infinite loop
for trying to reclaim the delegated state.

Signed-off-by: Olga Kornievskaia <kolga-HgOvQuBEEgTQT0dZR+***@public.gmane.org>
---
fs/nfs/delegation.c | 5 +++--
fs/nfs/nfs4proc.c | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 5853f53..8016d89 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -394,7 +394,7 @@ static int nfs_end_delegation_return(struct inode
*inode, struct nfs_delegation
err = nfs4_wait_clnt_recover(clp);
} while (err == 0);

- if (err) {
+ if (err && err != -EIO) {
nfs_abort_delegation_return(delegation, clp);
goto out;
}
@@ -458,7 +458,8 @@ restart:
iput(inode);
if (!err)
goto restart;
- set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state);
+ if (err != -EIO)
+ set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state);
return err;
}
}
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 5aa55c1..6871055 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1655,7 +1655,7 @@ static int
nfs4_handle_delegation_recall_error(struct nfs_server *server, struct
nfs_inode_find_state_and_recover(state->inode,
stateid);
nfs4_schedule_stateid_recovery(server, state);
- return 0;
+ return -EIO;
case -NFS4ERR_DELAY:
case -NFS4ERR_GRACE:
set_bit(NFS_DELEGATED_STATE, &state->flags);
--
1.7.1
Post by Olga Kornievskaia
Post by Trond Myklebust
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Olga Kornievskaia
2014-10-21 18:22:22 UTC
Permalink
I'd like to checkin to see what folks think about the proposed fix?
Post by Olga Kornievskaia
Post by Olga Kornievskaia
On Mon, Oct 13, 2014 at 6:24 PM, Trond Myklebust
Post by Trond Myklebust
Post by Olga Kornievskaia
On Mon, Oct 13, 2014 at 5:29 PM, Trond Myklebust
Post by Trond Myklebust
On Mon, Oct 13, 2014 at 5:12 PM, Trond Myklebust
Post by Trond Myklebust
Post by Olga Kornievskaia
On Mon, Oct 13, 2014 at 3:53 PM, Trond Myklebust
Post by Trond Myklebust
Post by Olga Kornievskaia
+ }
+ }
return nfs4_handle_delegation_recall_error(server, state, stateid, err);
}
@@ -5957,6 +5973,10 @@ int nfs4_lock_delegation_recall(struct
file_lock *fl, struct nfs4_state *state,
err = nfs4_set_lock_state(state, fl);
if (err != 0)
return err;
+ if (test_bit(NFS_LOCK_LOST, &fl->fl_u.nfs4_fl.owner->ls_flags)) {
+ pr_warn_ratelimited("NFS: %s: Lock reclaim failed!\n",
__func__);
+ return -EIO;
+ }
err = _nfs4_do_setlk(state, F_SETLK, fl, NFS_LOCK_NEW);
Note that there is a potential race here if the server is playing with
NFS4ERR_DELEG_REVOKED or NFS4ERR_ADMIN_REVOKED. Since we do not
present the delegation as part of the LOCK request, we have no way of
knowing if the delegation is still in effect. I guess we can check the
return value of DELEGRETURN, but if it returns NFS4ERR_DELEG_REVOKED
or NFS4ERR_ADMIN_REVOKED, then we still do not know whether or not the
LOCK is safe.
I'm not following you. We send LOCK before we send DELEGRETURN? Also,
we normally send in LOCK the open_stateid returned by the open with
cur so do we know that delegation is still in effect.
How so? The open stateid doesn't tell you that the delegation is still
in effect. If the DELEGRETURN returns NFS4ERR_DELEG_REVOKED, how can
you tell if the delegation was revoked before or after the LOCK
request was handled?
Actually, let me answer that myself. You can sort of figure things out
in NFSv4.1 if you look at the SEQ4_STATUS_RECALLABLE_STATE_REVOKED
flag. If it is set, you should probably distrust the lock stateid that
you just attempted to recover, since you now know that at least one
delegation was just revoked.
In that case, we probably also have to do a TEST_STATEID+FREE_STATEID
on the delegation stateid.
I think we are mis-communicating here by talking about different
nuances. I agree with you that when an operation is sent there is no
way of knowing if in the mean while the server has decided to revoke
the delegation. However, this is not what I'm confused about regarding
your comment. I'm noticing that in the flow of operations, we send (1)
open with cur, then (2) lock, then (3) delegreturn. So I was confused
about how can we check return of delegreturn, step 3, if we are in
step 2.
I think the LOCK is safe if the reply to the LOCK is successful.
It needs to be concurrent with the delegation as well, otherwise
general lock atomicity is broken.
Post by Olga Kornievskaia
Let me just step back from this to note that your solution to "lost
locks during delegation" is to recognize the open with cure failure
and skip locking and delegreturn. I can work on a patch for that.
Do you agree that the state recovery should not be initiated in case
we get those errors?
State recovery _should_ be initiated so that we do reclaim opens (I
don't care about share lock atomicity on Linux). However
nfs_delegation_claim_locks() _should_not_ be called.
I'll give some more thought as to how we can ensure the missing lock atomicity.
If state recover is initiated, it will go into an infinite loop. There
is no way to stop it once it's initiated. A "recovery" open will get a
BAD_STATEID which will again initiated state recovery.
Are proposing to add smarts to the state manager when it should not
recover from a BAD_STATEID?
Ok. How about something like this?
[PATCH 1/1] Fixing infinite state recovery loop due to failed delegation return
If we get a bad-stateid-type of error when we send OPEN with delegate_cur
to return currently held delegation, we shouldn't be trying to reclaim locks
associated with that delegation state_id because we don't have an
open_stateid to be used for the LOCK operation. Thus, we should
return an error from the nfs4_open_delegation_recall() in that case.
Furthermore, if an error occurs the delegation code will call
nfs_abort_delegation_return() which sets again the NFS4CLNT_DELEGRETURN
flags in the state and it leads the state manager to into an infinite loop
for trying to reclaim the delegated state.
---
fs/nfs/delegation.c | 5 +++--
fs/nfs/nfs4proc.c | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 5853f53..8016d89 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -394,7 +394,7 @@ static int nfs_end_delegation_return(struct inode
*inode, struct nfs_delegation
err = nfs4_wait_clnt_recover(clp);
} while (err == 0);
- if (err) {
+ if (err && err != -EIO) {
nfs_abort_delegation_return(delegation, clp);
goto out;
}
iput(inode);
if (!err)
goto restart;
- set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state);
+ if (err != -EIO)
+ set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state);
return err;
}
}
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 5aa55c1..6871055 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1655,7 +1655,7 @@ static int
nfs4_handle_delegation_recall_error(struct nfs_server *server, struct
nfs_inode_find_state_and_recover(state->inode,
stateid);
nfs4_schedule_stateid_recovery(server, state);
- return 0;
+ return -EIO;
set_bit(NFS_DELEGATED_STATE, &state->flags);
--
1.7.1
Post by Olga Kornievskaia
Post by Trond Myklebust
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Loading...