Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190122101531.30893-1-hans@owltronix.com> <251D7CB9-A9B0-4474-85A0-B0C23DB31272@javigon.com>
In-Reply-To: <251D7CB9-A9B0-4474-85A0-B0C23DB31272@javigon.com>
From:   Hans Holmberg <hans.ml.holmberg@owltronix.com>
Date:   Fri, 25 Jan 2019 10:15:31 +0100
Message-ID: <CANr-nt04b06LAXm0LUV4wtGM18Qb2UZ2+Kq+D7XC79nyiptUug@mail.gmail.com>
Subject: Re: [PATCH] lightnvm: pblk: stop taking the free lock in in pblk_lines_free
To:     =?UTF-8?Q?Javier_Gonz=C3=A1lez?= <javier@javigon.com>
Cc:     =?UTF-8?Q?Matias_Bj=C3=B8rling?= <mb@lightnvm.io>,
        linux-block@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Hans Holmberg <hans.holmberg@cnexlabs.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jan 24, 2019 at 2:19 PM Javier Gonz=C3=A1lez <javier@javigon.com> w=
rote:
>
> > On 22 Jan 2019, at 11.15, hans@owltronix.com wrote:
> >
> > From: Hans Holmberg <hans.holmberg@cnexlabs.com>
> >
> > pblk_line_meta_free might sleep (it can end up calling vfree, depending
> > on how we allocate lba lists), and this can lead to a BUG()
> > if we wake up on a different cpu and release the lock.
> >
> > As there is no point of grabbing the free lock when pblk has shut down,
> > remove the lock.
> >
> > Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
> > ---
> > drivers/lightnvm/pblk-init.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.=
c
> > index f9a3e47b6a93..eb0135c77805 100644
> > --- a/drivers/lightnvm/pblk-init.c
> > +++ b/drivers/lightnvm/pblk-init.c
> > @@ -584,14 +584,12 @@ static void pblk_lines_free(struct pblk *pblk)
> >       struct pblk_line *line;
> >       int i;
> >
> > -     spin_lock(&l_mg->free_lock);
> >       for (i =3D 0; i < l_mg->nr_lines; i++) {
> >               line =3D &pblk->lines[i];
> >
> >               pblk_line_free(line);
> >               pblk_line_meta_free(l_mg, line);
> >       }
> > -     spin_unlock(&l_mg->free_lock);
> >
> >       pblk_line_mg_free(pblk);
> >
> > --
> > 2.17.1
>
> Can you add a comment too indicating that this is only safe on a single
> threaded shutdown?

To be able to free the lines, we need have stopped anything accessing
the lines first. That seems obvious to me.

It would be nice to make a pass over the code and document pblk's
locking(and other concurrency handling, like the line krefs) though.

Thanks,
Hans

>
> Otherwise the patch looks good.
>
> Reviewed-by: Javier Gonz=C3=A1lez <javier@javigon.com>
>