Received-SPF: pass (google.com: domain of linux-kernel+bounces-176410-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240510221901.520546-1-stephen.s.brennan@oracle.com>
In-Reply-To: <20240510221901.520546-1-stephen.s.brennan@oracle.com>
From: Amir Goldstein <amir73il@gmail.com>
Date: Fri, 10 May 2024 23:42:34 -0400
Message-ID: <CAOQ4uxjKdkXLi6w2az9a3dnEkX1-w771ZUz1Lr2ToFFUGvf8Ng@mail.gmail.com>
Subject: Re: [PATCH 0/1] fsnotify: clear PARENT_WATCHED flags lazily
To: Stephen Brennan <stephen.s.brennan@oracle.com>
Cc: Jan Kara <jack@suse.cz>, linux-fsdevel@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, May 10, 2024 at 6:21=E2=80=AFPM 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 aim=
ed to
> make __fsnotify_update_child_dentry_flags() a sleepable function. That wo=
rk 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 a=
ble 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 wi=
th the
> larger change, there was one patch which Amir shared, that probably resol=
ves
> more than 90% of the issues that people may see. I'm sending that here, s=
ince it
> still applies to the latest master branch, and I think it's a very good i=
dea.
>
> 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 cou=
ld 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 se=
cond or
> two, if you have a dozen or so CPUs that end up in that call, some CPUs w=
ill
> spin a long time.
>
> Amir's patch to clear PARENT_WATCHED flags lazily resolves that easily. I=
n
> __fsnotify_parent(), if callers notice that the parent is no longer watch=
ing,
> they merely update the flags for the current dentry (not all the other
> children). The __fsnotify_recalc_mask() function further avoids excess ca=
lls by
> only updating children if the parent started watching. This easily handle=
s case
> #2 above. Perhaps case #1 could still cause issues, for the cases of trul=
y huge
> dentry counts, but we shouldn't let "perfect" get in the way of "good eno=
ugh" :)
>

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.

Thanks,
Amir.

> [1]: https://lore.kernel.org/all/20221013222719.277923-1-stephen.s.brenna=
n@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
>