[PATCH] lightnvm: pblk: use vfree_atomic when freeing line metadata

[PATCH] lightnvm: pblk: use vfree_atomic when freeing line metadata

[Hans Holmberg]

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

The commit bf22e37a6413 ("mm: add vfree_atomic()") made vfree unsafe to
call in atomic context (unless the call came from an interrupt) and
introduced vfree_atomic that is safe to call in atomic context.

So, since we're holding locks when freeing line metadata, we need to
use the atomic version of vfree.

Fix this by introducing an atomic variant of pblk_mfree and
switching to that in pblk_line_meta_free.

Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
---

The patch is for:
https://github.com/OpenChannelSSD/linux branch for-4.15/pblk

 drivers/lightnvm/pblk-init.c | 3 ++-
 drivers/lightnvm/pblk.h      | 8 ++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index c452478..3a191a6 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -396,7 +396,8 @@ static void pblk_line_meta_free(struct pblk *pblk)
 	spin_lock(&l_mg->free_lock);
 	for (i = 0; i < PBLK_DATA_LINES; i++) {
 		kfree(l_mg->sline_meta[i]);
-		pblk_mfree(l_mg->eline_meta[i]->buf, l_mg->emeta_alloc_type);
+		pblk_mfree_atomic(l_mg->eline_meta[i]->buf,
+				  l_mg->emeta_alloc_type);
 		kfree(l_mg->eline_meta[i]);
 	}
 	spin_unlock(&l_mg->free_lock);
diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
index 03965da..93f98e3 100644
--- a/drivers/lightnvm/pblk.h
+++ b/drivers/lightnvm/pblk.h
@@ -881,6 +881,14 @@ static inline void pblk_mfree(void *ptr, int type)
 		vfree(ptr);
 }
 
+static inline void pblk_mfree_atomic(void *ptr, int type)
+{
+	if (type == PBLK_KMALLOC_META)
+		kfree(ptr);
+	else
+		vfree_atomic(ptr);
+}
+
 static inline struct nvm_rq *nvm_rq_from_c_ctx(void *c_ctx)
 {
 	return c_ctx - sizeof(struct nvm_rq);
-- 
2.7.4

Re: [PATCH] lightnvm: pblk: use vfree_atomic when freeing line metadata

[Andrey Ryabinin]

On 10/03/2017 04:48 PM, Hans Holmberg wrote:
> From: Hans Holmberg <hans.holmberg@cnexlabs.com>
> 
> The commit bf22e37a6413 ("mm: add vfree_atomic()") made vfree unsafe to
> call in atomic context (unless the call came from an interrupt) and
> introduced vfree_atomic that is safe to call in atomic context.
> 
> So, since we're holding locks when freeing line metadata, we need to
> use the atomic version of vfree.
> 
> Fix this by introducing an atomic variant of pblk_mfree and
> switching to that in pblk_line_meta_free.
> 
> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
> ---
> 
> The patch is for:
> https://github.com/OpenChannelSSD/linux branch for-4.15/pblk
> 
>  drivers/lightnvm/pblk-init.c | 3 ++-
>  drivers/lightnvm/pblk.h      | 8 ++++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
> index c452478..3a191a6 100644
> --- a/drivers/lightnvm/pblk-init.c
> +++ b/drivers/lightnvm/pblk-init.c
> @@ -396,7 +396,8 @@ static void pblk_line_meta_free(struct pblk *pblk)
>  	spin_lock(&l_mg->free_lock);

What's the point in holding ->free_lock here? It seems like it could be just dropped.

>  	for (i = 0; i < PBLK_DATA_LINES; i++) {
>  		kfree(l_mg->sline_meta[i]);
> -		pblk_mfree(l_mg->eline_meta[i]->buf, l_mg->emeta_alloc_type);
> +		pblk_mfree_atomic(l_mg->eline_meta[i]->buf,
> +				  l_mg->emeta_alloc_type);
>  		kfree(l_mg->eline_meta[i]);
>  	}
>  	spin_unlock(&l_mg->free_lock);
> diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
> index 03965da..93f98e3 100644
> --- a/drivers/lightnvm/pblk.h
> +++ b/drivers/lightnvm/pblk.h
> @@ -881,6 +881,14 @@ static inline void pblk_mfree(void *ptr, int type)
>  		vfree(ptr);
>  }
>  
> +static inline void pblk_mfree_atomic(void *ptr, int type)
> +{
> +	if (type == PBLK_KMALLOC_META)
> +		kfree(ptr);
> +	else
> +		vfree_atomic(ptr);
> +}
> +
>  static inline struct nvm_rq *nvm_rq_from_c_ctx(void *c_ctx)
>  {
>  	return c_ctx - sizeof(struct nvm_rq);
> 

Re: [PATCH] lightnvm: pblk: use vfree_atomic when freeing line metadata

[Javier González]

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

> On 3 Oct 2017, at 16.07, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> 
> 
> 
> On 10/03/2017 04:48 PM, Hans Holmberg wrote:
>> From: Hans Holmberg <hans.holmberg@cnexlabs.com>
>> 
>> The commit bf22e37a6413 ("mm: add vfree_atomic()") made vfree unsafe to
>> call in atomic context (unless the call came from an interrupt) and
>> introduced vfree_atomic that is safe to call in atomic context.
>> 
>> So, since we're holding locks when freeing line metadata, we need to
>> use the atomic version of vfree.
>> 
>> Fix this by introducing an atomic variant of pblk_mfree and
>> switching to that in pblk_line_meta_free.
>> 
>> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
>> ---
>> 
>> The patch is for:
>> https://github.com/OpenChannelSSD/linux branch for-4.15/pblk
>> 
>> drivers/lightnvm/pblk-init.c | 3 ++-
>> drivers/lightnvm/pblk.h      | 8 ++++++++
>> 2 files changed, 10 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>> index c452478..3a191a6 100644
>> --- a/drivers/lightnvm/pblk-init.c
>> +++ b/drivers/lightnvm/pblk-init.c
>> @@ -396,7 +396,8 @@ static void pblk_line_meta_free(struct pblk *pblk)
>> 	spin_lock(&l_mg->free_lock);
> 
> What's the point in holding ->free_lock here? It seems like it could be just dropped.
> 

This lock can indeed be dropped, but the general pblk semaphore, which
serializes initialization and tear down cannot. This is taken on
pblk_exit().

Javier

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

Re: [PATCH] lightnvm: pblk: use vfree_atomic when freeing line metadata

[Andrey Ryabinin]

On 10/03/2017 05:11 PM, Javier González wrote:
>> On 3 Oct 2017, at 16.07, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>>
>>
>>
>> On 10/03/2017 04:48 PM, Hans Holmberg wrote:
>>> From: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>>
>>> The commit bf22e37a6413 ("mm: add vfree_atomic()") made vfree unsafe to
>>> call in atomic context (unless the call came from an interrupt) and
>>> introduced vfree_atomic that is safe to call in atomic context.
>>>
>>> So, since we're holding locks when freeing line metadata, we need to
>>> use the atomic version of vfree.
>>>
>>> Fix this by introducing an atomic variant of pblk_mfree and
>>> switching to that in pblk_line_meta_free.
>>>
>>> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>> ---
>>>
>>> The patch is for:
>>> https://github.com/OpenChannelSSD/linux branch for-4.15/pblk
>>>
>>> drivers/lightnvm/pblk-init.c | 3 ++-
>>> drivers/lightnvm/pblk.h      | 8 ++++++++
>>> 2 files changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>>> index c452478..3a191a6 100644
>>> --- a/drivers/lightnvm/pblk-init.c
>>> +++ b/drivers/lightnvm/pblk-init.c
>>> @@ -396,7 +396,8 @@ static void pblk_line_meta_free(struct pblk *pblk)
>>> 	spin_lock(&l_mg->free_lock);
>>
>> What's the point in holding ->free_lock here? It seems like it could be just dropped.
>>
> 
> This lock can indeed be dropped,

So, let's do this. This would be the best way to fix this.

> but the general pblk semaphore, which
> serializes initialization and tear down cannot. This is taken on
> pblk_exit().
> 

But semaphore is not the problem here. We can sleep under semaphore, so it's fine.

> Javier
> 

Re: [PATCH] lightnvm: pblk: use vfree_atomic when freeing line metadata

[Javier González]

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

> On 3 Oct 2017, at 16.20, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> 
> 
> 
> On 10/03/2017 05:11 PM, Javier González wrote:
>>> On 3 Oct 2017, at 16.07, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>>> 
>>> 
>>> 
>>> On 10/03/2017 04:48 PM, Hans Holmberg wrote:
>>>> From: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>>> 
>>>> The commit bf22e37a6413 ("mm: add vfree_atomic()") made vfree unsafe to
>>>> call in atomic context (unless the call came from an interrupt) and
>>>> introduced vfree_atomic that is safe to call in atomic context.
>>>> 
>>>> So, since we're holding locks when freeing line metadata, we need to
>>>> use the atomic version of vfree.
>>>> 
>>>> Fix this by introducing an atomic variant of pblk_mfree and
>>>> switching to that in pblk_line_meta_free.
>>>> 
>>>> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>>> ---
>>>> 
>>>> The patch is for:
>>>> https://github.com/OpenChannelSSD/linux branch for-4.15/pblk
>>>> 
>>>> drivers/lightnvm/pblk-init.c | 3 ++-
>>>> drivers/lightnvm/pblk.h      | 8 ++++++++
>>>> 2 files changed, 10 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>>>> index c452478..3a191a6 100644
>>>> --- a/drivers/lightnvm/pblk-init.c
>>>> +++ b/drivers/lightnvm/pblk-init.c
>>>> @@ -396,7 +396,8 @@ static void pblk_line_meta_free(struct pblk *pblk)
>>>> 	spin_lock(&l_mg->free_lock);
>>> 
>>> What's the point in holding ->free_lock here? It seems like it could be just dropped.
>> 
>> This lock can indeed be dropped,
> 
> So, let's do this. This would be the best way to fix this.
> 
>> but the general pblk semaphore, which
>> serializes initialization and tear down cannot. This is taken on
>> pblk_exit().
> 
> But semaphore is not the problem here. We can sleep under semaphore, so it's fine.
> 

It seems to me like a false positive, but lockdep complains on the
mentioned rw_semaphore held by pblk, and on the mutex held by the
lightnvm subsystem when removing a target (dev->mlock).

[ 6037.778889] BUG: sleeping function called from invalid context at mm/vmalloc.c:1492
[ 6037.786579] in_atomic(): 1, irqs_disabled(): 0, pid: 1282, name: nvme
[ 6037.793050] 3 locks held by nvme/1282:
[ 6037.793053]  #0:  (&dev->mlock){+.+.+.}, at: [<ffffffff8ddff395>] nvm_ctl_ioctl+0x3c5/0x6a0
[ 6037.793075]  #1:  (pblk_lock){+.+.+.}, at: [<ffffffff8de0439b>] pblk_exit+0x1b/0x100
[ 6037.793092]  #2:  (&(&l_mg->free_lock)->rlock){+.+...}, at: [<ffffffff8de040ea>] pblk_line_meta_free+0x8a/0x130

Any ideas?

Thanks,
Javier




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

Re: [PATCH] lightnvm: pblk: use vfree_atomic when freeing line metadata

[Javier González]

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

> On 3 Oct 2017, at 16.43, Javier González <jg@lightnvm.io> wrote:
> 
>> On 3 Oct 2017, at 16.20, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>> 
>> 
>> 
>> On 10/03/2017 05:11 PM, Javier González wrote:
>>>> On 3 Oct 2017, at 16.07, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>>>> 
>>>> 
>>>> 
>>>> On 10/03/2017 04:48 PM, Hans Holmberg wrote:
>>>>> From: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>>>> 
>>>>> The commit bf22e37a6413 ("mm: add vfree_atomic()") made vfree unsafe to
>>>>> call in atomic context (unless the call came from an interrupt) and
>>>>> introduced vfree_atomic that is safe to call in atomic context.
>>>>> 
>>>>> So, since we're holding locks when freeing line metadata, we need to
>>>>> use the atomic version of vfree.
>>>>> 
>>>>> Fix this by introducing an atomic variant of pblk_mfree and
>>>>> switching to that in pblk_line_meta_free.
>>>>> 
>>>>> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
>>>>> ---
>>>>> 
>>>>> The patch is for:
>>>>> https://github.com/OpenChannelSSD/linux branch for-4.15/pblk
>>>>> 
>>>>> drivers/lightnvm/pblk-init.c | 3 ++-
>>>>> drivers/lightnvm/pblk.h      | 8 ++++++++
>>>>> 2 files changed, 10 insertions(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>>>>> index c452478..3a191a6 100644
>>>>> --- a/drivers/lightnvm/pblk-init.c
>>>>> +++ b/drivers/lightnvm/pblk-init.c
>>>>> @@ -396,7 +396,8 @@ static void pblk_line_meta_free(struct pblk *pblk)
>>>>> 	spin_lock(&l_mg->free_lock);
>>>> 
>>>> What's the point in holding ->free_lock here? It seems like it could be just dropped.
>>> 
>>> This lock can indeed be dropped,
>> 
>> So, let's do this. This would be the best way to fix this.
>> 
>>> but the general pblk semaphore, which
>>> serializes initialization and tear down cannot. This is taken on
>>> pblk_exit().
>> 
>> But semaphore is not the problem here. We can sleep under semaphore, so it's fine.
> 
> It seems to me like a false positive, but lockdep complains on the
> mentioned rw_semaphore held by pblk, and on the mutex held by the
> lightnvm subsystem when removing a target (dev->mlock).
> 
> [ 6037.778889] BUG: sleeping function called from invalid context at mm/vmalloc.c:1492
> [ 6037.786579] in_atomic(): 1, irqs_disabled(): 0, pid: 1282, name: nvme
> [ 6037.793050] 3 locks held by nvme/1282:
> [ 6037.793053]  #0:  (&dev->mlock){+.+.+.}, at: [<ffffffff8ddff395>] nvm_ctl_ioctl+0x3c5/0x6a0
> [ 6037.793075]  #1:  (pblk_lock){+.+.+.}, at: [<ffffffff8de0439b>] pblk_exit+0x1b/0x100
> [ 6037.793092]  #2:  (&(&l_mg->free_lock)->rlock){+.+...}, at: [<ffffffff8de040ea>] pblk_line_meta_free+0x8a/0x130
> 
> Any ideas?
> 

Ok. When dropping ->free_lock, lockdep does not complain. It's just a
misleading notification from lockdep, signalling semaphores as "held
locks" when a real non sleeping lock is being taken.

We will just remove ->free_lock then.

Thanks Andrey.

Javier


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

Re: [PATCH] lightnvm: pblk: use vfree_atomic when freeing line metadata

[Hans Holmberg]

Thanks for the review Andrey, i'll send a new patch removing the lock.