[PATCH] lightnvm: pblk: assume that chunks are closed on 1.2 devices

[PATCH] lightnvm: pblk: assume that chunks are closed on 1.2 devices

[Hans Holmberg]

From: Hans Holmberg <hans.holmberg@cnexlabs.com>

We can't know if a block is closed or not on 1.2 devices, so assume
closed state to make sure that blocks are erased before writing.

Fixes: 32ef9412c114 ("lightnvm: pblk: implement get log report chunk")

Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
---

This patch applies on:
ssh://github.com/OpenChannelSSD/linux branch for-4.19/core

 drivers/lightnvm/pblk-init.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index aa24264..3b8aa4a 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -717,10 +717,11 @@ static int pblk_setup_line_meta_12(struct pblk *pblk, struct pblk_line *line,
 
 		/*
 		 * In 1.2 spec. chunk state is not persisted by the device. Thus
-		 * some of the values are reset each time pblk is instantiated.
+		 * some of the values are reset each time pblk is instantiated,
+		 * so we have to assume that the block is closed.
 		 */
 		if (lun_bb_meta[line->id] == NVM_BLK_T_FREE)
-			chunk->state =  NVM_CHK_ST_FREE;
+			chunk->state =  NVM_CHK_ST_CLOSED;
 		else
 			chunk->state = NVM_CHK_ST_OFFLINE;
 
-- 
2.7.4

Re: [PATCH] lightnvm: pblk: assume that chunks are closed on 1.2 devices

[Matias Bjørling]

On 06/19/2018 11:06 AM, Hans Holmberg wrote:
> From: Hans Holmberg <hans.holmberg@cnexlabs.com>
> 
> We can't know if a block is closed or not on 1.2 devices, so assume
> closed state to make sure that blocks are erased before writing.
> 
> Fixes: 32ef9412c114 ("lightnvm: pblk: implement get log report chunk")
> 
> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
> ---
> 
> This patch applies on:
> ssh://github.com/OpenChannelSSD/linux branch for-4.19/core
> 
>   drivers/lightnvm/pblk-init.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
> index aa24264..3b8aa4a 100644
> --- a/drivers/lightnvm/pblk-init.c
> +++ b/drivers/lightnvm/pblk-init.c
> @@ -717,10 +717,11 @@ static int pblk_setup_line_meta_12(struct pblk *pblk, struct pblk_line *line,
>   
>   		/*
>   		 * In 1.2 spec. chunk state is not persisted by the device. Thus
> -		 * some of the values are reset each time pblk is instantiated.
> +		 * some of the values are reset each time pblk is instantiated,
> +		 * so we have to assume that the block is closed.
>   		 */
>   		if (lun_bb_meta[line->id] == NVM_BLK_T_FREE)
> -			chunk->state =  NVM_CHK_ST_FREE;
> +			chunk->state =  NVM_CHK_ST_CLOSED;
>   		else
>   			chunk->state = NVM_CHK_ST_OFFLINE;
>   
> 

pblk should scan (or the lightnvm subsystem) the blocks for their state, 
such that it doesn't have to reinitialize a full drive if it is already 
in a closed state. If marking closed, it does a full erase cycle on 
initialization, which should be avoided since it is a limited resource.

Re: [PATCH] lightnvm: pblk: assume that chunks are closed on 1.2 devices

[Javier Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 2698 bytes --]



> On 26 Jun 2018, at 10.41, Matias Bjørling <mb@lightnvm.io> wrote:
> 
> On 06/19/2018 11:06 AM, Hans Holmberg wrote:
>> From: Hans Holmberg <hans.holmberg@cnexlabs.com>
>> We can't know if a block is closed or not on 1.2 devices, so assume
>> closed state to make sure that blocks are erased before writing.
>> Fixes: 32ef9412c114 ("lightnvm: pblk: implement get log report chunk")
>> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
>> ---
>> This patch applies on:
>> ssh://github.com/OpenChannelSSD/linux branch for-4.19/core
>>  drivers/lightnvm/pblk-init.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>> index aa24264..3b8aa4a 100644
>> --- a/drivers/lightnvm/pblk-init.c
>> +++ b/drivers/lightnvm/pblk-init.c
>> @@ -717,10 +717,11 @@ static int pblk_setup_line_meta_12(struct pblk *pblk, struct pblk_line *line,
>>    		/*
>>  		 * In 1.2 spec. chunk state is not persisted by the device. Thus
>> -		 * some of the values are reset each time pblk is instantiated.
>> +		 * some of the values are reset each time pblk is instantiated,
>> +		 * so we have to assume that the block is closed.
>>  		 */
>>  		if (lun_bb_meta[line->id] == NVM_BLK_T_FREE)
>> -			chunk->state =  NVM_CHK_ST_FREE;
>> +			chunk->state =  NVM_CHK_ST_CLOSED;
>>  		else
>>  			chunk->state = NVM_CHK_ST_OFFLINE;
>> 
> 
> pblk should scan (or the lightnvm subsystem) the blocks for their
> state, such that it doesn't have to reinitialize a full drive if it is
> already in a closed state. If marking closed, it does a full erase
> cycle on initialization, which should be avoided since it is a limited
> resource.

In 1.2 there is no such state unfortunately. However, pblk will never
attempt to reinitialize the whole drive - metadata for closed blocks
will be recovered and only those going to GC will be erased before
usage. In fact, a full close drive is the state pblk expects.

This patch only affects "unknown blocks", thus the only case in which
pblk would attempt to double erase is when blocks have been pre-erased
(e.g., factory or through liblightnvm). After an erase round though,
pblk will only erase pre-usage. One thing we could do is attempting to
read the first page of these unknown blocks and mark them as free if
"empty page" is returned. Is this what you mean? Note that this can be
costly on large drives; this is the reason we returned to the pre-2.0
behaviour with this patch. We are implementing a log that, among other
things, keeps the state so that pblk can have an accurate state for the
cases this can be a problem.

Javier

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] lightnvm: pblk: assume that chunks are closed on 1.2 devices

[Matias Bjørling]

On 06/26/2018 11:37 AM, Javier Gonzalez wrote:
> 
> 
>> On 26 Jun 2018, at 10.41, Matias Bjørling <mb@lightnvm.io> wrote:
>>
>> On 06/19/2018 11:06 AM, Hans Holmberg wrote:
>>> From: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>> We can't know if a block is closed or not on 1.2 devices, so assume
>>> closed state to make sure that blocks are erased before writing.
>>> Fixes: 32ef9412c114 ("lightnvm: pblk: implement get log report chunk")
>>> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>> ---
>>> This patch applies on:
>>> ssh://github.com/OpenChannelSSD/linux branch for-4.19/core
>>>   drivers/lightnvm/pblk-init.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>>> index aa24264..3b8aa4a 100644
>>> --- a/drivers/lightnvm/pblk-init.c
>>> +++ b/drivers/lightnvm/pblk-init.c
>>> @@ -717,10 +717,11 @@ static int pblk_setup_line_meta_12(struct pblk *pblk, struct pblk_line *line,
>>>     		/*
>>>   		 * In 1.2 spec. chunk state is not persisted by the device. Thus
>>> -		 * some of the values are reset each time pblk is instantiated.
>>> +		 * some of the values are reset each time pblk is instantiated,
>>> +		 * so we have to assume that the block is closed.
>>>   		 */
>>>   		if (lun_bb_meta[line->id] == NVM_BLK_T_FREE)
>>> -			chunk->state =  NVM_CHK_ST_FREE;
>>> +			chunk->state =  NVM_CHK_ST_CLOSED;
>>>   		else
>>>   			chunk->state = NVM_CHK_ST_OFFLINE;
>>>
>>
>> pblk should scan (or the lightnvm subsystem) the blocks for their
>> state, such that it doesn't have to reinitialize a full drive if it is
>> already in a closed state. If marking closed, it does a full erase
>> cycle on initialization, which should be avoided since it is a limited
>> resource.
> 
> In 1.2 there is no such state unfortunately. However, pblk will never
> attempt to reinitialize the whole drive - metadata for closed blocks
> will be recovered and only those going to GC will be erased before
> usage. In fact, a full close drive is the state pblk expects.
> 
> This patch only affects "unknown blocks", thus the only case in which
> pblk would attempt to double erase is when blocks have been pre-erased
> (e.g., factory or through liblightnvm). After an erase round though,
> pblk will only erase pre-usage. One thing we could do is attempting to
> read the first page of these unknown blocks and mark them as free if
> "empty page" is returned. Is this what you mean? 

Yes, that is what I mean.

Note that this can be
> costly on large drives; this is the reason we returned to the pre-2.0
> behaviour with this patch. We are implementing a log that, among other
> things, keeps the state so that pblk can have an accurate state for the
> cases this can be a problem.

Yep, it will take some time. Good to hear with the log.

-Matias

Re: [PATCH] lightnvm: pblk: assume that chunks are closed on 1.2 devices

[Hans Holmberg]

On Tue, Jun 26, 2018 at 1:38 PM, Matias Bjørling <mb@lightnvm.io> wrote:
> On 06/26/2018 11:37 AM, Javier Gonzalez wrote:
>>
>>
>>
>>> On 26 Jun 2018, at 10.41, Matias Bjørling <mb@lightnvm.io> wrote:
>>>
>>> On 06/19/2018 11:06 AM, Hans Holmberg wrote:
>>>>
>>>> From: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>>> We can't know if a block is closed or not on 1.2 devices, so assume
>>>> closed state to make sure that blocks are erased before writing.
>>>> Fixes: 32ef9412c114 ("lightnvm: pblk: implement get log report chunk")
>>>> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>>> ---
>>>> This patch applies on:
>>>> ssh://github.com/OpenChannelSSD/linux branch for-4.19/core
>>>>   drivers/lightnvm/pblk-init.c | 5 +++--
>>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>>>> index aa24264..3b8aa4a 100644
>>>> --- a/drivers/lightnvm/pblk-init.c
>>>> +++ b/drivers/lightnvm/pblk-init.c
>>>> @@ -717,10 +717,11 @@ static int pblk_setup_line_meta_12(struct pblk
>>>> *pblk, struct pblk_line *line,
>>>>                 /*
>>>>                  * In 1.2 spec. chunk state is not persisted by the
>>>> device. Thus
>>>> -                * some of the values are reset each time pblk is
>>>> instantiated.
>>>> +                * some of the values are reset each time pblk is
>>>> instantiated,
>>>> +                * so we have to assume that the block is closed.
>>>>                  */
>>>>                 if (lun_bb_meta[line->id] == NVM_BLK_T_FREE)
>>>> -                       chunk->state =  NVM_CHK_ST_FREE;
>>>> +                       chunk->state =  NVM_CHK_ST_CLOSED;
>>>>                 else
>>>>                         chunk->state = NVM_CHK_ST_OFFLINE;
>>>>
>>>
>>> pblk should scan (or the lightnvm subsystem) the blocks for their
>>> state, such that it doesn't have to reinitialize a full drive if it is
>>> already in a closed state. If marking closed, it does a full erase
>>> cycle on initialization, which should be avoided since it is a limited
>>> resource.
>>
>>
>> In 1.2 there is no such state unfortunately. However, pblk will never
>> attempt to reinitialize the whole drive - metadata for closed blocks
>> will be recovered and only those going to GC will be erased before
>> usage. In fact, a full close drive is the state pblk expects.
>>
>> This patch only affects "unknown blocks", thus the only case in which
>> pblk would attempt to double erase is when blocks have been pre-erased
>> (e.g., factory or through liblightnvm). After an erase round though,
>> pblk will only erase pre-usage. One thing we could do is attempting to
>> read the first page of these unknown blocks and mark them as free if
>> "empty page" is returned. Is this what you mean?
>
>
> Yes, that is what I mean.
>
> Note that this can be
>>
>> costly on large drives; this is the reason we returned to the pre-2.0
>> behaviour with this patch. We are implementing a log that, among other
>> things, keeps the state so that pblk can have an accurate state for the
>> cases this can be a problem.
>
>
> Yep, it will take some time. Good to hear with the log.

Until we have a log in place, this patch unbreaks 1.2 support and has
no negative impact on performance (as compared to pre 2.0 support), so
please consider it for the next window.
The current state is that if a pblk instance is created on a 1.2 disk
with written blocks, writes will fail.

 / Hans

Re: [PATCH] lightnvm: pblk: assume that chunks are closed on 1.2 devices

[Matias Bjørling]

On 06/26/2018 01:31 PM, Hans Holmberg wrote:
> On Tue, Jun 26, 2018 at 1:38 PM, Matias Bjørling <mb@lightnvm.io> wrote:
>> On 06/26/2018 11:37 AM, Javier Gonzalez wrote:
>>>
>>>
>>>
>>>> On 26 Jun 2018, at 10.41, Matias Bjørling <mb@lightnvm.io> wrote:
>>>>
>>>> On 06/19/2018 11:06 AM, Hans Holmberg wrote:
>>>>>
>>>>> From: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>>>> We can't know if a block is closed or not on 1.2 devices, so assume
>>>>> closed state to make sure that blocks are erased before writing.
>>>>> Fixes: 32ef9412c114 ("lightnvm: pblk: implement get log report chunk")
>>>>> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>>>> ---
>>>>> This patch applies on:
>>>>> ssh://github.com/OpenChannelSSD/linux branch for-4.19/core
>>>>>    drivers/lightnvm/pblk-init.c | 5 +++--
>>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>>>>> index aa24264..3b8aa4a 100644
>>>>> --- a/drivers/lightnvm/pblk-init.c
>>>>> +++ b/drivers/lightnvm/pblk-init.c
>>>>> @@ -717,10 +717,11 @@ static int pblk_setup_line_meta_12(struct pblk
>>>>> *pblk, struct pblk_line *line,
>>>>>                  /*
>>>>>                   * In 1.2 spec. chunk state is not persisted by the
>>>>> device. Thus
>>>>> -                * some of the values are reset each time pblk is
>>>>> instantiated.
>>>>> +                * some of the values are reset each time pblk is
>>>>> instantiated,
>>>>> +                * so we have to assume that the block is closed.
>>>>>                   */
>>>>>                  if (lun_bb_meta[line->id] == NVM_BLK_T_FREE)
>>>>> -                       chunk->state =  NVM_CHK_ST_FREE;
>>>>> +                       chunk->state =  NVM_CHK_ST_CLOSED;
>>>>>                  else
>>>>>                          chunk->state = NVM_CHK_ST_OFFLINE;
>>>>>
>>>>
>>>> pblk should scan (or the lightnvm subsystem) the blocks for their
>>>> state, such that it doesn't have to reinitialize a full drive if it is
>>>> already in a closed state. If marking closed, it does a full erase
>>>> cycle on initialization, which should be avoided since it is a limited
>>>> resource.
>>>
>>>
>>> In 1.2 there is no such state unfortunately. However, pblk will never
>>> attempt to reinitialize the whole drive - metadata for closed blocks
>>> will be recovered and only those going to GC will be erased before
>>> usage. In fact, a full close drive is the state pblk expects.
>>>
>>> This patch only affects "unknown blocks", thus the only case in which
>>> pblk would attempt to double erase is when blocks have been pre-erased
>>> (e.g., factory or through liblightnvm). After an erase round though,
>>> pblk will only erase pre-usage. One thing we could do is attempting to
>>> read the first page of these unknown blocks and mark them as free if
>>> "empty page" is returned. Is this what you mean?
>>
>>
>> Yes, that is what I mean.
>>
>> Note that this can be
>>>
>>> costly on large drives; this is the reason we returned to the pre-2.0
>>> behaviour with this patch. We are implementing a log that, among other
>>> things, keeps the state so that pblk can have an accurate state for the
>>> cases this can be a problem.
>>
>>
>> Yep, it will take some time. Good to hear with the log.
> 
> Until we have a log in place, this patch unbreaks 1.2 support and has
> no negative impact on performance (as compared to pre 2.0 support), so
> please consider it for the next window.
> The current state is that if a pblk instance is created on a 1.2 disk
> with written blocks, writes will fail.
> 
>   / Hans
> 

The negative impact is that all blocks are erased, even if they are in 
free state. This is a showstopper. We cannot throw out 1/X of the 
lifetime of the drive on each initialization. The 1.2 spec is made such 
that a scan can recover the block state accurately.

Re: [PATCH] lightnvm: pblk: assume that chunks are closed on 1.2 devices

[Javier Gonzalez]

> On 26 Jun 2018, at 13.44, Matias Bjørling <mb@lightnvm.io> wrote:
> 
>> On 06/26/2018 01:31 PM, Hans Holmberg wrote:
>>> On Tue, Jun 26, 2018 at 1:38 PM, Matias Bjørling <mb@lightnvm.io> wrote:
>>>> On 06/26/2018 11:37 AM, Javier Gonzalez wrote:
>>>> 
>>>> 
>>>> 
>>>>> On 26 Jun 2018, at 10.41, Matias Bjørling <mb@lightnvm.io> wrote:
>>>>> 
>>>>> On 06/19/2018 11:06 AM, Hans Holmberg wrote:
>>>>>> 
>>>>>> From: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>>>>> We can't know if a block is closed or not on 1.2 devices, so assume
>>>>>> closed state to make sure that blocks are erased before writing.
>>>>>> Fixes: 32ef9412c114 ("lightnvm: pblk: implement get log report chunk")
>>>>>> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>>>>> ---
>>>>>> This patch applies on:
>>>>>> ssh://github.com/OpenChannelSSD/linux branch for-4.19/core
>>>>>>   drivers/lightnvm/pblk-init.c | 5 +++--
>>>>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>>>>>> index aa24264..3b8aa4a 100644
>>>>>> --- a/drivers/lightnvm/pblk-init.c
>>>>>> +++ b/drivers/lightnvm/pblk-init.c
>>>>>> @@ -717,10 +717,11 @@ static int pblk_setup_line_meta_12(struct pblk
>>>>>> *pblk, struct pblk_line *line,
>>>>>>                 /*
>>>>>>                  * In 1.2 spec. chunk state is not persisted by the
>>>>>> device. Thus
>>>>>> -                * some of the values are reset each time pblk is
>>>>>> instantiated.
>>>>>> +                * some of the values are reset each time pblk is
>>>>>> instantiated,
>>>>>> +                * so we have to assume that the block is closed.
>>>>>>                  */
>>>>>>                 if (lun_bb_meta[line->id] == NVM_BLK_T_FREE)
>>>>>> -                       chunk->state =  NVM_CHK_ST_FREE;
>>>>>> +                       chunk->state =  NVM_CHK_ST_CLOSED;
>>>>>>                 else
>>>>>>                         chunk->state = NVM_CHK_ST_OFFLINE;
>>>>>> 
>>>>> 
>>>>> pblk should scan (or the lightnvm subsystem) the blocks for their
>>>>> state, such that it doesn't have to reinitialize a full drive if it is
>>>>> already in a closed state. If marking closed, it does a full erase
>>>>> cycle on initialization, which should be avoided since it is a limited
>>>>> resource.
>>>> 
>>>> 
>>>> In 1.2 there is no such state unfortunately. However, pblk will never
>>>> attempt to reinitialize the whole drive - metadata for closed blocks
>>>> will be recovered and only those going to GC will be erased before
>>>> usage. In fact, a full close drive is the state pblk expects.
>>>> 
>>>> This patch only affects "unknown blocks", thus the only case in which
>>>> pblk would attempt to double erase is when blocks have been pre-erased
>>>> (e.g., factory or through liblightnvm). After an erase round though,
>>>> pblk will only erase pre-usage. One thing we could do is attempting to
>>>> read the first page of these unknown blocks and mark them as free if
>>>> "empty page" is returned. Is this what you mean?
>>> 
>>> 
>>> Yes, that is what I mean.
>>> 
>>> Note that this can be
>>>> 
>>>> costly on large drives; this is the reason we returned to the pre-2.0
>>>> behaviour with this patch. We are implementing a log that, among other
>>>> things, keeps the state so that pblk can have an accurate state for the
>>>> cases this can be a problem.
>>> 
>>> 
>>> Yep, it will take some time. Good to hear with the log.
>> Until we have a log in place, this patch unbreaks 1.2 support and has
>> no negative impact on performance (as compared to pre 2.0 support), so
>> please consider it for the next window.
>> The current state is that if a pblk instance is created on a 1.2 disk
>> with written blocks, writes will fail.
>>  / Hans
> 
> The negative impact is that all blocks are erased, even if they are in free state. This is a showstopper. We cannot throw out 1/X of the lifetime of the drive on each initialization. The 1.2 spec is made such that a scan can recover the block state accurately.

This fixes patch returns to the original behavior, so it’s not introducing a worse behavior than before 2.0. But you’re right, it is not the way it should be. 

Can you consider taking this as a fix for 4.18 to avoid writes failing on 1.2 devices and I promise to send a patch this week to implement the state based on reads? This new patch would be for 4.19. 

Javier

Re: [PATCH] lightnvm: pblk: assume that chunks are closed on 1.2 devices

[Matias Bjørling]

On 06/19/2018 11:06 AM, Hans Holmberg wrote:
> From: Hans Holmberg <hans.holmberg@cnexlabs.com>
> 
> We can't know if a block is closed or not on 1.2 devices, so assume
> closed state to make sure that blocks are erased before writing.
> 
> Fixes: 32ef9412c114 ("lightnvm: pblk: implement get log report chunk")
> 
> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
> ---
> 
> This patch applies on:
> ssh://github.com/OpenChannelSSD/linux branch for-4.19/core
> 
>   drivers/lightnvm/pblk-init.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
> index aa24264..3b8aa4a 100644
> --- a/drivers/lightnvm/pblk-init.c
> +++ b/drivers/lightnvm/pblk-init.c
> @@ -717,10 +717,11 @@ static int pblk_setup_line_meta_12(struct pblk *pblk, struct pblk_line *line,
>   
>   		/*
>   		 * In 1.2 spec. chunk state is not persisted by the device. Thus
> -		 * some of the values are reset each time pblk is instantiated.
> +		 * some of the values are reset each time pblk is instantiated,
> +		 * so we have to assume that the block is closed.
>   		 */
>   		if (lun_bb_meta[line->id] == NVM_BLK_T_FREE)
> -			chunk->state =  NVM_CHK_ST_FREE;
> +			chunk->state =  NVM_CHK_ST_CLOSED;
>   		else
>   			chunk->state = NVM_CHK_ST_OFFLINE;
>   
> 

Hi Jens,

Would it be possible to pick this patch up for 4.18? It is a temporary 
fix that lets pblk continue to work with 1.2 OCSSD drives that has open 
blocks on initialization. Without it, writes will fail, as pblk assumes 
that the blocks are free.

A proper fix that scans the block state on initialization will be ready 
for 4.19.

Thank you,
Matias

Re: [PATCH] lightnvm: pblk: assume that chunks are closed on 1.2 devices

[Matias Bjørling]

On 06/26/2018 01:54 PM, Javier Gonzalez wrote:
> 
>> On 26 Jun 2018, at 13.44, Matias Bjørling <mb@lightnvm.io> wrote:
>>
>>> On 06/26/2018 01:31 PM, Hans Holmberg wrote:
>>>> On Tue, Jun 26, 2018 at 1:38 PM, Matias Bjørling <mb@lightnvm.io> wrote:
>>>>> On 06/26/2018 11:37 AM, Javier Gonzalez wrote:
>>>>>
>>>>>
>>>>>
>>>>>> On 26 Jun 2018, at 10.41, Matias Bjørling <mb@lightnvm.io> wrote:
>>>>>>
>>>>>> On 06/19/2018 11:06 AM, Hans Holmberg wrote:
>>>>>>>
>>>>>>> From: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>>>>>> We can't know if a block is closed or not on 1.2 devices, so assume
>>>>>>> closed state to make sure that blocks are erased before writing.
>>>>>>> Fixes: 32ef9412c114 ("lightnvm: pblk: implement get log report chunk")
>>>>>>> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>>>>>> ---
>>>>>>> This patch applies on:
>>>>>>> ssh://github.com/OpenChannelSSD/linux branch for-4.19/core
>>>>>>>    drivers/lightnvm/pblk-init.c | 5 +++--
>>>>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>>>>>>> index aa24264..3b8aa4a 100644
>>>>>>> --- a/drivers/lightnvm/pblk-init.c
>>>>>>> +++ b/drivers/lightnvm/pblk-init.c
>>>>>>> @@ -717,10 +717,11 @@ static int pblk_setup_line_meta_12(struct pblk
>>>>>>> *pblk, struct pblk_line *line,
>>>>>>>                  /*
>>>>>>>                   * In 1.2 spec. chunk state is not persisted by the
>>>>>>> device. Thus
>>>>>>> -                * some of the values are reset each time pblk is
>>>>>>> instantiated.
>>>>>>> +                * some of the values are reset each time pblk is
>>>>>>> instantiated,
>>>>>>> +                * so we have to assume that the block is closed.
>>>>>>>                   */
>>>>>>>                  if (lun_bb_meta[line->id] == NVM_BLK_T_FREE)
>>>>>>> -                       chunk->state =  NVM_CHK_ST_FREE;
>>>>>>> +                       chunk->state =  NVM_CHK_ST_CLOSED;
>>>>>>>                  else
>>>>>>>                          chunk->state = NVM_CHK_ST_OFFLINE;
>>>>>>>
>>>>>>
>>>>>> pblk should scan (or the lightnvm subsystem) the blocks for their
>>>>>> state, such that it doesn't have to reinitialize a full drive if it is
>>>>>> already in a closed state. If marking closed, it does a full erase
>>>>>> cycle on initialization, which should be avoided since it is a limited
>>>>>> resource.
>>>>>
>>>>>
>>>>> In 1.2 there is no such state unfortunately. However, pblk will never
>>>>> attempt to reinitialize the whole drive - metadata for closed blocks
>>>>> will be recovered and only those going to GC will be erased before
>>>>> usage. In fact, a full close drive is the state pblk expects.
>>>>>
>>>>> This patch only affects "unknown blocks", thus the only case in which
>>>>> pblk would attempt to double erase is when blocks have been pre-erased
>>>>> (e.g., factory or through liblightnvm). After an erase round though,
>>>>> pblk will only erase pre-usage. One thing we could do is attempting to
>>>>> read the first page of these unknown blocks and mark them as free if
>>>>> "empty page" is returned. Is this what you mean?
>>>>
>>>>
>>>> Yes, that is what I mean.
>>>>
>>>> Note that this can be
>>>>>
>>>>> costly on large drives; this is the reason we returned to the pre-2.0
>>>>> behaviour with this patch. We are implementing a log that, among other
>>>>> things, keeps the state so that pblk can have an accurate state for the
>>>>> cases this can be a problem.
>>>>
>>>>
>>>> Yep, it will take some time. Good to hear with the log.
>>> Until we have a log in place, this patch unbreaks 1.2 support and has
>>> no negative impact on performance (as compared to pre 2.0 support), so
>>> please consider it for the next window.
>>> The current state is that if a pblk instance is created on a 1.2 disk
>>> with written blocks, writes will fail.
>>>   / Hans
>>
>> The negative impact is that all blocks are erased, even if they are in free state. This is a showstopper. We cannot throw out 1/X of the lifetime of the drive on each initialization. The 1.2 spec is made such that a scan can recover the block state accurately.
> 
> This fixes patch returns to the original behavior, so it’s not introducing a worse behavior than before 2.0. But you’re right, it is not the way it should be.
> 
> Can you consider taking this as a fix for 4.18 to avoid writes failing on 1.2 devices and I promise to send a patch this week to implement the state based on reads? This new patch would be for 4.19.
> 
> Javier
> 

Okay, sounds good to me. Thanks

Re: [PATCH] lightnvm: pblk: assume that chunks are closed on 1.2 devices

[Javier González]

> On 26 Jun 2018, at 14.13, Matias Bjørling <mb@lightnvm.io> wrote:
> 
> On 06/26/2018 01:54 PM, Javier Gonzalez wrote:
>>> On 26 Jun 2018, at 13.44, Matias Bjørling <mb@lightnvm.io> wrote:
>>> 
>>>>> On 06/26/2018 01:31 PM, Hans Holmberg wrote:
>>>>>> On Tue, Jun 26, 2018 at 1:38 PM, Matias Bjørling <mb@lightnvm.io> wrote:
>>>>>> On 06/26/2018 11:37 AM, Javier Gonzalez wrote:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>>> On 26 Jun 2018, at 10.41, Matias Bjørling <mb@lightnvm.io> wrote:
>>>>>>> 
>>>>>>> On 06/19/2018 11:06 AM, Hans Holmberg wrote:
>>>>>>>> 
>>>>>>>> From: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>>>>>>> We can't know if a block is closed or not on 1.2 devices, so assume
>>>>>>>> closed state to make sure that blocks are erased before writing.
>>>>>>>> Fixes: 32ef9412c114 ("lightnvm: pblk: implement get log report chunk")
>>>>>>>> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>>>>>>> ---
>>>>>>>> This patch applies on:
>>>>>>>> ssh://github.com/OpenChannelSSD/linux branch for-4.19/core
>>>>>>>>   drivers/lightnvm/pblk-init.c | 5 +++--
>>>>>>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>>>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>>>>>>>> index aa24264..3b8aa4a 100644
>>>>>>>> --- a/drivers/lightnvm/pblk-init.c
>>>>>>>> +++ b/drivers/lightnvm/pblk-init.c
>>>>>>>> @@ -717,10 +717,11 @@ static int pblk_setup_line_meta_12(struct pblk
>>>>>>>> *pblk, struct pblk_line *line,
>>>>>>>>                 /*
>>>>>>>>                  * In 1.2 spec. chunk state is not persisted by the
>>>>>>>> device. Thus
>>>>>>>> -                * some of the values are reset each time pblk is
>>>>>>>> instantiated.
>>>>>>>> +                * some of the values are reset each time pblk is
>>>>>>>> instantiated,
>>>>>>>> +                * so we have to assume that the block is closed.
>>>>>>>>                  */
>>>>>>>>                 if (lun_bb_meta[line->id] == NVM_BLK_T_FREE)
>>>>>>>> -                       chunk->state =  NVM_CHK_ST_FREE;
>>>>>>>> +                       chunk->state =  NVM_CHK_ST_CLOSED;
>>>>>>>>                 else
>>>>>>>>                         chunk->state = NVM_CHK_ST_OFFLINE;
>>>>>>>> 
>>>>>>> 
>>>>>>> pblk should scan (or the lightnvm subsystem) the blocks for their
>>>>>>> state, such that it doesn't have to reinitialize a full drive if it is
>>>>>>> already in a closed state. If marking closed, it does a full erase
>>>>>>> cycle on initialization, which should be avoided since it is a limited
>>>>>>> resource.
>>>>>> 
>>>>>> 
>>>>>> In 1.2 there is no such state unfortunately. However, pblk will never
>>>>>> attempt to reinitialize the whole drive - metadata for closed blocks
>>>>>> will be recovered and only those going to GC will be erased before
>>>>>> usage. In fact, a full close drive is the state pblk expects.
>>>>>> 
>>>>>> This patch only affects "unknown blocks", thus the only case in which
>>>>>> pblk would attempt to double erase is when blocks have been pre-erased
>>>>>> (e.g., factory or through liblightnvm). After an erase round though,
>>>>>> pblk will only erase pre-usage. One thing we could do is attempting to
>>>>>> read the first page of these unknown blocks and mark them as free if
>>>>>> "empty page" is returned. Is this what you mean?
>>>>> 
>>>>> 
>>>>> Yes, that is what I mean.
>>>>> 
>>>>> Note that this can be
>>>>>> 
>>>>>> costly on large drives; this is the reason we returned to the pre-2.0
>>>>>> behaviour with this patch. We are implementing a log that, among other
>>>>>> things, keeps the state so that pblk can have an accurate state for the
>>>>>> cases this can be a problem.
>>>>> 
>>>>> 
>>>>> Yep, it will take some time. Good to hear with the log.
>>>> Until we have a log in place, this patch unbreaks 1.2 support and has
>>>> no negative impact on performance (as compared to pre 2.0 support), so
>>>> please consider it for the next window.
>>>> The current state is that if a pblk instance is created on a 1.2 disk
>>>> with written blocks, writes will fail.
>>>>  / Hans
>>> 
>>> The negative impact is that all blocks are erased, even if they are in free state. This is a showstopper. We cannot throw out 1/X of the lifetime of the drive on each initialization. The 1.2 spec is made such that a scan can recover the block state accurately.
>> This fixes patch returns to the original behavior, so it’s not introducing a worse behavior than before 2.0. But you’re right, it is not the way it should be.
>> Can you consider taking this as a fix for 4.18 to avoid writes failing on 1.2 devices and I promise to send a patch this week to implement the state based on reads? This new patch would be for 4.19.
>> Javier
> 
> Okay, sounds good to me. Thanks

Thanks!

Javier