[PATCH] lightnvm: pblk: stop taking the free lock in in pblk_lines_free

[PATCH] lightnvm: pblk: stop taking the free lock in in pblk_lines_free

[hans]

From: Hans Holmberg <hans.holmberg@cnexlabs.com>

pblk_line_meta_free might sleep (it can end up calling vfree, depending
on how we allocate lba lists), and this can lead to a BUG()
if we wake up on a different cpu and release the lock.

As there is no point of grabbing the free lock when pblk has shut down,
remove the lock.

Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
---
 drivers/lightnvm/pblk-init.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index f9a3e47b6a93..eb0135c77805 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -584,14 +584,12 @@ static void pblk_lines_free(struct pblk *pblk)
 	struct pblk_line *line;
 	int i;
 
-	spin_lock(&l_mg->free_lock);
 	for (i = 0; i < l_mg->nr_lines; i++) {
 		line = &pblk->lines[i];
 
 		pblk_line_free(line);
 		pblk_line_meta_free(l_mg, line);
 	}
-	spin_unlock(&l_mg->free_lock);
 
 	pblk_line_mg_free(pblk);
 
-- 
2.17.1

Re: [PATCH] lightnvm: pblk: stop taking the free lock in in pblk_lines_free

[Matias Bjørling]

On 1/22/19 11:15 AM, hans@owltronix.com wrote:
> From: Hans Holmberg <hans.holmberg@cnexlabs.com>
> 
> pblk_line_meta_free might sleep (it can end up calling vfree, depending
> on how we allocate lba lists), and this can lead to a BUG()
> if we wake up on a different cpu and release the lock.
> 
> As there is no point of grabbing the free lock when pblk has shut down,
> remove the lock.
> 
> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
> ---
>   drivers/lightnvm/pblk-init.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
> index f9a3e47b6a93..eb0135c77805 100644
> --- a/drivers/lightnvm/pblk-init.c
> +++ b/drivers/lightnvm/pblk-init.c
> @@ -584,14 +584,12 @@ static void pblk_lines_free(struct pblk *pblk)
>   	struct pblk_line *line;
>   	int i;
>   
> -	spin_lock(&l_mg->free_lock);
>   	for (i = 0; i < l_mg->nr_lines; i++) {
>   		line = &pblk->lines[i];
>   
>   		pblk_line_free(line);
>   		pblk_line_meta_free(l_mg, line);
>   	}
> -	spin_unlock(&l_mg->free_lock);
>   
>   	pblk_line_mg_free(pblk);
>   
> 

Thanks Hans. Applied for 5.1.

Re: [PATCH] lightnvm: pblk: stop taking the free lock in in pblk_lines_free

[Javier González]

[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]

> On 22 Jan 2019, at 11.15, hans@owltronix.com wrote:
> 
> From: Hans Holmberg <hans.holmberg@cnexlabs.com>
> 
> pblk_line_meta_free might sleep (it can end up calling vfree, depending
> on how we allocate lba lists), and this can lead to a BUG()
> if we wake up on a different cpu and release the lock.
> 
> As there is no point of grabbing the free lock when pblk has shut down,
> remove the lock.
> 
> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
> ---
> drivers/lightnvm/pblk-init.c | 2 --
> 1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
> index f9a3e47b6a93..eb0135c77805 100644
> --- a/drivers/lightnvm/pblk-init.c
> +++ b/drivers/lightnvm/pblk-init.c
> @@ -584,14 +584,12 @@ static void pblk_lines_free(struct pblk *pblk)
> 	struct pblk_line *line;
> 	int i;
> 
> -	spin_lock(&l_mg->free_lock);
> 	for (i = 0; i < l_mg->nr_lines; i++) {
> 		line = &pblk->lines[i];
> 
> 		pblk_line_free(line);
> 		pblk_line_meta_free(l_mg, line);
> 	}
> -	spin_unlock(&l_mg->free_lock);
> 
> 	pblk_line_mg_free(pblk);
> 
> --
> 2.17.1

Can you add a comment too indicating that this is only safe on a single
threaded shutdown?

Otherwise the patch looks good.

Reviewed-by: Javier González <javier@javigon.com>


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] lightnvm: pblk: stop taking the free lock in in pblk_lines_free

[Hans Holmberg]

On Thu, Jan 24, 2019 at 2:19 PM Javier González <javier@javigon.com> wrote:
>
> > On 22 Jan 2019, at 11.15, hans@owltronix.com wrote:
> >
> > From: Hans Holmberg <hans.holmberg@cnexlabs.com>
> >
> > pblk_line_meta_free might sleep (it can end up calling vfree, depending
> > on how we allocate lba lists), and this can lead to a BUG()
> > if we wake up on a different cpu and release the lock.
> >
> > As there is no point of grabbing the free lock when pblk has shut down,
> > remove the lock.
> >
> > Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
> > ---
> > drivers/lightnvm/pblk-init.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
> > index f9a3e47b6a93..eb0135c77805 100644
> > --- a/drivers/lightnvm/pblk-init.c
> > +++ b/drivers/lightnvm/pblk-init.c
> > @@ -584,14 +584,12 @@ static void pblk_lines_free(struct pblk *pblk)
> >       struct pblk_line *line;
> >       int i;
> >
> > -     spin_lock(&l_mg->free_lock);
> >       for (i = 0; i < l_mg->nr_lines; i++) {
> >               line = &pblk->lines[i];
> >
> >               pblk_line_free(line);
> >               pblk_line_meta_free(l_mg, line);
> >       }
> > -     spin_unlock(&l_mg->free_lock);
> >
> >       pblk_line_mg_free(pblk);
> >
> > --
> > 2.17.1
>
> Can you add a comment too indicating that this is only safe on a single
> threaded shutdown?

To be able to free the lines, we need have stopped anything accessing
the lines first. That seems obvious to me.

It would be nice to make a pass over the code and document pblk's
locking(and other concurrency handling, like the line krefs) though.

Thanks,
Hans

>
> Otherwise the patch looks good.
>
> Reviewed-by: Javier González <javier@javigon.com>
>

Re: [PATCH] lightnvm: pblk: stop taking the free lock in in pblk_lines_free

[Javier González]

[-- Attachment #1: Type: text/plain, Size: 2767 bytes --]


> On 25 Jan 2019, at 10.15, Hans Holmberg <hans.ml.holmberg@owltronix.com> wrote:
> 
> On Thu, Jan 24, 2019 at 2:19 PM Javier González <javier@javigon.com> wrote:
>>> On 22 Jan 2019, at 11.15, hans@owltronix.com wrote:
>>> 
>>> From: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>> 
>>> pblk_line_meta_free might sleep (it can end up calling vfree, depending
>>> on how we allocate lba lists), and this can lead to a BUG()
>>> if we wake up on a different cpu and release the lock.
>>> 
>>> As there is no point of grabbing the free lock when pblk has shut down,
>>> remove the lock.
>>> 
>>> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>> ---
>>> drivers/lightnvm/pblk-init.c | 2 --
>>> 1 file changed, 2 deletions(-)
>>> 
>>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>>> index f9a3e47b6a93..eb0135c77805 100644
>>> --- a/drivers/lightnvm/pblk-init.c
>>> +++ b/drivers/lightnvm/pblk-init.c
>>> @@ -584,14 +584,12 @@ static void pblk_lines_free(struct pblk *pblk)
>>>      struct pblk_line *line;
>>>      int i;
>>> 
>>> -     spin_lock(&l_mg->free_lock);
>>>      for (i = 0; i < l_mg->nr_lines; i++) {
>>>              line = &pblk->lines[i];
>>> 
>>>              pblk_line_free(line);
>>>              pblk_line_meta_free(l_mg, line);
>>>      }
>>> -     spin_unlock(&l_mg->free_lock);
>>> 
>>>      pblk_line_mg_free(pblk);
>>> 
>>> --
>>> 2.17.1
>> 
>> Can you add a comment too indicating that this is only safe on a single
>> threaded shutdown?
> 
> To be able to free the lines, we need have stopped anything accessing
> the lines first. That seems obvious to me.
> 

The reason I mention this is that there is assumptions on the shutdown
logic that the line freeup will be single threaded - you can see that we
do not lock pblk_line_mg_free() either, for the same reason as you are
removing this lock. We can do this is only because we have a single open
line at the time (which in close down we fill up in parallel to speed up
the process BTW). If we have more open lines, it would be desirable to
close them in parallel. At this point we either have a sync point when
they are all closed and then free, or we allow them to free themselves.
In the second case, locking will be necessary.

IMHO, a comment does not hurt.

> It would be nice to make a pass over the code and document pblk's
> locking(and other concurrency handling, like the line krefs) though.
> 

True that - krefs specially would deserve some documentation. Let me see
if I can allocate some time the following weeks to write up some
documentation.

> Thanks,
> Hans
> 
>> Otherwise the patch looks good.
>> 
>> Reviewed-by: Javier González <javier@javigon.com>

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]