Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ad06d3c3-7878-9106-e118-f89e2de91a1f@samsung.com>
Date:   Fri, 1 Jul 2022 14:49:57 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0)
        Gecko/20100101 Thunderbird/91.10.0
Subject: Re: [PATCH v7 2/4] kernfs: Change kernfs_notify_list to llist.
Content-Language: en-US
To:     Imran Khan <imran.f.khan@oracle.com>, tj@kernel.org,
        gregkh@linuxfoundation.org, viro@zeniv.linux.org.uk
Cc:     linux-kernel@vger.kernel.org
From:   Marek Szyprowski <m.szyprowski@samsung.com>
In-Reply-To: <9e95f138-9ec5-90fc-7ea9-cf8cff8bf180@oracle.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <20220615021059.862643-1-imran.f.khan@oracle.com>
        <20220615021059.862643-3-imran.f.khan@oracle.com>
        <CGME20220701112210eucas1p2d2db45881086f41b73527f7536537aa5@eucas1p2.samsung.com>
        <270b640d-d5e8-b775-9a16-5d5d07f959ff@samsung.com>
        <9e95f138-9ec5-90fc-7ea9-cf8cff8bf180@oracle.com>
Precedence: bulk

Hi,

On 01.07.2022 14:20, Imran Khan wrote:
> On 1/7/22 9:22 pm, Marek Szyprowski wrote:
>> On 15.06.2022 04:10, Imran Khan wrote:
>>> At present kernfs_notify_list is implemented as a singly linked
>>> list of kernfs_node(s), where last element points to itself and
>>> value of ->attr.next tells if node is present on the list or not.
>>> Both addition and deletion to list happen under kernfs_notify_lock.
>>>
>>> Change kernfs_notify_list to llist so that addition to list can heppen
>>> locklessly.
>>>
>>> Suggested by: Al Viro <viro@zeniv.linux.org.uk>
>>> Signed-off-by: Imran Khan <imran.f.khan@oracle.com>
>>> Acked-by: Tejun Heo <tj@kernel.org>
>> This patch landed in linux next-20220630 as commit b8f35fa1188b
>> ("kernfs: Change kernfs_notify_list to llist."). Unfortunately, it
>> causes serious regression on my test systems. It can be easily noticed
>> in the logs by the following warning:
>>
>> ------------[ cut here ]------------
>> WARNING: CPU: 1 PID: 34 at fs/kernfs/dir.c:531 kernfs_put.part.0+0x1a4/0x1d8
>> kernfs_put: console/active: released with incorrect active_ref 0
>> Modules linked in:
>> CPU: 1 PID: 34 Comm: kworker/1:4 Not tainted
>> 5.19.0-rc4-05465-g5732b42edfd1 #12317
>> Hardware name: Samsung Exynos (Flattened Device Tree)
>> Workqueue: events kernfs_notify_workfn
>>    unwind_backtrace from show_stack+0x10/0x14
>>    show_stack from dump_stack_lvl+0x40/0x4c
>>    dump_stack_lvl from __warn+0xc8/0x13c
>>    __warn from warn_slowpath_fmt+0x90/0xb4
>>    warn_slowpath_fmt from kernfs_put.part.0+0x1a4/0x1d8
>>    kernfs_put.part.0 from kernfs_notify_workfn+0x1a0/0x1d0
>>    kernfs_notify_workfn from process_one_work+0x1ec/0x4cc
>>    process_one_work from worker_thread+0x58/0x54c
>>    worker_thread from kthread+0xd0/0xec
>>    kthread from ret_from_fork+0x14/0x2c
>> Exception stack(0xf099dfb0 to 0xf099dff8)
>> ...
>> ---[ end trace 0000000000000000 ]---
>>
> Thanks for reporting this issue. It has been reported earlier in [1] as well. I
> am unable to reproduce it locally. Could you please test with following patch on
> top of linux next-20220630 and let me know if it helps:

Yes, this fixes the issue. Feel free to add:

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Maybe it is related to the fact, that I have earlycon enabled on those 
machines?

> >From 6bf7f1adc4b091dc6d6c60e0dd0f16247f61f374 Mon Sep 17 00:00:00 2001
> From: Imran Khan <imran.f.khan@oracle.com>
> Date: Fri, 1 Jul 2022 14:27:52 +1000
> Subject: [PATCH] kernfs: Avoid re-adding kernfs_node into kernfs_notify_list.
>
> ---
>   fs/kernfs/file.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> index bb933221b4bae..e8ec054e11c63 100644
> --- a/fs/kernfs/file.c
> +++ b/fs/kernfs/file.c
> @@ -917,6 +917,7 @@ static void kernfs_notify_workfn(struct work_struct *work)
>          if (free == NULL)
>                  return;
>
> +       free->next = NULL;
>          attr = llist_entry(free, struct kernfs_elem_attr, notify_next);
>          kn = attribute_to_node(attr, struct kernfs_node, attr);
>          root = kernfs_root(kn);
> @@ -992,9 +993,11 @@ void kernfs_notify(struct kernfs_node *kn)
>          rcu_read_unlock();
>
>          /* schedule work to kick fsnotify */
> -       kernfs_get(kn);
> -       llist_add(&kn->attr.notify_next, &kernfs_notify_list);
> -       schedule_work(&kernfs_notify_work);
> +       if (kn->attr.notify_next.next != NULL) {
> +               kernfs_get(kn);
> +               llist_add(&kn->attr.notify_next, &kernfs_notify_list);
> +               schedule_work(&kernfs_notify_work);
> +       }
>   }
>   EXPORT_SYMBOL_GPL(kernfs_notify);
>
>
> base-commit: 6cc11d2a1759275b856e464265823d94aabd5eaf

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland