On Fri, Jan 25, 2019 at 2:33 PM Javier González <javier@xxxxxxxxxxx> wrote:
>
>
> > On 25 Jan 2019, at 13.59, Hans Holmberg <hans.ml.holmberg@xxxxxxxxxxxxx> wrote:
> >
> > On Fri, Jan 25, 2019 at 10:41 AM Javier González <javier@xxxxxxxxxxx> wrote:
> >>> On 25 Jan 2019, at 09.47, Hans Holmberg <hans.ml.holmberg@xxxxxxxxxxxxx> wrote:
> >>>
> >>> On Thu, Jan 24, 2019 at 8:51 PM Zhoujie Wu <zjwu@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >>>> ---
> >>>> 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 :)
