* [PATCH] f2fs: do not use mutex lock in atomic context @ 2019-02-04 8:06 Sahitya Tummala 2019-02-13 3:25 ` Chao Yu 0 siblings, 1 reply; 7+ messages in thread From: Sahitya Tummala @ 2019-02-04 8:06 UTC (permalink / raw) To: Jaegeuk Kim, Chao Yu, linux-f2fs-devel; +Cc: linux-kernel, 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. ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] f2fs: do not use mutex lock in atomic context 2019-02-04 8:06 [PATCH] f2fs: do not use mutex lock in atomic context Sahitya Tummala @ 2019-02-13 3:25 ` Chao Yu 2019-02-14 7:46 ` Sahitya Tummala 0 siblings, 1 reply; 7+ messages in thread From: Chao Yu @ 2019-02-13 3:25 UTC (permalink / raw) To: Sahitya Tummala, Jaegeuk Kim, linux-f2fs-devel; +Cc: linux-kernel 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); > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] f2fs: do not use mutex lock in atomic context 2019-02-13 3:25 ` Chao Yu @ 2019-02-14 7:46 ` Sahitya Tummala 2019-02-14 16:10 ` [f2fs-dev] " Chao Yu 0 siblings, 1 reply; 7+ messages in thread From: Sahitya Tummala @ 2019-02-14 7:46 UTC (permalink / raw) To: Chao Yu; +Cc: Jaegeuk Kim, linux-f2fs-devel, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: do not use mutex lock in atomic context 2019-02-14 7:46 ` Sahitya Tummala @ 2019-02-14 16:10 ` Chao Yu 2019-02-15 4:28 ` Ritesh Harjani 0 siblings, 1 reply; 7+ messages in thread From: Chao Yu @ 2019-02-14 16:10 UTC (permalink / raw) To: Sahitya Tummala, Chao Yu; +Cc: Jaegeuk Kim, linux-kernel, linux-f2fs-devel 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); >>> } >>> >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: do not use mutex lock in atomic context 2019-02-14 16:10 ` [f2fs-dev] " Chao Yu @ 2019-02-15 4:28 ` Ritesh Harjani 2019-02-15 9:10 ` Chao Yu 0 siblings, 1 reply; 7+ messages in thread From: Ritesh Harjani @ 2019-02-15 4:28 UTC (permalink / raw) To: Chao Yu, Sahitya Tummala, Chao Yu Cc: Jaegeuk Kim, linux-kernel, linux-f2fs-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: do not use mutex lock in atomic context 2019-02-15 4:28 ` Ritesh Harjani @ 2019-02-15 9:10 ` Chao Yu 2019-02-18 2:04 ` Ritesh Harjani 0 siblings, 1 reply; 7+ messages in thread From: Chao Yu @ 2019-02-15 9:10 UTC (permalink / raw) To: Ritesh Harjani, Chao Yu, Sahitya Tummala Cc: Jaegeuk Kim, linux-kernel, 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 <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 > > . > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: do not use mutex lock in atomic context 2019-02-15 9:10 ` Chao Yu @ 2019-02-18 2:04 ` Ritesh Harjani 0 siblings, 0 replies; 7+ messages in thread From: Ritesh Harjani @ 2019-02-18 2:04 UTC (permalink / raw) To: Chao Yu, Chao Yu, Sahitya Tummala Cc: Jaegeuk Kim, linux-kernel, 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 <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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-02-18 2:05 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-04 8:06 [PATCH] f2fs: do not use mutex lock in atomic context Sahitya Tummala 2019-02-13 3:25 ` Chao Yu 2019-02-14 7:46 ` Sahitya Tummala 2019-02-14 16:10 ` [f2fs-dev] " Chao Yu 2019-02-15 4:28 ` Ritesh Harjani 2019-02-15 9:10 ` Chao Yu 2019-02-18 2:04 ` Ritesh Harjani
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).