[PATCH] f2fs: do not use mutex lock in atomic context

[PATCH] f2fs: do not use mutex lock in atomic context

[Sahitya Tummala]

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 <stummala@codeaurora.org>
---
 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.

Re: [PATCH] f2fs: do not use mutex lock in atomic context

[Chao Yu]

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 <stummala@codeaurora.org>
> ---
>  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);
>  }
> 

Re: [PATCH] f2fs: do not use mutex lock in atomic context

[Sahitya Tummala]

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 <stummala@codeaurora.org>
> > ---
> >  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.

Re: [f2fs-dev] [PATCH] f2fs: do not use mutex lock in atomic context

[Chao Yu]

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 <stummala@codeaurora.org>
>>> ---
>>>  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 <yuchao0@huawei.com>

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);
>>>  }
>>>
>>
> 

Re: [f2fs-dev] [PATCH] f2fs: do not use mutex lock in atomic context

[Ritesh Harjani]

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 <stummala@codeaurora.org>
>>>> ---
>>>>   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 <yuchao0@huawei.com>
>
> 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
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: do not use mutex lock in atomic context

[Chao Yu]

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 <stummala@codeaurora.org>
>>>>> ---
>>>>>   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 <yuchao0@huawei.com>
>>
>> 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
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 
> .
> 

Re: [f2fs-dev] [PATCH] f2fs: do not use mutex lock in atomic context

[Ritesh Harjani]

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 <stummala@codeaurora.org>
>>>>>> ---
>>>>>>    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