| Message ID | 20130808125937.126d8ef1@notabene.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2013-08-08 at 12:59 +1000, NeilBrown wrote: > Hi, > I'm trying to track down a strange problem with state ids going bad > (possibly linked to ntp changing the system time on the non-Linux server) > and am still learning about how the state management works. > > But I've come across an error where I don't think there should be one. > > For whatever reason the client gets a BAD_STATEID on a file that it has a > lock on. The open gets a write delegation so that when it runs > nfs4_reclaim_locks(), nfs4_lock_reclaim aborts early without doing anything > (it doesn't need to because there is a delegation). > But the code below then checks that NFS_LOCK_INITIALIZED is set on all lock > states. But it isn't because nfs4_clear_open_state cleared it and > nfs4_lock_reclaim didn't bother setting it. > > So I think the error should only be printed if there is no delegated state, > hence this patch. > > Does it look right, or have I misunderstood something? > Hi Neil, That analysis looks correct. Can you resend the patch with an appropriate signed-off-by and changelog entry? Thanks! Trond -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com
On Thu, 8 Aug 2013 15:51:30 +0000 "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: > On Thu, 2013-08-08 at 12:59 +1000, NeilBrown wrote: > > Hi, > > I'm trying to track down a strange problem with state ids going bad > > (possibly linked to ntp changing the system time on the non-Linux server) > > and am still learning about how the state management works. > > > > But I've come across an error where I don't think there should be one. > > > > For whatever reason the client gets a BAD_STATEID on a file that it has a > > lock on. The open gets a write delegation so that when it runs > > nfs4_reclaim_locks(), nfs4_lock_reclaim aborts early without doing anything > > (it doesn't need to because there is a delegation). > > But the code below then checks that NFS_LOCK_INITIALIZED is set on all lock > > states. But it isn't because nfs4_clear_open_state cleared it and > > nfs4_lock_reclaim didn't bother setting it. > > > > So I think the error should only be printed if there is no delegated state, > > hence this patch. > > > > Does it look right, or have I misunderstood something? > > > > Hi Neil, > > That analysis looks correct. Can you resend the patch with an > appropriate signed-off-by and changelog entry? Thanks. I've resent separately. NeilBrown
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c index 1fab140..1876ee7 100644 --- a/fs/nfs/nfs4state.c +++ b/fs/nfs/nfs4state.c @@ -1444,14 +1444,16 @@ restart: if (status >= 0) { status = nfs4_reclaim_locks(state, ops); if (status >= 0) { - spin_lock(&state->state_lock); - list_for_each_entry(lock, &state->lock_states, ls_locks) { - if (!test_bit(NFS_LOCK_INITIALIZED, &lock->ls_flags)) - pr_warn_ratelimited("NFS: " - "%s: Lock reclaim " - "failed!\n", __func__); + if (test_bit(NFS_DELEGATED_STATE, &state->flags) != 0) { + spin_lock(&state->state_lock); + list_for_each_entry(lock, &state->lock_states, ls_locks) { + if (!test_bit(NFS_LOCK_INITIALIZED, &lock->ls_flags)) + pr_warn_ratelimited("NFS: " + "%s: Lock reclaim " + "failed!\n", __func__); + } + spin_unlock(&state->state_lock); } - spin_unlock(&state->state_lock); nfs4_put_open_state(state); spin_lock(&sp->so_lock); goto restart;