On Tue, Aug 05, 2014 at 01:01:05PM -0400, Jeff Layton wrote:
> On Tue, 5 Aug 2014 11:36:28 -0400
> "J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote:
>
> > Thanks for the documentation! A couple comments:
> >
> > On Wed, Jul 30, 2014 at 08:27:38AM -0400, Jeff Layton wrote:
> > > Add some comments that describe what each of these objects is, and how
> > > they related to one another.
> > >
> > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
> > > ---
> > > fs/nfsd/netns.h | 8 +++++
> > > fs/nfsd/state.h | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
> > > 2 files changed, 92 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> > > index 3831ef6e5c75..46680da55cd7 100644
> > > --- a/fs/nfsd/netns.h
> > > +++ b/fs/nfsd/netns.h
> > > @@ -34,6 +34,14 @@
> > > struct cld_net;
> > > struct nfsd4_client_tracking_ops;
> > >
> > > +/*
> > > + * Represents a nfsd "container". With respect to nfsv4 state tracking, the
> > > + * fields of interest are the *_id_hashtbls and the *_name_tree. These track
> > > + * the nfs4_client objects by either short or long form clientid.
> > > + *
> > > + * Each nfsd_net runs a nfs4_laundromat workqueue job every lease period to
> >
> > It's a little more complicated than that suggests--maybe replace "every
> > lease period" by "when necessary"?
> >
>
> Ok, sure...
>
> > > + * clean up expired clients and delegations within the container.
> > > + */
> > > struct nfsd_net {
> > > struct cld_net *cld_net;
> > >
> > > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > > index a363f6bb621e..52ccd07d92ed 100644
> > > --- a/fs/nfsd/state.h
> > > +++ b/fs/nfsd/state.h
> > > @@ -72,6 +72,11 @@ struct nfsd4_callback {
> > > bool cb_done;
> > > };
> > >
> > > +/*
> > > + * A core object that represents a "common" stateid. These are generally
> > > + * embedded within the different (more specific) stateid objects and contain
> > > + * fields that are of general use to any stateid.
> > > + */
> > > struct nfs4_stid {
> > > atomic_t sc_count;
> > > #define NFS4_OPEN_STID 1
> > > @@ -89,6 +94,18 @@ struct nfs4_stid {
> > > void (*sc_free)(struct nfs4_stid *);
> > > };
> > >
> > > +/*
> > > + * Represents a delegation stateid. The nfs4_client holds references to these
> > > + * and they are put when it is being destroyed or when the delegation is
> > > + * returned by the client.
> >
> > This might be worth another sentence or two. I believe the references
> > are:
> >
> > - 1 reference as long as a delegation is still in force (taken
> > when it's alloc'd, put when it's returned or revoked)
> > - 1 reference as long as a recall rpc is in progress (taken when
> > the lease is broken, put when the rpc exits.
> > - 1 more ephemeral reference for each nfsd thread currently
> > doing something with that delegation without holding
> > cl_lock?
> >
>
> Sounds about right. Would you rather just add that and the other
> changes you suggested above, or do you want me to resend the patch?
Would you mind resending?
> > (By the way, I wonder if that list_for_each_entry() at the end of
> > nfsd4_process_cb_update actually does anything: hasn't
> > nfsd4_cb_recall_release() already run and removed any delegation from
> > the cl_callbacks list? I may just be forgetting how this works.)
> >
>
> Oh, hm. I'm not sure about that. I didn't touch that bit of code, so I
> don't profess to really understand it either.
>
> Let's see...
>
> It's only going to remove it from the list if cb_done is true, and that
> doesn't happen if there was an error and dl_retries isn't exhausted
> yet. So no, I don't think it's dead code...
Ah, I think you're right. So "when the rpc exits" above should be
something like "when we get a reply to the recall or give up trying".
Thanks again for the documentation here. Notes on the various data
structures' lifetimes are especially useful, I think.
--b.
--
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
