Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190708192352.12614-1-olga.kornievskaia@gmail.com>
 <20190708192352.12614-5-olga.kornievskaia@gmail.com> <20190717230726.GA26801@fieldses.org>
 <CAN-5tyHmODP2+nMiinTEP5WZzXz=m=j9LBSWv=b=N3C211JaLg@mail.gmail.com>
 <20190723204537.GA19559@fieldses.org> <CAN-5tyGL+BR+1E1N-HzH3-mmjze8AkBHpYAm0k3i0Dt+iP1ORQ@mail.gmail.com>
 <20190730155528.GE31707@fieldses.org> <CAN-5tyGwyasodrUe4Y+p_Er6XNOBk+mgiaXKXWSBsM5ac4areg@mail.gmail.com>
In-Reply-To: <CAN-5tyGwyasodrUe4Y+p_Er6XNOBk+mgiaXKXWSBsM5ac4areg@mail.gmail.com>
From:   Olga Kornievskaia <olga.kornievskaia@gmail.com>
Date:   Tue, 30 Jul 2019 13:10:07 -0400
Message-ID: <CAN-5tyErgJ6TFLJzmRypmfqObPoA7-LEvFJ0iC4ky=yXyeRBGw@mail.gmail.com>
Subject: Re: [PATCH v4 4/8] NFSD add COPY_NOTIFY operation
To:     "J. Bruce Fields" <bfields@fieldses.org>
Cc:     "J. Bruce Fields" <bfields@redhat.com>,
        linux-nfs <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Tue, Jul 30, 2019 at 12:13 PM Olga Kornievskaia
<olga.kornievskaia@gmail.com> wrote:
>
> On Tue, Jul 30, 2019 at 11:55 AM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Tue, Jul 30, 2019 at 11:48:27AM -0400, Olga Kornievskaia wrote:
> > > On Tue, Jul 23, 2019 at 4:46 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> > > >
> > > > On Mon, Jul 22, 2019 at 04:17:44PM -0400, Olga Kornievskaia wrote:
> > > > > 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.
> > > >
> > > > You could move the initialization under the s2s_cp_lock.  But there's
> > > > additional initialization that's done in the caller.
> > >
> > > I still don't understand what you are looking for here and why. I'm
> > > following what the normal stid allocation does.  There is no extra code
> > > there to see if it initiated or not. nfs4_alloc_stid() calls
> > > idr_alloc_cyclic() creates an association between the stid pointer and
> > > at the time uninitialized nfs4_stid structure which is then filled in
> > > with the return of the idr_alloc_cyclic(). That's exactly what the new
> > > code is doing (well accept that i'll change it to store the
> > > stateid_t).
> >
> > Yes, I'm a little worried about normal stid allocation too.  It's got
> > one extra safeguard because of the check for 0 sc_type in the lookup,
> > I haven't yet convinced myself that's enough.
> >
> > The race I'm worried about is: one task does the idr allocation and
> > drops locks.  Before it has the chance to finish initializing the
> > object, a second task looks it up in the idr and does something with it.
> > It sees the not-yet-initialized fields.
>
> Can the spin_lock() that we call before the idr_alloc_cyclic() be held
> thru the initialization of the stid then? I'm just not sure what this
> idr_preload_end() with a spin_lock but otherwise I don't see why we
> can't and since idr_find() takes the same spin lock before the call,
> it would solve the problem.

actually instead moving initialization of other stid fields prior to
calling the idr_alloc_cycle would never expose the un-initialized
value so

stid->..cl_boot = nn->boot_time
stid->.. cl_id = nn->..id
..
spinlock()
newid = idr_alloc_cycle(stid)
stid->..id = newid
unlock()


>
> >
> > --b.