| Message ID | 20190227171442.11853-5-igor.j.konopko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lightnvm: bugfixes and improvements | expand |
> On 27 Feb 2019, at 18.14, Igor Konopko <igor.j.konopko@intel.com> wrote: > > Currently in case of error returned by pblk_gc_line > to pblk_gc_read we leave current line unassigned > from all the lists. This patch fixes that issue. > > Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> > --- > drivers/lightnvm/pblk-gc.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c > index 511ed0d5333c..533da6ea3e15 100644 > --- a/drivers/lightnvm/pblk-gc.c > +++ b/drivers/lightnvm/pblk-gc.c > @@ -361,8 +361,13 @@ static int pblk_gc_read(struct pblk *pblk) > > pblk_gc_kick(pblk); > > - if (pblk_gc_line(pblk, line)) > + if (pblk_gc_line(pblk, line)) { > pblk_err(pblk, "failed to GC line %d\n", line->id); > + /* rollback */ > + spin_lock(&gc->r_lock); > + list_add_tail(&line->list, &gc->r_list); > + spin_unlock(&gc->r_lock); > + } > > return 0; > } > -- > 2.17.1 Looks good to me. Reviewed-by: Javier González <javier@javigon.com>
Did you ever see this in the wild? The only time pblk_gc_line returns an error is if kmalloc(sizeof(struct pblk_line_ws), GFP_KERNEL); fails, and then we're in real trouble :) Reviewed-by: Hans Holmberg <hans.holmberg@cnexlabs.com> On Mon, Mar 4, 2019 at 8:38 AM Javier González <javier@javigon.com> wrote: > > > On 27 Feb 2019, at 18.14, Igor Konopko <igor.j.konopko@intel.com> wrote: > > > > Currently in case of error returned by pblk_gc_line > > to pblk_gc_read we leave current line unassigned > > from all the lists. This patch fixes that issue. > > > > Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> > > --- > > drivers/lightnvm/pblk-gc.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c > > index 511ed0d5333c..533da6ea3e15 100644 > > --- a/drivers/lightnvm/pblk-gc.c > > +++ b/drivers/lightnvm/pblk-gc.c > > @@ -361,8 +361,13 @@ static int pblk_gc_read(struct pblk *pblk) > > > > pblk_gc_kick(pblk); > > > > - if (pblk_gc_line(pblk, line)) > > + if (pblk_gc_line(pblk, line)) { > > pblk_err(pblk, "failed to GC line %d\n", line->id); > > + /* rollback */ > > + spin_lock(&gc->r_lock); > > + list_add_tail(&line->list, &gc->r_list); > > + spin_unlock(&gc->r_lock); > > + } > > > > return 0; > > } > > -- > > 2.17.1 > > Looks good to me. > > Reviewed-by: Javier González <javier@javigon.com>
On 04.03.2019 09:44, Hans Holmberg wrote: > Did you ever see this in the wild? > The only time pblk_gc_line returns an error is if > kmalloc(sizeof(struct pblk_line_ws), GFP_KERNEL); fails, and then > we're in real trouble :) I never saw this in real life, but still probably good to have a "proper" handling - since we already have "some" handling. > > Reviewed-by: Hans Holmberg <hans.holmberg@cnexlabs.com> > > On Mon, Mar 4, 2019 at 8:38 AM Javier González <javier@javigon.com> wrote: >> >>> On 27 Feb 2019, at 18.14, Igor Konopko <igor.j.konopko@intel.com> wrote: >>> >>> Currently in case of error returned by pblk_gc_line >>> to pblk_gc_read we leave current line unassigned >>> from all the lists. This patch fixes that issue. >>> >>> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> >>> --- >>> drivers/lightnvm/pblk-gc.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c >>> index 511ed0d5333c..533da6ea3e15 100644 >>> --- a/drivers/lightnvm/pblk-gc.c >>> +++ b/drivers/lightnvm/pblk-gc.c >>> @@ -361,8 +361,13 @@ static int pblk_gc_read(struct pblk *pblk) >>> >>> pblk_gc_kick(pblk); >>> >>> - if (pblk_gc_line(pblk, line)) >>> + if (pblk_gc_line(pblk, line)) { >>> pblk_err(pblk, "failed to GC line %d\n", line->id); >>> + /* rollback */ >>> + spin_lock(&gc->r_lock); >>> + list_add_tail(&line->list, &gc->r_list); >>> + spin_unlock(&gc->r_lock); >>> + } >>> >>> return 0; >>> } >>> -- >>> 2.17.1 >> >> Looks good to me. >> >> Reviewed-by: Javier González <javier@javigon.com>
On Mon, Mar 4, 2019 at 1:39 PM Igor Konopko <igor.j.konopko@intel.com> wrote: > > > > On 04.03.2019 09:44, Hans Holmberg wrote: > > Did you ever see this in the wild? > > The only time pblk_gc_line returns an error is if > > kmalloc(sizeof(struct pblk_line_ws), GFP_KERNEL); fails, and then > > we're in real trouble :) > > I never saw this in real life, but still probably good to have a > "proper" handling - since we already have "some" handling. Yes! > > > > > Reviewed-by: Hans Holmberg <hans.holmberg@cnexlabs.com> > > > > On Mon, Mar 4, 2019 at 8:38 AM Javier González <javier@javigon.com> wrote: > >> > >>> On 27 Feb 2019, at 18.14, Igor Konopko <igor.j.konopko@intel.com> wrote: > >>> > >>> Currently in case of error returned by pblk_gc_line > >>> to pblk_gc_read we leave current line unassigned > >>> from all the lists. This patch fixes that issue. > >>> > >>> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> > >>> --- > >>> drivers/lightnvm/pblk-gc.c | 7 ++++++- > >>> 1 file changed, 6 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c > >>> index 511ed0d5333c..533da6ea3e15 100644 > >>> --- a/drivers/lightnvm/pblk-gc.c > >>> +++ b/drivers/lightnvm/pblk-gc.c > >>> @@ -361,8 +361,13 @@ static int pblk_gc_read(struct pblk *pblk) > >>> > >>> pblk_gc_kick(pblk); > >>> > >>> - if (pblk_gc_line(pblk, line)) > >>> + if (pblk_gc_line(pblk, line)) { > >>> pblk_err(pblk, "failed to GC line %d\n", line->id); > >>> + /* rollback */ > >>> + spin_lock(&gc->r_lock); > >>> + list_add_tail(&line->list, &gc->r_list); > >>> + spin_unlock(&gc->r_lock); > >>> + } > >>> > >>> return 0; > >>> } > >>> -- > >>> 2.17.1 > >> > >> Looks good to me. > >> > >> Reviewed-by: Javier González <javier@javigon.com>
On 2/27/19 6:14 PM, Igor Konopko wrote: > Currently in case of error returned by pblk_gc_line > to pblk_gc_read we leave current line unassigned > from all the lists. This patch fixes that issue. > > Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> > --- > drivers/lightnvm/pblk-gc.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c > index 511ed0d5333c..533da6ea3e15 100644 > --- a/drivers/lightnvm/pblk-gc.c > +++ b/drivers/lightnvm/pblk-gc.c > @@ -361,8 +361,13 @@ static int pblk_gc_read(struct pblk *pblk) > > pblk_gc_kick(pblk); > > - if (pblk_gc_line(pblk, line)) > + if (pblk_gc_line(pblk, line)) { > pblk_err(pblk, "failed to GC line %d\n", line->id); > + /* rollback */ > + spin_lock(&gc->r_lock); > + list_add_tail(&line->list, &gc->r_list); > + spin_unlock(&gc->r_lock); > + } > > return 0; > } > Thanks Igor. I've reworded your description a bit. Applied for 5.2.
diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c index 511ed0d5333c..533da6ea3e15 100644 --- a/drivers/lightnvm/pblk-gc.c +++ b/drivers/lightnvm/pblk-gc.c @@ -361,8 +361,13 @@ static int pblk_gc_read(struct pblk *pblk) pblk_gc_kick(pblk); - if (pblk_gc_line(pblk, line)) + if (pblk_gc_line(pblk, line)) { pblk_err(pblk, "failed to GC line %d\n", line->id); + /* rollback */ + spin_lock(&gc->r_lock); + list_add_tail(&line->list, &gc->r_list); + spin_unlock(&gc->r_lock); + } return 0; }
Currently in case of error returned by pblk_gc_line to pblk_gc_read we leave current line unassigned from all the lists. This patch fixes that issue. Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> --- drivers/lightnvm/pblk-gc.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)