From: Amir Goldstein <amir73il@gmail.com>
To: Stephen Brennan <stephen.s.brennan@oracle.com>
Cc: Jan Kara <jack@suse.cz>, Alexander Viro <viro@zeniv.linux.org.uk>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] fsnotify: Protect i_fsnotify_mask and child flags with inode rwsem
Date: Fri, 21 Oct 2022 10:22:42 +0300 [thread overview]
Message-ID: <CAOQ4uxg0u+eEpyjW7ACn60wXDG1OAWZ8YF0H_OyTgZ2YLxQsXQ@mail.gmail.com> (raw)
In-Reply-To: <87y1taggmu.fsf@oracle.com>
()
On Fri, Oct 21, 2022 at 3:33 AM Stephen Brennan
<stephen.s.brennan@oracle.com> wrote:
>
> Amir Goldstein <amir73il@gmail.com> writes:
>
> > On Tue, Oct 18, 2022 at 7:12 AM Stephen Brennan
> > <stephen.s.brennan@oracle.com> wrote:
> >>
> >> When an inode is interested in events on its children, it must set
> >> DCACHE_FSNOTIFY_PARENT_WATCHED flag on all its children. Currently, when
> >> the fsnotify connector is removed and i_fsnotify_mask becomes zero, we
> >> lazily allow __fsnotify_parent() to do this the next time we see an
> >> event on a child.
> >>
> >> However, if the list of children is very long (e.g., in the millions),
> >> and lots of activity is occurring on the directory, then it's possible
> >> for many CPUs to end up blocked on the inode spinlock in
> >> __fsnotify_update_child_flags(). Each CPU will then redundantly iterate
> >> over the very long list of children. This situation can cause soft
> >> lockups.
> >>
> >> To avoid this, stop lazily updating child flags in __fsnotify_parent().
> >> Protect the child flag update with i_rwsem held exclusive, to ensure
> >> that we only iterate over the child list when it's absolutely necessary,
> >> and even then, only once.
> >>
> >> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> >> ---
> >>
> >> Notes:
> >>
> >> It seems that there are two implementation options for this, regarding
> >> what i_rwsem protects:
> >>
> >> 1. Both updates to i_fsnotify_mask, and the child dentry flags, or
> >> 2. Only updates to the child dentry flags
> >>
> >> I wanted to do #1, but it got really tricky with fsnotify_put_mark(). We
> >> don't want to hold the inode lock whenever we decrement the refcount,
> >> but if we don't, then we're stuck holding a spinlock when the refcount
> >> goes to zero, and we need to grab the inode rwsem to synchronize the
> >> update to the child flags. I'm sure there's a way around this, but I
> >> didn't keep going with it.
> >>
> >> With #1, as currently implemented, we have the unfortunate effect of
> >> that a mark can be added, can see that no update is required, and
> >> return, despite the fact that the flag update is still in progress on a
> >> different CPU/thread. From our discussion, that seems to be the current
> >> status quo, but I wanted to explicitly point that out. If we want to
> >> move to #1, it should be possible with some work.
> >
> > I think the solution may be to store the state of children in conn
> > like you suggested.
> >
> > See fsnotify_update_iref() and conn flag
> > FSNOTIFY_CONN_FLAG_HAS_IREF.
> >
> > You can add a conn flag
> > FSNOTIFY_CONN_FLAG_WATCHES_CHILDREN
> > that caches the result of the last invocation of update children flags.
> >
> > For example, fsnotify_update_iref() becomes
> > fsnotify_update_inode_conn_flags() and
> > returns inode if either inode ref should be dropped
> > or if children flags need to be updated (or both)
> > maybe use some out argument to differentiate the cases.
> > Same for fsnotify_detach_connector_from_object().
> >
> > Then, where fsnotify_drop_object() is called, for the
> > case that inode children need to be updated,
> > take inode_lock(), take connector spin lock
> > to check if another thread has already done the update
> > if not release spin lock, perform the update under inode lock
> > and at the end, take spin lock again and set the
> > FSNOTIFY_CONN_FLAG_WATCHES_CHILDREN
> > connector flag.
> >
> > Not sure if it all works out... maybe
>
> I did this for v2 and I think it has worked well, all threads seem to
> block until the flags are updated on all dentries.
>
It did work out pretty well. v2 looks nice :)
> >>
> >> fs/notify/fsnotify.c | 12 ++++++++--
> >> fs/notify/mark.c | 55 ++++++++++++++++++++++++++++++++++----------
> >> 2 files changed, 53 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> >> index 7974e91ffe13..e887a195983b 100644
> >> --- a/fs/notify/fsnotify.c
> >> +++ b/fs/notify/fsnotify.c
> >> @@ -207,8 +207,16 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
> >> parent = dget_parent(dentry);
> >> p_inode = parent->d_inode;
> >> p_mask = fsnotify_inode_watches_children(p_inode);
> >> - if (unlikely(parent_watched && !p_mask))
> >> - __fsnotify_update_child_dentry_flags(p_inode);
> >> + if (unlikely(parent_watched && !p_mask)) {
> >> + /*
> >> + * Flag would be cleared soon by
> >> + * __fsnotify_update_child_dentry_flags(), but as an
> >> + * optimization, clear it now.
> >> + */
> >
> > I think that we need to also take p_inode spin_lock here and
> > check fsnotify_inode_watches_children() under lock
> > otherwise, we could be clearing the WATCHED flag
> > *after* __fsnotify_update_child_dentry_flags() had
> > already set it, because you we not observe the change to
> > p_inode mask.
>
> I'm not sure I follow. The i_fsnotify_mask field isn't protected by the
> p_inode spinlock. It isn't really protected at all, though it mainly
> gets modified with the conn->lock held.
>
Right.
> Wouldn't it be sufficient to do in a new helper:
>
> spin_lock(&dentry->d_lock);
> if (!fsnotify_inode_watches_children(p_inode))
> dentry->d_flags &= ~DCACHE_FSNOTIFY_PARENT_WATCHED;
> spin_unlock(&dentry->d_lock);
>
Yes, I think this is better.
I am not sure if that data race can lead to the wrong state of dentry
flags forever on upstream, but I'm pretty sure that it can on v1.
> I'm sure I'm missing something about your comment. For the moment I left
> it as is in the second version of the patch, we can discuss it more and
> I can update it for a v3.
>
v1 doesn't have a memory barrier between loading value into
parent_watched and into p_mask, so the check wasn't safe.
In v2, __fsnotify_update_child_dentry_flags() rechecks
fsnotify_inode_watches_children() after the the barrier provided
by the dentry spin lock, so the "lockless" check is safe:
Writer:
store i_fsnotify_mask
smp_store_release(d_lock) // spin unlock
store d_flags
Reader:
load d_flags
smp_load_acquire(d_lock) // spin_lock
load i_fsnotify_mask
Best resource I like for refreshing my "lockless" skills:
https://lwn.net/Articles/844224/
> >
> > I would consider renaming __fsnotify_update_child_dentry_flags()
> > to __fsnotify_update_children_dentry_flags(struct inode *dir)
> >
> > and creating another inline helper for this call site called:
> > fsnotify_update_child_dentry_flags(struct inode *dir, struct dentry *child)
> >
> >
> >> + spin_lock(&dentry->d_lock);
> >> + dentry->d_flags &= ~DCACHE_FSNOTIFY_PARENT_WATCHED;
> >> + spin_unlock(&dentry->d_lock);
> >> + }
> >>
> >> /*
> >> * Include parent/name in notification either if some notification
> >> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> >> index c74ef947447d..da9f944fcbbb 100644
> >> --- a/fs/notify/mark.c
> >> +++ b/fs/notify/mark.c
> >> @@ -184,15 +184,36 @@ static void *__fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
> >> */
> >> void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
> >> {
> >> + struct inode *inode = NULL;
> >> + int watched_before, watched_after;
> >> +
> >> if (!conn)
> >> return;
> >>
> >> - spin_lock(&conn->lock);
> >> - __fsnotify_recalc_mask(conn);
> >> - spin_unlock(&conn->lock);
> >> - if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
> >> - __fsnotify_update_child_dentry_flags(
> >> - fsnotify_conn_inode(conn));
> >> + if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) {
> >> + /*
> >> + * For inodes, we may need to update flags on the child
> >> + * dentries. To ensure these updates occur exactly once,
> >> + * synchronize the recalculation with the inode mutex.
> >> + */
> >> + inode = fsnotify_conn_inode(conn);
> >> + spin_lock(&conn->lock);
> >> + watched_before = fsnotify_inode_watches_children(inode);
> >> + __fsnotify_recalc_mask(conn);
> >> + watched_after = fsnotify_inode_watches_children(inode);
> >> + spin_unlock(&conn->lock);
> >> +
> >> + inode_lock(inode);
> >
> > With the pattern that I suggested above, this if / else would
> > be unified to code that looks something like this:
> >
> > spin_lock(&conn->lock);
> > inode = __fsnotify_recalc_mask(conn);
> > spin_unlock(&conn->lock);
> >
> > if (inode)
> > fsnotify_update_children_dentry_flags(conn, inode);
> >
> > Where fsnotify_update_children_dentry_flags()
> > takes inode lock around entire update and conn spin lock
> > only around check and update of conn flags.
> >
> > FYI, at this time in the code, adding a mark or updating
> > existing mark mask cannot result in the need to drop iref.
> > That is the reason that return value of __fsnotify_recalc_mask()
> > is not checked here.
>
> For v3 I tried this with a new "flags" out variable and two flags - one
> for requiring an iput(), and one for calling
> fsnotify_update_children_dentry_flags(). As a result, I did stick a
> WARN_ON_ONCE here, but it more or less looks just like this code :)
>
Cool.
> >> + if ((watched_before && !watched_after) ||
> >> + (!watched_before && watched_after)) {
> >> + __fsnotify_update_child_dentry_flags(inode);
> >> + }
> >> + inode_unlock(inode);
> >> + } else {
> >> + spin_lock(&conn->lock);
> >> + __fsnotify_recalc_mask(conn);
> >> + spin_unlock(&conn->lock);
> >> + }
> >> }
> >>
> >> /* Free all connectors queued for freeing once SRCU period ends */
> >> @@ -295,6 +316,8 @@ void fsnotify_put_mark(struct fsnotify_mark *mark)
> >> struct fsnotify_mark_connector *conn = READ_ONCE(mark->connector);
> >> void *objp = NULL;
> >> unsigned int type = FSNOTIFY_OBJ_TYPE_DETACHED;
> >> + struct inode *inode = NULL;
> >> + int watched_before, watched_after;
> >> bool free_conn = false;
> >>
> >> /* Catch marks that were actually never attached to object */
> >> @@ -311,17 +334,31 @@ void fsnotify_put_mark(struct fsnotify_mark *mark)
> >> if (!refcount_dec_and_lock(&mark->refcnt, &conn->lock))
> >> return;
> >>
> >> + if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) {
> >> + inode = fsnotify_conn_inode(conn);
> >> + watched_before = fsnotify_inode_watches_children(inode);
> >> + }
> >> +
> >> hlist_del_init_rcu(&mark->obj_list);
> >> if (hlist_empty(&conn->list)) {
> >> objp = fsnotify_detach_connector_from_object(conn, &type);
> >> free_conn = true;
> >> + watched_after = 0;
> >> } else {
> >> objp = __fsnotify_recalc_mask(conn);
> >> type = conn->type;
> >> + watched_after = fsnotify_inode_watches_children(inode);
> >> }
> >> WRITE_ONCE(mark->connector, NULL);
> >> spin_unlock(&conn->lock);
> >>
> >> + if (inode) {
> >> + inode_lock(inode);
> >> + if (watched_before && !watched_after)
> >> + __fsnotify_update_child_dentry_flags(inode);
> >> + inode_unlock(inode);
> >> + }
> >> +
> >> fsnotify_drop_object(type, objp);
> >>
> >
> > Here as well something like:
> > if (objp)
> > fsnotify_update_children_dentry_flags(conn, obj);
> >
> > But need to distinguish when inode ref needs to be dropped
> > children flags updates or both.
>
> With a flags out-param, it works well. I actually was able to stuff this
> into fsnotify_drop_object, which was good because I had missed a whole
> other function that can detach a connector from an inode
> (fsnotify_destroy_marks()).
>
Haha, this call is coming from __fsnotify_inode_delete()
the evicted dir inode cannot have any children in dache,
but it's good to have robust code ;)
Thanks,
Amir.
next prev parent reply other threads:[~2022-10-21 7:23 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-13 22:27 [RFC] fsnotify: allow sleepable child dentry flag update Stephen Brennan
2022-10-13 23:51 ` Al Viro
2022-11-01 21:47 ` Stephen Brennan
2022-10-14 8:01 ` Amir Goldstein
2022-10-17 7:59 ` Stephen Brennan
2022-10-17 11:44 ` Amir Goldstein
2022-10-17 16:59 ` Stephen Brennan
2022-10-17 17:42 ` Amir Goldstein
2022-10-17 9:09 ` Jan Kara
2022-10-18 4:12 ` [PATCH 0/2] fsnotify: fix softlockups iterating over d_subdirs Stephen Brennan
2022-10-18 4:12 ` [PATCH 1/2] fsnotify: Protect i_fsnotify_mask and child flags with inode rwsem Stephen Brennan
2022-10-18 7:39 ` Amir Goldstein
2022-10-21 0:33 ` Stephen Brennan
2022-10-21 7:22 ` Amir Goldstein [this message]
2022-10-18 4:12 ` [PATCH 2/2] fsnotify: allow sleepable child flag update Stephen Brennan
2022-10-18 5:36 ` Amir Goldstein
2022-10-27 7:50 ` kernel test robot
2022-10-27 8:44 ` Yujie Liu
2022-10-27 22:12 ` Stephen Brennan
2022-10-18 8:07 ` [PATCH 0/2] fsnotify: fix softlockups iterating over d_subdirs Amir Goldstein
2022-10-18 23:52 ` Stephen Brennan
2022-10-19 5:33 ` Amir Goldstein
2022-10-27 22:06 ` Stephen Brennan
2022-10-28 8:58 ` Amir Goldstein
2022-10-21 1:03 ` [PATCH v2 0/3] " Stephen Brennan
2022-10-21 1:03 ` [PATCH v2 1/3] fsnotify: Use d_find_any_alias to get dentry associated with inode Stephen Brennan
2022-10-21 9:25 ` Amir Goldstein
2022-10-21 1:03 ` [PATCH v2 2/3] fsnotify: Protect i_fsnotify_mask and child flags with inode rwsem Stephen Brennan
2022-10-21 4:01 ` kernel test robot
2022-10-21 8:22 ` Amir Goldstein
2022-10-21 9:18 ` Amir Goldstein
2022-10-25 18:02 ` Stephen Brennan
2022-10-26 5:41 ` Amir Goldstein
2022-10-21 9:17 ` Christian Brauner
2022-10-21 9:21 ` Amir Goldstein
2022-10-21 1:03 ` [PATCH v2 3/3] fsnotify: allow sleepable child flag update Stephen Brennan
2022-10-28 0:10 ` [PATCH v3 0/3] fsnotify: fix softlockups iterating over d_subdirs Stephen Brennan
2022-10-28 0:10 ` [PATCH v3 1/3] fsnotify: Use d_find_any_alias to get dentry associated with inode Stephen Brennan
2022-11-10 1:12 ` Stephen Brennan
2022-10-28 0:10 ` [PATCH v3 2/3] fsnotify: Protect i_fsnotify_mask and child flags with inode rwsem Stephen Brennan
2022-10-28 9:11 ` Amir Goldstein
2022-11-10 0:03 ` kernel test robot
2022-11-10 1:06 ` Stephen Brennan
2022-10-28 0:10 ` [PATCH v3 3/3] fsnotify: allow sleepable child flag update Stephen Brennan
2022-10-28 9:32 ` Amir Goldstein
2022-11-01 21:25 ` Stephen Brennan
2022-11-01 17:51 ` [PATCH v3 0/3] fsnotify: fix softlockups iterating over d_subdirs Jan Kara
2022-11-01 20:48 ` Stephen Brennan
2022-11-02 8:55 ` Amir Goldstein
2022-11-10 20:04 ` Stephen Brennan
2022-11-02 17:52 ` Jan Kara
2022-11-04 23:33 ` Stephen Brennan
2022-11-07 11:56 ` Jan Kara
2022-11-11 22:06 ` [PATCH v4 0/5] " Stephen Brennan
2022-11-11 22:06 ` [PATCH v4 1/5] fsnotify: clear PARENT_WATCHED flags lazily Stephen Brennan
2022-11-11 22:06 ` [PATCH v4 2/5] fsnotify: Use d_find_any_alias to get dentry associated with inode Stephen Brennan
2022-11-12 8:53 ` Amir Goldstein
2022-11-11 22:06 ` [PATCH v4 3/5] dnotify: move fsnotify_recalc_mask() outside spinlock Stephen Brennan
2022-11-12 9:06 ` Amir Goldstein
2022-11-11 22:06 ` [PATCH v4 4/5] fsnotify: allow sleepable child flag update Stephen Brennan
2022-11-12 10:00 ` Amir Goldstein
2022-11-15 7:10 ` kernel test robot
2022-11-11 22:06 ` [PATCH v4 5/5] fsnotify: require inode lock held during " Stephen Brennan
2022-11-12 9:42 ` Amir Goldstein
2022-11-11 22:08 ` [PATCH v4 0/5] fsnotify: fix softlockups iterating over d_subdirs Stephen Brennan
2022-11-22 11:50 ` Jan Kara
2022-11-22 14:03 ` Amir Goldstein
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=CAOQ4uxg0u+eEpyjW7ACn60wXDG1OAWZ8YF0H_OyTgZ2YLxQsXQ@mail.gmail.com \
--to=amir73il@gmail.com \
--cc=jack@suse.cz \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=stephen.s.brennan@oracle.com \
--cc=viro@zeniv.linux.org.uk \
/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 NNTP newsgroup(s).