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

[Igor Konopko <igor.j.konopko@xxxxxxxxx>]

On 06.03.2019 15:19, Hans Holmberg wrote:
> 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.

Let's drop this patch then - it is not a critical modification, we can 
definitely leave without it.