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

From: Javier González <hidden>
Date: 2018-06-26 12:27:07
Also in: lkml

On 26 Jun 2018, at 14.13, Matias Bj=C3=B8rling [off-list ref] wrote:
=20
On 06/26/2018 01:54 PM, Javier Gonzalez wrote:

quoted

On 26 Jun 2018, at 13.44, Matias Bj=C3=B8rling [off-list ref] wrote:
=20

quoted

On 06/26/2018 01:31 PM, Hans Holmberg wrote:

quoted

On Tue, Jun 26, 2018 at 1:38 PM, Matias Bj=C3=B8rling <mb@lightnvm.io=

wrote:

quoted

On 06/26/2018 11:37 AM, Javier Gonzalez wrote:
=20
=20
=20

quoted

On 26 Jun 2018, at 10.41, Matias Bj=C3=B8rling [off-list ref] wrot=

e:

quoted

=20
On 06/19/2018 11:06 AM, Hans Holmberg wrote:

quoted

=20
From: Hans Holmberg <redacted>
We can't know if a block is closed or not on 1.2 devices, so assume=

quoted

closed state to make sure that blocks are erased before writing.
Fixes: 32ef9412c114 ("lightnvm: pblk: implement get log report chun=

k")

quoted

Signed-off-by: Hans Holmberg <redacted>
---
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-i=

nit.c

quoted

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 pbl=

quoted

*pblk, struct pblk_line *line,
                /*
                 * In 1.2 spec. chunk state is not persisted by the=

quoted

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] =3D=3D NVM_BLK_T_FREE)
-                       chunk->state =3D  NVM_CHK_ST_FREE;
+                       chunk->state =3D  NVM_CHK_ST_CLOSED;
                else
                        chunk->state =3D NVM_CHK_ST_OFFLINE;
=20

=20
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 i=

quoted

already in a closed state. If marking closed, it does a full erase
cycle on initialization, which should be avoided since it is a limit=

ed

quoted

resource.

=20
=20
In 1.2 there is no such state unfortunately. However, pblk will never=

quoted

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.
=20
This patch only affects "unknown blocks", thus the only case in which=

quoted

pblk would attempt to double erase is when blocks have been pre-erase=

quoted

(e.g., factory or through liblightnvm). After an erase round though,
pblk will only erase pre-usage. One thing we could do is attempting t=

quoted

read the first page of these unknown blocks and mark them as free if
"empty page" is returned. Is this what you mean?

=20
=20
Yes, that is what I mean.
=20
Note that this can be

quoted

=20
costly on large drives; this is the reason we returned to the pre-2.0=

quoted

behaviour with this patch. We are implementing a log that, among othe=

quoted

things, keeps the state so that pblk can have an accurate state for t=

he

quoted

cases this can be a problem.

=20
=20
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

=20
The negative impact is that all blocks are erased, even if they are in f=

ree 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.

quoted

This fixes patch returns to the original behavior, so it=E2=80=99s not in=

troducing a worse behavior than before 2.0. But you=E2=80=99re right, it is n=
ot the way it should be.

quoted

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 b=
ased on reads? This new patch would be for 4.19.

quoted

Javier

=20
Okay, sounds good to me. Thanks

Thanks!

Javier=

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help