Re: [PATCH 08/20] lightnvm: pblk: sched. metadata on write thread

[Rakesh Pandit]

On Wed, Sep 20, 2017 at 09:28:34PM +0300, Rakesh Pandit wrote:
> Hi Javier,
> 
> one small issue I found for error path while going through changes:
> 
> On Mon, Jun 26, 2017 at 11:57:17AM +0200, Javier González wrote:
> ..
> > +static int pblk_lines_alloc_metadata(struct pblk *pblk)
> > +{
[..]
> > +   if (!l_mg->sline_meta[i])
> > +   goto fail_free_smeta;
> 
> For this error path the the goto label at end doesn't free up
> resources correctly.  It needs a
> 
> while (--index >= 0)...
> 
> logic with appropriate adjustment.
> 
> > +   }
[..]
> > +   if (!l_mg->vsc_list)
> > +   goto fail_free_emeta;
> > +
> > +   for (i = 0; i < l_mg->nr_lines; i++)
> > +   l_mg->vsc_list[i] = cpu_to_le32(EMPTY_ENTRY);
> > +
> > +   return 0;
> > +
> > +fail_free_emeta:
> > +   while (--i >= 0) {
> > +   vfree(l_mg->eline_meta[i]->buf);
> 
> This would need l_mg->emeta_alloc_type check to decide whether
> allocation was done with kmalloc or vmalloc.
> 
> > +   kfree(_mg->eline_meta[i]);
> > +   }
> > +
> > +fail_free_smeta:
> > +   for (i = 0; i < PBLK_DATA_LINES; i++)
> > +   pblk_mfree(_mg->sline_meta[i], l_mg->smeta_alloc_type);
> > +
> > +   return -ENOMEM;
> > +}
> > +
> 
> Thanks,

I failed to see earlier that it has been merged already.  Will post a
patch in a while.

Thanks,

Re: [PATCH 08/20] lightnvm: pblk: sched. metadata on write thread

[Rakesh Pandit]

Hi Javier,

one small issue I found for error path while going through changes:

On Mon, Jun 26, 2017 at 11:57:17AM +0200, Javier González wrote:
..
> +static int pblk_lines_alloc_metadata(struct pblk *pblk)
> +{
> + struct pblk_line_mgmt *l_mg = >l_mg;
> + struct pblk_line_meta *lm = >lm;
> + int i;
> +
> + /* smeta is always small enough to fit on a kmalloc memory allocation,
> +  * emeta depends on the number of LUNs allocated to the pblk instance
> +  */
> + l_mg->smeta_alloc_type = PBLK_KMALLOC_META;
> + for (i = 0; i < PBLK_DATA_LINES; i++) {
> + l_mg->sline_meta[i] = kmalloc(lm->smeta_len, GFP_KERNEL);
> + if (!l_mg->sline_meta[i])
> + goto fail_free_smeta;

For this error path the the goto label at end doesn't free up
resources correctly.  It needs a

while (--index >= 0)...

logic with appropriate adjustment.

> + }
> +
> + /* emeta allocates three different buffers for managing metadata with
> +  * in-memory and in-media layouts
> +  */
> + for (i = 0; i < PBLK_DATA_LINES; i++) {
> + struct pblk_emeta *emeta;
> +
> + emeta = kmalloc(sizeof(struct pblk_emeta), GFP_KERNEL);
> + if (!emeta)
> + goto fail_free_emeta;
> +
> + if (lm->emeta_len[0] > KMALLOC_MAX_CACHE_SIZE) {
> + l_mg->emeta_alloc_type = PBLK_VMALLOC_META;
> +
> + emeta->buf = vmalloc(lm->emeta_len[0]);
> + if (!emeta->buf) {
> + kfree(emeta);
> + goto fail_free_emeta;
> + }
> +
> + emeta->nr_entries = lm->emeta_sec[0];
> + l_mg->eline_meta[i] = emeta;
> + } else {
> + l_mg->emeta_alloc_type = PBLK_KMALLOC_META;
> +
> + emeta->buf = kmalloc(lm->emeta_len[0], GFP_KERNEL);
> + if (!emeta->buf) {
> + kfree(emeta);
> + goto fail_free_emeta;
> + }
> +
> + emeta->nr_entries = lm->emeta_sec[0];
> + l_mg->eline_meta[i] = emeta;
> + }
> + }
> +
> + l_mg->vsc_list = kcalloc(l_mg->nr_lines, sizeof(__le32), GFP_KERNEL);
> + if (!l_mg->vsc_list)
> + goto fail_free_emeta;
> +
> + for (i = 0; i < l_mg->nr_lines; i++)
> + l_mg->vsc_list[i] = cpu_to_le32(EMPTY_ENTRY);
> +
> + return 0;
> +
> +fail_free_emeta:
> + while (--i >= 0) {
> + vfree(l_mg->eline_meta[i]->buf);

This would need l_mg->emeta_alloc_type check to decide whether
allocation was done with kmalloc or vmalloc.

> + kfree(_mg->eline_meta[i]);
> + }
> +
> +fail_free_smeta:
> + for (i = 0; i < PBLK_DATA_LINES; i++)
> + pblk_mfree(_mg->sline_meta[i], l_mg->smeta_alloc_type);
> +
> + return -ENOMEM;
> +}
> +

Thanks,

[PATCH 08/20] lightnvm: pblk: sched. metadata on write thread

[Javier González]

At the moment, line metadata is persisted on a separate work queue, that
is kicked each time that a line is closed. The assumption when designing
this was that freeing the write thread from creating a new write request
was better than the potential impact of writes colliding on the media
(user I/O and metadata I/O). Experimentation has proven that this
assumption is wrong; collision can cause up to 25% of bandwidth and
introduce long tail latencies on the write thread, which potentially
cause user write threads to spend more time spinning to get a free entry
on the write buffer.

This patch moves the metadata logic to the write thread. When a line is
closed, remaining metadata is written in memory and is placed on a
metadata queue. The write thread then takes the metadata corresponding
to the previous line, creates the write request and schedules it to
minimize collisions on the media. Using this approach, we see that we
can saturate the media's bandwidth, which helps reducing both write
latencies and the spinning time for user writer threads.

Signed-off-by: Javier González 
Signed-off-by: Matias Bjørling 
---
 drivers/lightnvm/pblk-core.c | 216 -
 drivers/lightnvm/pblk-gc.c   |  41 +++
 drivers/lightnvm/pblk-init.c | 240 ++---
 drivers/lightnvm/pblk-map.c  |  13 +-
 drivers/lightnvm/pblk-recovery.c |  67 ++-
 drivers/lightnvm/pblk-sysfs.c|  16 ++-
 drivers/lightnvm/pblk-write.c| 249 +++
 drivers/lightnvm/pblk.h  | 114 +-
 8 files changed, 672 insertions(+), 284 deletions(-)

diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
index 6fa51eb9d681..6e4b06f841e7 100644
--- a/drivers/lightnvm/pblk-core.c
+++ b/drivers/lightnvm/pblk-core.c
@@ -87,7 +87,7 @@ static void __pblk_map_invalidate(struct pblk *pblk, struct 
pblk_line *line,
spin_unlock(>lock);
return;
}
-   line->vsc--;
+   le32_add_cpu(line->vsc, -1);
 
if (line->state == PBLK_LINESTATE_CLOSED)
move_list = pblk_line_gc_list(pblk, line);
@@ -306,28 +306,29 @@ struct list_head *pblk_line_gc_list(struct pblk *pblk, 
struct pblk_line *line)
struct pblk_line_meta *lm = >lm;
struct pblk_line_mgmt *l_mg = >l_mg;
struct list_head *move_list = NULL;
+   int vsc = le32_to_cpu(*line->vsc);
 
-   if (!line->vsc) {
+   if (!vsc) {
if (line->gc_group != PBLK_LINEGC_FULL) {
line->gc_group = PBLK_LINEGC_FULL;
move_list = _mg->gc_full_list;
}
-   } else if (line->vsc < lm->mid_thrs) {
+   } else if (vsc < lm->mid_thrs) {
if (line->gc_group != PBLK_LINEGC_HIGH) {
line->gc_group = PBLK_LINEGC_HIGH;
move_list = _mg->gc_high_list;
}
-   } else if (line->vsc < lm->high_thrs) {
+   } else if (vsc < lm->high_thrs) {
if (line->gc_group != PBLK_LINEGC_MID) {
line->gc_group = PBLK_LINEGC_MID;
move_list = _mg->gc_mid_list;
}
-   } else if (line->vsc < line->sec_in_line) {
+   } else if (vsc < line->sec_in_line) {
if (line->gc_group != PBLK_LINEGC_LOW) {
line->gc_group = PBLK_LINEGC_LOW;
move_list = _mg->gc_low_list;
}
-   } else if (line->vsc == line->sec_in_line) {
+   } else if (vsc == line->sec_in_line) {
if (line->gc_group != PBLK_LINEGC_EMPTY) {
line->gc_group = PBLK_LINEGC_EMPTY;
move_list = _mg->gc_empty_list;
@@ -337,7 +338,7 @@ struct list_head *pblk_line_gc_list(struct pblk *pblk, 
struct pblk_line *line)
line->gc_group = PBLK_LINEGC_NONE;
move_list =  _mg->corrupt_list;
pr_err("pblk: corrupted vsc for line %d, vsc:%d (%d/%d/%d)\n",
-   line->id, line->vsc,
+   line->id, vsc,
line->sec_in_line,
lm->high_thrs, lm->mid_thrs);
}
@@ -496,8 +497,20 @@ int pblk_calc_secs(struct pblk *pblk, unsigned long 
secs_avail,
return secs_to_sync;
 }
 
-static u64 __pblk_alloc_page(struct pblk *pblk, struct pblk_line *line,
-int nr_secs)
+void pblk_dealloc_page(struct pblk *pblk, struct pblk_line *line, int nr_secs)
+{
+   u64 addr;
+   int i;
+
+   addr = find_next_zero_bit(line->map_bitmap,
+   pblk->lm.sec_per_line, line->cur_sec);
+   line->cur_sec = addr - nr_secs;
+
+   for (i = 0; i < nr_secs; i++, line->cur_sec--)
+