| Message ID | 20170921112620.GA28524@dhcp-216.srv.tuxera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> On 21 Sep 2017, at 13.26, Rakesh Pandit <rakesh@tuxera.com> wrote: > > It seems pblk_dealloc_page would race against pblk_alloc_pages for > line bitmap for sector allocation. The chances are very low but might > as well protect the bitmap properly. It's not even in fast path. > > Signed-off-by: Rakesh Pandit <rakesh@tuxera.com> > --- > drivers/lightnvm/pblk-core.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c > index a230125..b92eabc 100644 > --- a/drivers/lightnvm/pblk-core.c > +++ b/drivers/lightnvm/pblk-core.c > @@ -502,12 +502,14 @@ void pblk_dealloc_page(struct pblk *pblk, struct pblk_line *line, int nr_secs) > u64 addr; > int i; > > + spin_lock(&line->lock); > addr = find_next_zero_bit(line->map_bitmap, > pblk->lm.sec_per_line, line->cur_sec); > line->cur_sec = addr - nr_secs; > > for (i = 0; i < nr_secs; i++, line->cur_sec--) > WARN_ON(!test_and_clear_bit(line->cur_sec, line->map_bitmap)); > + spin_lock(&line->lock); > } > > u64 __pblk_alloc_page(struct pblk *pblk, struct pblk_line *line, int nr_secs) > -- > 2.5.0 Looks good. The reason not to have locks here was that the caller is always on the write thread - who did the allocation -, since it is error handling. So there is no protection needed. In any case, it is better to have it since it is implemented as a helper function. Reviewed-by: Javier González <javier@cnexlabs.com>
On 09/22/2017 10:49 AM, Javier González wrote: >> On 21 Sep 2017, at 13.26, Rakesh Pandit <rakesh@tuxera.com> wrote: >> >> It seems pblk_dealloc_page would race against pblk_alloc_pages for >> line bitmap for sector allocation. The chances are very low but might >> as well protect the bitmap properly. It's not even in fast path. >> >> Signed-off-by: Rakesh Pandit <rakesh@tuxera.com> >> --- >> drivers/lightnvm/pblk-core.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c >> index a230125..b92eabc 100644 >> --- a/drivers/lightnvm/pblk-core.c >> +++ b/drivers/lightnvm/pblk-core.c >> @@ -502,12 +502,14 @@ void pblk_dealloc_page(struct pblk *pblk, struct pblk_line *line, int nr_secs) >> u64 addr; >> int i; >> >> + spin_lock(&line->lock); >> addr = find_next_zero_bit(line->map_bitmap, >> pblk->lm.sec_per_line, line->cur_sec); >> line->cur_sec = addr - nr_secs; >> >> for (i = 0; i < nr_secs; i++, line->cur_sec--) >> WARN_ON(!test_and_clear_bit(line->cur_sec, line->map_bitmap)); >> + spin_lock(&line->lock); >> } >> >> u64 __pblk_alloc_page(struct pblk *pblk, struct pblk_line *line, int nr_secs) >> -- >> 2.5.0 > > Looks good. The reason not to have locks here was that the caller is > always on the write thread - who did the allocation -, since it is error > handling. So there is no protection needed. In any case, it is better to > have it since it is implemented as a helper function. > > > Reviewed-by: Javier González <javier@cnexlabs.com> > Thanks, I picked it up.
> On 21 Sep 2017, at 13.26, Rakesh Pandit <rakesh@tuxera.com> wrote: > > It seems pblk_dealloc_page would race against pblk_alloc_pages for > line bitmap for sector allocation. The chances are very low but might > as well protect the bitmap properly. It's not even in fast path. > > Signed-off-by: Rakesh Pandit <rakesh@tuxera.com> > --- > drivers/lightnvm/pblk-core.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c > index a230125..b92eabc 100644 > --- a/drivers/lightnvm/pblk-core.c > +++ b/drivers/lightnvm/pblk-core.c > @@ -502,12 +502,14 @@ void pblk_dealloc_page(struct pblk *pblk, struct pblk_line *line, int nr_secs) > u64 addr; > int i; > > + spin_lock(&line->lock); > addr = find_next_zero_bit(line->map_bitmap, > pblk->lm.sec_per_line, line->cur_sec); > line->cur_sec = addr - nr_secs; > > for (i = 0; i < nr_secs; i++, line->cur_sec--) > WARN_ON(!test_and_clear_bit(line->cur_sec, line->map_bitmap)); > + spin_lock(&line->lock); Changed this to spin_unlock() when picking it up. Javier.
diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c index a230125..b92eabc 100644 --- a/drivers/lightnvm/pblk-core.c +++ b/drivers/lightnvm/pblk-core.c @@ -502,12 +502,14 @@ void pblk_dealloc_page(struct pblk *pblk, struct pblk_line *line, int nr_secs) u64 addr; int i; + spin_lock(&line->lock); addr = find_next_zero_bit(line->map_bitmap, pblk->lm.sec_per_line, line->cur_sec); line->cur_sec = addr - nr_secs; for (i = 0; i < nr_secs; i++, line->cur_sec--) WARN_ON(!test_and_clear_bit(line->cur_sec, line->map_bitmap)); + spin_lock(&line->lock); } u64 __pblk_alloc_page(struct pblk *pblk, struct pblk_line *line, int nr_secs)
It seems pblk_dealloc_page would race against pblk_alloc_pages for line bitmap for sector allocation. The chances are very low but might as well protect the bitmap properly. It's not even in fast path. Signed-off-by: Rakesh Pandit <rakesh@tuxera.com> --- drivers/lightnvm/pblk-core.c | 2 ++ 1 file changed, 2 insertions(+)