Re: [PATCH v2 09/10] nfsd: Fix delegation revocation

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

On Wed, 16 Jul 2014 11:30:47 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> > -/* Called under the state lock. */
> 
> You're removing a comment hat only becomes true after your patch :)
> 
> But I agree that it's obsolete, the lockdep_assert_held is better than
> the comment.
> 

Yeah, the other problem is that that comment is ambiguous. Does it mean
the state_lock or the client_mutex (which is also sometimes termed
"state lock" in this code).

> > @@ -685,11 +686,10 @@ static void revoke_delegation(struct nfs4_delegation *dp)
> >  	struct nfs4_client *clp = dp->dl_stid.sc_client;
> >  
> >  	if (clp->cl_minorversion == 0)
> > -		destroy_delegation(dp);
> > +		destroy_revoked_delegation(dp);
> 
> Seems a little confusing as we didn't turn the code into a REVOKED_DELEG
> stateid type yet, but given that destroy_revoked_delegation doesn't
> care this is probably fine.
> 

True, but since we're destroying it anyway we don't need to revoke it.
Still it is less confusing and since I'm respinning for other reasons,
I can change that too...

> >  	else {
> > -		unhash_delegation(dp);
> >  		dp->dl_stid.sc_type = NFS4_REVOKED_DELEG_STID;
> > -		list_add(&dp->dl_recall_lru, &clp->cl_revoked);
> > +		list_move(&dp->dl_recall_lru, &clp->cl_revoked);
> 
> What protects access to cl_revoked, btw?
> 

At this point in the series, the client_mutex does. There's a patch
later that protects the cl_revoked list with the cl_lock, but it
depends on some other locking changes and so it's harder to move it to
the front of the series just yet.

> Otherwise this looks fine to me,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> (would have been a little nicer to read if you had kept
> destroy_revoked_delegation, otherwise diff obsfucates the fact that it
> didn't change at all..

Thanks. Do you mean destroy_delegation? destroy_revoked_delegation
still exists -- destroy_delegation is removed, but it's just a wrapper
around nfs4_put_delegation at this point...

-- 
Jeff Layton <jlayton@primarydata.com>