Re: [PATCH 4/5] lightnvm: pblk: add padding distribution sysfs attribute

[Hans Holmberg]

On Tue, Feb 6, 2018 at 11:50 AM, Matias Bjørling  wrote:
> On 02/06/2018 10:27 AM, Hans Holmberg wrote:
>>
>> Hi Matias,
>>
>> On Wed, Jan 31, 2018 at 9:44 AM, Matias Bjørling  wrote:
>>>
>>> On 01/31/2018 03:06 AM, Javier González wrote:


 From: Hans Holmberg 

 When pblk receives a sync, all data up to that point in the write buffer
 must be comitted to persistent storage, and as flash memory comes with a
 minimal write size there is a significant cost involved both in terms
 of time for completing the sync and in terms of write amplification
 padded sectors for filling up to the minimal write size.

 In order to get a better understanding of the costs involved for syncs,
 Add a sysfs attribute to pblk: padded_dist, showing a normalized
 distribution of sectors padded. In order to facilitate measurements of
 specific workloads during the lifetime of the pblk instance, the
 distribution can be reset by writing 0 to the attribute.

 Do this by introducing counters for each possible padding:
 {0..(minimal write size - 1)} and calculate the normalized distribution
 when showing the attribute.

 Signed-off-by: Hans Holmberg 
 Signed-off-by: Javier González 
 ---
drivers/lightnvm/pblk-init.c  | 16 --
drivers/lightnvm/pblk-rb.c| 15 +-
drivers/lightnvm/pblk-sysfs.c | 68
 +++
drivers/lightnvm/pblk.h   |  6 +++-
4 files changed, 95 insertions(+), 10 deletions(-)

 diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
 index 7eedc5daebc8..a5e3510c0f38 100644
 --- a/drivers/lightnvm/pblk-init.c
 +++ b/drivers/lightnvm/pblk-init.c
 @@ -921,6 +921,7 @@ static void pblk_free(struct pblk *pblk)
{
  pblk_luns_free(pblk);
  pblk_lines_free(pblk);
 +   kfree(pblk->pad_dist);
  pblk_line_meta_free(pblk);
  pblk_core_free(pblk);
  pblk_l2p_free(pblk);
 @@ -998,11 +999,13 @@ static void *pblk_init(struct nvm_tgt_dev *dev,
 struct gendisk *tdisk,
  pblk->pad_rst_wa = 0;
  pblk->gc_rst_wa = 0;
+ atomic_long_set(>nr_flush, 0);
 +   pblk->nr_flush_rst = 0;
 +
#ifdef CONFIG_NVM_DEBUG
  atomic_long_set(>inflight_writes, 0);
  atomic_long_set(>padded_writes, 0);
  atomic_long_set(>padded_wb, 0);
 -   atomic_long_set(>nr_flush, 0);
  atomic_long_set(>req_writes, 0);
  atomic_long_set(>sub_writes, 0);
  atomic_long_set(>sync_writes, 0);
 @@ -1034,10 +1037,17 @@ static void *pblk_init(struct nvm_tgt_dev *dev,
 struct gendisk *tdisk,
  goto fail_free_luns;
  }
+ pblk->pad_dist = kzalloc((pblk->min_write_pgs - 1) *
 sizeof(atomic64_t),
 +GFP_KERNEL);
 +   if (!pblk->pad_dist) {
 +   ret = -ENOMEM;
 +   goto fail_free_line_meta;
 +   }
 +
  ret = pblk_core_init(pblk);
  if (ret) {
  pr_err("pblk: could not initialize core\n");
 -   goto fail_free_line_meta;
 +   goto fail_free_pad_dist;
  }
  ret = pblk_l2p_init(pblk);
 @@ -1097,6 +1107,8 @@ static void *pblk_init(struct nvm_tgt_dev *dev,
 struct gendisk *tdisk,
  pblk_l2p_free(pblk);
fail_free_core:
  pblk_core_free(pblk);
 +fail_free_pad_dist:
 +   kfree(pblk->pad_dist);
fail_free_line_meta:
  pblk_line_meta_free(pblk);
fail_free_luns:
 diff --git a/drivers/lightnvm/pblk-rb.c b/drivers/lightnvm/pblk-rb.c
 index 7044b5599cc4..264372d7b537 100644
 --- a/drivers/lightnvm/pblk-rb.c
 +++ b/drivers/lightnvm/pblk-rb.c
 @@ -437,9 +437,7 @@ static int pblk_rb_may_write_flush(struct pblk_rb
 *rb,
 unsigned int nr_entries,
  if (bio->bi_opf & REQ_PREFLUSH) {
  struct pblk *pblk = container_of(rb, struct pblk, rwb);
-#ifdef CONFIG_NVM_DEBUG
  atomic_long_inc(>nr_flush);
 -#endif
  if (pblk_rb_flush_point_set(>rwb, bio, mem))
  *io_ret = NVM_IO_OK;
  }
 @@ -620,14 +618,17 @@ unsigned int pblk_rb_read_to_bio(struct pblk_rb
 *rb,
 struct nvm_rq *rqd,
  pr_err("pblk: could not pad page in write
 bio\n");
  return NVM_IO_ERR;
  }
 +
 +   if (pad < pblk->min_write_pgs)
 +   atomic64_inc(>pad_dist[pad - 1]);
 +   else
 

Re: [PATCH 4/5] lightnvm: pblk: add padding distribution sysfs attribute

[Matias Bjørling]

On 02/06/2018 10:27 AM, Hans Holmberg wrote:

Hi Matias,

On Wed, Jan 31, 2018 at 9:44 AM, Matias Bjørling  wrote:

On 01/31/2018 03:06 AM, Javier González wrote:


From: Hans Holmberg 

When pblk receives a sync, all data up to that point in the write buffer
must be comitted to persistent storage, and as flash memory comes with a
minimal write size there is a significant cost involved both in terms
of time for completing the sync and in terms of write amplification
padded sectors for filling up to the minimal write size.

In order to get a better understanding of the costs involved for syncs,
Add a sysfs attribute to pblk: padded_dist, showing a normalized
distribution of sectors padded. In order to facilitate measurements of
specific workloads during the lifetime of the pblk instance, the
distribution can be reset by writing 0 to the attribute.

Do this by introducing counters for each possible padding:
{0..(minimal write size - 1)} and calculate the normalized distribution
when showing the attribute.

Signed-off-by: Hans Holmberg 
Signed-off-by: Javier González 
---
   drivers/lightnvm/pblk-init.c  | 16 --
   drivers/lightnvm/pblk-rb.c| 15 +-
   drivers/lightnvm/pblk-sysfs.c | 68
+++
   drivers/lightnvm/pblk.h   |  6 +++-
   4 files changed, 95 insertions(+), 10 deletions(-)

diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index 7eedc5daebc8..a5e3510c0f38 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -921,6 +921,7 @@ static void pblk_free(struct pblk *pblk)
   {
 pblk_luns_free(pblk);
 pblk_lines_free(pblk);
+   kfree(pblk->pad_dist);
 pblk_line_meta_free(pblk);
 pblk_core_free(pblk);
 pblk_l2p_free(pblk);
@@ -998,11 +999,13 @@ static void *pblk_init(struct nvm_tgt_dev *dev,
struct gendisk *tdisk,
 pblk->pad_rst_wa = 0;
 pblk->gc_rst_wa = 0;
   + atomic_long_set(>nr_flush, 0);
+   pblk->nr_flush_rst = 0;
+
   #ifdef CONFIG_NVM_DEBUG
 atomic_long_set(>inflight_writes, 0);
 atomic_long_set(>padded_writes, 0);
 atomic_long_set(>padded_wb, 0);
-   atomic_long_set(>nr_flush, 0);
 atomic_long_set(>req_writes, 0);
 atomic_long_set(>sub_writes, 0);
 atomic_long_set(>sync_writes, 0);
@@ -1034,10 +1037,17 @@ static void *pblk_init(struct nvm_tgt_dev *dev,
struct gendisk *tdisk,
 goto fail_free_luns;
 }
   + pblk->pad_dist = kzalloc((pblk->min_write_pgs - 1) *
sizeof(atomic64_t),
+GFP_KERNEL);
+   if (!pblk->pad_dist) {
+   ret = -ENOMEM;
+   goto fail_free_line_meta;
+   }
+
 ret = pblk_core_init(pblk);
 if (ret) {
 pr_err("pblk: could not initialize core\n");
-   goto fail_free_line_meta;
+   goto fail_free_pad_dist;
 }
 ret = pblk_l2p_init(pblk);
@@ -1097,6 +1107,8 @@ static void *pblk_init(struct nvm_tgt_dev *dev,
struct gendisk *tdisk,
 pblk_l2p_free(pblk);
   fail_free_core:
 pblk_core_free(pblk);
+fail_free_pad_dist:
+   kfree(pblk->pad_dist);
   fail_free_line_meta:
 pblk_line_meta_free(pblk);
   fail_free_luns:
diff --git a/drivers/lightnvm/pblk-rb.c b/drivers/lightnvm/pblk-rb.c
index 7044b5599cc4..264372d7b537 100644
--- a/drivers/lightnvm/pblk-rb.c
+++ b/drivers/lightnvm/pblk-rb.c
@@ -437,9 +437,7 @@ static int pblk_rb_may_write_flush(struct pblk_rb *rb,
unsigned int nr_entries,
 if (bio->bi_opf & REQ_PREFLUSH) {
 struct pblk *pblk = container_of(rb, struct pblk, rwb);
   -#ifdef CONFIG_NVM_DEBUG
 atomic_long_inc(>nr_flush);
-#endif
 if (pblk_rb_flush_point_set(>rwb, bio, mem))
 *io_ret = NVM_IO_OK;
 }
@@ -620,14 +618,17 @@ unsigned int pblk_rb_read_to_bio(struct pblk_rb *rb,
struct nvm_rq *rqd,
 pr_err("pblk: could not pad page in write bio\n");
 return NVM_IO_ERR;
 }
+
+   if (pad < pblk->min_write_pgs)
+   atomic64_inc(>pad_dist[pad - 1]);
+   else
+   pr_warn("pblk: padding more than min. sectors\n");



Curious, when would this happen? Would it be an implementation error or
something a user did wrong?


This would be an implementation error - and this check is just a
safeguard to make sure we won't go out of bounds when updating the
statistics.



Ah, does it make sense to have such defensive programming, when the 
value is never persisted? An 64 bit integer is quite large, and if only 
used for workloads for a single instance, it would practically never 
trigger, and if it did, do one care?






Looks good to me. Let me know if you want to send a new patch, or if 

Re: [PATCH 4/5] lightnvm: pblk: add padding distribution sysfs attribute

[Hans Holmberg]

Hi Matias,

On Wed, Jan 31, 2018 at 9:44 AM, Matias Bjørling  wrote:
> On 01/31/2018 03:06 AM, Javier González wrote:
>>
>> From: Hans Holmberg 
>>
>> When pblk receives a sync, all data up to that point in the write buffer
>> must be comitted to persistent storage, and as flash memory comes with a
>> minimal write size there is a significant cost involved both in terms
>> of time for completing the sync and in terms of write amplification
>> padded sectors for filling up to the minimal write size.
>>
>> In order to get a better understanding of the costs involved for syncs,
>> Add a sysfs attribute to pblk: padded_dist, showing a normalized
>> distribution of sectors padded. In order to facilitate measurements of
>> specific workloads during the lifetime of the pblk instance, the
>> distribution can be reset by writing 0 to the attribute.
>>
>> Do this by introducing counters for each possible padding:
>> {0..(minimal write size - 1)} and calculate the normalized distribution
>> when showing the attribute.
>>
>> Signed-off-by: Hans Holmberg 
>> Signed-off-by: Javier González 
>> ---
>>   drivers/lightnvm/pblk-init.c  | 16 --
>>   drivers/lightnvm/pblk-rb.c| 15 +-
>>   drivers/lightnvm/pblk-sysfs.c | 68
>> +++
>>   drivers/lightnvm/pblk.h   |  6 +++-
>>   4 files changed, 95 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>> index 7eedc5daebc8..a5e3510c0f38 100644
>> --- a/drivers/lightnvm/pblk-init.c
>> +++ b/drivers/lightnvm/pblk-init.c
>> @@ -921,6 +921,7 @@ static void pblk_free(struct pblk *pblk)
>>   {
>> pblk_luns_free(pblk);
>> pblk_lines_free(pblk);
>> +   kfree(pblk->pad_dist);
>> pblk_line_meta_free(pblk);
>> pblk_core_free(pblk);
>> pblk_l2p_free(pblk);
>> @@ -998,11 +999,13 @@ static void *pblk_init(struct nvm_tgt_dev *dev,
>> struct gendisk *tdisk,
>> pblk->pad_rst_wa = 0;
>> pblk->gc_rst_wa = 0;
>>   + atomic_long_set(>nr_flush, 0);
>> +   pblk->nr_flush_rst = 0;
>> +
>>   #ifdef CONFIG_NVM_DEBUG
>> atomic_long_set(>inflight_writes, 0);
>> atomic_long_set(>padded_writes, 0);
>> atomic_long_set(>padded_wb, 0);
>> -   atomic_long_set(>nr_flush, 0);
>> atomic_long_set(>req_writes, 0);
>> atomic_long_set(>sub_writes, 0);
>> atomic_long_set(>sync_writes, 0);
>> @@ -1034,10 +1037,17 @@ static void *pblk_init(struct nvm_tgt_dev *dev,
>> struct gendisk *tdisk,
>> goto fail_free_luns;
>> }
>>   + pblk->pad_dist = kzalloc((pblk->min_write_pgs - 1) *
>> sizeof(atomic64_t),
>> +GFP_KERNEL);
>> +   if (!pblk->pad_dist) {
>> +   ret = -ENOMEM;
>> +   goto fail_free_line_meta;
>> +   }
>> +
>> ret = pblk_core_init(pblk);
>> if (ret) {
>> pr_err("pblk: could not initialize core\n");
>> -   goto fail_free_line_meta;
>> +   goto fail_free_pad_dist;
>> }
>> ret = pblk_l2p_init(pblk);
>> @@ -1097,6 +1107,8 @@ static void *pblk_init(struct nvm_tgt_dev *dev,
>> struct gendisk *tdisk,
>> pblk_l2p_free(pblk);
>>   fail_free_core:
>> pblk_core_free(pblk);
>> +fail_free_pad_dist:
>> +   kfree(pblk->pad_dist);
>>   fail_free_line_meta:
>> pblk_line_meta_free(pblk);
>>   fail_free_luns:
>> diff --git a/drivers/lightnvm/pblk-rb.c b/drivers/lightnvm/pblk-rb.c
>> index 7044b5599cc4..264372d7b537 100644
>> --- a/drivers/lightnvm/pblk-rb.c
>> +++ b/drivers/lightnvm/pblk-rb.c
>> @@ -437,9 +437,7 @@ static int pblk_rb_may_write_flush(struct pblk_rb *rb,
>> unsigned int nr_entries,
>> if (bio->bi_opf & REQ_PREFLUSH) {
>> struct pblk *pblk = container_of(rb, struct pblk, rwb);
>>   -#ifdef CONFIG_NVM_DEBUG
>> atomic_long_inc(>nr_flush);
>> -#endif
>> if (pblk_rb_flush_point_set(>rwb, bio, mem))
>> *io_ret = NVM_IO_OK;
>> }
>> @@ -620,14 +618,17 @@ unsigned int pblk_rb_read_to_bio(struct pblk_rb *rb,
>> struct nvm_rq *rqd,
>> pr_err("pblk: could not pad page in write bio\n");
>> return NVM_IO_ERR;
>> }
>> +
>> +   if (pad < pblk->min_write_pgs)
>> +   atomic64_inc(>pad_dist[pad - 1]);
>> +   else
>> +   pr_warn("pblk: padding more than min. sectors\n");
>
>
> Curious, when would this happen? Would it be an implementation error or
> something a user did wrong?

This would be an implementation error - and this check is just a
safeguard to make sure we won't go out of bounds when updating the
statistics.

>
>
>> +
>> +   atomic64_add(pad, >pad_wa);
>> }
>>   - 

Re: [PATCH 4/5] lightnvm: pblk: add padding distribution sysfs attribute

[Matias Bjørling]

On 01/31/2018 03:06 AM, Javier González wrote:

From: Hans Holmberg 

When pblk receives a sync, all data up to that point in the write buffer
must be comitted to persistent storage, and as flash memory comes with a
minimal write size there is a significant cost involved both in terms
of time for completing the sync and in terms of write amplification
padded sectors for filling up to the minimal write size.

In order to get a better understanding of the costs involved for syncs,
Add a sysfs attribute to pblk: padded_dist, showing a normalized
distribution of sectors padded. In order to facilitate measurements of
specific workloads during the lifetime of the pblk instance, the
distribution can be reset by writing 0 to the attribute.

Do this by introducing counters for each possible padding:
{0..(minimal write size - 1)} and calculate the normalized distribution
when showing the attribute.

Signed-off-by: Hans Holmberg 
Signed-off-by: Javier González 
---
  drivers/lightnvm/pblk-init.c  | 16 --
  drivers/lightnvm/pblk-rb.c| 15 +-
  drivers/lightnvm/pblk-sysfs.c | 68 +++
  drivers/lightnvm/pblk.h   |  6 +++-
  4 files changed, 95 insertions(+), 10 deletions(-)

diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index 7eedc5daebc8..a5e3510c0f38 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -921,6 +921,7 @@ static void pblk_free(struct pblk *pblk)
  {
pblk_luns_free(pblk);
pblk_lines_free(pblk);
+   kfree(pblk->pad_dist);
pblk_line_meta_free(pblk);
pblk_core_free(pblk);
pblk_l2p_free(pblk);
@@ -998,11 +999,13 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct 
gendisk *tdisk,
pblk->pad_rst_wa = 0;
pblk->gc_rst_wa = 0;
  
+	atomic_long_set(>nr_flush, 0);

+   pblk->nr_flush_rst = 0;
+
  #ifdef CONFIG_NVM_DEBUG
atomic_long_set(>inflight_writes, 0);
atomic_long_set(>padded_writes, 0);
atomic_long_set(>padded_wb, 0);
-   atomic_long_set(>nr_flush, 0);
atomic_long_set(>req_writes, 0);
atomic_long_set(>sub_writes, 0);
atomic_long_set(>sync_writes, 0);
@@ -1034,10 +1037,17 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct 
gendisk *tdisk,
goto fail_free_luns;
}
  
+	pblk->pad_dist = kzalloc((pblk->min_write_pgs - 1) * sizeof(atomic64_t),

+GFP_KERNEL);
+   if (!pblk->pad_dist) {
+   ret = -ENOMEM;
+   goto fail_free_line_meta;
+   }
+
ret = pblk_core_init(pblk);
if (ret) {
pr_err("pblk: could not initialize core\n");
-   goto fail_free_line_meta;
+   goto fail_free_pad_dist;
}
  
  	ret = pblk_l2p_init(pblk);

@@ -1097,6 +1107,8 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct 
gendisk *tdisk,
pblk_l2p_free(pblk);
  fail_free_core:
pblk_core_free(pblk);
+fail_free_pad_dist:
+   kfree(pblk->pad_dist);
  fail_free_line_meta:
pblk_line_meta_free(pblk);
  fail_free_luns:
diff --git a/drivers/lightnvm/pblk-rb.c b/drivers/lightnvm/pblk-rb.c
index 7044b5599cc4..264372d7b537 100644
--- a/drivers/lightnvm/pblk-rb.c
+++ b/drivers/lightnvm/pblk-rb.c
@@ -437,9 +437,7 @@ static int pblk_rb_may_write_flush(struct pblk_rb *rb, 
unsigned int nr_entries,
if (bio->bi_opf & REQ_PREFLUSH) {
struct pblk *pblk = container_of(rb, struct pblk, rwb);
  
-#ifdef CONFIG_NVM_DEBUG

atomic_long_inc(>nr_flush);
-#endif
if (pblk_rb_flush_point_set(>rwb, bio, mem))
*io_ret = NVM_IO_OK;
}
@@ -620,14 +618,17 @@ unsigned int pblk_rb_read_to_bio(struct pblk_rb *rb, 
struct nvm_rq *rqd,
pr_err("pblk: could not pad page in write bio\n");
return NVM_IO_ERR;
}
+
+   if (pad < pblk->min_write_pgs)
+   atomic64_inc(>pad_dist[pad - 1]);
+   else
+   pr_warn("pblk: padding more than min. sectors\n");


Curious, when would this happen? Would it be an implementation error or 
something a user did wrong?



+
+   atomic64_add(pad, >pad_wa);
}
  
-	atomic64_add(pad, &((struct pblk *)

-   (container_of(rb, struct pblk, rwb)))->pad_wa);
-
  #ifdef CONFIG_NVM_DEBUG
-   atomic_long_add(pad, &((struct pblk *)
-   (container_of(rb, struct pblk, rwb)))->padded_writes);
+   atomic_long_add(pad, >padded_writes);
  #endif
  
  	return NVM_IO_OK;

diff --git a/drivers/lightnvm/pblk-sysfs.c b/drivers/lightnvm/pblk-sysfs.c
index 4804bbd32d5f..f902ac00d071 100644
--- a/drivers/lightnvm/pblk-sysfs.c
+++ b/drivers/lightnvm/pblk-sysfs.c
@@ -341,6 +341,38 @@ static ssize_t