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

[Igor Konopko <igor.j.konopko@intel.com>]

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.