| Message ID | 20181105122610.1555-2-hans.ml.holmberg@owltronix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PBLK Bugfixes and cleanups | expand |
On 2018-11-05 7:26 a.m., Hans Holmberg wrote: > From: Hans Holmberg <hans.holmberg@cnexlabs.com> > > Make sure we only look up valid lba addresses on the resubmission path. > > If an lba is invalidated in the write buffer, that sector will be > submitted to disk(as it is already mapped to a ppa), and that write submitted to disk(as it is already mapped to a ppa), and that write ^ add a space > might fail, resulting in a crash when trying to look up the lba in the > mapping table (as the lba is marked as invalid). > > Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com> > --- > drivers/lightnvm/pblk-write.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/lightnvm/pblk-write.c b/drivers/lightnvm/pblk-write.c > index fa8726493b39..3ddd16f47106 100644 > --- a/drivers/lightnvm/pblk-write.c > +++ b/drivers/lightnvm/pblk-write.c > @@ -148,9 +148,11 @@ static void pblk_prepare_resubmit(struct pblk *pblk, unsigned int sentry, > w_ctx = &entry->w_ctx; > > /* Check if the lba has been overwritten */ > - ppa_l2p = pblk_trans_map_get(pblk, w_ctx->lba); > - if (!pblk_ppa_comp(ppa_l2p, entry->cacheline)) > - w_ctx->lba = ADDR_EMPTY; > + if (w_ctx->lba != ADDR_EMPTY) { > + ppa_l2p = pblk_trans_map_get(pblk, w_ctx->lba); > + if (!pblk_ppa_comp(ppa_l2p, entry->cacheline)) > + w_ctx->lba = ADDR_EMPTY; > + } Was w_ctx->lba set to ADDR_EMPTY in the same kernel I/O thread ? I wonder if w_ctx->lba could become ADDR_EMPTY after if (w_ctx->lba != ADDR_EMPTY) { but before ppa_l2p = pblk_trans_map_get(pblk, w_ctx->lba); > > /* Mark up the entry as submittable again */ > flags = READ_ONCE(w_ctx->flags); >
On Mon, Nov 5, 2018 at 4:12 PM Sebastien Boisvert <sboisvert@gydle.com> wrote: > > > > On 2018-11-05 7:26 a.m., Hans Holmberg wrote: > > From: Hans Holmberg <hans.holmberg@cnexlabs.com> > > > > Make sure we only look up valid lba addresses on the resubmission path. > > > > If an lba is invalidated in the write buffer, that sector will be > > submitted to disk(as it is already mapped to a ppa), and that write > > submitted to disk(as it is already mapped to a ppa), and that write > ^ > add a space > > > might fail, resulting in a crash when trying to look up the lba in the > > mapping table (as the lba is marked as invalid). > > > > Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com> > > --- > > drivers/lightnvm/pblk-write.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/lightnvm/pblk-write.c b/drivers/lightnvm/pblk-write.c > > index fa8726493b39..3ddd16f47106 100644 > > --- a/drivers/lightnvm/pblk-write.c > > +++ b/drivers/lightnvm/pblk-write.c > > @@ -148,9 +148,11 @@ static void pblk_prepare_resubmit(struct pblk *pblk, unsigned int sentry, > > w_ctx = &entry->w_ctx; > > > > /* Check if the lba has been overwritten */ > > - ppa_l2p = pblk_trans_map_get(pblk, w_ctx->lba); > > - if (!pblk_ppa_comp(ppa_l2p, entry->cacheline)) > > - w_ctx->lba = ADDR_EMPTY; > > + if (w_ctx->lba != ADDR_EMPTY) { > > + ppa_l2p = pblk_trans_map_get(pblk, w_ctx->lba); > > + if (!pblk_ppa_comp(ppa_l2p, entry->cacheline)) > > + w_ctx->lba = ADDR_EMPTY; > > + } > > Was w_ctx->lba set to ADDR_EMPTY in the same kernel I/O thread ? > > I wonder if w_ctx->lba could become ADDR_EMPTY after > > if (w_ctx->lba != ADDR_EMPTY) { > > but before > > ppa_l2p = pblk_trans_map_get(pblk, w_ctx->lba); It can't, the lba adress can only be changed on the write error handling path, and we'll have to resubmit the write before that can happen. Thanks for the review. / Hans > > > > > > /* Mark up the entry as submittable again */ > > flags = READ_ONCE(w_ctx->flags); > >
diff --git a/drivers/lightnvm/pblk-write.c b/drivers/lightnvm/pblk-write.c index fa8726493b39..3ddd16f47106 100644 --- a/drivers/lightnvm/pblk-write.c +++ b/drivers/lightnvm/pblk-write.c @@ -148,9 +148,11 @@ static void pblk_prepare_resubmit(struct pblk *pblk, unsigned int sentry, w_ctx = &entry->w_ctx; /* Check if the lba has been overwritten */ - ppa_l2p = pblk_trans_map_get(pblk, w_ctx->lba); - if (!pblk_ppa_comp(ppa_l2p, entry->cacheline)) - w_ctx->lba = ADDR_EMPTY; + if (w_ctx->lba != ADDR_EMPTY) { + ppa_l2p = pblk_trans_map_get(pblk, w_ctx->lba); + if (!pblk_ppa_comp(ppa_l2p, entry->cacheline)) + w_ctx->lba = ADDR_EMPTY; + } /* Mark up the entry as submittable again */ flags = READ_ONCE(w_ctx->flags);