| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 6 Feb 2018 11:50:27 +0100
From: Matias Bjørling <mb@...htnvm.io>
To: Hans Holmberg <hans.ml.holmberg@...tronix.com>
Cc: Javier González <jg@...htnvm.io>,
linux-block@...r.kernel.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Hans Holmberg <hans.holmberg@...xlabs.com>,
Javier González <javier@...xlabs.com>
Subject: Re: [PATCH 4/5] lightnvm: pblk: add padding distribution sysfs
attribute
On 02/06/2018 10:27 AM, Hans Holmberg wrote:
> Hi Matias,
>
> On Wed, Jan 31, 2018 at 9:44 AM, Matias Bjørling <mb@...htnvm.io> wrote:
>> On 01/31/2018 03:06 AM, Javier González wrote:
>>>
>>> From: Hans Holmberg <hans.holmberg@...xlabs.com>
>>>
>>> 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 <hans.holmberg@...xlabs.com>
>>> Signed-off-by: Javier González <javier@...xlabs.com>
>>> ---
>>> 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(&pblk->nr_flush, 0);
>>> + pblk->nr_flush_rst = 0;
>>> +
>>> #ifdef CONFIG_NVM_DEBUG
>>> atomic_long_set(&pblk->inflight_writes, 0);
>>> atomic_long_set(&pblk->padded_writes, 0);
>>> atomic_long_set(&pblk->padded_wb, 0);
>>> - atomic_long_set(&pblk->nr_flush, 0);
>>> atomic_long_set(&pblk->req_writes, 0);
>>> atomic_long_set(&pblk->sub_writes, 0);
>>> atomic_long_set(&pblk->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(&pblk->nr_flush);
>>> -#endif
>>> if (pblk_rb_flush_point_set(&pblk->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(&pblk->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?
<snip>
>>
>> Looks good to me. Let me know if you want to send a new patch, or if I may
>> make the change when I pick it up.
>
> Thanks for the review, i saw that you already reworked the patch a bit
> on your core branch.
> However, i found a build issue when building for i386, so i'll fix
> that and submit a V2(that includes your update)
Ok. I assume this is the kbuild mail I asked about a couple of days ago.
I'll wait for v2.
Powered by blists - more mailing lists