Return-Path: linux-nfs-owner@vger.kernel.org Received: from fieldses.org ([174.143.236.118]:60104 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754684AbaGUVbB (ORCPT ); Mon, 21 Jul 2014 17:31:01 -0400 Date: Mon, 21 Jul 2014 17:30:56 -0400 From: "J. Bruce Fields" To: Jeff Layton Cc: hch@infradead.org, linux-nfs@vger.kernel.org, Trond Myklebust Subject: Re: [PATCH v5 03/10] nfsd: simplify stateid allocation and file handling Message-ID: <20140721213056.GM8438@fieldses.org> References: <1405949706-27757-1-git-send-email-jlayton@primarydata.com> <1405949706-27757-4-git-send-email-jlayton@primarydata.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <1405949706-27757-4-git-send-email-jlayton@primarydata.com> Sender: linux-nfs-owner@vger.kernel.org List-ID: On Mon, Jul 21, 2014 at 09:34:59AM -0400, Jeff Layton wrote: > From: Trond Myklebust > > 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. 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. --b. > > Signed-off-by: Trond Myklebust > Signed-off-by: Jeff Layton > Reviewed-by: Christoph Hellwig > --- > 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 >