On Wed, Jul 17, 2019 at 7:07 PM J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
>
> On Mon, Jul 08, 2019 at 03:23:48PM -0400, Olga Kornievskaia wrote:
> > @@ -726,24 +727,53 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla
> > /*
> > * Create a unique stateid_t to represent each COPY.
> > */
> > -int nfs4_init_cp_state(struct nfsd_net *nn, struct nfsd4_copy *copy)
> > +static int nfs4_init_cp_state(struct nfsd_net *nn, void *ptr, stateid_t *stid)
> > {
> > int new_id;
> >
> > idr_preload(GFP_KERNEL);
> > spin_lock(&nn->s2s_cp_lock);
> > - new_id = idr_alloc_cyclic(&nn->s2s_cp_stateids, copy, 0, 0, GFP_NOWAIT);
> > + new_id = idr_alloc_cyclic(&nn->s2s_cp_stateids, ptr, 0, 0, GFP_NOWAIT);
> > spin_unlock(&nn->s2s_cp_lock);
> > idr_preload_end();
> > if (new_id < 0)
> > return 0;
> > - copy->cp_stateid.si_opaque.so_id = new_id;
> > - copy->cp_stateid.si_opaque.so_clid.cl_boot = nn->boot_time;
> > - copy->cp_stateid.si_opaque.so_clid.cl_id = nn->s2s_cp_cl_id;
> > + stid->si_opaque.so_id = new_id;
> > + stid->si_opaque.so_clid.cl_boot = nn->boot_time;
> > + stid->si_opaque.so_clid.cl_id = nn->s2s_cp_cl_id;
> > return 1;
> > }
> >
> > -void nfs4_free_cp_state(struct nfsd4_copy *copy)
> > +int nfs4_init_copy_state(struct nfsd_net *nn, struct nfsd4_copy *copy)
> > +{
> > + return nfs4_init_cp_state(nn, copy, ©->cp_stateid);
> > +}
>
> This little bit of refactoring could go into a seperate patch. It's
> easier for me to review lots of smaller patches.
>
> But I don't understand why you're doing it.
>
> Also, I'm a little suspicious of code that doesn't initialize an object
> till after it's been added to a global structure. The more typical
> pattern is:
>
>
> initialize foo
> take locks, add foo global structure, drop locks.
>
> This prevents anyone doing a lookup from finding "foo" while it's still
> in a partially initialized state.
Let me try to explain the change. This change is due to the fact that
now both COPY_NOTIFY and COPY both are generating unique stateid
(COPY_NOTIFY needs a unique stateid to passed into the COPY and COPY
is generating a unique stateid to be referred to by callbacks).
Previously we had just the COPY generating the stateid (so it was
stored in the nfs4_copy structure) but now we have the COPY_NOTIFY
which doesn't create nfs4_copy when it's processing the operation but
still needs a unique stateid (stored in the stateid structure).
Let me see if I understand your suspicion and ask for guidance how to
resolve it as perhaps I'm misusing the function. idr_alloc_cyclic()
keeps track of the structure of the 2nd arguments with a value it
returns. How do I initiate the structure with the value of the
function without knowing the value which can only be returned when I
call the function to add it to the list? what you are suggesting is to
somehow get the value for the new_id but not associate anything then
update the copy structure with that value and then call
idr_alloc_cyclic() (or something else) to create that association of
the new_id and the structure? I don't know how to do that.
>
> --b.
