> 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.
Javier
Attachment:
signature.asc
Description: Message signed with OpenPGP
