Fix below warning coming because of using mutex lock in atomic context.
BUG: sleeping function called from invalid context at kernel/locking/mutex.c:98
in_atomic(): 1, irqs_disabled(): 0, pid: 585, name: sh
Preemption disabled at: __radix_tree_preload+0x28/0x130
Call trace:
dump_backtrace+0x0/0x2b4
show_stack+0x20/0x28
dump_stack+0xa8/0xe0
___might_sleep+0x144/0x194
__might_sleep+0x58/0x8c
mutex_lock+0x2c/0x48
f2fs_trace_pid+0x88/0x14c
f2fs_set_node_page_dirty+0xd0/0x184
Do not use f2fs_radix_tree_insert() to avoid doing cond_resched() with
spin_lock() acquired.
Signed-off-by: Sahitya Tummala <[email protected]>
---
fs/f2fs/trace.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/fs/f2fs/trace.c b/fs/f2fs/trace.c
index ce2a5eb..d0ab533 100644
--- a/fs/f2fs/trace.c
+++ b/fs/f2fs/trace.c
@@ -14,7 +14,7 @@
#include "trace.h"
static RADIX_TREE(pids, GFP_ATOMIC);
-static struct mutex pids_lock;
+static spinlock_t pids_lock;
static struct last_io_info last_io;
static inline void __print_last_io(void)
@@ -58,23 +58,29 @@ void f2fs_trace_pid(struct page *page)
set_page_private(page, (unsigned long)pid);
+retry:
if (radix_tree_preload(GFP_NOFS))
return;
- mutex_lock(&pids_lock);
+ spin_lock(&pids_lock);
p = radix_tree_lookup(&pids, pid);
if (p == current)
goto out;
if (p)
radix_tree_delete(&pids, pid);
- f2fs_radix_tree_insert(&pids, pid, current);
+ if (radix_tree_insert(&pids, pid, current)) {
+ spin_unlock(&pids_lock);
+ radix_tree_preload_end();
+ cond_resched();
+ goto retry;
+ }
trace_printk("%3x:%3x %4x %-16s\n",
MAJOR(inode->i_sb->s_dev), MINOR(inode->i_sb->s_dev),
pid, current->comm);
out:
- mutex_unlock(&pids_lock);
+ spin_unlock(&pids_lock);
radix_tree_preload_end();
}
@@ -119,7 +125,7 @@ void f2fs_trace_ios(struct f2fs_io_info *fio, int flush)
void f2fs_build_trace_ios(void)
{
- mutex_init(&pids_lock);
+ spin_lock_init(&pids_lock);
}
#define PIDVEC_SIZE 128
@@ -147,7 +153,7 @@ void f2fs_destroy_trace_ios(void)
pid_t next_pid = 0;
unsigned int found;
- mutex_lock(&pids_lock);
+ spin_lock(&pids_lock);
while ((found = gang_lookup_pids(pid, next_pid, PIDVEC_SIZE))) {
unsigned idx;
@@ -155,5 +161,5 @@ void f2fs_destroy_trace_ios(void)
for (idx = 0; idx < found; idx++)
radix_tree_delete(&pids, pid[idx]);
}
- mutex_unlock(&pids_lock);
+ spin_unlock(&pids_lock);
}
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
On 2019/2/4 16:06, Sahitya Tummala wrote:
> Fix below warning coming because of using mutex lock in atomic context.
>
> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:98
> in_atomic(): 1, irqs_disabled(): 0, pid: 585, name: sh
> Preemption disabled at: __radix_tree_preload+0x28/0x130
> Call trace:
> dump_backtrace+0x0/0x2b4
> show_stack+0x20/0x28
> dump_stack+0xa8/0xe0
> ___might_sleep+0x144/0x194
> __might_sleep+0x58/0x8c
> mutex_lock+0x2c/0x48
> f2fs_trace_pid+0x88/0x14c
> f2fs_set_node_page_dirty+0xd0/0x184
>
> Do not use f2fs_radix_tree_insert() to avoid doing cond_resched() with
> spin_lock() acquired.
>
> Signed-off-by: Sahitya Tummala <[email protected]>
> ---
> fs/f2fs/trace.c | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/fs/f2fs/trace.c b/fs/f2fs/trace.c
> index ce2a5eb..d0ab533 100644
> --- a/fs/f2fs/trace.c
> +++ b/fs/f2fs/trace.c
> @@ -14,7 +14,7 @@
> #include "trace.h"
>
> static RADIX_TREE(pids, GFP_ATOMIC);
> -static struct mutex pids_lock;
> +static spinlock_t pids_lock;
> static struct last_io_info last_io;
>
> static inline void __print_last_io(void)
> @@ -58,23 +58,29 @@ void f2fs_trace_pid(struct page *page)
>
> set_page_private(page, (unsigned long)pid);
>
> +retry:
> if (radix_tree_preload(GFP_NOFS))
> return;
>
> - mutex_lock(&pids_lock);
> + spin_lock(&pids_lock);
> p = radix_tree_lookup(&pids, pid);
> if (p == current)
> goto out;
> if (p)
> radix_tree_delete(&pids, pid);
>
> - f2fs_radix_tree_insert(&pids, pid, current);
> + if (radix_tree_insert(&pids, pid, current)) {
> + spin_unlock(&pids_lock);
> + radix_tree_preload_end();
> + cond_resched();
> + goto retry;
> + }
>
> trace_printk("%3x:%3x %4x %-16s\n",
> MAJOR(inode->i_sb->s_dev), MINOR(inode->i_sb->s_dev),
> pid, current->comm);
Hi Sahitya,
Can trace_printk sleep? For safety, how about moving it out of spinlock?
Thanks,
> out:
> - mutex_unlock(&pids_lock);
> + spin_unlock(&pids_lock);
> radix_tree_preload_end();
> }
>
> @@ -119,7 +125,7 @@ void f2fs_trace_ios(struct f2fs_io_info *fio, int flush)
>
> void f2fs_build_trace_ios(void)
> {
> - mutex_init(&pids_lock);
> + spin_lock_init(&pids_lock);
> }
>
> #define PIDVEC_SIZE 128
> @@ -147,7 +153,7 @@ void f2fs_destroy_trace_ios(void)
> pid_t next_pid = 0;
> unsigned int found;
>
> - mutex_lock(&pids_lock);
> + spin_lock(&pids_lock);
> while ((found = gang_lookup_pids(pid, next_pid, PIDVEC_SIZE))) {
> unsigned idx;
>
> @@ -155,5 +161,5 @@ void f2fs_destroy_trace_ios(void)
> for (idx = 0; idx < found; idx++)
> radix_tree_delete(&pids, pid[idx]);
> }
> - mutex_unlock(&pids_lock);
> + spin_unlock(&pids_lock);
> }
>
On Wed, Feb 13, 2019 at 11:25:31AM +0800, Chao Yu wrote:
> On 2019/2/4 16:06, Sahitya Tummala wrote:
> > Fix below warning coming because of using mutex lock in atomic context.
> >
> > BUG: sleeping function called from invalid context at kernel/locking/mutex.c:98
> > in_atomic(): 1, irqs_disabled(): 0, pid: 585, name: sh
> > Preemption disabled at: __radix_tree_preload+0x28/0x130
> > Call trace:
> > dump_backtrace+0x0/0x2b4
> > show_stack+0x20/0x28
> > dump_stack+0xa8/0xe0
> > ___might_sleep+0x144/0x194
> > __might_sleep+0x58/0x8c
> > mutex_lock+0x2c/0x48
> > f2fs_trace_pid+0x88/0x14c
> > f2fs_set_node_page_dirty+0xd0/0x184
> >
> > Do not use f2fs_radix_tree_insert() to avoid doing cond_resched() with
> > spin_lock() acquired.
> >
> > Signed-off-by: Sahitya Tummala <[email protected]>
> > ---
> > fs/f2fs/trace.c | 20 +++++++++++++-------
> > 1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/f2fs/trace.c b/fs/f2fs/trace.c
> > index ce2a5eb..d0ab533 100644
> > --- a/fs/f2fs/trace.c
> > +++ b/fs/f2fs/trace.c
> > @@ -14,7 +14,7 @@
> > #include "trace.h"
> >
> > static RADIX_TREE(pids, GFP_ATOMIC);
> > -static struct mutex pids_lock;
> > +static spinlock_t pids_lock;
> > static struct last_io_info last_io;
> >
> > static inline void __print_last_io(void)
> > @@ -58,23 +58,29 @@ void f2fs_trace_pid(struct page *page)
> >
> > set_page_private(page, (unsigned long)pid);
> >
> > +retry:
> > if (radix_tree_preload(GFP_NOFS))
> > return;
> >
> > - mutex_lock(&pids_lock);
> > + spin_lock(&pids_lock);
> > p = radix_tree_lookup(&pids, pid);
> > if (p == current)
> > goto out;
> > if (p)
> > radix_tree_delete(&pids, pid);
> >
> > - f2fs_radix_tree_insert(&pids, pid, current);
> > + if (radix_tree_insert(&pids, pid, current)) {
> > + spin_unlock(&pids_lock);
> > + radix_tree_preload_end();
> > + cond_resched();
> > + goto retry;
> > + }
> >
> > trace_printk("%3x:%3x %4x %-16s\n",
> > MAJOR(inode->i_sb->s_dev), MINOR(inode->i_sb->s_dev),
> > pid, current->comm);
>
> Hi Sahitya,
>
> Can trace_printk sleep? For safety, how about moving it out of spinlock?
>
Hi Chao,
Yes, trace_printk() is safe to use in atomic context (unlike printk).
Thanks,
Sahitya.
> Thanks,
>
> > out:
> > - mutex_unlock(&pids_lock);
> > + spin_unlock(&pids_lock);
> > radix_tree_preload_end();
> > }
> >
> > @@ -119,7 +125,7 @@ void f2fs_trace_ios(struct f2fs_io_info *fio, int flush)
> >
> > void f2fs_build_trace_ios(void)
> > {
> > - mutex_init(&pids_lock);
> > + spin_lock_init(&pids_lock);
> > }
> >
> > #define PIDVEC_SIZE 128
> > @@ -147,7 +153,7 @@ void f2fs_destroy_trace_ios(void)
> > pid_t next_pid = 0;
> > unsigned int found;
> >
> > - mutex_lock(&pids_lock);
> > + spin_lock(&pids_lock);
> > while ((found = gang_lookup_pids(pid, next_pid, PIDVEC_SIZE))) {
> > unsigned idx;
> >
> > @@ -155,5 +161,5 @@ void f2fs_destroy_trace_ios(void)
> > for (idx = 0; idx < found; idx++)
> > radix_tree_delete(&pids, pid[idx]);
> > }
> > - mutex_unlock(&pids_lock);
> > + spin_unlock(&pids_lock);
> > }
> >
>
--
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On 2019-2-14 15:46, Sahitya Tummala wrote:
> On Wed, Feb 13, 2019 at 11:25:31AM +0800, Chao Yu wrote:
>> On 2019/2/4 16:06, Sahitya Tummala wrote:
>>> Fix below warning coming because of using mutex lock in atomic context.
>>>
>>> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:98
>>> in_atomic(): 1, irqs_disabled(): 0, pid: 585, name: sh
>>> Preemption disabled at: __radix_tree_preload+0x28/0x130
>>> Call trace:
>>> dump_backtrace+0x0/0x2b4
>>> show_stack+0x20/0x28
>>> dump_stack+0xa8/0xe0
>>> ___might_sleep+0x144/0x194
>>> __might_sleep+0x58/0x8c
>>> mutex_lock+0x2c/0x48
>>> f2fs_trace_pid+0x88/0x14c
>>> f2fs_set_node_page_dirty+0xd0/0x184
>>>
>>> Do not use f2fs_radix_tree_insert() to avoid doing cond_resched() with
>>> spin_lock() acquired.
>>>
>>> Signed-off-by: Sahitya Tummala <[email protected]>
>>> ---
>>> fs/f2fs/trace.c | 20 +++++++++++++-------
>>> 1 file changed, 13 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/f2fs/trace.c b/fs/f2fs/trace.c
>>> index ce2a5eb..d0ab533 100644
>>> --- a/fs/f2fs/trace.c
>>> +++ b/fs/f2fs/trace.c
>>> @@ -14,7 +14,7 @@
>>> #include "trace.h"
>>>
>>> static RADIX_TREE(pids, GFP_ATOMIC);
>>> -static struct mutex pids_lock;
>>> +static spinlock_t pids_lock;
>>> static struct last_io_info last_io;
>>>
>>> static inline void __print_last_io(void)
>>> @@ -58,23 +58,29 @@ void f2fs_trace_pid(struct page *page)
>>>
>>> set_page_private(page, (unsigned long)pid);
>>>
>>> +retry:
>>> if (radix_tree_preload(GFP_NOFS))
>>> return;
>>>
>>> - mutex_lock(&pids_lock);
>>> + spin_lock(&pids_lock);
>>> p = radix_tree_lookup(&pids, pid);
>>> if (p == current)
>>> goto out;
>>> if (p)
>>> radix_tree_delete(&pids, pid);
>>>
>>> - f2fs_radix_tree_insert(&pids, pid, current);
>>> + if (radix_tree_insert(&pids, pid, current)) {
>>> + spin_unlock(&pids_lock);
>>> + radix_tree_preload_end();
>>> + cond_resched();
>>> + goto retry;
>>> + }
>>>
>>> trace_printk("%3x:%3x %4x %-16s\n",
>>> MAJOR(inode->i_sb->s_dev), MINOR(inode->i_sb->s_dev),
>>> pid, current->comm);
>>
>> Hi Sahitya,
>>
>> Can trace_printk sleep? For safety, how about moving it out of spinlock?
>>
> Hi Chao,
>
> Yes, trace_printk() is safe to use in atomic context (unlike printk).
Hi Sahitya,
Thanks for your confirmation. :)
Reviewed-by: Chao Yu <[email protected]>
Thanks,
>
> Thanks,
> Sahitya.
>
>> Thanks,
>>
>>> out:
>>> - mutex_unlock(&pids_lock);
>>> + spin_unlock(&pids_lock);
>>> radix_tree_preload_end();
>>> }
>>>
>>> @@ -119,7 +125,7 @@ void f2fs_trace_ios(struct f2fs_io_info *fio, int flush)
>>>
>>> void f2fs_build_trace_ios(void)
>>> {
>>> - mutex_init(&pids_lock);
>>> + spin_lock_init(&pids_lock);
>>> }
>>>
>>> #define PIDVEC_SIZE 128
>>> @@ -147,7 +153,7 @@ void f2fs_destroy_trace_ios(void)
>>> pid_t next_pid = 0;
>>> unsigned int found;
>>>
>>> - mutex_lock(&pids_lock);
>>> + spin_lock(&pids_lock);
>>> while ((found = gang_lookup_pids(pid, next_pid, PIDVEC_SIZE))) {
>>> unsigned idx;
>>>
>>> @@ -155,5 +161,5 @@ void f2fs_destroy_trace_ios(void)
>>> for (idx = 0; idx < found; idx++)
>>> radix_tree_delete(&pids, pid[idx]);
>>> }
>>> - mutex_unlock(&pids_lock);
>>> + spin_unlock(&pids_lock);
>>> }
>>>
>>
>
On 2/14/2019 9:40 PM, Chao Yu wrote:
> On 2019-2-14 15:46, Sahitya Tummala wrote:
>> On Wed, Feb 13, 2019 at 11:25:31AM +0800, Chao Yu wrote:
>>> On 2019/2/4 16:06, Sahitya Tummala wrote:
>>>> Fix below warning coming because of using mutex lock in atomic context.
>>>>
>>>> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:98
>>>> in_atomic(): 1, irqs_disabled(): 0, pid: 585, name: sh
>>>> Preemption disabled at: __radix_tree_preload+0x28/0x130
>>>> Call trace:
>>>> dump_backtrace+0x0/0x2b4
>>>> show_stack+0x20/0x28
>>>> dump_stack+0xa8/0xe0
>>>> ___might_sleep+0x144/0x194
>>>> __might_sleep+0x58/0x8c
>>>> mutex_lock+0x2c/0x48
>>>> f2fs_trace_pid+0x88/0x14c
>>>> f2fs_set_node_page_dirty+0xd0/0x184
>>>>
>>>> Do not use f2fs_radix_tree_insert() to avoid doing cond_resched() with
>>>> spin_lock() acquired.
>>>>
>>>> Signed-off-by: Sahitya Tummala <[email protected]>
>>>> ---
>>>> fs/f2fs/trace.c | 20 +++++++++++++-------
>>>> 1 file changed, 13 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/trace.c b/fs/f2fs/trace.c
>>>> index ce2a5eb..d0ab533 100644
>>>> --- a/fs/f2fs/trace.c
>>>> +++ b/fs/f2fs/trace.c
>>>> @@ -14,7 +14,7 @@
>>>> #include "trace.h"
>>>>
>>>> static RADIX_TREE(pids, GFP_ATOMIC);
>>>> -static struct mutex pids_lock;
>>>> +static spinlock_t pids_lock;
>>>> static struct last_io_info last_io;
>>>>
>>>> static inline void __print_last_io(void)
>>>> @@ -58,23 +58,29 @@ void f2fs_trace_pid(struct page *page)
>>>>
>>>> set_page_private(page, (unsigned long)pid);
>>>>
>>>> +retry:
>>>> if (radix_tree_preload(GFP_NOFS))
>>>> return;
>>>>
>>>> - mutex_lock(&pids_lock);
>>>> + spin_lock(&pids_lock);
>>>> p = radix_tree_lookup(&pids, pid);
>>>> if (p == current)
>>>> goto out;
>>>> if (p)
>>>> radix_tree_delete(&pids, pid);
>>>>
>>>> - f2fs_radix_tree_insert(&pids, pid, current);
Do you know why do we have a retry logic here? When anyways we have
called for radix_tree_delete with pid key?
Which should ensure the slot is empty, no?
Then why in the original code (f2fs_radix_tree_insert), we were
retrying. For what condition a retry was needed?
Regards
Ritesh
>>>> + if (radix_tree_insert(&pids, pid, current)) {
>>>> + spin_unlock(&pids_lock);
>>>> + radix_tree_preload_end();
>>>> + cond_resched();
>>>> + goto retry;
>>>> + }
>>>>
>>>> trace_printk("%3x:%3x %4x %-16s\n",
>>>> MAJOR(inode->i_sb->s_dev), MINOR(inode->i_sb->s_dev),
>>>> pid, current->comm);
>>> Hi Sahitya,
>>>
>>> Can trace_printk sleep? For safety, how about moving it out of spinlock?
>>>
>> Hi Chao,
>>
>> Yes, trace_printk() is safe to use in atomic context (unlike printk).
> Hi Sahitya,
>
> Thanks for your confirmation. :)
>
> Reviewed-by: Chao Yu <[email protected]>
>
> Thanks,
>
>> Thanks,
>> Sahitya.
>>
>>> Thanks,
>>>
>>>> out:
>>>> - mutex_unlock(&pids_lock);
>>>> + spin_unlock(&pids_lock);
>>>> radix_tree_preload_end();
>>>> }
>>>>
>>>> @@ -119,7 +125,7 @@ void f2fs_trace_ios(struct f2fs_io_info *fio, int flush)
>>>>
>>>> void f2fs_build_trace_ios(void)
>>>> {
>>>> - mutex_init(&pids_lock);
>>>> + spin_lock_init(&pids_lock);
>>>> }
>>>>
>>>> #define PIDVEC_SIZE 128
>>>> @@ -147,7 +153,7 @@ void f2fs_destroy_trace_ios(void)
>>>> pid_t next_pid = 0;
>>>> unsigned int found;
>>>>
>>>> - mutex_lock(&pids_lock);
>>>> + spin_lock(&pids_lock);
>>>> while ((found = gang_lookup_pids(pid, next_pid, PIDVEC_SIZE))) {
>>>> unsigned idx;
>>>>
>>>> @@ -155,5 +161,5 @@ void f2fs_destroy_trace_ios(void)
>>>> for (idx = 0; idx < found; idx++)
>>>> radix_tree_delete(&pids, pid[idx]);
>>>> }
>>>> - mutex_unlock(&pids_lock);
>>>> + spin_unlock(&pids_lock);
>>>> }
>>>>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On 2019/2/15 12:28, Ritesh Harjani wrote:
>
> On 2/14/2019 9:40 PM, Chao Yu wrote:
>> On 2019-2-14 15:46, Sahitya Tummala wrote:
>>> On Wed, Feb 13, 2019 at 11:25:31AM +0800, Chao Yu wrote:
>>>> On 2019/2/4 16:06, Sahitya Tummala wrote:
>>>>> Fix below warning coming because of using mutex lock in atomic context.
>>>>>
>>>>> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:98
>>>>> in_atomic(): 1, irqs_disabled(): 0, pid: 585, name: sh
>>>>> Preemption disabled at: __radix_tree_preload+0x28/0x130
>>>>> Call trace:
>>>>> dump_backtrace+0x0/0x2b4
>>>>> show_stack+0x20/0x28
>>>>> dump_stack+0xa8/0xe0
>>>>> ___might_sleep+0x144/0x194
>>>>> __might_sleep+0x58/0x8c
>>>>> mutex_lock+0x2c/0x48
>>>>> f2fs_trace_pid+0x88/0x14c
>>>>> f2fs_set_node_page_dirty+0xd0/0x184
>>>>>
>>>>> Do not use f2fs_radix_tree_insert() to avoid doing cond_resched() with
>>>>> spin_lock() acquired.
>>>>>
>>>>> Signed-off-by: Sahitya Tummala <[email protected]>
>>>>> ---
>>>>> fs/f2fs/trace.c | 20 +++++++++++++-------
>>>>> 1 file changed, 13 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/trace.c b/fs/f2fs/trace.c
>>>>> index ce2a5eb..d0ab533 100644
>>>>> --- a/fs/f2fs/trace.c
>>>>> +++ b/fs/f2fs/trace.c
>>>>> @@ -14,7 +14,7 @@
>>>>> #include "trace.h"
>>>>>
>>>>> static RADIX_TREE(pids, GFP_ATOMIC);
>>>>> -static struct mutex pids_lock;
>>>>> +static spinlock_t pids_lock;
>>>>> static struct last_io_info last_io;
>>>>>
>>>>> static inline void __print_last_io(void)
>>>>> @@ -58,23 +58,29 @@ void f2fs_trace_pid(struct page *page)
>>>>>
>>>>> set_page_private(page, (unsigned long)pid);
>>>>>
>>>>> +retry:
>>>>> if (radix_tree_preload(GFP_NOFS))
>>>>> return;
>>>>>
>>>>> - mutex_lock(&pids_lock);
>>>>> + spin_lock(&pids_lock);
>>>>> p = radix_tree_lookup(&pids, pid);
>>>>> if (p == current)
>>>>> goto out;
>>>>> if (p)
>>>>> radix_tree_delete(&pids, pid);
>>>>>
>>>>> - f2fs_radix_tree_insert(&pids, pid, current);
>
> Do you know why do we have a retry logic here? When anyways we have
> called for radix_tree_delete with pid key?
> Which should ensure the slot is empty, no?
> Then why in the original code (f2fs_radix_tree_insert), we were
> retrying. For what condition a retry was needed?
Hi,
f2fs_radix_tree_insert is used in many places, it was introduced to used in
some paths we should not failed.
And here, I guess we used it for the same purpose, if we failed to insert
@current pointer into radix, next time, we may not skip calling
trace_printk, actually it will print the same current->comm info as
previous one, it's redundant.
Thanks,
>
> Regards
> Ritesh
>
>
>>>>> + if (radix_tree_insert(&pids, pid, current)) {
>>>>> + spin_unlock(&pids_lock);
>>>>> + radix_tree_preload_end();
>>>>> + cond_resched();
>>>>> + goto retry;
>>>>> + }
>>>>>
>>>>> trace_printk("%3x:%3x %4x %-16s\n",
>>>>> MAJOR(inode->i_sb->s_dev), MINOR(inode->i_sb->s_dev),
>>>>> pid, current->comm);
>>>> Hi Sahitya,
>>>>
>>>> Can trace_printk sleep? For safety, how about moving it out of spinlock?
>>>>
>>> Hi Chao,
>>>
>>> Yes, trace_printk() is safe to use in atomic context (unlike printk).
>> Hi Sahitya,
>>
>> Thanks for your confirmation. :)
>>
>> Reviewed-by: Chao Yu <[email protected]>
>>
>> Thanks,
>>
>>> Thanks,
>>> Sahitya.
>>>
>>>> Thanks,
>>>>
>>>>> out:
>>>>> - mutex_unlock(&pids_lock);
>>>>> + spin_unlock(&pids_lock);
>>>>> radix_tree_preload_end();
>>>>> }
>>>>>
>>>>> @@ -119,7 +125,7 @@ void f2fs_trace_ios(struct f2fs_io_info *fio, int flush)
>>>>>
>>>>> void f2fs_build_trace_ios(void)
>>>>> {
>>>>> - mutex_init(&pids_lock);
>>>>> + spin_lock_init(&pids_lock);
>>>>> }
>>>>>
>>>>> #define PIDVEC_SIZE 128
>>>>> @@ -147,7 +153,7 @@ void f2fs_destroy_trace_ios(void)
>>>>> pid_t next_pid = 0;
>>>>> unsigned int found;
>>>>>
>>>>> - mutex_lock(&pids_lock);
>>>>> + spin_lock(&pids_lock);
>>>>> while ((found = gang_lookup_pids(pid, next_pid, PIDVEC_SIZE))) {
>>>>> unsigned idx;
>>>>>
>>>>> @@ -155,5 +161,5 @@ void f2fs_destroy_trace_ios(void)
>>>>> for (idx = 0; idx < found; idx++)
>>>>> radix_tree_delete(&pids, pid[idx]);
>>>>> }
>>>>> - mutex_unlock(&pids_lock);
>>>>> + spin_unlock(&pids_lock);
>>>>> }
>>>>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> [email protected]
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
> .
>
On 2/15/2019 2:40 PM, Chao Yu wrote:
> On 2019/2/15 12:28, Ritesh Harjani wrote:
>> On 2/14/2019 9:40 PM, Chao Yu wrote:
>>> On 2019-2-14 15:46, Sahitya Tummala wrote:
>>>> On Wed, Feb 13, 2019 at 11:25:31AM +0800, Chao Yu wrote:
>>>>> On 2019/2/4 16:06, Sahitya Tummala wrote:
>>>>>> Fix below warning coming because of using mutex lock in atomic context.
>>>>>>
>>>>>> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:98
>>>>>> in_atomic(): 1, irqs_disabled(): 0, pid: 585, name: sh
>>>>>> Preemption disabled at: __radix_tree_preload+0x28/0x130
>>>>>> Call trace:
>>>>>> dump_backtrace+0x0/0x2b4
>>>>>> show_stack+0x20/0x28
>>>>>> dump_stack+0xa8/0xe0
>>>>>> ___might_sleep+0x144/0x194
>>>>>> __might_sleep+0x58/0x8c
>>>>>> mutex_lock+0x2c/0x48
>>>>>> f2fs_trace_pid+0x88/0x14c
>>>>>> f2fs_set_node_page_dirty+0xd0/0x184
>>>>>>
>>>>>> Do not use f2fs_radix_tree_insert() to avoid doing cond_resched() with
>>>>>> spin_lock() acquired.
>>>>>>
>>>>>> Signed-off-by: Sahitya Tummala <[email protected]>
>>>>>> ---
>>>>>> fs/f2fs/trace.c | 20 +++++++++++++-------
>>>>>> 1 file changed, 13 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/trace.c b/fs/f2fs/trace.c
>>>>>> index ce2a5eb..d0ab533 100644
>>>>>> --- a/fs/f2fs/trace.c
>>>>>> +++ b/fs/f2fs/trace.c
>>>>>> @@ -14,7 +14,7 @@
>>>>>> #include "trace.h"
>>>>>>
>>>>>> static RADIX_TREE(pids, GFP_ATOMIC);
>>>>>> -static struct mutex pids_lock;
>>>>>> +static spinlock_t pids_lock;
>>>>>> static struct last_io_info last_io;
>>>>>>
>>>>>> static inline void __print_last_io(void)
>>>>>> @@ -58,23 +58,29 @@ void f2fs_trace_pid(struct page *page)
>>>>>>
>>>>>> set_page_private(page, (unsigned long)pid);
>>>>>>
>>>>>> +retry:
>>>>>> if (radix_tree_preload(GFP_NOFS))
>>>>>> return;
>>>>>>
>>>>>> - mutex_lock(&pids_lock);
>>>>>> + spin_lock(&pids_lock);
>>>>>> p = radix_tree_lookup(&pids, pid);
>>>>>> if (p == current)
>>>>>> goto out;
>>>>>> if (p)
>>>>>> radix_tree_delete(&pids, pid);
>>>>>>
>>>>>> - f2fs_radix_tree_insert(&pids, pid, current);
>> Do you know why do we have a retry logic here? When anyways we have
>> called for radix_tree_delete with pid key?
>> Which should ensure the slot is empty, no?
>> Then why in the original code (f2fs_radix_tree_insert), we were
>> retrying. For what condition a retry was needed?
> Hi,
>
> f2fs_radix_tree_insert is used in many places, it was introduced to used in
> some paths we should not failed.
>
> And here, I guess we used it for the same purpose, if we failed to insert
> @current pointer into radix, next time, we may not skip calling
> trace_printk, actually it will print the same current->comm info as
> previous one, it's redundant.
Sure, thanks for the info.
Regards
Ritesh