On Wed, 16 Jul 2014 11:30:47 -0700
Christoph Hellwig <hch@xxxxxxxxxxxxx> 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@xxxxxx>
>
> (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@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
