On Fri, 2010-08-06 at 12:10 +0800, Bian Naimeng wrote:
>
> Trond Myklebust 写道:
> > On Thu, 2010-08-05 at 10:26 +0800, Bian Naimeng wrote:
> >>> On Wed, 2010-08-04 at 17:18 +0800, Bian Naimeng wrote:
> >>>> We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.
> >>>>
>
> ... snip ...
>
> >> Thanks Trond.
> >> But why we must remove test_and_clear_bit behind memcmp?
> >>
> >> Always test_bit and memcmp have same result, and I think test_and_clear_bit is fast
> >> than memcmp, so i suggest we should call test_and_clear_bit first. right?
> >
> > We can't clear the bit before the memcmp() test. What we could do is
> > keep the current test_bit() and then do a clear_bit after the memcmp().
> >
>
> When i apply the patch, my test still fail.
>
> The ctx->state will set set after open success, but this ctx add to the
> the nfsi->open_files when we call nfs_open that is so early. So maybe
> there are some states can be found at nfsi->open_states, but not at
> ctx->state of nfsi->open_files, so we will miss clear NFS_DELEGATED_STATE
> for these state.
>
> Regards
> Bian Naimeng
>
> --------------------------------------------------
>
> NFSv4: Remember to clear NFS_DELEGATED_STATE in nfs_delegation_claim_opens
>
> Signed-off-by: Bian Naimeng <biannm@xxxxxxxxxxxxxx>
>
> ---
> fs/nfs/delegation.c | 15 +++++++++++----
> 1 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index 3016345..94a63e3 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -94,7 +94,7 @@ static int nfs_delegation_claim_opens(struct inode *inode, const nfs4_stateid *s
> struct nfs_inode *nfsi = NFS_I(inode);
> struct nfs_open_context *ctx;
> struct nfs4_state *state;
> - int err;
> + int err = 0;
>
> again:
> spin_lock(&inode->i_lock);
> @@ -112,12 +112,19 @@ again:
> if (err >= 0)
> err = nfs_delegation_claim_locks(ctx, state);
> put_nfs_open_context(ctx);
> - if (err != 0)
> - return err;
> + if (err != 0) {
> + spin_lock(&inode->i_lock);
> + break;
> + }
> goto again;
> }
> +
> + list_for_each_entry(state, &nfsi->open_states, inode_states) {
> + clear_bit(NFS_DELEGATED_STATE, &state->flags);
> + }
> +
> spin_unlock(&inode->i_lock);
> - return 0;
> + return err;
> }
>
I still don't see why this should be necessary. In the case of a server
reboot or network partition, the state recovery thread ought to be
taking care of this for us.
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
