Re: [PATCH v3] lightnvm: pblk: ignore bad block wp for pblk_line_wp_is_unbalanced

From: Hans Holmberg <hans.ml.holmberg@owltronix.com>
To: "Matias Bjørling" <mb@lightnvm.io>
Cc: "Javier González" <javier@javigon.com>,
	"Zhoujie Wu" <zjwu@marvell.com>,
	linux-block@vger.kernel.org, hongd@marvell.com
Subject: Re: [PATCH v3] lightnvm: pblk: ignore bad block wp for pblk_line_wp_is_unbalanced
Date: Fri, 25 Jan 2019 17:46:58 +0100	[thread overview]
Message-ID: <CANr-nt1BTrJZbGB307tHFqxLDx1QRc99PQTL8YgidGu18Bju-g@mail.gmail.com> (raw)
In-Reply-To: <089cd48a-37f0-59e1-ac5f-78dd9cebba05@lightnvm.io>

On Fri, Jan 25, 2019 at 3:35 PM Matias Bjørling <mb@lightnvm.io> wrote:
>
> On 1/25/19 3:21 PM, Hans Holmberg wrote:
> > On Fri, Jan 25, 2019 at 2:33 PM Javier González <javier@javigon.com> wrote:
> >>
> >>
> >>> On 25 Jan 2019, at 13.59, Hans Holmberg <hans.ml.holmberg@owltronix.com> wrote:
> >>>
> >>> On Fri, Jan 25, 2019 at 10:41 AM Javier González <javier@javigon.com> wrote:
> >>>>> On 25 Jan 2019, at 09.47, Hans Holmberg <hans.ml.holmberg@owltronix.com> wrote:
> >>>>>
> >>>>> On Thu, Jan 24, 2019 at 8:51 PM Zhoujie Wu <zjwu@marvell.com> wrote:
> >>>>>> The write pointer of the bad block could be 0 or undefined, ignore
> >>>>>> the checking of the bad block wp for pblk_line_wp_is_unbalanced to
> >>>>>> avoid fake warning.
> >>>>>
> >>>>> fake -> spurious?
> >>>>>
> >>>>>> Signed-off-by: Zhoujie Wu <zjwu@marvell.com>
> >>>>>> ---
> >>>>>> v3: return in case bit >= lm->blk_per_line.
> >>>>>> v2: changed according to Javier's comments.
> >>>>>>
> >>>>>> drivers/lightnvm/pblk-recovery.c | 12 +++++++++---
> >>>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/lightnvm/pblk-recovery.c b/drivers/lightnvm/pblk-recovery.c
> >>>>>> index 6761d2a..02d466e 100644
> >>>>>> --- a/drivers/lightnvm/pblk-recovery.c
> >>>>>> +++ b/drivers/lightnvm/pblk-recovery.c
> >>>>>> @@ -312,21 +312,27 @@ static int pblk_line_wp_is_unbalanced(struct pblk *pblk,
> >>>>>>        struct nvm_chk_meta *chunk;
> >>>>>>        struct ppa_addr ppa;
> >>>>>>        u64 line_wp;
> >>>>>> -       int pos, i;
> >>>>>> +       int pos, i, bit;
> >>>>>
> >>>>> We don't need both bit and i, one of them is enough.
> >>>>>
> >>>>>> -       rlun = &pblk->luns[0];
> >>>>>> +       bit = find_first_zero_bit(line->blk_bitmap, lm->blk_per_line);
> >>>>>> +       if (bit >= lm->blk_per_line)
> >>>>>> +               return 0;
> >>>>>
> >>>>> If there is only one non-offline chunk in the line, the wp can't be unbalanced,
> >>>>> so it should be safe to return 0 here if bit >= lm->blk_per_line - 1
> >>>>>
> >>>>> If you change this please document why using a comment, as it might
> >>>>> not be obvious
> >>>>>
> >>>>>> +       rlun = &pblk->luns[bit];
> >>>>>>        ppa = rlun->bppa;
> >>>>>>        pos = pblk_ppa_to_pos(geo, ppa);
> >>>>>>        chunk = &line->chks[pos];
> >>>>>
> >>>>>>        line_wp = chunk->wp;
> >>>>>>
> >>>>>> -       for (i = 1; i < lm->blk_per_line; i++) {
> >>>>>> +       for (i = bit + 1; i < lm->blk_per_line; i++) {
> >>>>>
> >>>>>>                rlun = &pblk->luns[i];
> >>>>>>                ppa = rlun->bppa;
> >>>>>>                pos = pblk_ppa_to_pos(geo, ppa);
> >>>>>>                chunk = &line->chks[pos];
> >>>>>
> >>>>> This code is a copy of the code above, it'd be nice to refactor it
> >>>>> into a helper function or just do the chunk lookups in one place.
> >>>>>
> >>>>>> +               if (chunk->state & NVM_CHK_ST_OFFLINE)
> >>>>>> +                       continue;
> >>>>>> +
> >>>>>
> >>>>> Since we rely on the block bitmap anyway, we might as well just
> >>>>> iterate over the zeroes in the block bitmap using find_next_zero_bit
> >>>>> instead.
> >>>>> We do this in lots of other places, see: git grep -n
> >>>>> find_next_zero_bit -- drivers/lightnvm
> >>>>
> >>>> Hans, I proposed him to use the chunk->state instead. I think it is way
> >>>> more robust. We introduced the block bitmap for OCSSD 1.2, because there
> >>>> was no state. Now that we have state, it is better to use it instead. In
> >>>> fact, we should remove the bock bitmap as we have to check for the state
> >>>> either way - note that this aligns also very well with you patches
> >>>> removing the other bitmaps.
> >>>
> >>> These are just nitpicks.
> >>>
> >>> Relying on two data structures(chunks, block bitmap) to be in sync in
> >>> this function in stead of one does not make it more robust imho.
> >>> Either or (checking chunks or the block bitmap) is fine by me.
> >>> Searching the bitmap is more efficient, so that is what I proposed.
> >>
> >> chunk log page is the ground truth, so it is more robust.
> >>
> >> Also, pblk has a long way to start seeing bitmap search vs. integer
> >> comparisons in profiling.
> >
> > Hehe, yeah, but it does not hurt to use the better alternative when available.
> >
> >>>
> >>> If we want to remove the block bitmap, we can do that as a separate patch(set)
> >>> I do agree, having two copies of the chunk state is something worth
> >>> getting rid of :)
> >>>
> >>
> >> A good start is not adding code using what we want to remove.
> >
> > Well, I think it's very confusing to use both copies in the same function.
> >
> > Now we're nitpicking nitpicks :)
> >
>
> I look forward to a patch. Will one of you volunteer a patch?

Sure! It'd be easier to Illustrate what I mean with a patch.