On Mon, 21 Jul 2014 17:30:56 -0400
"J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote:
> On Mon, Jul 21, 2014 at 09:34:59AM -0400, Jeff Layton wrote:
> > From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> >
> > Don't allow stateids to clear the open file pointer until they are
> > being destroyed. In a later patch we'll want to move the putting of
> > the nfs4_file into generic stateid handling code and this will help
> > facilitate that change.
> >
> > Also, move to allocating stateids with kzalloc and get rid of the
> > explicit zeroing of fields.
>
> There's a regression here: we end up holding an unnecessary reference to
> the inode a long time just because some client isn't responding to a
> recall.
>
I must have missed that exchange...
So that's because we're putting the file reference when we put the
last delegation reference instead of when we unhash the
delegation...ok, good point.
I don't see any real harm right offhand of putting the file reference
early when we unhash, so I think that'll work. We do that already for
v4.0 open stateids when they are closed, so we might as well do it for
delegations too. I'll double check that we never use the file after
unhashing and send up a patch to change that unless I see a problem.
> When I complained about this before Trond said something about replacing
> fi_inode by a filehandle? But I can't remember if we've seen code to do
> that.
>
No I don't think we have. That sounds like a bit of an overhaul of the
nfs4_file code.
Personally, I think that hashing on the filehandle makes a lot of
sense, and I don't see a reason to pin down the inode except by virtue
of the fi_fds file references. That said, I'd prefer to defer that sort
of change until after this series is merged.
> >
> > Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
> > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> > ---
> > fs/nfsd/nfs4state.c | 22 ++++++++++------------
> > 1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index a47c97586c76..8ff5c97a2c5a 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -459,7 +459,7 @@ kmem_cache *slab)
> > struct nfs4_stid *stid;
> > int new_id;
> >
> > - stid = kmem_cache_alloc(slab, GFP_KERNEL);
> > + stid = kmem_cache_zalloc(slab, GFP_KERNEL);
> > if (!stid)
> > return NULL;
> >
> > @@ -467,11 +467,9 @@ kmem_cache *slab)
> > if (new_id < 0)
> > goto out_free;
> > stid->sc_client = cl;
> > - stid->sc_type = 0;
> > stid->sc_stateid.si_opaque.so_id = new_id;
> > stid->sc_stateid.si_opaque.so_clid = cl->cl_clientid;
> > /* Will be incremented before return to client: */
> > - stid->sc_stateid.si_generation = 0;
> > atomic_set(&stid->sc_count, 1);
> >
> > /*
> > @@ -592,10 +590,8 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
> > INIT_LIST_HEAD(&dp->dl_perfile);
> > INIT_LIST_HEAD(&dp->dl_perclnt);
> > INIT_LIST_HEAD(&dp->dl_recall_lru);
> > - dp->dl_file = NULL;
> > dp->dl_type = NFS4_OPEN_DELEGATE_READ;
> > fh_copy_shallow(&dp->dl_fh, ¤t_fh->fh_handle);
> > - dp->dl_time = 0;
> > INIT_WORK(&dp->dl_recall.cb_work, nfsd4_run_cb_recall);
> > return dp;
> > }
> > @@ -616,6 +612,8 @@ void
> > nfs4_put_delegation(struct nfs4_delegation *dp)
> > {
> > if (atomic_dec_and_test(&dp->dl_stid.sc_count)) {
> > + if (dp->dl_file)
> > + put_nfs4_file(dp->dl_file);
> > remove_stid(&dp->dl_stid);
> > nfs4_free_stid(deleg_slab, &dp->dl_stid);
> > num_delegations--;
> > @@ -665,13 +663,9 @@ unhash_delegation(struct nfs4_delegation *dp)
> > list_del_init(&dp->dl_recall_lru);
> > list_del_init(&dp->dl_perfile);
> > spin_unlock(&fp->fi_lock);
> > - if (fp) {
> > + if (fp)
> > nfs4_put_deleg_lease(fp);
> > - dp->dl_file = NULL;
> > - }
> > spin_unlock(&state_lock);
> > - if (fp)
> > - put_nfs4_file(fp);
> > }
> >
> > static void destroy_revoked_delegation(struct nfs4_delegation *dp)
> > @@ -879,12 +873,12 @@ static void unhash_generic_stateid(struct nfs4_ol_stateid *stp)
> > static void close_generic_stateid(struct nfs4_ol_stateid *stp)
> > {
> > release_all_access(stp);
> > - put_nfs4_file(stp->st_file);
> > - stp->st_file = NULL;
> > }
> >
> > static void free_generic_stateid(struct nfs4_ol_stateid *stp)
> > {
> > + if (stp->st_file)
> > + put_nfs4_file(stp->st_file);
> > remove_stid(&stp->st_stid);
> > nfs4_free_stid(stateid_slab, &stp->st_stid);
> > }
> > @@ -4462,6 +4456,10 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
> > if (list_empty(&oo->oo_owner.so_stateids))
> > release_openowner(oo);
> > } else {
> > + if (s->st_file) {
> > + put_nfs4_file(s->st_file);
> > + s->st_file = NULL;
> > + }
> > oo->oo_last_closed_stid = s;
> > /*
> > * In the 4.0 case we need to keep the owners around a
> > --
> > 1.9.3
> >
--
Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
--
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
