On Wed, 2010-09-08 at 11:11 +0800, Bian Naimeng wrote: > If there are not any delegation at this inode, we should clear stateid's > NFS_DELEGATED_STATE when update it. > > Signed-off-by: Bian Naimeng <biannm@xxxxxxxxxxxxxx> > > --- > fs/nfs/delegation.c | 1 + > fs/nfs/nfs4proc.c | 12 +++++++++++- > 2 files changed, 12 insertions(+), 1 deletions(-) > > diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c > index b9c3c43..62f296e 100644 > --- a/fs/nfs/delegation.c > +++ b/fs/nfs/delegation.c > @@ -106,6 +106,7 @@ again: > continue; > if (memcmp(state->stateid.data, stateid->data, sizeof(state->stateid.data)) != 0) > continue; > + clear_bit(NFS_DELEGATED_STATE, &state->flags); > get_nfs_open_context(ctx); > spin_unlock(&inode->i_lock); > err = nfs4_open_delegation_recall(ctx, state, stateid); > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 089da5b..f7e45b4 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -919,8 +919,18 @@ static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat > > rcu_read_lock(); > deleg_cur = rcu_dereference(nfsi->delegation); > - if (deleg_cur == NULL) > + if (deleg_cur == NULL) { > + if (delegation == NULL && open_stateid != NULL) { Well... What I really meant was that we should make sure that we don't get into this situation. I think the clear_bit() should be unconditional if delegation == NULL, but if the (delegation == NULL && open_stateid == NULL) _can_ occur, then we should probably mark the nfs4_state for recovery using nfs4_state_mark_reclaim_nograce(), and then fire of a recovery thread. > + /* > + * FIXME: If the state has NFS_DELEGATED_STATE bit > + * we catch a race. Maybe should recover its open > + * stateid, now we just clear the NFS_DELEGATED_STATE > + * bit. > + */ > + clear_bit(NFS_DELEGATED_STATE, &state->flags); > + } > goto no_delegation; > + } > > spin_lock(&deleg_cur->lock); > if (nfsi->delegation != deleg_cur || > -- > 1.7.0 > > > >>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > >>>> index 36400d3..c0c0320 100644 > >>>> --- a/fs/nfs/nfs4proc.c > >>>> +++ b/fs/nfs/nfs4proc.c > >>>> @@ -900,8 +900,18 @@ static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat > >>>> > >>>> rcu_read_lock(); > >>>> deleg_cur = rcu_dereference(nfsi->delegation); > >>>> - if (deleg_cur == NULL) > >>>> + if (deleg_cur == NULL) { > >>>> + if (delegation == NULL && > >>>> + test_bit(NFS_DELEGATED_STATE, &state->flags)) { > >>> Any reason why we can't ditch the 'test_bit'? > >>> > >> Because i am not sure it's ok that we just clear_bit at here. > >> If you think it's ok, i'd be fine with removing this test_bit. > > > > How is it different from just clearing the bit? > > > >>>> + /*FIXME: If the state has NFS_DELEGATED_STATE bit, > >>>> + * we catch a race. Maybe should recover its open > >>>> + * stateid, here we just clear the NFS_DELEGATED_STATE > >>>> + * bit. > >>> Can this ever be called with both deleg_cur==NULL and > >>> open_stateid==NULL? If so, then we still have a bug. > >>> > >> Yes, i will add a checking. Thanks very much. > >> > >>>> + */ > >>>> + clear_bit(NFS_DELEGATED_STATE, &state->flags); > >>>> + } > >>>> goto no_delegation; > >>>> + } > >>>> > >>>> spin_lock(&deleg_cur->lock); > >>>> if (nfsi->delegation != deleg_cur || > > > > Cheers > > Trond > > > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html