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
Apart from these nitpicks, the change looks good to me. Great to have
this fixed.
> if (chunk->wp > line_wp)
> return 1;
> else if (chunk->wp < line_wp)
> --
> 1.9.1
>
