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: <20190201023806.39895-1-hlitz@ucsc.edu> <7BC68605-5E76-4E0D-8C3E-29B2CB329FEB@javigon.com>
In-Reply-To: <7BC68605-5E76-4E0D-8C3E-29B2CB329FEB@javigon.com>
From:   Hans Holmberg <hans.ml.holmberg@owltronix.com>
Date:   Mon, 4 Feb 2019 22:01:45 +0100
Message-ID: <CANr-nt0uVoOhObd7rh2o9z1EYbHRPxszUwGbstaDnKrjP4v_sQ@mail.gmail.com>
Subject: Re: [PATCH V2] lightnvm: pblk: fix race condition on GC
To:     =?UTF-8?Q?Javier_Gonz=C3=A1lez?= <javier@javigon.com>
Cc:     Heiner Litz <hlitz@ucsc.edu>,
        =?UTF-8?Q?Matias_Bj=C3=B8rling?= <mb@lightnvm.io>,
        Hans Holmberg <hans.holmberg@cnexlabs.com>,
        linux-block@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thanks Heiner, it's awesome to get this fixed!

Reviewed-by: Hans Holmberg <hans.holmberg@cnexlabs.com>

On Mon, Feb 4, 2019 at 9:19 AM Javier Gonz=C3=A1lez <javier@javigon.com> wr=
ote:
>
> > On 1 Feb 2019, at 03.38, Heiner Litz <hlitz@ucsc.edu> wrote:
> >
> > This patch fixes a race condition where a write is mapped to the last
> > sectors of a line. The write is synced to the device but the L2P is not
> > updated yet. When the line is garbage collected before the L2P update i=
s
> > performed, the sectors are ignored by the GC logic and the line is free=
d
> > before all sectors are moved. When the L2P is finally updated, it conta=
ins
> > a mapping to a freed line, subsequent reads of the corresponding LBAs f=
ail.
> >
> > This patch introduces a per line counter specifying the number of secto=
rs
> > that are synced to the device but have not been updated in the L2P. Lin=
es
> > with a counter of greater than zero will not be selected for GC.
> >
> > Signed-off-by: Heiner Litz <hlitz@ucsc.edu>
> > ---
> >
> > v2: changed according to Javier's comment. Instead of performing check
> > while holding the trans_lock, add an atomic per line counter
> >
> > drivers/lightnvm/pblk-core.c  |  1 +
> > drivers/lightnvm/pblk-gc.c    | 20 +++++++++++++-------
> > drivers/lightnvm/pblk-map.c   |  1 +
> > drivers/lightnvm/pblk-rb.c    |  1 +
> > drivers/lightnvm/pblk-write.c |  1 +
> > drivers/lightnvm/pblk.h       |  1 +
> > 6 files changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.=
c
> > index eabcbc119681..b7ed0502abef 100644
> > --- a/drivers/lightnvm/pblk-core.c
> > +++ b/drivers/lightnvm/pblk-core.c
> > @@ -1278,6 +1278,7 @@ static int pblk_line_prepare(struct pblk *pblk, s=
truct pblk_line *line)
> >       spin_unlock(&line->lock);
> >
> >       kref_init(&line->ref);
> > +     atomic_set(&line->sec_to_update, 0);
> >
> >       return 0;
> > }
> > diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c
> > index 2fa118c8eb71..26a52ea7ec45 100644
> > --- a/drivers/lightnvm/pblk-gc.c
> > +++ b/drivers/lightnvm/pblk-gc.c
> > @@ -365,16 +365,22 @@ static struct pblk_line *pblk_gc_get_victim_line(=
struct pblk *pblk,
> >                                                struct list_head *group_=
list)
> > {
> >       struct pblk_line *line, *victim;
> > -     int line_vsc, victim_vsc;
> > +     unsigned int line_vsc =3D ~0x0L, victim_vsc =3D ~0x0L;
> >
> >       victim =3D list_first_entry(group_list, struct pblk_line, list);
> > +
> >       list_for_each_entry(line, group_list, list) {
> > -             line_vsc =3D le32_to_cpu(*line->vsc);
> > -             victim_vsc =3D le32_to_cpu(*victim->vsc);
> > -             if (line_vsc < victim_vsc)
> > +             if (!atomic_read(&line->sec_to_update))
> > +                     line_vsc =3D le32_to_cpu(*line->vsc);
> > +             if (line_vsc < victim_vsc) {
> >                       victim =3D line;
> > +                     victim_vsc =3D le32_to_cpu(*victim->vsc);
> > +             }
> >       }
> >
> > +     if (victim_vsc =3D=3D ~0x0)
> > +             return NULL;
> > +
> >       return victim;
> > }
> >
> > @@ -448,13 +454,13 @@ static void pblk_gc_run(struct pblk *pblk)
> >
> >       do {
> >               spin_lock(&l_mg->gc_lock);
> > -             if (list_empty(group_list)) {
> > +
> > +             line =3D pblk_gc_get_victim_line(pblk, group_list);
> > +             if (!line) {
> >                       spin_unlock(&l_mg->gc_lock);
> >                       break;
> >               }
> >
> > -             line =3D pblk_gc_get_victim_line(pblk, group_list);
> > -
> >               spin_lock(&line->lock);
> >               WARN_ON(line->state !=3D PBLK_LINESTATE_CLOSED);
> >               line->state =3D PBLK_LINESTATE_GC;
> > diff --git a/drivers/lightnvm/pblk-map.c b/drivers/lightnvm/pblk-map.c
> > index 79df583ea709..7fbc99b60cac 100644
> > --- a/drivers/lightnvm/pblk-map.c
> > +++ b/drivers/lightnvm/pblk-map.c
> > @@ -73,6 +73,7 @@ static int pblk_map_page_data(struct pblk *pblk, unsi=
gned int sentry,
> >                */
> >               if (i < valid_secs) {
> >                       kref_get(&line->ref);
> > +                     atomic_inc(&line->sec_to_update);
> >                       w_ctx =3D pblk_rb_w_ctx(&pblk->rwb, sentry + i);
> >                       w_ctx->ppa =3D ppa_list[i];
> >                       meta->lba =3D cpu_to_le64(w_ctx->lba);
> > diff --git a/drivers/lightnvm/pblk-rb.c b/drivers/lightnvm/pblk-rb.c
> > index a6133b50ed9c..03c241b340ea 100644
> > --- a/drivers/lightnvm/pblk-rb.c
> > +++ b/drivers/lightnvm/pblk-rb.c
> > @@ -260,6 +260,7 @@ static int __pblk_rb_update_l2p(struct pblk_rb *rb,=
 unsigned int to_update)
> >                                                       entry->cacheline)=
;
> >
> >               line =3D pblk_ppa_to_line(pblk, w_ctx->ppa);
> > +             atomic_dec(&line->sec_to_update);
> >               kref_put(&line->ref, pblk_line_put);
> >               clean_wctx(w_ctx);
> >               rb->l2p_update =3D pblk_rb_ptr_wrap(rb, rb->l2p_update, 1=
);
> > diff --git a/drivers/lightnvm/pblk-write.c b/drivers/lightnvm/pblk-writ=
e.c
> > index 06d56deb645d..6593deab52da 100644
> > --- a/drivers/lightnvm/pblk-write.c
> > +++ b/drivers/lightnvm/pblk-write.c
> > @@ -177,6 +177,7 @@ static void pblk_prepare_resubmit(struct pblk *pblk=
, unsigned int sentry,
> >                * re-map these entries
> >                */
> >               line =3D pblk_ppa_to_line(pblk, w_ctx->ppa);
> > +             atomic_dec(&line->sec_to_update);
> >               kref_put(&line->ref, pblk_line_put);
> >       }
> >       spin_unlock(&pblk->trans_lock);
> > diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
> > index a6386d5acd73..ac3ab778e976 100644
> > --- a/drivers/lightnvm/pblk.h
> > +++ b/drivers/lightnvm/pblk.h
> > @@ -487,6 +487,7 @@ struct pblk_line {
> >       __le32 *vsc;                    /* Valid sector count in line */
> >
> >       struct kref ref;                /* Write buffer L2P references */
> > +     atomic_t sec_to_update;         /* Outstanding L2P updates to ppa=
 */
> >
> >       struct pblk_w_err_gc *w_err_gc; /* Write error gc recovery metada=
ta */
> >
> > --
> > 2.17.1
>
>
> Looks good to me. Again, good marathon-catch! :)
>
> Reviewed-by: Javier Gonz=C3=A1lez <javier@javigon.com>
>
>