Re: [PATCH v4 4/8] NFSD add COPY_NOTIFY operation

[Olga Kornievskaia <olga.kornievskaia@gmail.com>]

On Wed, Jul 17, 2019 at 7:07 PM J. Bruce Fields <bfields@fieldses.org> 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, &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.