From: Stephen Brennan <stephen.s.brennan@oracle.com>
To: Jan Kara <jack@suse.cz>
Cc: Amir Goldstein <amir73il@gmail.com>, Jan Kara <jack@suse.cz>,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/1] fsnotify: clear PARENT_WATCHED flags lazily
Date: Wed, 15 May 2024 15:14:52 -0700 [thread overview]
Message-ID: <87ttiypvs3.fsf@oracle.com> (raw)
In-Reply-To: <20240515171503.loxpdv3xumgbc44w@quack3>
Jan Kara <jack@suse.cz> writes:
> On Mon 13-05-24 17:04:12, Stephen Brennan wrote:
>> Amir Goldstein <amir73il@gmail.com> writes:
>>
>> > On Fri, May 10, 2024 at 6:21 PM Stephen Brennan
>> > <stephen.s.brennan@oracle.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.
>
> Thanks for the patch and for testing! I've added your patch to my tree (not
> for this merge window though) with a cosmetic tweak that instead of
> fsnotify_update_child_dentry_flags() we just have
> fsnotify_clear_child_dentry_flag() and fsnotify_set_children_dentry_flags()
> functions to make naming somewhat clearer.
Thank you Jan! I agree that change will make it clearer when reading
code and stack traces :)
-Stephen
prev parent reply other threads:[~2024-05-15 22:15 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-10 22:18 [PATCH 0/1] fsnotify: clear PARENT_WATCHED flags lazily Stephen Brennan
2024-05-10 22:19 ` [PATCH 1/1] " Stephen Brennan
2024-05-11 3:42 ` [PATCH 0/1] " Amir Goldstein
2024-05-14 0:04 ` Stephen Brennan
2024-05-15 17:15 ` Jan Kara
2024-05-15 22:14 ` Stephen Brennan [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87ttiypvs3.fsf@oracle.com \
--to=stephen.s.brennan@oracle.com \
--cc=amir73il@gmail.com \
--cc=jack@suse.cz \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).