by J. Bruce Fields

[permalink] [raw]

Subject: Re: [PATCH 08/14] nfsd4: fail when delegreturn gets a non-delegation stateid

On Wed, Mar 11, 2009 at 08:29:52PM +0200, Benny Halevy wrote:
> On Mar. 11, 2009, 2:26 +0200, "J. Bruce Fields" <[email protected]> wrote:
> > From: J. Bruce Fields <[email protected]>
> >
> > Previous cleanup reveals an obvious (though harmless) bug: when
> > delegreturn gets a stateid that isn't for a delegation, it should return
> > an error rather than doing nothing.
> >
> > Signed-off-by: J. Bruce Fields <[email protected]>
> > ---
> > fs/nfsd/nfs4state.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index de1d68a..4a92d1e 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -2418,7 +2418,7 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > status = nfserr_stale_stateid;
> > if (STALE_STATEID(stateid))
> > goto out;
> > - status = nfs_ok;
> > + status = nfserr_bad_stateid;
> > if (is_delegation_stateid(stateid))
> > goto out;
> > status = nfserr_bad_stateid;
>
> This assignment is redundant now, isn't it?

Yep, thanks.--b.

2009-03-12 00:05:42

by J. Bruce Fields

[permalink] [raw]

Subject: Re: [PATCH 12/14] nfsd4: remove use of mutex for file_hashtable

On Wed, Mar 11, 2009 at 08:52:14PM +0200, Benny Halevy wrote:
> On Mar. 11, 2009, 2:27 +0200, "J. Bruce Fields" <[email protected]> wrote:
> > From: J. Bruce Fields <[email protected]>
> >
> > As part of reducing the scope of the client_mutex, and in order to
> > remove the need for mutexes from the callback code (so that callbacks
> > can be done as asynchronous rpc calls), move manipulations of the
> > file_hashtable under the recall_lock.
> >
> > Update the relevant comments while we're here.
> >
> > Signed-off-by: J. Bruce Fields <[email protected]>
> > Cc: Alexandros Batsakis <[email protected]>
> > ---
> > fs/nfsd/nfs4callback.c | 2 --
> > fs/nfsd/nfs4state.c | 29 +++++++++++++++++------------
> > 2 files changed, 17 insertions(+), 14 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > index 3fd7136..5dcd38e 100644
> > --- a/fs/nfsd/nfs4callback.c
> > +++ b/fs/nfsd/nfs4callback.c
> > @@ -491,8 +491,6 @@ out_put_cred:
> > * or deleg_return.
> > */
> > put_nfs4_client(clp);
> > - nfs4_lock_state();
> > nfs4_put_delegation(dp);
> > - nfs4_unlock_state();
> > return;
> > }
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 56e81f9..2f4cb9a 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -78,14 +78,18 @@ static struct nfs4_delegation * find_delegation_stateid(struct inode *ino, state
> > static char user_recovery_dirname[PATH_MAX] = "/var/lib/nfs/v4recovery";
> > static void nfs4_set_recdir(char *recdir);
> >
> > -/* Locking:
> > - *
> > - * client_mutex:
> > - * protects clientid_hashtbl[], clientstr_hashtbl[],
> > - * unconfstr_hashtbl[], uncofid_hashtbl[].
> > - */
> > +/* Locking: */
> > +
> > +/* Currently used for almost all code touching nfsv4 state: */
> > static DEFINE_MUTEX(client_mutex);
> >
> > +/*
> > + * Currently used for the del_recall_lru and file hash table. In an
> > + * effort to decrease the scope of the client_mutex, this spinlock may
> > + * eventually cover more:
> > + */
> > +static DEFINE_SPINLOCK(recall_lock);
> > +
> > static struct kmem_cache *stateowner_slab = NULL;
> > static struct kmem_cache *file_slab = NULL;
> > static struct kmem_cache *stateid_slab = NULL;
> > @@ -116,19 +120,15 @@ opaque_hashval(const void *ptr, int nbytes)
> > return x;
> > }
> >
> > -/*
> > - * Delegation state
> > - */
> > -
> > -/* recall_lock protects the del_recall_lru */
> > -static DEFINE_SPINLOCK(recall_lock);
> > static struct list_head del_recall_lru;
> >
> > static void
> > free_nfs4_file(struct kref *kref)
> > {
> > struct nfs4_file *fp = container_of(kref, struct nfs4_file, fi_ref);
> > + spin_lock(&recall_lock);
> > list_del(&fp->fi_hash);
> > + spin_unlock(&recall_lock);
> > iput(fp->fi_inode);
> > kmem_cache_free(file_slab, fp);
> > }
> > @@ -1004,7 +1004,9 @@ alloc_init_file(struct inode *ino)
> > INIT_LIST_HEAD(&fp->fi_hash);
> > INIT_LIST_HEAD(&fp->fi_stateids);
> > INIT_LIST_HEAD(&fp->fi_delegations);
> > + spin_lock(&recall_lock);
> > list_add(&fp->fi_hash, &file_hashtbl[hashval]);
> > + spin_unlock(&recall_lock);
> > fp->fi_inode = igrab(ino);
> > fp->fi_id = current_fileid++;
> > fp->fi_had_conflict = false;
> > @@ -1177,12 +1179,15 @@ find_file(struct inode *ino)
> > unsigned int hashval = file_hashval(ino);
> > struct nfs4_file *fp;
> >
> > + spin_lock(&recall_lock);
> > list_for_each_entry(fp, &file_hashtbl[hashval], fi_hash) {
> > if (fp->fi_inode == ino) {
> > + spin_unlock(&recall_lock);
>
> Hmm, I'm afraid there's a race here since potentially
> you could take the reference on the file after it has been freed.
> find_file should call get_nfs4_file if and only if it's still hashed,
> i.e. atomically with looking it up on the list.
>
> On the same lines, the lock should be taken in put_nfs4_file
> rather than in free_nfs4_file.
>
> That being said, I'm not sure we're unhashing the file in the right
> place... it's too late for me right now but my hunch is that open
> should alloc_init the nfs4_file as done today and close should unhash
> it (under the recall spinlock), and then put it.
> put_nfs4_file can stay the same and free_nfs4_file should just do the
> iput and kmem_cache_free.
>
> The main difference is that find_file will fail finding the nfs4_file
> struct after close. (get_nfs4_file in find_file should still happen
> under the lock though)

It's probably better for the nfs4_file to be visible longer, but either
is correct.

The only reason the delegation has a reference on this is to keep track
of which files have seen conflicts. Maybe there's some better way.

Thanks for catching this....

--b.