On Wed, Mar 6, 2019 at 8:28 AM Javier González <javier@xxxxxxxxxxx> wrote:
>
> > On 5 Mar 2019, at 14.51, Igor Konopko <igor.j.konopko@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxx>
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.
