* [PATCH] lightnvm: pblk: adjust the position of the lock
@ 2018-11-27 1:53 Hua Su
2018-11-27 12:57 ` Javier Gonzalez
0 siblings, 1 reply; 6+ messages in thread
From: Hua Su @ 2018-11-27 1:53 UTC (permalink / raw)
To: mb; +Cc: javier, axboe, linux-block, linux-kernel, suhua.tanke
Add lock protection for list operations.
Signed-off-by: Hua Su <suhua.tanke@gmail.com>
---
drivers/lightnvm/pblk-core.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
index 6944aac43b01..e490df217dac 100644
--- a/drivers/lightnvm/pblk-core.c
+++ b/drivers/lightnvm/pblk-core.c
@@ -1286,24 +1286,27 @@ int pblk_line_recov_alloc(struct pblk *pblk, struct pblk_line *line)
list_del(&line->list);
ret = pblk_line_prepare(pblk, line);
- if (ret) {
- list_add(&line->list, &l_mg->free_list);
- spin_unlock(&l_mg->free_lock);
- return ret;
- }
- spin_unlock(&l_mg->free_lock);
+ if (ret)
+ goto out;
ret = pblk_line_alloc_bitmaps(pblk, line);
if (ret)
- return ret;
+ goto out;
if (!pblk_line_init_bb(pblk, line, 0)) {
list_add(&line->list, &l_mg->free_list);
+ spin_unlock(&l_mg->free_lock);
return -EINTR;
}
+ spin_unlock(&l_mg->free_lock);
pblk_rl_free_lines_dec(&pblk->rl, line, true);
return 0;
+
+out:
+ list_add(&line->list, &l_mg->free_list);
+ spin_unlock(&l_mg->free_lock);
+ return ret;
}
void pblk_line_recov_close(struct pblk *pblk, struct pblk_line *line)
--
2.19.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] lightnvm: pblk: adjust the position of the lock
2018-11-27 1:53 [PATCH] lightnvm: pblk: adjust the position of the lock Hua Su
@ 2018-11-27 12:57 ` Javier Gonzalez
2018-11-27 14:22 ` Matias Bjørling
0 siblings, 1 reply; 6+ messages in thread
From: Javier Gonzalez @ 2018-11-27 12:57 UTC (permalink / raw)
To: Hua Su; +Cc: Matias Bjørling, Jens Axboe, linux-block, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2945 bytes --]
> On 27 Nov 2018, at 02.53, Hua Su <suhua.tanke@gmail.com> wrote:
>
> Add lock protection for list operations.
> Signed-off-by: Hua Su <suhua.tanke@gmail.com>
> ---
> drivers/lightnvm/pblk-core.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
> index 6944aac43b01..e490df217dac 100644
> --- a/drivers/lightnvm/pblk-core.c
> +++ b/drivers/lightnvm/pblk-core.c
> @@ -1286,24 +1286,27 @@ int pblk_line_recov_alloc(struct pblk *pblk, struct pblk_line *line)
> list_del(&line->list);
>
> ret = pblk_line_prepare(pblk, line);
> - if (ret) {
> - list_add(&line->list, &l_mg->free_list);
> - spin_unlock(&l_mg->free_lock);
> - return ret;
> - }
> - spin_unlock(&l_mg->free_lock);
> + if (ret)
> + goto out;
>
> ret = pblk_line_alloc_bitmaps(pblk, line);
> if (ret)
> - return ret;
> + goto out;
>
> if (!pblk_line_init_bb(pblk, line, 0)) {
> list_add(&line->list, &l_mg->free_list);
> + spin_unlock(&l_mg->free_lock);
> return -EINTR;
> }
> + spin_unlock(&l_mg->free_lock);
>
> pblk_rl_free_lines_dec(&pblk->rl, line, true);
> return 0;
> +
> +out:
> + list_add(&line->list, &l_mg->free_list);
> + spin_unlock(&l_mg->free_lock);
> + return ret;
> }
>
> void pblk_line_recov_close(struct pblk *pblk, struct pblk_line *line)
> --
> 2.19.1
This path is only touched by the recovery path, which is single
threaded, so there is no race condition as is. Also, if recovery fails,
pblk will not create the instance at all. This said, it would be
good to protect the list_add on the pblk_line_init_bb() error path in
case this code is used for some other purpose in the future.
However, the implementation you propose has some issues due to the
extended critical zone, which covers pblk_line_alloc_bitmaps(). Here,
allocations are intentionally non-atomic (i.e., GFP_KERNEL).
You can instead do the following:
diff --git i/drivers/lightnvm/pblk-core.c w/drivers/lightnvm/pblk-core.c
index 615817bf97e3..02c4c1708c45 100644
--- i/drivers/lightnvm/pblk-core.c
+++ w/drivers/lightnvm/pblk-core.c
@@ -1301,15 +1301,22 @@ int pblk_line_recov_alloc(struct pblk *pblk, struct pblk_line *line)
ret = pblk_line_alloc_bitmaps(pblk, line);
if (ret)
- return ret;
+ goto fail;
if (!pblk_line_init_bb(pblk, line, 0)) {
- list_add(&line->list, &l_mg->free_list);
- return -EINTR;
+ ret = -EINTR;
+ goto fail;
}
pblk_rl_free_lines_dec(&pblk->rl, line, true);
return 0;
+
+fail:
+ spin_lock(&l_mg->free_lock);
+ list_add(&line->list, &l_mg->free_list);
+ spin_unlock(&l_mg->free_lock);
+
+ return ret;
}
void pblk_line_recov_close(struct pblk *pblk, struct pblk_line *line)
Javier
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] lightnvm: pblk: adjust the position of the lock
2018-11-27 12:57 ` Javier Gonzalez
@ 2018-11-27 14:22 ` Matias Bjørling
2018-11-27 14:24 ` Javier Gonzalez
0 siblings, 1 reply; 6+ messages in thread
From: Matias Bjørling @ 2018-11-27 14:22 UTC (permalink / raw)
To: javier, suhua.tanke; +Cc: axboe, linux-block, linux-kernel
On 11/27/2018 01:57 PM, Javier Gonzalez wrote:
>
>> On 27 Nov 2018, at 02.53, Hua Su <suhua.tanke@gmail.com> wrote:
>>
>> Add lock protection for list operations.
>> Signed-off-by: Hua Su <suhua.tanke@gmail.com>
>> ---
>> drivers/lightnvm/pblk-core.c | 17 ++++++++++-------
>> 1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
>> index 6944aac43b01..e490df217dac 100644
>> --- a/drivers/lightnvm/pblk-core.c
>> +++ b/drivers/lightnvm/pblk-core.c
>> @@ -1286,24 +1286,27 @@ int pblk_line_recov_alloc(struct pblk *pblk, struct pblk_line *line)
>> list_del(&line->list);
>>
>> ret = pblk_line_prepare(pblk, line);
>> - if (ret) {
>> - list_add(&line->list, &l_mg->free_list);
>> - spin_unlock(&l_mg->free_lock);
>> - return ret;
>> - }
>> - spin_unlock(&l_mg->free_lock);
>> + if (ret)
>> + goto out;
>>
>> ret = pblk_line_alloc_bitmaps(pblk, line);
>> if (ret)
>> - return ret;
>> + goto out;
>>
>> if (!pblk_line_init_bb(pblk, line, 0)) {
>> list_add(&line->list, &l_mg->free_list);
>> + spin_unlock(&l_mg->free_lock);
>> return -EINTR;
>> }
>> + spin_unlock(&l_mg->free_lock);
>>
>> pblk_rl_free_lines_dec(&pblk->rl, line, true);
>> return 0;
>> +
>> +out:
>> + list_add(&line->list, &l_mg->free_list);
>> + spin_unlock(&l_mg->free_lock);
>> + return ret;
>> }
>>
>> void pblk_line_recov_close(struct pblk *pblk, struct pblk_line *line)
>> --
>> 2.19.1
>
> This path is only touched by the recovery path, which is single
> threaded, so there is no race condition as is. Also, if recovery fails,
> pblk will not create the instance at all. This said, it would be
> good to protect the list_add on the pblk_line_init_bb() error path in
> case this code is used for some other purpose in the future.
I like your explanation here. Another option is that we could add a
comment to notify the developer that it safe in this context?
>
> However, the implementation you propose has some issues due to the
> extended critical zone, which covers pblk_line_alloc_bitmaps(). Here,
> allocations are intentionally non-atomic (i.e., GFP_KERNEL).
>
> You can instead do the following:
>
> diff --git i/drivers/lightnvm/pblk-core.c w/drivers/lightnvm/pblk-core.c
> index 615817bf97e3..02c4c1708c45 100644
> --- i/drivers/lightnvm/pblk-core.c
> +++ w/drivers/lightnvm/pblk-core.c
> @@ -1301,15 +1301,22 @@ int pblk_line_recov_alloc(struct pblk *pblk, struct pblk_line *line)
>
> ret = pblk_line_alloc_bitmaps(pblk, line);
> if (ret)
> - return ret;
> + goto fail;
>
> if (!pblk_line_init_bb(pblk, line, 0)) {
> - list_add(&line->list, &l_mg->free_list);
> - return -EINTR;
> + ret = -EINTR;
> + goto fail;
> }
>
> pblk_rl_free_lines_dec(&pblk->rl, line, true);
> return 0;
> +
> +fail:
> + spin_lock(&l_mg->free_lock);
> + list_add(&line->list, &l_mg->free_list);
> + spin_unlock(&l_mg->free_lock);
> +
> + return ret;
> }
>
> void pblk_line_recov_close(struct pblk *pblk, struct pblk_line *line)
>
> Javier
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] lightnvm: pblk: adjust the position of the lock
2018-11-27 14:22 ` Matias Bjørling
@ 2018-11-27 14:24 ` Javier Gonzalez
2018-11-27 14:26 ` Matias Bjørling
[not found] ` <633950CD-FBB6-4228-BB8C-8FD946FB3286@gmail.com>
0 siblings, 2 replies; 6+ messages in thread
From: Javier Gonzalez @ 2018-11-27 14:24 UTC (permalink / raw)
To: Matias Bjørling; +Cc: suhua.tanke, axboe, linux-block, linux-kernel
> On 27 Nov 2018, at 15.22, Matias Bjørling <mb@lightnvm.io> wrote:
>
> On 11/27/2018 01:57 PM, Javier Gonzalez wrote:
>>> On 27 Nov 2018, at 02.53, Hua Su <suhua.tanke@gmail.com> wrote:
>>>
>>> Add lock protection for list operations.
>>> Signed-off-by: Hua Su <suhua.tanke@gmail.com>
>>> ---
>>> drivers/lightnvm/pblk-core.c | 17 ++++++++++-------
>>> 1 file changed, 10 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
>>> index 6944aac43b01..e490df217dac 100644
>>> --- a/drivers/lightnvm/pblk-core.c
>>> +++ b/drivers/lightnvm/pblk-core.c
>>> @@ -1286,24 +1286,27 @@ int pblk_line_recov_alloc(struct pblk *pblk, struct pblk_line *line)
>>> list_del(&line->list);
>>>
>>> ret = pblk_line_prepare(pblk, line);
>>> - if (ret) {
>>> - list_add(&line->list, &l_mg->free_list);
>>> - spin_unlock(&l_mg->free_lock);
>>> - return ret;
>>> - }
>>> - spin_unlock(&l_mg->free_lock);
>>> + if (ret)
>>> + goto out;
>>>
>>> ret = pblk_line_alloc_bitmaps(pblk, line);
>>> if (ret)
>>> - return ret;
>>> + goto out;
>>>
>>> if (!pblk_line_init_bb(pblk, line, 0)) {
>>> list_add(&line->list, &l_mg->free_list);
>>> + spin_unlock(&l_mg->free_lock);
>>> return -EINTR;
>>> }
>>> + spin_unlock(&l_mg->free_lock);
>>>
>>> pblk_rl_free_lines_dec(&pblk->rl, line, true);
>>> return 0;
>>> +
>>> +out:
>>> + list_add(&line->list, &l_mg->free_list);
>>> + spin_unlock(&l_mg->free_lock);
>>> + return ret;
>>> }
>>>
>>> void pblk_line_recov_close(struct pblk *pblk, struct pblk_line *line)
>>> --
>>> 2.19.1
>> This path is only touched by the recovery path, which is single
>> threaded, so there is no race condition as is. Also, if recovery fails,
>> pblk will not create the instance at all. This said, it would be
>> good to protect the list_add on the pblk_line_init_bb() error path in
>> case this code is used for some other purpose in the future.
>
> I like your explanation here. Another option is that we could add a comment to notify the developer that it safe in this context?
Sure. Do you want to add it? Or should I send it?
Javier
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] lightnvm: pblk: adjust the position of the lock
2018-11-27 14:24 ` Javier Gonzalez
@ 2018-11-27 14:26 ` Matias Bjørling
[not found] ` <633950CD-FBB6-4228-BB8C-8FD946FB3286@gmail.com>
1 sibling, 0 replies; 6+ messages in thread
From: Matias Bjørling @ 2018-11-27 14:26 UTC (permalink / raw)
To: javier; +Cc: suhua.tanke, axboe, linux-block, linux-kernel
On 11/27/2018 03:24 PM, Javier Gonzalez wrote:
>
>> On 27 Nov 2018, at 15.22, Matias Bjørling <mb@lightnvm.io> wrote:
>>
>> On 11/27/2018 01:57 PM, Javier Gonzalez wrote:
>>>> On 27 Nov 2018, at 02.53, Hua Su <suhua.tanke@gmail.com> wrote:
>>>>
>>>> Add lock protection for list operations.
>>>> Signed-off-by: Hua Su <suhua.tanke@gmail.com>
>>>> ---
>>>> drivers/lightnvm/pblk-core.c | 17 ++++++++++-------
>>>> 1 file changed, 10 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
>>>> index 6944aac43b01..e490df217dac 100644
>>>> --- a/drivers/lightnvm/pblk-core.c
>>>> +++ b/drivers/lightnvm/pblk-core.c
>>>> @@ -1286,24 +1286,27 @@ int pblk_line_recov_alloc(struct pblk *pblk, struct pblk_line *line)
>>>> list_del(&line->list);
>>>>
>>>> ret = pblk_line_prepare(pblk, line);
>>>> - if (ret) {
>>>> - list_add(&line->list, &l_mg->free_list);
>>>> - spin_unlock(&l_mg->free_lock);
>>>> - return ret;
>>>> - }
>>>> - spin_unlock(&l_mg->free_lock);
>>>> + if (ret)
>>>> + goto out;
>>>>
>>>> ret = pblk_line_alloc_bitmaps(pblk, line);
>>>> if (ret)
>>>> - return ret;
>>>> + goto out;
>>>>
>>>> if (!pblk_line_init_bb(pblk, line, 0)) {
>>>> list_add(&line->list, &l_mg->free_list);
>>>> + spin_unlock(&l_mg->free_lock);
>>>> return -EINTR;
>>>> }
>>>> + spin_unlock(&l_mg->free_lock);
>>>>
>>>> pblk_rl_free_lines_dec(&pblk->rl, line, true);
>>>> return 0;
>>>> +
>>>> +out:
>>>> + list_add(&line->list, &l_mg->free_list);
>>>> + spin_unlock(&l_mg->free_lock);
>>>> + return ret;
>>>> }
>>>>
>>>> void pblk_line_recov_close(struct pblk *pblk, struct pblk_line *line)
>>>> --
>>>> 2.19.1
>>> This path is only touched by the recovery path, which is single
>>> threaded, so there is no race condition as is. Also, if recovery fails,
>>> pblk will not create the instance at all. This said, it would be
>>> good to protect the list_add on the pblk_line_init_bb() error path in
>>> case this code is used for some other purpose in the future.
>>
>> I like your explanation here. Another option is that we could add a comment to notify the developer that it safe in this context?
>
> Sure. Do you want to add it? Or should I send it?
>
> Javier
>
Please send it. Thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] lightnvm: pblk: adjust the position of the lock
[not found] ` <633950CD-FBB6-4228-BB8C-8FD946FB3286@gmail.com>
@ 2018-11-28 7:58 ` Javier Gonzalez
0 siblings, 0 replies; 6+ messages in thread
From: Javier Gonzalez @ 2018-11-28 7:58 UTC (permalink / raw)
To: HUA SU; +Cc: Matias Bjørling, Jens Axboe, linux-block, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 355 bytes --]
> On 28 Nov 2018, at 05.26, HUA SU <suhua.tanke@gmail.com> wrote:
>
> Thank you for your kind correction, I ignored some of the content, it will be perfected by Javier.
>
You are welcome to re-send the patch with the code I proposed. I'll move
a couple of things around to better comment the single-threaded recovery
path afterwards.
Javier
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-11-28 7:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-27 1:53 [PATCH] lightnvm: pblk: adjust the position of the lock Hua Su
2018-11-27 12:57 ` Javier Gonzalez
2018-11-27 14:22 ` Matias Bjørling
2018-11-27 14:24 ` Javier Gonzalez
2018-11-27 14:26 ` Matias Bjørling
[not found] ` <633950CD-FBB6-4228-BB8C-8FD946FB3286@gmail.com>
2018-11-28 7:58 ` Javier Gonzalez
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).