Re: [PATCH 2/7] nfsd: Ensure stateids remain unique until they are freed

[Jeff Layton <jeff.layton@primarydata.com>]

On Tue, 15 Jul 2014 10:15:47 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, Jul 15, 2014 at 06:37:44AM -0400, Jeff Layton wrote:
> > From: Trond Myklebust <trond.myklebust@primarydata.com>
> > 
> > Add an extra delegation state to allow the stateid to remain in the idr
> > tree until the last reference has been released. This will be necessary
> > to ensure uniqueness once the client_mutex is removed.
> > 
> > [jlayton: reset the sc_type under the state_lock in unhash_delegation]
> > 
> > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> > Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> > ---
> >  fs/nfsd/nfs4state.c | 9 ++++-----
> >  fs/nfsd/state.h     | 1 +
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 1b01a20827ab..df3e9ef1fb38 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -609,6 +609,7 @@ static void remove_stid(struct nfs4_stid *s)
> >  
> >  static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
> >  {
> > +	remove_stid(s);
> >  	kmem_cache_free(slab, s);
> >  }
> 
> Might make sense to just keep it in the caller.  In your big series
> remove_stid_locked ends up there again anyway, and it avoids some churn
> in this series that made me investiage how you keep open and lock
> stateids hashed longer in this patch (you don't of course..).
> 

The problem with keeping them in the caller is that it defeats the main
purpose of the patch. The idea here is to make sure that we keep the
stateids in the IDR tree for as long as possible to help ensure
uniqueness. If we keep it in the caller, we're still removing the stateids
from the IDR hash earlier than we should, and that's a larger potential
for collisions.

> Otherwise looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Jeff Layton <jlayton@primarydata.com>