On Wed, 2010-09-08 at 09:33 +0800, Bian Naimeng wrote:
> >>> 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.
> >
>
> Thanks for your answer.
>
> >> 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
