Re: [PATCH] lightnvm: pblk: stop taking the free lock in in pblk_lines_free

[Hans Holmberg]

On Thu, Jan 24, 2019 at 2:19 PM Javier GonzÃlez <javier@xxxxxxxxxxx> wrote:
>
> > On 22 Jan 2019, at 11.15, hans@xxxxxxxxxxxxx wrote:
> >
> > From: Hans Holmberg <hans.holmberg@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > ---
> > 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 = 0; i < l_mg->nr_lines; i++) {
> >               line = &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Ãlez <javier@xxxxxxxxxxx>
>