[PATCH] axmap: fix deadlock

[PATCH] axmap: fix deadlock

[Ming Lei]

axmap_first_free() is always called with axmap->lock held,
so needn't to acquire the lock inside the function.

The deadlock is introduced in commit 12bde3697fc230d7a(axmap:
ensure we lock down the maps for shared access).

Given axmap_first_free() is only called inside lib/axmap.c,
this patch declares the function as static. In the future,
if external users need the function, axmap lock can be
considered at that time.

Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 lib/axmap.c |    4 +---
 lib/axmap.h |    1 -
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/lib/axmap.c b/lib/axmap.c
index e847a38..8247fc1 100644
--- a/lib/axmap.c
+++ b/lib/axmap.c
@@ -387,17 +387,15 @@ static uint64_t axmap_find_first_free(struct axmap *axmap, unsigned int level,
 	return (uint64_t) -1ULL;
 }
 
-uint64_t axmap_first_free(struct axmap *axmap)
+static uint64_t axmap_first_free(struct axmap *axmap)
 {
 	uint64_t ret;
 
 	if (firstfree_valid(axmap))
 		return axmap->first_free;
 
-	fio_mutex_down(&axmap->lock);
 	ret = axmap_find_first_free(axmap, axmap->nr_levels - 1, 0);
 	axmap->first_free = ret;
-	fio_mutex_up(&axmap->lock);
 
 	return ret;
 }
diff --git a/lib/axmap.h b/lib/axmap.h
index edfeba8..3705a1d 100644
--- a/lib/axmap.h
+++ b/lib/axmap.h
@@ -11,7 +11,6 @@ void axmap_clear(struct axmap *axmap, uint64_t bit_nr);
 void axmap_set(struct axmap *axmap, uint64_t bit_nr);
 unsigned int axmap_set_nr(struct axmap *axmap, uint64_t bit_nr, unsigned int nr_bits);
 int axmap_isset(struct axmap *axmap, uint64_t bit_nr);
-uint64_t axmap_first_free(struct axmap *axmap);
 uint64_t axmap_next_free(struct axmap *axmap, uint64_t bit_nr);
 void axmap_reset(struct axmap *axmap);
 
-- 
1.7.9.5

Re: [PATCH] axmap: fix deadlock

[Sedat Dilek]

On Sat, Jan 3, 2015 at 3:21 PM, Ming Lei <ming.lei@canonical.com> wrote:
> axmap_first_free() is always called with axmap->lock held,
> so needn't to acquire the lock inside the function.
>
> The deadlock is introduced in commit 12bde3697fc230d7a(axmap:
> ensure we lock down the maps for shared access).
>
> Given axmap_first_free() is only called inside lib/axmap.c,
> this patch declares the function as static. In the future,
> if external users need the function, axmap lock can be
> considered at that time.
>
> Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>

Tested-by: Sedat Dilek <sedat.dilek@gmail.com>

- Sedat -

Re: [PATCH] axmap: fix deadlock

[Jens Axboe]

On 01/03/2015 07:21 AM, Ming Lei wrote:
> axmap_first_free() is always called with axmap->lock held,
> so needn't to acquire the lock inside the function.
> 
> The deadlock is introduced in commit 12bde3697fc230d7a(axmap:
> ensure we lock down the maps for shared access).
> 
> Given axmap_first_free() is only called inside lib/axmap.c,
> this patch declares the function as static. In the future,
> if external users need the function, axmap lock can be
> considered at that time.

Thanks Ming, applied.

-- 
Jens Axboe

Re: [PATCH] axmap: fix deadlock

[Sedat Dilek]

On Sat, Jan 3, 2015 at 7:11 PM, Jens Axboe <axboe@fb.com> wrote:
> On 01/03/2015 07:21 AM, Ming Lei wrote:
>> axmap_first_free() is always called with axmap->lock held,
>> so needn't to acquire the lock inside the function.
>>
>> The deadlock is introduced in commit 12bde3697fc230d7a(axmap:
>> ensure we lock down the maps for shared access).
>>
>> Given axmap_first_free() is only called inside lib/axmap.c,
>> this patch declares the function as static. In the future,
>> if external users need the function, axmap lock can be
>> considered at that time.
>
> Thanks Ming, applied.
>

Do you use the "Fixes:" tag in fio development [1]?

Fixes: 12bde3697fc2 ("axmap: ensure we lock down the maps for shared access")

- Sedat -

[1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n159

Re: [PATCH] axmap: fix deadlock

[Jens Axboe]

On 01/03/2015 12:39 PM, Sedat Dilek wrote:
> On Sat, Jan 3, 2015 at 7:11 PM, Jens Axboe <axboe@fb.com> wrote:
>> On 01/03/2015 07:21 AM, Ming Lei wrote:
>>> axmap_first_free() is always called with axmap->lock held,
>>> so needn't to acquire the lock inside the function.
>>>
>>> The deadlock is introduced in commit 12bde3697fc230d7a(axmap:
>>> ensure we lock down the maps for shared access).
>>>
>>> Given axmap_first_free() is only called inside lib/axmap.c,
>>> this patch declares the function as static. In the future,
>>> if external users need the function, axmap lock can be
>>> considered at that time.
>>
>> Thanks Ming, applied.
>>
>
> Do you use the "Fixes:" tag in fio development [1]?
>
> Fixes: 12bde3697fc2 ("axmap: ensure we lock down the maps for shared access")

I sometimes mention the commit that it fixes, if I remember I do. It's 
usually done like Ming did it in his commit:

     The deadlock is introduced in commit 12bde3697fc230d7a(axmap:
     ensure we lock down the maps for shared access).

and not as a specific Fixes: like we use on the linux kernel side. It 
makes more sense to do for the kernel since we maintain stable branches, 
so you can have tools figure out how far back to backport certain fixes. 
Fio doesn't maintain stable branches off releases, so it's less relevant 
there. I just people to use the latest release, and/or tip.

-- 
Jens Axboe

Re: [PATCH] axmap: fix deadlock

[Sedat Dilek]

On Sat, Jan 3, 2015 at 9:13 PM, Jens Axboe <axboe@fb.com> wrote:
> On 01/03/2015 12:39 PM, Sedat Dilek wrote:
>>
>> On Sat, Jan 3, 2015 at 7:11 PM, Jens Axboe <axboe@fb.com> wrote:
>>>
>>> On 01/03/2015 07:21 AM, Ming Lei wrote:
>>>>
>>>> axmap_first_free() is always called with axmap->lock held,
>>>> so needn't to acquire the lock inside the function.
>>>>
>>>> The deadlock is introduced in commit 12bde3697fc230d7a(axmap:
>>>> ensure we lock down the maps for shared access).
>>>>
>>>> Given axmap_first_free() is only called inside lib/axmap.c,
>>>> this patch declares the function as static. In the future,
>>>> if external users need the function, axmap lock can be
>>>> considered at that time.
>>>
>>>
>>> Thanks Ming, applied.
>>>
>>
>> Do you use the "Fixes:" tag in fio development [1]?
>>
>> Fixes: 12bde3697fc2 ("axmap: ensure we lock down the maps for shared
>> access")
>
>
> I sometimes mention the commit that it fixes, if I remember I do. It's
> usually done like Ming did it in his commit:
>
>     The deadlock is introduced in commit 12bde3697fc230d7a(axmap:
>     ensure we lock down the maps for shared access).
>
> and not as a specific Fixes: like we use on the linux kernel side. It makes
> more sense to do for the kernel since we maintain stable branches, so you
> can have tools figure out how far back to backport certain fixes. Fio
> doesn't maintain stable branches off releases, so it's less relevant there.
> I just people to use the latest release, and/or tip.
>

Thanks, it's  helpful to mention the culprit commit, personally I like
the idea of the Fixes-tag.

You have reverted recent changes to axmap in fio.git.
Do you plan a new fio v2.2.4 release as v2.2.3 has this serious "hanging" issue?

- Sedat -

Re: [PATCH] axmap: fix deadlock

[Jens Axboe]

On 01/03/2015 04:51 PM, Sedat Dilek wrote:
> On Sat, Jan 3, 2015 at 9:13 PM, Jens Axboe <axboe@fb.com> wrote:
>> On 01/03/2015 12:39 PM, Sedat Dilek wrote:
>>>
>>> On Sat, Jan 3, 2015 at 7:11 PM, Jens Axboe <axboe@fb.com> wrote:
>>>>
>>>> On 01/03/2015 07:21 AM, Ming Lei wrote:
>>>>>
>>>>> axmap_first_free() is always called with axmap->lock held,
>>>>> so needn't to acquire the lock inside the function.
>>>>>
>>>>> The deadlock is introduced in commit 12bde3697fc230d7a(axmap:
>>>>> ensure we lock down the maps for shared access).
>>>>>
>>>>> Given axmap_first_free() is only called inside lib/axmap.c,
>>>>> this patch declares the function as static. In the future,
>>>>> if external users need the function, axmap lock can be
>>>>> considered at that time.
>>>>
>>>>
>>>> Thanks Ming, applied.
>>>>
>>>
>>> Do you use the "Fixes:" tag in fio development [1]?
>>>
>>> Fixes: 12bde3697fc2 ("axmap: ensure we lock down the maps for shared
>>> access")
>>
>>
>> I sometimes mention the commit that it fixes, if I remember I do. It's
>> usually done like Ming did it in his commit:
>>
>>      The deadlock is introduced in commit 12bde3697fc230d7a(axmap:
>>      ensure we lock down the maps for shared access).
>>
>> and not as a specific Fixes: like we use on the linux kernel side. It makes
>> more sense to do for the kernel since we maintain stable branches, so you
>> can have tools figure out how far back to backport certain fixes. Fio
>> doesn't maintain stable branches off releases, so it's less relevant there.
>> I just people to use the latest release, and/or tip.
>>
>
> Thanks, it's  helpful to mention the culprit commit, personally I like
> the idea of the Fixes-tag.

Not disagreeing, I like them too. Just saying there's little reason to 
do it in fio, as we don't have specific stable branches. But there is 
something to be said for having it in a consistent format.

> You have reverted recent changes to axmap in fio.git.
> Do you plan a new fio v2.2.4 release as v2.2.3 has this serious "hanging" issue?

Yup, in fact I did that just now.

-- 
Jens Axboe

Re: [PATCH] axmap: fix deadlock

[Sedat Dilek]

On Sun, Jan 4, 2015 at 3:48 AM, Jens Axboe <axboe@fb.com> wrote:
> On 01/03/2015 04:51 PM, Sedat Dilek wrote:
>>
>> On Sat, Jan 3, 2015 at 9:13 PM, Jens Axboe <axboe@fb.com> wrote:
>>>
>>> On 01/03/2015 12:39 PM, Sedat Dilek wrote:
>>>>
>>>>
>>>> On Sat, Jan 3, 2015 at 7:11 PM, Jens Axboe <axboe@fb.com> wrote:
>>>>>
>>>>>
>>>>> On 01/03/2015 07:21 AM, Ming Lei wrote:
>>>>>>
>>>>>>
>>>>>> axmap_first_free() is always called with axmap->lock held,
>>>>>> so needn't to acquire the lock inside the function.
>>>>>>
>>>>>> The deadlock is introduced in commit 12bde3697fc230d7a(axmap:
>>>>>> ensure we lock down the maps for shared access).
>>>>>>
>>>>>> Given axmap_first_free() is only called inside lib/axmap.c,
>>>>>> this patch declares the function as static. In the future,
>>>>>> if external users need the function, axmap lock can be
>>>>>> considered at that time.
>>>>>
>>>>>
>>>>>
>>>>> Thanks Ming, applied.
>>>>>
>>>>
>>>> Do you use the "Fixes:" tag in fio development [1]?
>>>>
>>>> Fixes: 12bde3697fc2 ("axmap: ensure we lock down the maps for shared
>>>> access")
>>>
>>>
>>>
>>> I sometimes mention the commit that it fixes, if I remember I do. It's
>>> usually done like Ming did it in his commit:
>>>
>>>      The deadlock is introduced in commit 12bde3697fc230d7a(axmap:
>>>      ensure we lock down the maps for shared access).
>>>
>>> and not as a specific Fixes: like we use on the linux kernel side. It
>>> makes
>>> more sense to do for the kernel since we maintain stable branches, so you
>>> can have tools figure out how far back to backport certain fixes. Fio
>>> doesn't maintain stable branches off releases, so it's less relevant
>>> there.
>>> I just people to use the latest release, and/or tip.
>>>
>>
>> Thanks, it's  helpful to mention the culprit commit, personally I like
>> the idea of the Fixes-tag.
>
>
> Not disagreeing, I like them too. Just saying there's little reason to do it
> in fio, as we don't have specific stable branches. But there is something to
> be said for having it in a consistent format.
>
>> You have reverted recent changes to axmap in fio.git.
>> Do you plan a new fio v2.2.4 release as v2.2.3 has this serious "hanging"
>> issue?
>
>
> Yup, in fact I did that just now.
>

Thanks for the new release!

- Sedat -