On Wed, 2010-09-01 at 14:40 +0800, Bian Naimeng wrote:
> > On Mon, 2010-08-16 at 15:50 +0800, Bian Naimeng wrote:
> >>>>>>>> We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.
>
> ... snip ...
>
> >> A open state can be found at nfsi->open_states and owner->so_states always, but
> >> it not be found at nfsi->open_files until we call nfs4_intent_set_file.
> >>
> >> At _nfs4_do_open, we will invoke nfs4_return_incompatible_delegation to return a
> >> delegation, a open stateid which set NFS_DELEGATED_STATE bit may not be found at
> >> nfsi->open_files, but it still at nfsi->open_states, so we do not clear
> >> NFS_DELEGATED_STATE bit for it.
> >>
> >> Then _nfs4_do_open will find it by nfs4_get_open_state, and we still use it
> >> as a delegation, some error will occur.
> >>
> >
> > OK. I see agree that is a race, but AFAICS, your fix means that we end
> > up with an nfs_state structure that has cleared NFS_DELEGATED_STATE, but
> > that did not recover its open stateid.
> >
>
> Hi trond, what do you think about this one?
Hi Bian,
See comments/questions below.
> Best Regards
> Bian
>
> -------------------------------------------------------------------------
> 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 83bb16f..3a7a19d 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -109,6 +109,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 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'?
> + /*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.
> + */
> + 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
