[PATCH v2 0/8] lightnvm: bugfixes and improvements

[PATCH v2 0/8] lightnvm: bugfixes and improvements

[Igor Konopko]

This series provides a group of the bugfixes or improvements
for lightnvm and pblk device.

Most of the patches are rather simple and covers some corner cases
scenario, but we were able to hit most of them in some scenarios.
Few others close some existing gaps which we were able to found.

Changes v2->v1:
-skipped patches which were applied in v1
-dropped not needed patch for pblk_should_kick_write()
-patch which scan for opened chunks on pblk creation do not close
chunk anymore, just print the warning
-unknown read errors are counted seperately from read unrecoverable

Igor Konopko (8):
  lightnvm: pblk: Gracefully handle GC vmalloc fail
  lightnvm: pblk: Fix put line back behaviour
  lightnvm: pblk: Count all read errors in stats
  lightnvm: pblk: Ensure that erase is chunk aligned
  lightnvm: pblk: Cleanly fail when there is not enough memory
  lightnvm: pblk: Set proper read stutus in bio
  lightnvm: pblk: warn about opened chunk on factory init
  lightnvm: Inherit mdts from the parent nvme device

 drivers/lightnvm/core.c       |  9 +++++++--
 drivers/lightnvm/pblk-core.c  | 15 +++++++++++++++
 drivers/lightnvm/pblk-gc.c    | 35 +++++++++++++++++++----------------
 drivers/lightnvm/pblk-init.c  | 24 +++++++++++++++++++-----
 drivers/lightnvm/pblk-map.c   |  1 +
 drivers/lightnvm/pblk-read.c  | 11 +++++------
 drivers/lightnvm/pblk-sysfs.c |  3 ++-
 drivers/lightnvm/pblk.h       |  2 ++
 drivers/nvme/host/lightnvm.c  |  1 +
 include/linux/lightnvm.h      |  1 +
 10 files changed, 72 insertions(+), 30 deletions(-)

-- 
2.9.5

[PATCH v2 1/8] lightnvm: pblk: Gracefully handle GC vmalloc fail

[Igor Konopko]

Currently when we fail on gc rq data allocation we simply skip the data
which we wanted to move and finally move the line to free state and
lose that data due to that. This patch move the data allocation to some
earlier phase of GC, where we can still fail gracefully by moving line
back to closed state.

Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
Reviewed-by: Javier González <javier@javigon.com>
---
 drivers/lightnvm/pblk-gc.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c
index 65692e6..ea9f392 100644
--- a/drivers/lightnvm/pblk-gc.c
+++ b/drivers/lightnvm/pblk-gc.c
@@ -84,8 +84,6 @@ static void pblk_gc_line_ws(struct work_struct *work)
 	struct pblk_line_ws *gc_rq_ws = container_of(work,
 						struct pblk_line_ws, ws);
 	struct pblk *pblk = gc_rq_ws->pblk;
-	struct nvm_tgt_dev *dev = pblk->dev;
-	struct nvm_geo *geo = &dev->geo;
 	struct pblk_gc *gc = &pblk->gc;
 	struct pblk_line *line = gc_rq_ws->line;
 	struct pblk_gc_rq *gc_rq = gc_rq_ws->priv;
@@ -93,13 +91,6 @@ static void pblk_gc_line_ws(struct work_struct *work)
 
 	up(&gc->gc_sem);
 
-	gc_rq->data = vmalloc(array_size(gc_rq->nr_secs, geo->csecs));
-	if (!gc_rq->data) {
-		pblk_err(pblk, "could not GC line:%d (%d/%d)\n",
-					line->id, *line->vsc, gc_rq->nr_secs);
-		goto out;
-	}
-
 	/* Read from GC victim block */
 	ret = pblk_submit_read_gc(pblk, gc_rq);
 	if (ret) {
@@ -189,6 +180,8 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work)
 	struct pblk_line *line = line_ws->line;
 	struct pblk_line_mgmt *l_mg = &pblk->l_mg;
 	struct pblk_line_meta *lm = &pblk->lm;
+	struct nvm_tgt_dev *dev = pblk->dev;
+	struct nvm_geo *geo = &dev->geo;
 	struct pblk_gc *gc = &pblk->gc;
 	struct pblk_line_ws *gc_rq_ws;
 	struct pblk_gc_rq *gc_rq;
@@ -247,9 +240,13 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work)
 	gc_rq->nr_secs = nr_secs;
 	gc_rq->line = line;
 
+	gc_rq->data = vmalloc(array_size(gc_rq->nr_secs, geo->csecs));
+	if (!gc_rq->data)
+		goto fail_free_gc_rq;
+
 	gc_rq_ws = kmalloc(sizeof(struct pblk_line_ws), GFP_KERNEL);
 	if (!gc_rq_ws)
-		goto fail_free_gc_rq;
+		goto fail_free_gc_data;
 
 	gc_rq_ws->pblk = pblk;
 	gc_rq_ws->line = line;
@@ -281,6 +278,8 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work)
 
 	return;
 
+fail_free_gc_data:
+	vfree(gc_rq->data);
 fail_free_gc_rq:
 	kfree(gc_rq);
 fail_free_lba_list:
-- 
2.9.5

Re: [PATCH v2 1/8] lightnvm: pblk: Gracefully handle GC vmalloc fail

[Hans Holmberg]

Looks good,

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

On Tue, Mar 5, 2019 at 2:54 PM Igor Konopko <igor.j.konopko@intel.com> wrote:
>
> Currently when we fail on gc rq data allocation we simply skip the data
> which we wanted to move and finally move the line to free state and
> lose that data due to that. This patch move the data allocation to some
> earlier phase of GC, where we can still fail gracefully by moving line
> back to closed state.
>
> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
> Reviewed-by: Javier González <javier@javigon.com>
> ---
>  drivers/lightnvm/pblk-gc.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c
> index 65692e6..ea9f392 100644
> --- a/drivers/lightnvm/pblk-gc.c
> +++ b/drivers/lightnvm/pblk-gc.c
> @@ -84,8 +84,6 @@ static void pblk_gc_line_ws(struct work_struct *work)
>         struct pblk_line_ws *gc_rq_ws = container_of(work,
>                                                 struct pblk_line_ws, ws);
>         struct pblk *pblk = gc_rq_ws->pblk;
> -       struct nvm_tgt_dev *dev = pblk->dev;
> -       struct nvm_geo *geo = &dev->geo;
>         struct pblk_gc *gc = &pblk->gc;
>         struct pblk_line *line = gc_rq_ws->line;
>         struct pblk_gc_rq *gc_rq = gc_rq_ws->priv;
> @@ -93,13 +91,6 @@ static void pblk_gc_line_ws(struct work_struct *work)
>
>         up(&gc->gc_sem);
>
> -       gc_rq->data = vmalloc(array_size(gc_rq->nr_secs, geo->csecs));
> -       if (!gc_rq->data) {
> -               pblk_err(pblk, "could not GC line:%d (%d/%d)\n",
> -                                       line->id, *line->vsc, gc_rq->nr_secs);
> -               goto out;
> -       }
> -
>         /* Read from GC victim block */
>         ret = pblk_submit_read_gc(pblk, gc_rq);
>         if (ret) {
> @@ -189,6 +180,8 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work)
>         struct pblk_line *line = line_ws->line;
>         struct pblk_line_mgmt *l_mg = &pblk->l_mg;
>         struct pblk_line_meta *lm = &pblk->lm;
> +       struct nvm_tgt_dev *dev = pblk->dev;
> +       struct nvm_geo *geo = &dev->geo;
>         struct pblk_gc *gc = &pblk->gc;
>         struct pblk_line_ws *gc_rq_ws;
>         struct pblk_gc_rq *gc_rq;
> @@ -247,9 +240,13 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work)
>         gc_rq->nr_secs = nr_secs;
>         gc_rq->line = line;
>
> +       gc_rq->data = vmalloc(array_size(gc_rq->nr_secs, geo->csecs));
> +       if (!gc_rq->data)
> +               goto fail_free_gc_rq;
> +
>         gc_rq_ws = kmalloc(sizeof(struct pblk_line_ws), GFP_KERNEL);
>         if (!gc_rq_ws)
> -               goto fail_free_gc_rq;
> +               goto fail_free_gc_data;
>
>         gc_rq_ws->pblk = pblk;
>         gc_rq_ws->line = line;
> @@ -281,6 +278,8 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work)
>
>         return;
>
> +fail_free_gc_data:
> +       vfree(gc_rq->data);
>  fail_free_gc_rq:
>         kfree(gc_rq);
>  fail_free_lba_list:
> --
> 2.9.5
>

[PATCH v2 2/8] lightnvm: pblk: Fix put line back behaviour

[Igor Konopko]

In current implementation of pblk_put_line_back behaviour
there are two cases which are not handled.

First one is the race condition with __pblk_map_invalidate in
which function we check for line state, which might be closed,
but still not added to any list and thus explode in list_move_tail.
This is due to lack of locking both gc_lock and line lock
in pblk_put_line_back current implementation.

The second issue is that when we are in that function, line
is not on any list and pblk_line_gc_list might hit the same
gc group and thus not return any move_list. Then our line
will stuck forever unassigned to any list. Simply resetting
gc_list to none will fix that.

Fixes: a4bd217 ("lightnvm: physical block device (pblk) target")
Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
Reviewed-by: Javier González <javier@javigon.com>
Reviewed-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
---
 drivers/lightnvm/pblk-gc.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c
index ea9f392..e23b192 100644
--- a/drivers/lightnvm/pblk-gc.c
+++ b/drivers/lightnvm/pblk-gc.c
@@ -64,19 +64,23 @@ static void pblk_put_line_back(struct pblk *pblk, struct pblk_line *line)
 	struct pblk_line_mgmt *l_mg = &pblk->l_mg;
 	struct list_head *move_list;
 
+	spin_lock(&l_mg->gc_lock);
 	spin_lock(&line->lock);
 	WARN_ON(line->state != PBLK_LINESTATE_GC);
 	line->state = PBLK_LINESTATE_CLOSED;
 	trace_pblk_line_state(pblk_disk_name(pblk), line->id,
 					line->state);
+
+	/* We need to reset gc_group in order to ensure that
+	 * pblk_line_gc_list will return proper move_list
+	 * since right now current line is not on any of the
+	 * gc lists.
+	 */
+	line->gc_group = PBLK_LINEGC_NONE;
 	move_list = pblk_line_gc_list(pblk, line);
 	spin_unlock(&line->lock);
-
-	if (move_list) {
-		spin_lock(&l_mg->gc_lock);
-		list_add_tail(&line->list, move_list);
-		spin_unlock(&l_mg->gc_lock);
-	}
+	list_add_tail(&line->list, move_list);
+	spin_unlock(&l_mg->gc_lock);
 }
 
 static void pblk_gc_line_ws(struct work_struct *work)
-- 
2.9.5

[PATCH v2 3/8] lightnvm: pblk: Count all read errors in stats

[Igor Konopko]

Currently when unknown error occurs on read path there is only dmesg
information about it, but it is not counted in sysfs statistics.

We would also like to track the number of such a requests, so new type
of counter 'read_ctrl_errors" is added in that patch.

Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
---
 drivers/lightnvm/pblk-core.c  | 1 +
 drivers/lightnvm/pblk-init.c  | 1 +
 drivers/lightnvm/pblk-sysfs.c | 3 ++-
 drivers/lightnvm/pblk.h       | 1 +
 4 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
index 39280c1..64280e6 100644
--- a/drivers/lightnvm/pblk-core.c
+++ b/drivers/lightnvm/pblk-core.c
@@ -493,6 +493,7 @@ void pblk_log_read_err(struct pblk *pblk, struct nvm_rq *rqd)
 		atomic_long_inc(&pblk->read_failed);
 		break;
 	default:
+		atomic_long_inc(&pblk->read_ctrl_errors);
 		pblk_err(pblk, "unknown read error:%d\n", rqd->error);
 	}
 #ifdef CONFIG_NVM_PBLK_DEBUG
diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index 81e8ed4..f4b6d8f2 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -1230,6 +1230,7 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct gendisk *tdisk,
 	atomic_long_set(&pblk->read_empty, 0);
 	atomic_long_set(&pblk->read_high_ecc, 0);
 	atomic_long_set(&pblk->read_failed_gc, 0);
+	atomic_long_set(&pblk->read_ctrl_errors, 0);
 	atomic_long_set(&pblk->write_failed, 0);
 	atomic_long_set(&pblk->erase_failed, 0);
 
diff --git a/drivers/lightnvm/pblk-sysfs.c b/drivers/lightnvm/pblk-sysfs.c
index 7d8958d..922273c 100644
--- a/drivers/lightnvm/pblk-sysfs.c
+++ b/drivers/lightnvm/pblk-sysfs.c
@@ -94,11 +94,12 @@ static ssize_t pblk_sysfs_stats(struct pblk *pblk, char *page)
 	ssize_t sz;
 
 	sz = snprintf(page, PAGE_SIZE,
-			"read_failed=%lu, read_high_ecc=%lu, read_empty=%lu, read_failed_gc=%lu, write_failed=%lu, erase_failed=%lu\n",
+			"read_failed=%lu, read_high_ecc=%lu, read_empty=%lu, read_failed_gc=%lu, read_ctrl_errors=%lu, write_failed=%lu, erase_failed=%lu\n",
 			atomic_long_read(&pblk->read_failed),
 			atomic_long_read(&pblk->read_high_ecc),
 			atomic_long_read(&pblk->read_empty),
 			atomic_long_read(&pblk->read_failed_gc),
+			atomic_long_read(&pblk->read_ctrl_errors),
 			atomic_long_read(&pblk->write_failed),
 			atomic_long_read(&pblk->erase_failed));
 
diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
index 381f074..6c82776 100644
--- a/drivers/lightnvm/pblk.h
+++ b/drivers/lightnvm/pblk.h
@@ -684,6 +684,7 @@ struct pblk {
 	atomic_long_t read_empty;
 	atomic_long_t read_high_ecc;
 	atomic_long_t read_failed_gc;
+	atomic_long_t read_ctrl_errors;
 	atomic_long_t write_failed;
 	atomic_long_t erase_failed;
 
-- 
2.9.5

Re: [PATCH v2 3/8] lightnvm: pblk: Count all read errors in stats

[Javier González]

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

> On 5 Mar 2019, at 14.51, Igor Konopko <igor.j.konopko@intel.com> wrote:
> 
> Currently when unknown error occurs on read path there is only dmesg
> information about it, but it is not counted in sysfs statistics.
> 
> We would also like to track the number of such a requests, so new type
> of counter 'read_ctrl_errors" is added in that patch.
> 
> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
> ---
> drivers/lightnvm/pblk-core.c  | 1 +
> drivers/lightnvm/pblk-init.c  | 1 +
> drivers/lightnvm/pblk-sysfs.c | 3 ++-
> drivers/lightnvm/pblk.h       | 1 +
> 4 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
> index 39280c1..64280e6 100644
> --- a/drivers/lightnvm/pblk-core.c
> +++ b/drivers/lightnvm/pblk-core.c
> @@ -493,6 +493,7 @@ void pblk_log_read_err(struct pblk *pblk, struct nvm_rq *rqd)
> 		atomic_long_inc(&pblk->read_failed);
> 		break;
> 	default:
> +		atomic_long_inc(&pblk->read_ctrl_errors);
> 		pblk_err(pblk, "unknown read error:%d\n", rqd->error);
> 	}
> #ifdef CONFIG_NVM_PBLK_DEBUG
> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
> index 81e8ed4..f4b6d8f2 100644
> --- a/drivers/lightnvm/pblk-init.c
> +++ b/drivers/lightnvm/pblk-init.c
> @@ -1230,6 +1230,7 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct gendisk *tdisk,
> 	atomic_long_set(&pblk->read_empty, 0);
> 	atomic_long_set(&pblk->read_high_ecc, 0);
> 	atomic_long_set(&pblk->read_failed_gc, 0);
> +	atomic_long_set(&pblk->read_ctrl_errors, 0);
> 	atomic_long_set(&pblk->write_failed, 0);
> 	atomic_long_set(&pblk->erase_failed, 0);
> 
> diff --git a/drivers/lightnvm/pblk-sysfs.c b/drivers/lightnvm/pblk-sysfs.c
> index 7d8958d..922273c 100644
> --- a/drivers/lightnvm/pblk-sysfs.c
> +++ b/drivers/lightnvm/pblk-sysfs.c
> @@ -94,11 +94,12 @@ static ssize_t pblk_sysfs_stats(struct pblk *pblk, char *page)
> 	ssize_t sz;
> 
> 	sz = snprintf(page, PAGE_SIZE,
> -			"read_failed=%lu, read_high_ecc=%lu, read_empty=%lu, read_failed_gc=%lu, write_failed=%lu, erase_failed=%lu\n",
> +			"read_failed=%lu, read_high_ecc=%lu, read_empty=%lu, read_failed_gc=%lu, read_ctrl_errors=%lu, write_failed=%lu, erase_failed=%lu\n",
> 			atomic_long_read(&pblk->read_failed),
> 			atomic_long_read(&pblk->read_high_ecc),
> 			atomic_long_read(&pblk->read_empty),
> 			atomic_long_read(&pblk->read_failed_gc),
> +			atomic_long_read(&pblk->read_ctrl_errors),
> 			atomic_long_read(&pblk->write_failed),
> 			atomic_long_read(&pblk->erase_failed));
> 
> diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
> index 381f074..6c82776 100644
> --- a/drivers/lightnvm/pblk.h
> +++ b/drivers/lightnvm/pblk.h
> @@ -684,6 +684,7 @@ struct pblk {
> 	atomic_long_t read_empty;
> 	atomic_long_t read_high_ecc;
> 	atomic_long_t read_failed_gc;
> +	atomic_long_t read_ctrl_errors;
> 	atomic_long_t write_failed;
> 	atomic_long_t erase_failed;
> 
> --
> 2.9.5

The only nitpick would be that if any user space script is relying on
the syses output for the errors, this could break them. Maybe we could
add a sysfs version file to mange this? Otherwise, it looks good.

Reviewed-by: Javier González <javier@javigon.com>

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

Re: [PATCH v2 3/8] lightnvm: pblk: Count all read errors in stats

[Hans Holmberg]

On Wed, Mar 6, 2019 at 8:28 AM Javier González <javier@javigon.com> wrote:
>
> > On 5 Mar 2019, at 14.51, Igor Konopko <igor.j.konopko@intel.com> wrote:
> >
> > Currently when unknown error occurs on read path there is only dmesg
> > information about it, but it is not counted in sysfs statistics.
> >
> > We would also like to track the number of such a requests, so new type
> > of counter 'read_ctrl_errors" is added in that patch.
> >
> > Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
> > ---
> > drivers/lightnvm/pblk-core.c  | 1 +
> > drivers/lightnvm/pblk-init.c  | 1 +
> > drivers/lightnvm/pblk-sysfs.c | 3 ++-
> > drivers/lightnvm/pblk.h       | 1 +
> > 4 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
> > index 39280c1..64280e6 100644
> > --- a/drivers/lightnvm/pblk-core.c
> > +++ b/drivers/lightnvm/pblk-core.c
> > @@ -493,6 +493,7 @@ void pblk_log_read_err(struct pblk *pblk, struct nvm_rq *rqd)
> >               atomic_long_inc(&pblk->read_failed);
> >               break;
> >       default:
> > +             atomic_long_inc(&pblk->read_ctrl_errors);
> >               pblk_err(pblk, "unknown read error:%d\n", rqd->error);
> >       }
> > #ifdef CONFIG_NVM_PBLK_DEBUG
> > diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
> > index 81e8ed4..f4b6d8f2 100644
> > --- a/drivers/lightnvm/pblk-init.c
> > +++ b/drivers/lightnvm/pblk-init.c
> > @@ -1230,6 +1230,7 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct gendisk *tdisk,
> >       atomic_long_set(&pblk->read_empty, 0);
> >       atomic_long_set(&pblk->read_high_ecc, 0);
> >       atomic_long_set(&pblk->read_failed_gc, 0);
> > +     atomic_long_set(&pblk->read_ctrl_errors, 0);
> >       atomic_long_set(&pblk->write_failed, 0);
> >       atomic_long_set(&pblk->erase_failed, 0);
> >
> > diff --git a/drivers/lightnvm/pblk-sysfs.c b/drivers/lightnvm/pblk-sysfs.c
> > index 7d8958d..922273c 100644
> > --- a/drivers/lightnvm/pblk-sysfs.c
> > +++ b/drivers/lightnvm/pblk-sysfs.c
> > @@ -94,11 +94,12 @@ static ssize_t pblk_sysfs_stats(struct pblk *pblk, char *page)
> >       ssize_t sz;
> >
> >       sz = snprintf(page, PAGE_SIZE,
> > -                     "read_failed=%lu, read_high_ecc=%lu, read_empty=%lu, read_failed_gc=%lu, write_failed=%lu, erase_failed=%lu\n",
> > +                     "read_failed=%lu, read_high_ecc=%lu, read_empty=%lu, read_failed_gc=%lu, read_ctrl_errors=%lu, write_failed=%lu, erase_failed=%lu\n",
> >                       atomic_long_read(&pblk->read_failed),
> >                       atomic_long_read(&pblk->read_high_ecc),
> >                       atomic_long_read(&pblk->read_empty),
> >                       atomic_long_read(&pblk->read_failed_gc),
> > +                     atomic_long_read(&pblk->read_ctrl_errors),
> >                       atomic_long_read(&pblk->write_failed),
> >                       atomic_long_read(&pblk->erase_failed));
> >
> > diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
> > index 381f074..6c82776 100644
> > --- a/drivers/lightnvm/pblk.h
> > +++ b/drivers/lightnvm/pblk.h
> > @@ -684,6 +684,7 @@ struct pblk {
> >       atomic_long_t read_empty;
> >       atomic_long_t read_high_ecc;
> >       atomic_long_t read_failed_gc;
> > +     atomic_long_t read_ctrl_errors;
> >       atomic_long_t write_failed;
> >       atomic_long_t erase_failed;
> >
> > --
> > 2.9.5
>
> The only nitpick would be that if any user space script is relying on
> the syses output for the errors, this could break them. Maybe we could
> add a sysfs version file to mange this? Otherwise, it looks good.
>
> Reviewed-by: Javier González <javier@javigon.com>

Agree with Javier here, as sysfs interfaces are supposed to be stable
(in the absence of a version file), so adding one would be a good
idea.

Re: [PATCH v2 3/8] lightnvm: pblk: Count all read errors in stats

[Igor Konopko]

On 06.03.2019 15:19, Hans Holmberg wrote:
> On Wed, Mar 6, 2019 at 8:28 AM Javier González <javier@javigon.com> wrote:
>>
>>> On 5 Mar 2019, at 14.51, Igor Konopko <igor.j.konopko@intel.com> wrote:
>>>
>>> Currently when unknown error occurs on read path there is only dmesg
>>> information about it, but it is not counted in sysfs statistics.
>>>
>>> We would also like to track the number of such a requests, so new type
>>> of counter 'read_ctrl_errors" is added in that patch.
>>>
>>> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
>>> ---
>>> drivers/lightnvm/pblk-core.c  | 1 +
>>> drivers/lightnvm/pblk-init.c  | 1 +
>>> drivers/lightnvm/pblk-sysfs.c | 3 ++-
>>> drivers/lightnvm/pblk.h       | 1 +
>>> 4 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
>>> index 39280c1..64280e6 100644
>>> --- a/drivers/lightnvm/pblk-core.c
>>> +++ b/drivers/lightnvm/pblk-core.c
>>> @@ -493,6 +493,7 @@ void pblk_log_read_err(struct pblk *pblk, struct nvm_rq *rqd)
>>>                atomic_long_inc(&pblk->read_failed);
>>>                break;
>>>        default:
>>> +             atomic_long_inc(&pblk->read_ctrl_errors);
>>>                pblk_err(pblk, "unknown read error:%d\n", rqd->error);
>>>        }
>>> #ifdef CONFIG_NVM_PBLK_DEBUG
>>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>>> index 81e8ed4..f4b6d8f2 100644
>>> --- a/drivers/lightnvm/pblk-init.c
>>> +++ b/drivers/lightnvm/pblk-init.c
>>> @@ -1230,6 +1230,7 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct gendisk *tdisk,
>>>        atomic_long_set(&pblk->read_empty, 0);
>>>        atomic_long_set(&pblk->read_high_ecc, 0);
>>>        atomic_long_set(&pblk->read_failed_gc, 0);
>>> +     atomic_long_set(&pblk->read_ctrl_errors, 0);
>>>        atomic_long_set(&pblk->write_failed, 0);
>>>        atomic_long_set(&pblk->erase_failed, 0);
>>>
>>> diff --git a/drivers/lightnvm/pblk-sysfs.c b/drivers/lightnvm/pblk-sysfs.c
>>> index 7d8958d..922273c 100644
>>> --- a/drivers/lightnvm/pblk-sysfs.c
>>> +++ b/drivers/lightnvm/pblk-sysfs.c
>>> @@ -94,11 +94,12 @@ static ssize_t pblk_sysfs_stats(struct pblk *pblk, char *page)
>>>        ssize_t sz;
>>>
>>>        sz = snprintf(page, PAGE_SIZE,
>>> -                     "read_failed=%lu, read_high_ecc=%lu, read_empty=%lu, read_failed_gc=%lu, write_failed=%lu, erase_failed=%lu\n",
>>> +                     "read_failed=%lu, read_high_ecc=%lu, read_empty=%lu, read_failed_gc=%lu, read_ctrl_errors=%lu, write_failed=%lu, erase_failed=%lu\n",
>>>                        atomic_long_read(&pblk->read_failed),
>>>                        atomic_long_read(&pblk->read_high_ecc),
>>>                        atomic_long_read(&pblk->read_empty),
>>>                        atomic_long_read(&pblk->read_failed_gc),
>>> +                     atomic_long_read(&pblk->read_ctrl_errors),
>>>                        atomic_long_read(&pblk->write_failed),
>>>                        atomic_long_read(&pblk->erase_failed));
>>>
>>> diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
>>> index 381f074..6c82776 100644
>>> --- a/drivers/lightnvm/pblk.h
>>> +++ b/drivers/lightnvm/pblk.h
>>> @@ -684,6 +684,7 @@ struct pblk {
>>>        atomic_long_t read_empty;
>>>        atomic_long_t read_high_ecc;
>>>        atomic_long_t read_failed_gc;
>>> +     atomic_long_t read_ctrl_errors;
>>>        atomic_long_t write_failed;
>>>        atomic_long_t erase_failed;
>>>
>>> --
>>> 2.9.5
>>
>> The only nitpick would be that if any user space script is relying on
>> the syses output for the errors, this could break them. Maybe we could
>> add a sysfs version file to mange this? Otherwise, it looks good.
>>
>> Reviewed-by: Javier González <javier@javigon.com>
> 
> Agree with Javier here, as sysfs interfaces are supposed to be stable
> (in the absence of a version file), so adding one would be a good
> idea.
> 

Let's drop this patch then - it is not a critical modification, we can 
definitely leave without it.

[PATCH v2 4/8] lightnvm: pblk: Ensure that erase is chunk aligned

[Igor Konopko]

In current pblk implementation of erase command there is a chance
that the sector bits are set to some random values for erase PPA.
This is unexpected situation, since erase shall be always chunk
aligned based on OCSSD 2.0 specification. This patch fixes that issue.

Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
---
 drivers/lightnvm/pblk-map.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/lightnvm/pblk-map.c b/drivers/lightnvm/pblk-map.c
index 7fbc99b..5408e32 100644
--- a/drivers/lightnvm/pblk-map.c
+++ b/drivers/lightnvm/pblk-map.c
@@ -162,6 +162,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);
 
-- 
2.9.5

Re: [PATCH v2 4/8] lightnvm: pblk: Ensure that erase is chunk aligned

[Javier González]

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

> On 5 Mar 2019, at 14.51, Igor Konopko <igor.j.konopko@intel.com> wrote:
> 
> In current pblk implementation of erase command there is a chance
> that the sector bits are set to some random values for erase PPA.
> This is unexpected situation, since erase shall be always chunk
> aligned based on OCSSD 2.0 specification. This patch fixes that issue.
> 
> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
> ---
> drivers/lightnvm/pblk-map.c | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/lightnvm/pblk-map.c b/drivers/lightnvm/pblk-map.c
> index 7fbc99b..5408e32 100644
> --- a/drivers/lightnvm/pblk-map.c
> +++ b/drivers/lightnvm/pblk-map.c
> @@ -162,6 +162,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);
> 
> --
> 2.9.5

Looks good to me.

Reviewed-by: Javier González <javier@javigon.com>

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

Re: [PATCH v2 4/8] lightnvm: pblk: Ensure that erase is chunk aligned

[Hans Holmberg]

Looks good,

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

On Tue, Mar 5, 2019 at 2:54 PM Igor Konopko <igor.j.konopko@intel.com> wrote:
>
> In current pblk implementation of erase command there is a chance
> that the sector bits are set to some random values for erase PPA.
> This is unexpected situation, since erase shall be always chunk
> aligned based on OCSSD 2.0 specification. This patch fixes that issue.
>
> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
> ---
>  drivers/lightnvm/pblk-map.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/lightnvm/pblk-map.c b/drivers/lightnvm/pblk-map.c
> index 7fbc99b..5408e32 100644
> --- a/drivers/lightnvm/pblk-map.c
> +++ b/drivers/lightnvm/pblk-map.c
> @@ -162,6 +162,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);
>
> --
> 2.9.5
>

[PATCH v2 5/8] lightnvm: pblk: Cleanly fail when there is not enough memory

[Igor Konopko]

L2P table can be huge in many cases, since it typically requires 1GB
of DRAM for 1TB of drive. When there is not enough memory available,
OOM killer turns on and kills random processes, which can be very
annoying for users.

This patch changes the flag for L2P table allocation on order to handle
this situation in more user friendly way.

GFP_KERNEL and __GPF_HIGHMEM are default flags used in parameterless
vmalloc() calls, so they are also keeped in that patch. Additionally
__GFP_NOWARN flag is added in order to hide very long dmesg warn in
case of the allocation failures. The most important flag introduced
in that patch is __GFP_RETRY_MAYFAIL, which would cause allocator
to try use free memory and if not available to drop caches, but not
to run OOM killer.

Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
Reviewed-by: Javier González <javier@javigon.com>
---
 drivers/lightnvm/pblk-init.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index f4b6d8f2..97b4c6e 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -164,9 +164,14 @@ static int pblk_l2p_init(struct pblk *pblk, bool factory_init)
 	int ret = 0;
 
 	map_size = pblk_trans_map_size(pblk);
-	pblk->trans_map = vmalloc(map_size);
-	if (!pblk->trans_map)
+	pblk->trans_map = __vmalloc(map_size, GFP_KERNEL | __GFP_NOWARN
+					| __GFP_RETRY_MAYFAIL | __GFP_HIGHMEM,
+					PAGE_KERNEL);
+	if (!pblk->trans_map) {
+		pblk_err(pblk, "failed to allocate L2P (need %ld of memory)\n",
+				map_size);
 		return -ENOMEM;
+	}
 
 	pblk_ppa_set_empty(&ppa);
 
-- 
2.9.5

Re: [PATCH v2 5/8] lightnvm: pblk: Cleanly fail when there is not enough memory

[Hans Holmberg]

Looks good,

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

On Tue, Mar 5, 2019 at 2:54 PM Igor Konopko <igor.j.konopko@intel.com> wrote:
>
> L2P table can be huge in many cases, since it typically requires 1GB
> of DRAM for 1TB of drive. When there is not enough memory available,
> OOM killer turns on and kills random processes, which can be very
> annoying for users.
>
> This patch changes the flag for L2P table allocation on order to handle
> this situation in more user friendly way.
>
> GFP_KERNEL and __GPF_HIGHMEM are default flags used in parameterless
> vmalloc() calls, so they are also keeped in that patch. Additionally
> __GFP_NOWARN flag is added in order to hide very long dmesg warn in
> case of the allocation failures. The most important flag introduced
> in that patch is __GFP_RETRY_MAYFAIL, which would cause allocator
> to try use free memory and if not available to drop caches, but not
> to run OOM killer.
>
> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
> Reviewed-by: Javier González <javier@javigon.com>
> ---
>  drivers/lightnvm/pblk-init.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
> index f4b6d8f2..97b4c6e 100644
> --- a/drivers/lightnvm/pblk-init.c
> +++ b/drivers/lightnvm/pblk-init.c
> @@ -164,9 +164,14 @@ static int pblk_l2p_init(struct pblk *pblk, bool factory_init)
>         int ret = 0;
>
>         map_size = pblk_trans_map_size(pblk);
> -       pblk->trans_map = vmalloc(map_size);
> -       if (!pblk->trans_map)
> +       pblk->trans_map = __vmalloc(map_size, GFP_KERNEL | __GFP_NOWARN
> +                                       | __GFP_RETRY_MAYFAIL | __GFP_HIGHMEM,
> +                                       PAGE_KERNEL);
> +       if (!pblk->trans_map) {
> +               pblk_err(pblk, "failed to allocate L2P (need %ld of memory)\n",
> +                               map_size);
>                 return -ENOMEM;
> +       }
>
>         pblk_ppa_set_empty(&ppa);
>
> --
> 2.9.5
>

[PATCH v2 6/8] lightnvm: pblk: Set proper read stutus in bio

[Igor Konopko]

Currently in case of read errors, bi_status is not set properly which
leads to returning inproper data to higher layer. This patch fix that
by setting proper status in case of read errors

Patch also removes unnecessary warn_once(), which does not make sense
in that place, since user bio is not used for interation with drive
and thus bi_status will not be set here.

Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
---
 drivers/lightnvm/pblk-read.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
index 1f9b319..6569746 100644
--- a/drivers/lightnvm/pblk-read.c
+++ b/drivers/lightnvm/pblk-read.c
@@ -175,11 +175,10 @@ static void pblk_read_check_rand(struct pblk *pblk, struct nvm_rq *rqd,
 	WARN_ONCE(j != rqd->nr_ppas, "pblk: corrupted random request\n");
 }
 
-static void pblk_end_user_read(struct bio *bio)
+static void pblk_end_user_read(struct bio *bio, int error)
 {
-#ifdef CONFIG_NVM_PBLK_DEBUG
-	WARN_ONCE(bio->bi_status, "pblk: corrupted read bio\n");
-#endif
+	if (error && error != NVM_RSP_WARN_HIGHECC)
+		bio_io_error(bio);
 	bio_endio(bio);
 }
 
@@ -219,7 +218,7 @@ static void pblk_end_io_read(struct nvm_rq *rqd)
 	struct pblk_g_ctx *r_ctx = nvm_rq_to_pdu(rqd);
 	struct bio *bio = (struct bio *)r_ctx->private;
 
-	pblk_end_user_read(bio);
+	pblk_end_user_read(bio, rqd->error);
 	__pblk_end_io_read(pblk, rqd, true);
 }
 
@@ -292,7 +291,7 @@ static void pblk_end_partial_read(struct nvm_rq *rqd)
 	rqd->bio = NULL;
 	rqd->nr_ppas = nr_secs;
 
-	bio_endio(bio);
+	pblk_end_user_read(bio, rqd->error);
 	__pblk_end_io_read(pblk, rqd, false);
 }
 
-- 
2.9.5

Re: [PATCH v2 6/8] lightnvm: pblk: Set proper read stutus in bio

[Javier González]

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

> On 5 Mar 2019, at 14.51, Igor Konopko <igor.j.konopko@intel.com> wrote:
> 
> Currently in case of read errors, bi_status is not set properly which
> leads to returning inproper data to higher layer. This patch fix that
> by setting proper status in case of read errors
> 
> Patch also removes unnecessary warn_once(), which does not make sense
> in that place, since user bio is not used for interation with drive
> and thus bi_status will not be set here.
> 
> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
> ---
> drivers/lightnvm/pblk-read.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
> index 1f9b319..6569746 100644
> --- a/drivers/lightnvm/pblk-read.c
> +++ b/drivers/lightnvm/pblk-read.c
> @@ -175,11 +175,10 @@ static void pblk_read_check_rand(struct pblk *pblk, struct nvm_rq *rqd,
> 	WARN_ONCE(j != rqd->nr_ppas, "pblk: corrupted random request\n");
> }
> 
> -static void pblk_end_user_read(struct bio *bio)
> +static void pblk_end_user_read(struct bio *bio, int error)
> {
> -#ifdef CONFIG_NVM_PBLK_DEBUG
> -	WARN_ONCE(bio->bi_status, "pblk: corrupted read bio\n");
> -#endif
> +	if (error && error != NVM_RSP_WARN_HIGHECC)
> +		bio_io_error(bio);
> 	bio_endio(bio);
> }
> 
> @@ -219,7 +218,7 @@ static void pblk_end_io_read(struct nvm_rq *rqd)
> 	struct pblk_g_ctx *r_ctx = nvm_rq_to_pdu(rqd);
> 	struct bio *bio = (struct bio *)r_ctx->private;
> 
> -	pblk_end_user_read(bio);
> +	pblk_end_user_read(bio, rqd->error);
> 	__pblk_end_io_read(pblk, rqd, true);
> }
> 
> @@ -292,7 +291,7 @@ static void pblk_end_partial_read(struct nvm_rq *rqd)
> 	rqd->bio = NULL;
> 	rqd->nr_ppas = nr_secs;
> 
> -	bio_endio(bio);
> +	pblk_end_user_read(bio, rqd->error);
> 	__pblk_end_io_read(pblk, rqd, false);
> }
> 
> --
> 2.9.5

I still believe that we should work out the error model before.

Let me give an example. In the OCSSD 2.0 spec, we use the NVMe DULBE
bit. If the device does not report read errors, then invalid reads
(let’s pick a read ahead of the WP as an example) will return success +
predefined data. In this case, pblk would catch this in the read
integrity checks. Should pblk return an error in this case? I see the
point of catching this at the driver level and passing the error, but
the behavior is inconsistent as the same read issued outside of pblk
will succeed.

When I implemented the read integrity checks I considered to plug it
into the block layer Integrity framework (CONFIG_BLK_DEV_INTEGRITY), but
since we do not follow any PI standard I thought it was difficult to
argue for it. It is maybe time to revisit this approach.

This said, if you want it in to help debugging, I will not oppose to it.
I guess it is the same to fix it one way or the other when we
implement proper error handling.

Javier

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

Re: [PATCH v2 6/8] lightnvm: pblk: Set proper read stutus in bio

[Hans Holmberg]

On Tue, Mar 5, 2019 at 2:54 PM Igor Konopko <igor.j.konopko@intel.com> wrote:
>
> Currently in case of read errors, bi_status is not set properly which
> leads to returning inproper data to higher layer. This patch fix that
> by setting proper status in case of read errors
>
> Patch also removes unnecessary warn_once(), which does not make sense
> in that place, since user bio is not used for interation with drive
> and thus bi_status will not be set here.
>
> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
> ---
>  drivers/lightnvm/pblk-read.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
> index 1f9b319..6569746 100644
> --- a/drivers/lightnvm/pblk-read.c
> +++ b/drivers/lightnvm/pblk-read.c
> @@ -175,11 +175,10 @@ static void pblk_read_check_rand(struct pblk *pblk, struct nvm_rq *rqd,
>         WARN_ONCE(j != rqd->nr_ppas, "pblk: corrupted random request\n");
>  }
>
> -static void pblk_end_user_read(struct bio *bio)
> +static void pblk_end_user_read(struct bio *bio, int error)
>  {
> -#ifdef CONFIG_NVM_PBLK_DEBUG
> -       WARN_ONCE(bio->bi_status, "pblk: corrupted read bio\n");
> -#endif
> +       if (error && error != NVM_RSP_WARN_HIGHECC)
> +               bio_io_error(bio);
>         bio_endio(bio);
>  }
>
> @@ -219,7 +218,7 @@ static void pblk_end_io_read(struct nvm_rq *rqd)
>         struct pblk_g_ctx *r_ctx = nvm_rq_to_pdu(rqd);
>         struct bio *bio = (struct bio *)r_ctx->private;
>
> -       pblk_end_user_read(bio);
> +       pblk_end_user_read(bio, rqd->error);
>         __pblk_end_io_read(pblk, rqd, true);
>  }
>
> @@ -292,7 +291,7 @@ static void pblk_end_partial_read(struct nvm_rq *rqd)
>         rqd->bio = NULL;
>         rqd->nr_ppas = nr_secs;
>
> -       bio_endio(bio);
> +       pblk_end_user_read(bio, rqd->error);
>         __pblk_end_io_read(pblk, rqd, false);
>  }
>
> --
> 2.9.5
>

Looks good to me,

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

[PATCH v2 7/8] lightnvm: pblk: warn about opened chunk on factory init

[Igor Konopko]

When we creating pblk instance with factory flag, there is a
possibility that some chunks are in open state, which does not allow
to issue erase request to them directly, based on OCSSD 2.0 spec.
Still most of the controllers allows for such a transition, but it is
not guaranteed that the particular drive will do so. This patch adds
the proper warning during pblk factory creation to let user know about
number of chunks in such a state, which can potentially be a reason
of erase failures.

Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
---
 drivers/lightnvm/pblk-core.c | 14 ++++++++++++++
 drivers/lightnvm/pblk-init.c | 14 +++++++++++---
 drivers/lightnvm/pblk.h      |  1 +
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
index 64280e6..4f16596 100644
--- a/drivers/lightnvm/pblk-core.c
+++ b/drivers/lightnvm/pblk-core.c
@@ -161,6 +161,20 @@ struct nvm_chk_meta *pblk_chunk_get_off(struct pblk *pblk,
 	return meta + ch_off + lun_off + chk_off;
 }
 
+int pblk_count_opened_chunks(struct pblk *pblk, struct nvm_chk_meta *meta)
+{
+	struct nvm_tgt_dev *dev = pblk->dev;
+	struct nvm_geo *geo = &dev->geo;
+	int i, cnt = 0;
+
+	for (i = 0; i < geo->all_luns; i++) {
+		if (meta[i].state == NVM_CHK_ST_OPEN)
+			cnt++;
+	}
+
+	return cnt;
+}
+
 void __pblk_map_invalidate(struct pblk *pblk, struct pblk_line *line,
 			   u64 paddr)
 {
diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index 97b4c6e..f590f62 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -1028,12 +1028,12 @@ static int pblk_line_meta_init(struct pblk *pblk)
 	return 0;
 }
 
-static int pblk_lines_init(struct pblk *pblk)
+static int pblk_lines_init(struct pblk *pblk, bool factory_init)
 {
 	struct pblk_line_mgmt *l_mg = &pblk->l_mg;
 	struct pblk_line *line;
 	void *chunk_meta;
-	int nr_free_chks = 0;
+	int nr_free_chks = 0, nr_opened_chks;
 	int i, ret;
 
 	ret = pblk_line_meta_init(pblk);
@@ -1054,6 +1054,14 @@ static int pblk_lines_init(struct pblk *pblk)
 		goto fail_free_luns;
 	}
 
+	if (factory_init) {
+		nr_opened_chks = pblk_count_opened_chunks(pblk, chunk_meta);
+		if (nr_opened_chks) {
+			pblk_warn(pblk, "There are %d opened chunks\n",
+				  nr_opened_chks);
+		}
+	}
+
 	pblk->lines = kcalloc(l_mg->nr_lines, sizeof(struct pblk_line),
 								GFP_KERNEL);
 	if (!pblk->lines) {
@@ -1245,7 +1253,7 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct gendisk *tdisk,
 		goto fail;
 	}
 
-	ret = pblk_lines_init(pblk);
+	ret = pblk_lines_init(pblk, flags & NVM_TARGET_FACTORY);
 	if (ret) {
 		pblk_err(pblk, "could not initialize lines\n");
 		goto fail_free_core;
diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
index 6c82776..c2f07ec 100644
--- a/drivers/lightnvm/pblk.h
+++ b/drivers/lightnvm/pblk.h
@@ -795,6 +795,7 @@ struct nvm_chk_meta *pblk_get_chunk_meta(struct pblk *pblk);
 struct nvm_chk_meta *pblk_chunk_get_off(struct pblk *pblk,
 					      struct nvm_chk_meta *lp,
 					      struct ppa_addr ppa);
+int pblk_count_opened_chunks(struct pblk *pblk, struct nvm_chk_meta *_meta);
 void pblk_log_write_err(struct pblk *pblk, struct nvm_rq *rqd);
 void pblk_log_read_err(struct pblk *pblk, struct nvm_rq *rqd);
 int pblk_submit_io(struct pblk *pblk, struct nvm_rq *rqd);
-- 
2.9.5

Re: [PATCH v2 7/8] lightnvm: pblk: warn about opened chunk on factory init

[Javier González]

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


> On 5 Mar 2019, at 14.51, Igor Konopko <igor.j.konopko@intel.com> wrote:
> 
> When we creating pblk instance with factory flag, there is a
> possibility that some chunks are in open state, which does not allow
> to issue erase request to them directly, based on OCSSD 2.0 spec.
> Still most of the controllers allows for such a transition, but it is
> not guaranteed that the particular drive will do so. This patch adds
> the proper warning during pblk factory creation to let user know about
> number of chunks in such a state, which can potentially be a reason
> of erase failures.
> 
> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
> ---
> drivers/lightnvm/pblk-core.c | 14 ++++++++++++++
> drivers/lightnvm/pblk-init.c | 14 +++++++++++---
> drivers/lightnvm/pblk.h      |  1 +
> 3 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
> index 64280e6..4f16596 100644
> --- a/drivers/lightnvm/pblk-core.c
> +++ b/drivers/lightnvm/pblk-core.c
> @@ -161,6 +161,20 @@ struct nvm_chk_meta *pblk_chunk_get_off(struct pblk *pblk,
> 	return meta + ch_off + lun_off + chk_off;
> }
> 
> +int pblk_count_opened_chunks(struct pblk *pblk, struct nvm_chk_meta *meta)
> +{
> +	struct nvm_tgt_dev *dev = pblk->dev;
> +	struct nvm_geo *geo = &dev->geo;
> +	int i, cnt = 0;
> +
> +	for (i = 0; i < geo->all_luns; i++) {
> +		if (meta[i].state == NVM_CHK_ST_OPEN)
> +			cnt++;
> +	}
> +
> +	return cnt;
> +}
> +
> void __pblk_map_invalidate(struct pblk *pblk, struct pblk_line *line,
> 			   u64 paddr)
> {
> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
> index 97b4c6e..f590f62 100644
> --- a/drivers/lightnvm/pblk-init.c
> +++ b/drivers/lightnvm/pblk-init.c
> @@ -1028,12 +1028,12 @@ static int pblk_line_meta_init(struct pblk *pblk)
> 	return 0;
> }
> 
> -static int pblk_lines_init(struct pblk *pblk)
> +static int pblk_lines_init(struct pblk *pblk, bool factory_init)
> {
> 	struct pblk_line_mgmt *l_mg = &pblk->l_mg;
> 	struct pblk_line *line;
> 	void *chunk_meta;
> -	int nr_free_chks = 0;
> +	int nr_free_chks = 0, nr_opened_chks;
> 	int i, ret;
> 
> 	ret = pblk_line_meta_init(pblk);
> @@ -1054,6 +1054,14 @@ static int pblk_lines_init(struct pblk *pblk)
> 		goto fail_free_luns;
> 	}
> 
> +	if (factory_init) {
> +		nr_opened_chks = pblk_count_opened_chunks(pblk, chunk_meta);
> +		if (nr_opened_chks) {
> +			pblk_warn(pblk, "There are %d opened chunks\n",
> +				  nr_opened_chks);
> +		}
> +	}
> +
> 	pblk->lines = kcalloc(l_mg->nr_lines, sizeof(struct pblk_line),
> 								GFP_KERNEL);
> 	if (!pblk->lines) {
> @@ -1245,7 +1253,7 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct gendisk *tdisk,
> 		goto fail;
> 	}
> 
> -	ret = pblk_lines_init(pblk);
> +	ret = pblk_lines_init(pblk, flags & NVM_TARGET_FACTORY);
> 	if (ret) {
> 		pblk_err(pblk, "could not initialize lines\n");
> 		goto fail_free_core;
> diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
> index 6c82776..c2f07ec 100644
> --- a/drivers/lightnvm/pblk.h
> +++ b/drivers/lightnvm/pblk.h
> @@ -795,6 +795,7 @@ struct nvm_chk_meta *pblk_get_chunk_meta(struct pblk *pblk);
> struct nvm_chk_meta *pblk_chunk_get_off(struct pblk *pblk,
> 					      struct nvm_chk_meta *lp,
> 					      struct ppa_addr ppa);
> +int pblk_count_opened_chunks(struct pblk *pblk, struct nvm_chk_meta *_meta);
> void pblk_log_write_err(struct pblk *pblk, struct nvm_rq *rqd);
> void pblk_log_read_err(struct pblk *pblk, struct nvm_rq *rqd);
> int pblk_submit_io(struct pblk *pblk, struct nvm_rq *rqd);
> --
> 2.9.5

I think the warning is a good tradeoff.

If you want to avoid looping again across all chunks, note that we are
already doing this in pblk_setup_line_meta_chk(). It should be easy to
refactor it, check here for NVM_CHK_ST_OPEN and then return the counter.



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

Re: [PATCH v2 7/8] lightnvm: pblk: warn about opened chunk on factory init

[Hans Holmberg]

On Tue, Mar 5, 2019 at 2:54 PM Igor Konopko <igor.j.konopko@intel.com> wrote:
>
> When we creating pblk instance with factory flag, there is a
> possibility that some chunks are in open state, which does not allow
> to issue erase request to them directly, based on OCSSD 2.0 spec.
> Still most of the controllers allows for such a transition, but it is
> not guaranteed that the particular drive will do so. This patch adds
> the proper warning during pblk factory creation to let user know about
> number of chunks in such a state, which can potentially be a reason
> of erase failures.
>
> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
> ---
>  drivers/lightnvm/pblk-core.c | 14 ++++++++++++++
>  drivers/lightnvm/pblk-init.c | 14 +++++++++++---
>  drivers/lightnvm/pblk.h      |  1 +
>  3 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
> index 64280e6..4f16596 100644
> --- a/drivers/lightnvm/pblk-core.c
> +++ b/drivers/lightnvm/pblk-core.c
> @@ -161,6 +161,20 @@ struct nvm_chk_meta *pblk_chunk_get_off(struct pblk *pblk,
>         return meta + ch_off + lun_off + chk_off;
>  }
>
> +int pblk_count_opened_chunks(struct pblk *pblk, struct nvm_chk_meta *meta)
> +{
> +       struct nvm_tgt_dev *dev = pblk->dev;
> +       struct nvm_geo *geo = &dev->geo;
> +       int i, cnt = 0;
> +
> +       for (i = 0; i < geo->all_luns; i++) {

Shouldn't this be i < geo->all_chunks ?

Otherwise the patch looks good.

Thanks,
Hans

> +               if (meta[i].state == NVM_CHK_ST_OPEN)
> +                       cnt++;
> +       }
> +
> +       return cnt;
> +}
> +
>  void __pblk_map_invalidate(struct pblk *pblk, struct pblk_line *line,
>                            u64 paddr)
>  {
> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
> index 97b4c6e..f590f62 100644
> --- a/drivers/lightnvm/pblk-init.c
> +++ b/drivers/lightnvm/pblk-init.c
> @@ -1028,12 +1028,12 @@ static int pblk_line_meta_init(struct pblk *pblk)
>         return 0;
>  }
>
> -static int pblk_lines_init(struct pblk *pblk)
> +static int pblk_lines_init(struct pblk *pblk, bool factory_init)
>  {
>         struct pblk_line_mgmt *l_mg = &pblk->l_mg;
>         struct pblk_line *line;
>         void *chunk_meta;
> -       int nr_free_chks = 0;
> +       int nr_free_chks = 0, nr_opened_chks;
>         int i, ret;
>
>         ret = pblk_line_meta_init(pblk);
> @@ -1054,6 +1054,14 @@ static int pblk_lines_init(struct pblk *pblk)
>                 goto fail_free_luns;
>         }
>
> +       if (factory_init) {
> +               nr_opened_chks = pblk_count_opened_chunks(pblk, chunk_meta);
> +               if (nr_opened_chks) {
> +                       pblk_warn(pblk, "There are %d opened chunks\n",
> +                                 nr_opened_chks);
> +               }
> +       }
> +
>         pblk->lines = kcalloc(l_mg->nr_lines, sizeof(struct pblk_line),
>                                                                 GFP_KERNEL);
>         if (!pblk->lines) {
> @@ -1245,7 +1253,7 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct gendisk *tdisk,
>                 goto fail;
>         }
>
> -       ret = pblk_lines_init(pblk);
> +       ret = pblk_lines_init(pblk, flags & NVM_TARGET_FACTORY);
>         if (ret) {
>                 pblk_err(pblk, "could not initialize lines\n");
>                 goto fail_free_core;
> diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
> index 6c82776..c2f07ec 100644
> --- a/drivers/lightnvm/pblk.h
> +++ b/drivers/lightnvm/pblk.h
> @@ -795,6 +795,7 @@ struct nvm_chk_meta *pblk_get_chunk_meta(struct pblk *pblk);
>  struct nvm_chk_meta *pblk_chunk_get_off(struct pblk *pblk,
>                                               struct nvm_chk_meta *lp,
>                                               struct ppa_addr ppa);
> +int pblk_count_opened_chunks(struct pblk *pblk, struct nvm_chk_meta *_meta);
>  void pblk_log_write_err(struct pblk *pblk, struct nvm_rq *rqd);
>  void pblk_log_read_err(struct pblk *pblk, struct nvm_rq *rqd);
>  int pblk_submit_io(struct pblk *pblk, struct nvm_rq *rqd);
> --
> 2.9.5
>

Re: [PATCH v2 7/8] lightnvm: pblk: warn about opened chunk on factory init

[Igor Konopko]

On 06.03.2019 15:27, Hans Holmberg wrote:
> On Tue, Mar 5, 2019 at 2:54 PM Igor Konopko <igor.j.konopko@intel.com> wrote:
>>
>> When we creating pblk instance with factory flag, there is a
>> possibility that some chunks are in open state, which does not allow
>> to issue erase request to them directly, based on OCSSD 2.0 spec.
>> Still most of the controllers allows for such a transition, but it is
>> not guaranteed that the particular drive will do so. This patch adds
>> the proper warning during pblk factory creation to let user know about
>> number of chunks in such a state, which can potentially be a reason
>> of erase failures.
>>
>> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
>> ---
>>   drivers/lightnvm/pblk-core.c | 14 ++++++++++++++
>>   drivers/lightnvm/pblk-init.c | 14 +++++++++++---
>>   drivers/lightnvm/pblk.h      |  1 +
>>   3 files changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
>> index 64280e6..4f16596 100644
>> --- a/drivers/lightnvm/pblk-core.c
>> +++ b/drivers/lightnvm/pblk-core.c
>> @@ -161,6 +161,20 @@ struct nvm_chk_meta *pblk_chunk_get_off(struct pblk *pblk,
>>          return meta + ch_off + lun_off + chk_off;
>>   }
>>
>> +int pblk_count_opened_chunks(struct pblk *pblk, struct nvm_chk_meta *meta)
>> +{
>> +       struct nvm_tgt_dev *dev = pblk->dev;
>> +       struct nvm_geo *geo = &dev->geo;
>> +       int i, cnt = 0;
>> +
>> +       for (i = 0; i < geo->all_luns; i++) {
> 
> Shouldn't this be i < geo->all_chunks ?
> 

Yes, it should be.

Anyway, Javier proposed to use existing pblk_setup_line_meta_chk() 
function for counting that, so I'm gonna to rewrite this patch.

> Otherwise the patch looks good.
> 
> Thanks,
> Hans
> 
>> +               if (meta[i].state == NVM_CHK_ST_OPEN)
>> +                       cnt++;
>> +       }
>> +
>> +       return cnt;
>> +}
>> +
>>   void __pblk_map_invalidate(struct pblk *pblk, struct pblk_line *line,
>>                             u64 paddr)
>>   {
>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>> index 97b4c6e..f590f62 100644
>> --- a/drivers/lightnvm/pblk-init.c
>> +++ b/drivers/lightnvm/pblk-init.c
>> @@ -1028,12 +1028,12 @@ static int pblk_line_meta_init(struct pblk *pblk)
>>          return 0;
>>   }
>>
>> -static int pblk_lines_init(struct pblk *pblk)
>> +static int pblk_lines_init(struct pblk *pblk, bool factory_init)
>>   {
>>          struct pblk_line_mgmt *l_mg = &pblk->l_mg;
>>          struct pblk_line *line;
>>          void *chunk_meta;
>> -       int nr_free_chks = 0;
>> +       int nr_free_chks = 0, nr_opened_chks;
>>          int i, ret;
>>
>>          ret = pblk_line_meta_init(pblk);
>> @@ -1054,6 +1054,14 @@ static int pblk_lines_init(struct pblk *pblk)
>>                  goto fail_free_luns;
>>          }
>>
>> +       if (factory_init) {
>> +               nr_opened_chks = pblk_count_opened_chunks(pblk, chunk_meta);
>> +               if (nr_opened_chks) {
>> +                       pblk_warn(pblk, "There are %d opened chunks\n",
>> +                                 nr_opened_chks);
>> +               }
>> +       }
>> +
>>          pblk->lines = kcalloc(l_mg->nr_lines, sizeof(struct pblk_line),
>>                                                                  GFP_KERNEL);
>>          if (!pblk->lines) {
>> @@ -1245,7 +1253,7 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct gendisk *tdisk,
>>                  goto fail;
>>          }
>>
>> -       ret = pblk_lines_init(pblk);
>> +       ret = pblk_lines_init(pblk, flags & NVM_TARGET_FACTORY);
>>          if (ret) {
>>                  pblk_err(pblk, "could not initialize lines\n");
>>                  goto fail_free_core;
>> diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
>> index 6c82776..c2f07ec 100644
>> --- a/drivers/lightnvm/pblk.h
>> +++ b/drivers/lightnvm/pblk.h
>> @@ -795,6 +795,7 @@ struct nvm_chk_meta *pblk_get_chunk_meta(struct pblk *pblk);
>>   struct nvm_chk_meta *pblk_chunk_get_off(struct pblk *pblk,
>>                                                struct nvm_chk_meta *lp,
>>                                                struct ppa_addr ppa);
>> +int pblk_count_opened_chunks(struct pblk *pblk, struct nvm_chk_meta *_meta);
>>   void pblk_log_write_err(struct pblk *pblk, struct nvm_rq *rqd);
>>   void pblk_log_read_err(struct pblk *pblk, struct nvm_rq *rqd);
>>   int pblk_submit_io(struct pblk *pblk, struct nvm_rq *rqd);
>> --
>> 2.9.5
>>

[PATCH v2 8/8] lightnvm: Inherit mdts from the parent nvme device

[Igor Konopko]

Current lightnvm and pblk implementation does not care about NVMe max
data transfer size, which can be smaller than 64*K=256K. There are
existing NVMe controllers which NVMe max data transfer size is lower
that 256K (for example 128K, which happens for existing NVMe
controllers which are NVMe spec compliant). Such a controllers are not
able to handle command which contains 64 PPAs, since the the size of
DMAed buffer will be above the capabilities of such a controller.

Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
Reviewed-by: Javier González <javier@javigon.com>
---
 drivers/lightnvm/core.c      | 9 +++++++--
 drivers/nvme/host/lightnvm.c | 1 +
 include/linux/lightnvm.h     | 1 +
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index 5f82036..c01f83b 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -325,6 +325,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
 	struct nvm_target *t;
 	struct nvm_tgt_dev *tgt_dev;
 	void *targetdata;
+	unsigned int mdts;
 	int ret;
 
 	switch (create->conf.type) {
@@ -412,8 +413,12 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
 	tdisk->private_data = targetdata;
 	tqueue->queuedata = targetdata;
 
-	blk_queue_max_hw_sectors(tqueue,
-			(dev->geo.csecs >> 9) * NVM_MAX_VLBA);
+	mdts = (dev->geo.csecs >> 9) * NVM_MAX_VLBA;
+	if (dev->geo.mdts) {
+		mdts = min_t(u32, dev->geo.mdts,
+				(dev->geo.csecs >> 9) * NVM_MAX_VLBA);
+	}
+	blk_queue_max_hw_sectors(tqueue, mdts);
 
 	set_capacity(tdisk, tt->capacity(targetdata));
 	add_disk(tdisk);
diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index 949e29e..4f20a10 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -977,6 +977,7 @@ int nvme_nvm_register(struct nvme_ns *ns, char *disk_name, int node)
 	geo->csecs = 1 << ns->lba_shift;
 	geo->sos = ns->ms;
 	geo->ext = ns->ext;
+	geo->mdts = ns->ctrl->max_hw_sectors;
 
 	dev->q = q;
 	memcpy(dev->name, disk_name, DISK_NAME_LEN);
diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
index 5d865a5..d3b0270 100644
--- a/include/linux/lightnvm.h
+++ b/include/linux/lightnvm.h
@@ -358,6 +358,7 @@ struct nvm_geo {
 	u16	csecs;		/* sector size */
 	u16	sos;		/* out-of-band area size */
 	bool	ext;		/* metadata in extended data buffer */
+	u32	mdts;		/* Max data transfer size*/
 
 	/* device write constrains */
 	u32	ws_min;		/* minimum write size */
-- 
2.9.5

Re: [PATCH v2 8/8] lightnvm: Inherit mdts from the parent nvme device

[Matias Bjørling]

On 3/5/19 2:51 PM, Igor Konopko wrote:
> Current lightnvm and pblk implementation does not care about NVMe max
> data transfer size, which can be smaller than 64*K=256K. There are
> existing NVMe controllers which NVMe max data transfer size is lower
> that 256K (for example 128K, which happens for existing NVMe
> controllers which are NVMe spec compliant). Such a controllers are not
> able to handle command which contains 64 PPAs, since the the size of
> DMAed buffer will be above the capabilities of such a controller.
> 
> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
> Reviewed-by: Javier González <javier@javigon.com>
> ---
>   drivers/lightnvm/core.c      | 9 +++++++--
>   drivers/nvme/host/lightnvm.c | 1 +
>   include/linux/lightnvm.h     | 1 +
>   3 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index 5f82036..c01f83b 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -325,6 +325,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
>   	struct nvm_target *t;
>   	struct nvm_tgt_dev *tgt_dev;
>   	void *targetdata;
> +	unsigned int mdts;
>   	int ret;
>   
>   	switch (create->conf.type) {
> @@ -412,8 +413,12 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
>   	tdisk->private_data = targetdata;
>   	tqueue->queuedata = targetdata;
>   
> -	blk_queue_max_hw_sectors(tqueue,
> -			(dev->geo.csecs >> 9) * NVM_MAX_VLBA);
> +	mdts = (dev->geo.csecs >> 9) * NVM_MAX_VLBA;
> +	if (dev->geo.mdts) {
> +		mdts = min_t(u32, dev->geo.mdts,
> +				(dev->geo.csecs >> 9) * NVM_MAX_VLBA);
> +	}
> +	blk_queue_max_hw_sectors(tqueue, mdts);
>   
>   	set_capacity(tdisk, tt->capacity(targetdata));
>   	add_disk(tdisk);
> diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
> index 949e29e..4f20a10 100644
> --- a/drivers/nvme/host/lightnvm.c
> +++ b/drivers/nvme/host/lightnvm.c
> @@ -977,6 +977,7 @@ int nvme_nvm_register(struct nvme_ns *ns, char *disk_name, int node)
>   	geo->csecs = 1 << ns->lba_shift;
>   	geo->sos = ns->ms;
>   	geo->ext = ns->ext;
> +	geo->mdts = ns->ctrl->max_hw_sectors;
>   
>   	dev->q = q;
>   	memcpy(dev->name, disk_name, DISK_NAME_LEN);
> diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
> index 5d865a5..d3b0270 100644
> --- a/include/linux/lightnvm.h
> +++ b/include/linux/lightnvm.h
> @@ -358,6 +358,7 @@ struct nvm_geo {
>   	u16	csecs;		/* sector size */
>   	u16	sos;		/* out-of-band area size */
>   	bool	ext;		/* metadata in extended data buffer */
> +	u32	mdts;		/* Max data transfer size*/
>   
>   	/* device write constrains */
>   	u32	ws_min;		/* minimum write size */
> 

I think I can pick this up. I'll let Javier/Hans rereview.

Given Javier's feedback on broken existing mdts implementations. When 
they are brought to light, we may want to add in a quirk list to update 
MDTS accordingly. Javier, will this address the issue you have regarding 
using mdts?

-Matias

Re: [PATCH v2 8/8] lightnvm: Inherit mdts from the parent nvme device

[Javier González]

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

> On 5 Mar 2019, at 15.34, Matias Bjørling <mb@lightnvm.io> wrote:
> 
> On 3/5/19 2:51 PM, Igor Konopko wrote:
>> Current lightnvm and pblk implementation does not care about NVMe max
>> data transfer size, which can be smaller than 64*K=256K. There are
>> existing NVMe controllers which NVMe max data transfer size is lower
>> that 256K (for example 128K, which happens for existing NVMe
>> controllers which are NVMe spec compliant). Such a controllers are not
>> able to handle command which contains 64 PPAs, since the the size of
>> DMAed buffer will be above the capabilities of such a controller.
>> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
>> Reviewed-by: Javier González <javier@javigon.com>
>> ---
>>  drivers/lightnvm/core.c      | 9 +++++++--
>>  drivers/nvme/host/lightnvm.c | 1 +
>>  include/linux/lightnvm.h     | 1 +
>>  3 files changed, 9 insertions(+), 2 deletions(-)
>> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
>> index 5f82036..c01f83b 100644
>> --- a/drivers/lightnvm/core.c
>> +++ b/drivers/lightnvm/core.c
>> @@ -325,6 +325,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
>>  	struct nvm_target *t;
>>  	struct nvm_tgt_dev *tgt_dev;
>>  	void *targetdata;
>> +	unsigned int mdts;
>>  	int ret;
>>    	switch (create->conf.type) {
>> @@ -412,8 +413,12 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
>>  	tdisk->private_data = targetdata;
>>  	tqueue->queuedata = targetdata;
>>  -	blk_queue_max_hw_sectors(tqueue,
>> -			(dev->geo.csecs >> 9) * NVM_MAX_VLBA);
>> +	mdts = (dev->geo.csecs >> 9) * NVM_MAX_VLBA;
>> +	if (dev->geo.mdts) {
>> +		mdts = min_t(u32, dev->geo.mdts,
>> +				(dev->geo.csecs >> 9) * NVM_MAX_VLBA);
>> +	}
>> +	blk_queue_max_hw_sectors(tqueue, mdts);
>>    	set_capacity(tdisk, tt->capacity(targetdata));
>>  	add_disk(tdisk);
>> diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
>> index 949e29e..4f20a10 100644
>> --- a/drivers/nvme/host/lightnvm.c
>> +++ b/drivers/nvme/host/lightnvm.c
>> @@ -977,6 +977,7 @@ int nvme_nvm_register(struct nvme_ns *ns, char *disk_name, int node)
>>  	geo->csecs = 1 << ns->lba_shift;
>>  	geo->sos = ns->ms;
>>  	geo->ext = ns->ext;
>> +	geo->mdts = ns->ctrl->max_hw_sectors;
>>    	dev->q = q;
>>  	memcpy(dev->name, disk_name, DISK_NAME_LEN);
>> diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
>> index 5d865a5..d3b0270 100644
>> --- a/include/linux/lightnvm.h
>> +++ b/include/linux/lightnvm.h
>> @@ -358,6 +358,7 @@ struct nvm_geo {
>>  	u16	csecs;		/* sector size */
>>  	u16	sos;		/* out-of-band area size */
>>  	bool	ext;		/* metadata in extended data buffer */
>> +	u32	mdts;		/* Max data transfer size*/
>>    	/* device write constrains */
>>  	u32	ws_min;		/* minimum write size */
> 
> I think I can pick this up. I'll let Javier/Hans rereview.
> 
> Given Javier's feedback on broken existing mdts implementations. When
> they are brought to light, we may want to add in a quirk list to
> update MDTS accordingly. Javier, will this address the issue you have
> regarding using mdts?

Sounds good. Thanks for considering this.

Javier

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

Re: [PATCH v2 8/8] lightnvm: Inherit mdts from the parent nvme device

[Hans Holmberg]

Looks good to me too,

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

On Wed, Mar 6, 2019 at 8:44 AM Javier González <javier@javigon.com> wrote:
>
> > On 5 Mar 2019, at 15.34, Matias Bjørling <mb@lightnvm.io> wrote:
> >
> > On 3/5/19 2:51 PM, Igor Konopko wrote:
> >> Current lightnvm and pblk implementation does not care about NVMe max
> >> data transfer size, which can be smaller than 64*K=256K. There are
> >> existing NVMe controllers which NVMe max data transfer size is lower
> >> that 256K (for example 128K, which happens for existing NVMe
> >> controllers which are NVMe spec compliant). Such a controllers are not
> >> able to handle command which contains 64 PPAs, since the the size of
> >> DMAed buffer will be above the capabilities of such a controller.
> >> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
> >> Reviewed-by: Javier González <javier@javigon.com>
> >> ---
> >>  drivers/lightnvm/core.c      | 9 +++++++--
> >>  drivers/nvme/host/lightnvm.c | 1 +
> >>  include/linux/lightnvm.h     | 1 +
> >>  3 files changed, 9 insertions(+), 2 deletions(-)
> >> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> >> index 5f82036..c01f83b 100644
> >> --- a/drivers/lightnvm/core.c
> >> +++ b/drivers/lightnvm/core.c
> >> @@ -325,6 +325,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
> >>      struct nvm_target *t;
> >>      struct nvm_tgt_dev *tgt_dev;
> >>      void *targetdata;
> >> +    unsigned int mdts;
> >>      int ret;
> >>      switch (create->conf.type) {
> >> @@ -412,8 +413,12 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
> >>      tdisk->private_data = targetdata;
> >>      tqueue->queuedata = targetdata;
> >>  -   blk_queue_max_hw_sectors(tqueue,
> >> -                    (dev->geo.csecs >> 9) * NVM_MAX_VLBA);
> >> +    mdts = (dev->geo.csecs >> 9) * NVM_MAX_VLBA;
> >> +    if (dev->geo.mdts) {
> >> +            mdts = min_t(u32, dev->geo.mdts,
> >> +                            (dev->geo.csecs >> 9) * NVM_MAX_VLBA);
> >> +    }
> >> +    blk_queue_max_hw_sectors(tqueue, mdts);
> >>      set_capacity(tdisk, tt->capacity(targetdata));
> >>      add_disk(tdisk);
> >> diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
> >> index 949e29e..4f20a10 100644
> >> --- a/drivers/nvme/host/lightnvm.c
> >> +++ b/drivers/nvme/host/lightnvm.c
> >> @@ -977,6 +977,7 @@ int nvme_nvm_register(struct nvme_ns *ns, char *disk_name, int node)
> >>      geo->csecs = 1 << ns->lba_shift;
> >>      geo->sos = ns->ms;
> >>      geo->ext = ns->ext;
> >> +    geo->mdts = ns->ctrl->max_hw_sectors;
> >>      dev->q = q;
> >>      memcpy(dev->name, disk_name, DISK_NAME_LEN);
> >> diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
> >> index 5d865a5..d3b0270 100644
> >> --- a/include/linux/lightnvm.h
> >> +++ b/include/linux/lightnvm.h
> >> @@ -358,6 +358,7 @@ struct nvm_geo {
> >>      u16     csecs;          /* sector size */
> >>      u16     sos;            /* out-of-band area size */
> >>      bool    ext;            /* metadata in extended data buffer */
> >> +    u32     mdts;           /* Max data transfer size*/
> >>      /* device write constrains */
> >>      u32     ws_min;         /* minimum write size */
> >
> > I think I can pick this up. I'll let Javier/Hans rereview.
> >
> > Given Javier's feedback on broken existing mdts implementations. When
> > they are brought to light, we may want to add in a quirk list to
> > update MDTS accordingly. Javier, will this address the issue you have
> > regarding using mdts?
>
> Sounds good. Thanks for considering this.
>
> Javier

Re: [PATCH v2 0/8] lightnvm: bugfixes and improvements

[Matias Bjørling]

On 3/5/19 2:51 PM, Igor Konopko wrote:
> This series provides a group of the bugfixes or improvements
> for lightnvm and pblk device.
> 
> Most of the patches are rather simple and covers some corner cases
> scenario, but we were able to hit most of them in some scenarios.
> Few others close some existing gaps which we were able to found.
> 
> Changes v2->v1:
> -skipped patches which were applied in v1
> -dropped not needed patch for pblk_should_kick_write()
> -patch which scan for opened chunks on pblk creation do not close
> chunk anymore, just print the warning
> -unknown read errors are counted seperately from read unrecoverable
> 
> Igor Konopko (8):
>    lightnvm: pblk: Gracefully handle GC vmalloc fail
>    lightnvm: pblk: Fix put line back behaviour
>    lightnvm: pblk: Count all read errors in stats
>    lightnvm: pblk: Ensure that erase is chunk aligned
>    lightnvm: pblk: Cleanly fail when there is not enough memory
>    lightnvm: pblk: Set proper read stutus in bio
>    lightnvm: pblk: warn about opened chunk on factory init
>    lightnvm: Inherit mdts from the parent nvme device
> 
>   drivers/lightnvm/core.c       |  9 +++++++--
>   drivers/lightnvm/pblk-core.c  | 15 +++++++++++++++
>   drivers/lightnvm/pblk-gc.c    | 35 +++++++++++++++++++----------------
>   drivers/lightnvm/pblk-init.c  | 24 +++++++++++++++++++-----
>   drivers/lightnvm/pblk-map.c   |  1 +
>   drivers/lightnvm/pblk-read.c  | 11 +++++------
>   drivers/lightnvm/pblk-sysfs.c |  3 ++-
>   drivers/lightnvm/pblk.h       |  2 ++
>   drivers/nvme/host/lightnvm.c  |  1 +
>   include/linux/lightnvm.h      |  1 +
>   10 files changed, 72 insertions(+), 30 deletions(-)
> 

Thanks Igor. Thanks Javier and Hans for the reviews.

I've picked up 1, 2, 4, 5, 6, and 8. Note that I've lower-cased the 
first letter after "lightnvm: pblk:" in the patches, and also updated 
the wording a bit when appropriate.