linux-kernel - Re: [PATCH 0/1] fsnotify: clear PARENT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87bk59gsxv.fsf@oracle.com>
Date: Mon, 13 May 2024 17:04:12 -0700
From: Stephen Brennan <stephen.s.brennan@...cle.com>
To: Amir Goldstein <amir73il@...il.com>
Cc: Jan Kara <jack@...e.cz>, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/1] fsnotify: clear PARENT_WATCHED flags lazily

Amir Goldstein <amir73il@...il.com> writes:

> On Fri, May 10, 2024 at 6:21 PM Stephen Brennan
> <stephen.s.brennan@...cle.com> wrote:
>>
>> Hi Amir, Jan, et al,
>
> Hi Stephen,
>
>>
>> It's been a while since I worked with you on the patch series[1] that aimed to
>> make __fsnotify_update_child_dentry_flags() a sleepable function. That work got
>> to a point that it was close to ready, but there were some locking issues which
>> Jan found, and the kernel test robot reported, and I didn't find myself able to
>> tackle them in the amount of time I had.
>>
>> But looking back on that series, I think I threw out the baby with the
>> bathwater. While I may not have resolved the locking issues associated with the
>> larger change, there was one patch which Amir shared, that probably resolves
>> more than 90% of the issues that people may see. I'm sending that here, since it
>> still applies to the latest master branch, and I think it's a very good idea.
>>
>> To refresh you, the underlying issue I was trying to resolve was when
>> directories have many dentries (frequently, a ton of negative dentries), the
>> __fsnotify_update_child_dentry_flags() operation can take a while, and it
>> happens under spinlock.
>>
>> Case #1 - if the directory has tens of millions of dentries, then you could get
>> a soft lockup from a single call to this function. I have seen some cases where
>> a single directory had this many dentries, but it's pretty rare.
>>
>> Case #2 - suppose you have a system with many CPUs and a busy directory. Suppose
>> the directory watch is removed. The caller will begin executing
>> __fsnotify_update_child_dentry_flags() to clear the PARENT_WATCHED flag, but in
>> parallel, many other CPUs could wind up in __fsnotify_parent() and decide that
>> they, too, must call __fsnotify_update_child_dentry_flags() to clear the flags.
>> These CPUs will all spin waiting their turn, at which point they'll re-do the
>> long (and likely, useless) call. Even if the original call only took a second or
>> two, if you have a dozen or so CPUs that end up in that call, some CPUs will
>> spin a long time.
>>
>> Amir's patch to clear PARENT_WATCHED flags lazily resolves that easily. In
>> __fsnotify_parent(), if callers notice that the parent is no longer watching,
>> they merely update the flags for the current dentry (not all the other
>> children). The __fsnotify_recalc_mask() function further avoids excess calls by
>> only updating children if the parent started watching. This easily handles case
>> #2 above. Perhaps case #1 could still cause issues, for the cases of truly huge
>> dentry counts, but we shouldn't let "perfect" get in the way of "good enough" :)
>>
>
> The story sounds good :)
> Only thing I am worried about is: was case #2 tested to prove that
> the patch really imploves in practice and not only in theory?
>
> I am not asking that you write a test for this or even a reproducer
> just evidence that you collected from a case where improvement is observed
> and measurable.

I had not done so when you sent this, but I should have done it
beforehand. In any case, now I have. I got my hands on a 384-CPU machine
and extended my negative dentry creation tool so that it can run a
workload in which it constantly runs "open()" followed by "close()" on
1000 files in the same directory, per thread (so a total of 384,000
files, a large but not unreasonable amount of dentries).

Then I simply run "inotifywait /path/to/dir" a few times. Without the
patch, softlockups are easy to reproduce. With the patch, I haven't been
able to get a single soft lockup.

https://github.com/brenns10/kernel_stuff/tree/master/negdentcreate

    make
    mkdir test

    # create 384k files inside "test"
    ./negdentcreate -p test -c 384000 -t 384 -o create

    # start a loop opening and closing those files
    negdentcreate -p test -c 384000 -t 384 -o open -l

    # in another window:
    inotifywait test

Stephen

>
> Thanks,
> Amir.
>
>> [1]: https://lore.kernel.org/all/20221013222719.277923-1-stephen.s.brennan@oracle.com/
>>
>> Amir Goldstein (1):
>>   fsnotify: clear PARENT_WATCHED flags lazily
>>
>>  fs/notify/fsnotify.c             | 26 ++++++++++++++++++++------
>>  fs/notify/fsnotify.h             |  3 ++-
>>  fs/notify/mark.c                 | 32 +++++++++++++++++++++++++++++---
>>  include/linux/fsnotify_backend.h |  8 +++++---
>>  4 files changed, 56 insertions(+), 13 deletions(-)
>>
>> --
>> 2.43.0
>>