Messages in this thread | | | Subject | Re: [f2fs-dev] [PATCH] f2fs: do not use mutex lock in atomic context | From | Ritesh Harjani <> | Date | Fri, 15 Feb 2019 09:58:15 +0530 |
| |
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
| |