Re: [PATCH 06/13] lightnvm: pblk: Ensure that erase is chunk aligned

[Igor Konopko <igor.j.konopko@intel.com>]

On 04.03.2019 12:43, Hans Holmberg wrote:
> On Mon, Mar 4, 2019 at 10:11 AM Javier González <javier@javigon.com> wrote:
>>
>>> On 4 Mar 2019, at 10.05, Hans Holmberg <hans.ml.holmberg@owltronix.com> wrote:
>>>
>>> I strongly disagree with adding code that would mask implantation errors.
>>>
>>> If we want more internal checks, we could add an if statement that
>>> would only be compiled in if CONFIG_NVM_PBLK_DEBUG is enabled.
>>>
>>
>> Not sure who this is for - better not to top post.
>>
>> In any case, this is a spec grey zone. I’m ok with cleaning the bits as
>> they mean nothing for the reset command. If you feel that strongly about
>> this, you can take if with Igor.
> 
> Pardon the top-post. It was meant for both you and Igor.
> 

OCSSD 2.0 spec for vector chunk reset (chapter 2.2.2) explicitly says 
"The addresses in the LBA list shall be the first logical block address 
of each chunk to be reset.". So in my understanding we suppose to clear 
the sectors bits of the PPA address in order to be spec compliant.

>>
>>>
>>> On Mon, Mar 4, 2019 at 8:48 AM Javier González <javier@javigon.com> wrote:
>>>>> On 27 Feb 2019, at 18.14, Igor Konopko <igor.j.konopko@intel.com> wrote:
>>>>>
>>>>> In current pblk implementation of erase command
>>>>> there is a chance tha sector bits are set to some
>>>>> random values for erase PPA. This is unexpected
>>>>> situation, since erase shall be always chunk
>>>>> aligned. This patch fixes that issue
>>>>>
>>>>> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
>>>>> ---
>>>>> drivers/lightnvm/pblk-core.c | 1 +
>>>>> drivers/lightnvm/pblk-map.c  | 2 ++
>>>>> 2 files changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
>>>>> index a98b2255f963..78b1eea4ab67 100644
>>>>> --- a/drivers/lightnvm/pblk-core.c
>>>>> +++ b/drivers/lightnvm/pblk-core.c
>>>>> @@ -978,6 +978,7 @@ int pblk_line_erase(struct pblk *pblk, struct pblk_line *line)
>>>>>
>>>>>               ppa = pblk->luns[bit].bppa; /* set ch and lun */
>>>>>               ppa.a.blk = line->id;
>>>>> +             ppa.a.reserved = 0;
>>>>>
>>>>>               atomic_dec(&line->left_eblks);
>>>>>               WARN_ON(test_and_set_bit(bit, line->erase_bitmap));
>>>>> diff --git a/drivers/lightnvm/pblk-map.c b/drivers/lightnvm/pblk-map.c
>>>>> index 79df583ea709..aea46b4ec40f 100644
>>>>> --- a/drivers/lightnvm/pblk-map.c
>>>>> +++ b/drivers/lightnvm/pblk-map.c
>>>>> @@ -161,6 +161,7 @@ int pblk_map_erase_rq(struct pblk *pblk, struct nvm_rq *rqd,
>>>>>
>>>>>                       *erase_ppa = ppa_list[i];
>>>>>                       erase_ppa->a.blk = e_line->id;
>>>>> +                     erase_ppa->a.reserved = 0;
>>>>>
>>>>>                       spin_unlock(&e_line->lock);
>>>>>
>>>>> @@ -202,6 +203,7 @@ int pblk_map_erase_rq(struct pblk *pblk, struct nvm_rq *rqd,
>>>>>               atomic_dec(&e_line->left_eblks);
>>>>>               *erase_ppa = pblk->luns[bit].bppa; /* set ch and lun */
>>>>>               erase_ppa->a.blk = e_line->id;
>>>>> +             erase_ppa->a.reserved = 0;
>>>>>       }
>>>>>
>>>>>       return 0;
>>>>> --
>>>>> 2.17.1
>>>>
>>>> I’m fine with adding this, but note that there is actually no
>>>> requirement for the erase to be chunk aligned - the only bits that
>>>> should be looked at are group, PU and chunk.
>>>>
>>>> Reviewed-by: Javier González <javier@javigon.com>