From: Christian Brauner <brauner@kernel.org>
To: Stephen Brennan <stephen.s.brennan@oracle.com>
Cc: Jan Kara <jack@suse.cz>, Alexander Viro <viro@zeniv.linux.org.uk>,
linux-fsdevel@vger.kernel.org,
Amir Goldstein <amir73il@gmail.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/3] fsnotify: Protect i_fsnotify_mask and child flags with inode rwsem
Date: Fri, 21 Oct 2022 11:17:10 +0200 [thread overview]
Message-ID: <20221021091710.jxv6zi3nfkmqdmqy@wittgenstein> (raw)
In-Reply-To: <20221021010310.29521-3-stephen.s.brennan@oracle.com>
On Thu, Oct 20, 2022 at 06:03:09PM -0700, Stephen Brennan 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().
> Instead, update flags when we disconnect a mark connector. Remember the
> state of the children flags in the fsnotify_mark_connector flags.
> Provide mutual exclusion by holding i_rwsem exclusive while we update
> children, and use the cached state to avoid updating flags
> unnecessarily.
>
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> ---
>
> fs/notify/fsnotify.c | 22 ++++++-
> fs/notify/fsnotify.h | 31 ++++++++-
> fs/notify/mark.c | 106 ++++++++++++++++++++-----------
> include/linux/fsnotify_backend.h | 8 +++
> 4 files changed, 127 insertions(+), 40 deletions(-)
>
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 6c338322f0c3..f83eca4fb841 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -103,13 +103,15 @@ void fsnotify_sb_delete(struct super_block *sb)
> * parent cares. Thus when an event happens on a child it can quickly tell
> * if there is a need to find a parent and send the event to the parent.
> */
> -void __fsnotify_update_child_dentry_flags(struct inode *inode)
> +bool __fsnotify_update_children_dentry_flags(struct inode *inode)
> {
> struct dentry *alias, *child;
> int watched;
>
> if (!S_ISDIR(inode->i_mode))
> - return;
> + return false;
> +
> + lockdep_assert_held_write(&inode->i_rwsem);
>
> /* determine if the children should tell inode about their events */
> watched = fsnotify_inode_watches_children(inode);
> @@ -133,6 +135,20 @@ void __fsnotify_update_child_dentry_flags(struct inode *inode)
> spin_unlock(&child->d_lock);
> }
> spin_unlock(&alias->d_lock);
> + return watched;
> +}
> +
> +void __fsnotify_update_child_dentry_flags(struct inode *inode, struct dentry *dentry)
> +{
> + /*
> + * Flag would be cleared soon by
> + * __fsnotify_update_child_dentry_flags(), but as an
> + * optimization, clear it now.
> + */
> + spin_lock(&dentry->d_lock);
> + if (!fsnotify_inode_watches_children(inode))
> + dentry->d_flags &= ~DCACHE_FSNOTIFY_PARENT_WATCHED;
> + spin_unlock(&dentry->d_lock);
> }
>
> /* Are inode/sb/mount interested in parent and name info with this event? */
> @@ -203,7 +219,7 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
> 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);
> + __fsnotify_update_child_dentry_flags(p_inode, dentry);
>
> /*
> * Include parent/name in notification either if some notification
> diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h
> index fde74eb333cc..182d93014c6b 100644
> --- a/fs/notify/fsnotify.h
> +++ b/fs/notify/fsnotify.h
> @@ -70,11 +70,40 @@ static inline void fsnotify_clear_marks_by_sb(struct super_block *sb)
> fsnotify_destroy_marks(&sb->s_fsnotify_marks);
> }
>
> +static inline bool fsnotify_children_need_update(struct fsnotify_mark_connector *conn,
> + struct inode *inode)
> +{
> + bool watched, flags_set;
> + watched = fsnotify_inode_watches_children(inode);
nit: I'd leave a blank line after the variable declarations. Same for
fsnotify_update_children_dentry_flags() below.
> + flags_set = conn->flags & FSNOTIFY_CONN_FLAG_WATCHES_CHILDREN;
> + return (watched && !flags_set) || (!watched && flags_set);
> +}
> +
> /*
> * update the dentry->d_flags of all of inode's children to indicate if inode cares
> * about events that happen to its children.
> */
> -extern void __fsnotify_update_child_dentry_flags(struct inode *inode);
> +extern bool __fsnotify_update_children_dentry_flags(struct inode *inode);
> +
> +static inline void fsnotify_update_children_dentry_flags(struct fsnotify_mark_connector *conn,
> + struct inode *inode)
Should that be a static inline function in a header seems a bit big. :)
next prev parent reply other threads:[~2022-10-21 9:18 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
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 [this message]
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=20221021091710.jxv6zi3nfkmqdmqy@wittgenstein \
--to=brauner@kernel.org \
--cc=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).