RE: [GIT PULL 18/20] lightnvm: pblk: handle case when mw_cunits equals to 0

From: Dziegielewski, Marcin
Date: Mon Jun 04 2018 - 06:09:36 EST



Frist of all I want to say sorry for late response - I was on holiday.

> From: Javier Gonzalez [mailto:javier@xxxxxxxxxxxx]
> Sent: Monday, May 28, 2018 1:03 PM
> To: Matias BjÃrling <mb@xxxxxxxxxxx>
> Cc: Jens Axboe <axboe@xxxxxx>; linux-block@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Dziegielewski, Marcin
> <marcin.dziegielewski@xxxxxxxxx>; Konopko, Igor J
> <igor.j.konopko@xxxxxxxxx>
> Subject: Re: [GIT PULL 18/20] lightnvm: pblk: handle case when mw_cunits
> equals to 0
>
> > On 28 May 2018, at 10.58, Matias BjÃrling <mb@xxxxxxxxxxx> wrote:
> >
> > From: Marcin Dziegielewski <marcin.dziegielewski@xxxxxxxxx>
> >
> > Some devices can expose mw_cunits equal to 0, it can cause creation of
> > too small write buffer and cause performance to drop on write
> > workloads.
> >
> > To handle that, we use the default value for MLC and beacause it
> > covers both 1.2 and 2.0 OC specification, setting up mw_cunits in
> > nvme_nvm_setup_12 function isn't longer necessary.
> >
> > Signed-off-by: Marcin Dziegielewski <marcin.dziegielewski@xxxxxxxxx>
> > Signed-off-by: Igor Konopko <igor.j.konopko@xxxxxxxxx>
> > Signed-off-by: Matias BjÃrling <mb@xxxxxxxxxxx>
> > ---
> > drivers/lightnvm/pblk-init.c | 10 +++++++++-
> > drivers/nvme/host/lightnvm.c | 1 -
> > 2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/lightnvm/pblk-init.c
> > b/drivers/lightnvm/pblk-init.c index d65d2f972ccf..0f277744266b 100644
> > --- a/drivers/lightnvm/pblk-init.c
> > +++ b/drivers/lightnvm/pblk-init.c
> > @@ -356,7 +356,15 @@ static int pblk_core_init(struct pblk *pblk)
> > atomic64_set(&pblk->nr_flush, 0);
> > pblk->nr_flush_rst = 0;
> >
> > - pblk->pgs_in_buffer = geo->mw_cunits * geo->all_luns;
> > + if (geo->mw_cunits) {
> > + pblk->pgs_in_buffer = geo->mw_cunits * geo->all_luns;
> > + } else {
> > + pblk->pgs_in_buffer = (geo->ws_opt << 3) * geo->all_luns;
> > + /*
> > + * Some devices can expose mw_cunits equal to 0, so let's
> use
> > + * here default safe value for MLC.
> > + */
> > + }
> >
> > pblk->min_write_pgs = geo->ws_opt * (geo->csecs / PAGE_SIZE);
> > max_write_ppas = pblk->min_write_pgs * geo->all_luns; diff --git
> > a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c index
> > 41279da799ed..c747792da915 100644
> > --- a/drivers/nvme/host/lightnvm.c
> > +++ b/drivers/nvme/host/lightnvm.c
> > @@ -338,7 +338,6 @@ static int nvme_nvm_setup_12(struct
> nvme_nvm_id12
> > *id,
> >
> > geo->ws_min = sec_per_pg;
> > geo->ws_opt = sec_per_pg;
> > - geo->mw_cunits = geo->ws_opt << 3; /* default to MLC safe values
> */
> >
> > /* Do not impose values for maximum number of open blocks as it is
> > * unspecified in 1.2. Users of 1.2 must be aware of this and
> > eventually
> > --
> > 2.11.0
>
> By doing this, 1.2 future users (beyond pblk), will fail to have a valid
> mw_cunits value. It's ok to deal with the 0 case in pblk, but I believe that we
> should have the default value for 1.2 either way.

I'm not sure. From my understanding, setting of default value was workaround for pblk case, am I right ?. In my opinion any user of 1.2 spec should be aware that there is not mw_cunit value. From my point of view, leaving here 0 (and decision what do with it to lightnvm user) is more safer way, but maybe I'm wrong. I believe that it is topic to wider discussion with maintainers.

>
> A more generic way of doing this would be to have a default value for
> 2.0 too, in case mw_cunits is reported as 0.

Since 0 is correct value and users can make different decisions based on it, I think we shouldn't overwrite it by default value. Is it make sense?
>
> Javier

Thanks,
Marcin