From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D809DC43381 for ; Fri, 15 Feb 2019 09:11:14 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id ADE2421916 for ; Fri, 15 Feb 2019 09:11:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2392242AbfBOJK7 (ORCPT ); Fri, 15 Feb 2019 04:10:59 -0500 Received: from szxga05-in.huawei.com ([45.249.212.191]:3731 "EHLO huawei.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1728277AbfBOJK6 (ORCPT ); Fri, 15 Feb 2019 04:10:58 -0500 Received: from DGGEMS409-HUB.china.huawei.com (unknown [172.30.72.58]) by Forcepoint Email with ESMTP id 83D6395044CDAEEA3F01; Fri, 15 Feb 2019 17:10:55 +0800 (CST) Received: from [127.0.0.1] (10.134.22.195) by DGGEMS409-HUB.china.huawei.com (10.3.19.209) with Microsoft SMTP Server id 14.3.408.0; Fri, 15 Feb 2019 17:10:50 +0800 Subject: Re: [f2fs-dev] [PATCH] f2fs: do not use mutex lock in atomic context To: Ritesh Harjani , Chao Yu , Sahitya Tummala CC: Jaegeuk Kim , , References: <1549267613-9288-1-git-send-email-stummala@codeaurora.org> <20190214074628.GD7308@codeaurora.org> <5650af07-c55d-fcb4-ca98-eca45248392d@kernel.org> <77c63bf6-069c-704c-220a-b50d997d2463@codeaurora.org> From: Chao Yu Message-ID: <70630a8c-4f7d-7d97-54a2-b1e4bceca492@huawei.com> Date: Fri, 15 Feb 2019 17:10:57 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <77c63bf6-069c-704c-220a-b50d997d2463@codeaurora.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.134.22.195] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>>> --- >>>>> 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 >> >> 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 > > . >