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