Date: Wed, 15 May 2024 19:15:03 +0200
From: Jan Kara <jack@...e.cz>
To: Stephen Brennan <stephen.s.brennan@...cle.com>
Cc: Amir Goldstein <amir73il@...il.com>, 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

On Mon 13-05-24 17:04:12, Stephen Brennan wrote:
> 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.

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.

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists