lkml.org 
[lkml]   [2022]   [Oct]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 2/3] fsnotify: Protect i_fsnotify_mask and child flags with inode rwsem
    On Fri, Oct 28, 2022 at 3:10 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>
    > ---

    Looking good

    Reviewed-by: Amir Goldstein <amir73il@gmail.com>

    Regardless, some nits on comment styles...

    >
    > Notes:
    > v3 changes:
    >
    > * Moved fsnotify_update_children_dentry_flags() into fsnotify.c,
    > declared it in the header. Made
    > __fsnotify_update_children_dentry_flags() static since it has no
    > external callers except fsnotify_update...().
    > * Use bitwise xor operator in children_need_update()
    > * Eliminated FSNOTIFY_OBJ_FLAG_* constants, reused CONN_FLAG_*
    > * Updated documentation of fsnotify_update_inode_conn_flags() to
    > reflect its behavior
    > * Renamed "flags" to "update_flags" in all its uses, so that it's a
    > clear pattern and matches renamed fsnotify_update_object().
    >
    > fs/notify/fsnotify.c | 45 ++++++++++-
    > fs/notify/fsnotify.h | 13 +++-
    > fs/notify/mark.c | 124 ++++++++++++++++++++-----------
    > include/linux/fsnotify_backend.h | 1 +
    > 4 files changed, 137 insertions(+), 46 deletions(-)
    >
    > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
    > index 7939aa911931..ccb8a3a6c522 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)
    > +static 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);
    > @@ -136,6 +138,43 @@ void __fsnotify_update_child_dentry_flags(struct inode *inode)
    > }
    > spin_unlock(&alias->d_lock);
    > dput(alias);
    > + return watched;
    > +}
    > +
    > +void fsnotify_update_children_dentry_flags(struct fsnotify_mark_connector *conn,
    > + struct inode *inode)
    > +{
    > + bool need_update;
    > +
    > + inode_lock(inode);
    > + spin_lock(&conn->lock);
    > + need_update = fsnotify_children_need_update(conn, inode);
    > + spin_unlock(&conn->lock);
    > + if (need_update) {
    > + bool watched = __fsnotify_update_children_dentry_flags(inode);
    > +
    > + spin_lock(&conn->lock);
    > + if (watched)
    > + conn->flags |= FSNOTIFY_CONN_FLAG_WATCHES_CHILDREN;
    > + else
    > + conn->flags &= ~FSNOTIFY_CONN_FLAG_WATCHES_CHILDREN;
    > + spin_unlock(&conn->lock);
    > + }
    > + inode_unlock(inode);
    > +}
    > +
    > +
    > +static 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.
    > + */

    Line breaks are weird.
    Seems that this comment is out of context.
    It is only true in the context of the specific call site.
    Either remove it or move it.

    > + 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? */
    > @@ -206,7 +245,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..621e78a6f0fb 100644
    > --- a/fs/notify/fsnotify.h
    > +++ b/fs/notify/fsnotify.h
    > @@ -70,11 +70,22 @@ 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);
    > + flags_set = conn->flags & FSNOTIFY_CONN_FLAG_WATCHES_CHILDREN;
    > + return 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 void fsnotify_update_children_dentry_flags(struct fsnotify_mark_connector *conn,
    > + struct inode *inode);
    >
    > extern struct kmem_cache *fsnotify_mark_connector_cachep;
    >
    > diff --git a/fs/notify/mark.c b/fs/notify/mark.c
    > index c74ef947447d..8969128dacc1 100644
    > --- a/fs/notify/mark.c
    > +++ b/fs/notify/mark.c
    > @@ -123,37 +123,52 @@ static void fsnotify_get_inode_ref(struct inode *inode)
    > }
    >
    > /*
    > - * Grab or drop inode reference for the connector if needed.
    > + * Determine the connector flags that it is necessary to update
    > *
    > - * When it's time to drop the reference, we only clear the HAS_IREF flag and
    > - * return the inode object. fsnotify_drop_object() will be resonsible for doing
    > - * iput() outside of spinlocks. This happens when last mark that wanted iref is
    > - * detached.
    > + * If any action needs to be taken on the connector's inode outside of a spinlock,
    > + * we return the inode and set *update_flags accordingly.
    > + *
    > + * If FSNOTIFY_CONN_FLAG_HAS_IREF is set in *update_flags, then the caller needs
    > + * to drop the last inode reference using fsnotify_put_inode_ref().
    > + *
    > + * If FSNOTIFY_CONN_FLAG_WATCHES_CHILDREN is set in *update_flags, then the
    > + * caller needs to update the children dentry flags so that their
    > + * DCACHE_FSNOTIFY_PARENT_WATCHED flag matches the i_fsnotify_mask value, using
    > + * fsnotify_update_children_dentry_flags().
    > */
    > -static struct inode *fsnotify_update_iref(struct fsnotify_mark_connector *conn,
    > - bool want_iref)
    > +static struct inode *fsnotify_update_inode_conn_flags(struct fsnotify_mark_connector *conn,
    > + bool want_iref, int *update_flags)
    > {
    > bool has_iref = conn->flags & FSNOTIFY_CONN_FLAG_HAS_IREF;
    > - struct inode *inode = NULL;
    > + struct inode *inode = NULL, *ret = NULL;
    >
    > - if (conn->type != FSNOTIFY_OBJ_TYPE_INODE ||
    > - want_iref == has_iref)
    > + if (conn->type != FSNOTIFY_OBJ_TYPE_INODE)
    > return NULL;
    >
    > - if (want_iref) {
    > - /* Pin inode if any mark wants inode refcount held */
    > - fsnotify_get_inode_ref(fsnotify_conn_inode(conn));
    > - conn->flags |= FSNOTIFY_CONN_FLAG_HAS_IREF;
    > - } else {
    > - /* Unpin inode after detach of last mark that wanted iref */
    > - inode = fsnotify_conn_inode(conn);
    > - conn->flags &= ~FSNOTIFY_CONN_FLAG_HAS_IREF;
    > + inode = fsnotify_conn_inode(conn);
    > +
    > + if (want_iref != has_iref) {
    > + if (want_iref) {
    > + /* Pin inode if any mark wants inode refcount held */
    > + fsnotify_get_inode_ref(inode);
    > + conn->flags |= FSNOTIFY_CONN_FLAG_HAS_IREF;
    > + } else {
    > + /* Unpin inode after detach of last mark that wanted iref */
    > + conn->flags &= ~FSNOTIFY_CONN_FLAG_HAS_IREF;
    > + ret = inode;
    > + *update_flags |= FSNOTIFY_CONN_FLAG_HAS_IREF;
    > + }
    > + }
    > + if (fsnotify_children_need_update(conn, inode)) {
    > + ret = inode;
    > + *update_flags |= FSNOTIFY_CONN_FLAG_WATCHES_CHILDREN;
    > }
    >
    > - return inode;
    > + return ret;
    > }
    >
    > -static void *__fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
    > +static void *__fsnotify_recalc_mask(struct fsnotify_mark_connector *conn,
    > + int *update_flags)
    > {
    > u32 new_mask = 0;
    > bool want_iref = false;
    > @@ -173,7 +188,7 @@ static void *__fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
    > }
    > *fsnotify_conn_mask_p(conn) = new_mask;
    >
    > - return fsnotify_update_iref(conn, want_iref);
    > + return fsnotify_update_inode_conn_flags(conn, want_iref, update_flags);
    > }
    >
    > /*
    > @@ -184,15 +199,19 @@ static void *__fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
    > */
    > void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
    > {
    > + struct inode *inode = NULL;
    > + int flags = 0;
    > +
    > if (!conn)
    > return;
    >
    > spin_lock(&conn->lock);
    > - __fsnotify_recalc_mask(conn);
    > + inode = __fsnotify_recalc_mask(conn, &flags);
    > spin_unlock(&conn->lock);
    > - if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
    > - __fsnotify_update_child_dentry_flags(
    > - fsnotify_conn_inode(conn));
    > +
    > + if (flags & FSNOTIFY_CONN_FLAG_WATCHES_CHILDREN)
    > + fsnotify_update_children_dentry_flags(conn, inode);
    > + WARN_ON_ONCE(flags & FSNOTIFY_CONN_FLAG_HAS_IREF);
    > }
    >
    > /* Free all connectors queued for freeing once SRCU period ends */
    > @@ -240,7 +259,8 @@ static void fsnotify_put_sb_connectors(struct fsnotify_mark_connector *conn)
    >
    > static void *fsnotify_detach_connector_from_object(
    > struct fsnotify_mark_connector *conn,
    > - unsigned int *type)
    > + unsigned int *type,
    > + unsigned int *update_flags)
    > {
    > struct inode *inode = NULL;
    >
    > @@ -252,8 +272,10 @@ static void *fsnotify_detach_connector_from_object(
    > inode = fsnotify_conn_inode(conn);
    > inode->i_fsnotify_mask = 0;
    >
    > - /* Unpin inode when detaching from connector */
    > - if (!(conn->flags & FSNOTIFY_CONN_FLAG_HAS_IREF))
    > + *update_flags = conn->flags &
    > + (FSNOTIFY_CONN_FLAG_HAS_IREF |
    > + FSNOTIFY_CONN_FLAG_WATCHES_CHILDREN);
    > + if (!*update_flags)
    > inode = NULL;
    > } else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
    > fsnotify_conn_mount(conn)->mnt_fsnotify_mask = 0;
    > @@ -279,15 +301,37 @@ static void fsnotify_final_mark_destroy(struct fsnotify_mark *mark)
    > fsnotify_put_group(group);
    > }
    >
    > -/* Drop object reference originally held by a connector */
    > -static void fsnotify_drop_object(unsigned int type, void *objp)
    > +/* Apply the update_flags for a connector after recalculating mask */
    > +static void fsnotify_update_object(struct fsnotify_mark_connector *conn,
    > + unsigned int type, void *objp,
    > + int update_flags)
    > {
    > if (!objp)
    > return;
    > /* Currently only inode references are passed to be dropped */
    > if (WARN_ON_ONCE(type != FSNOTIFY_OBJ_TYPE_INODE))
    > return;
    > - fsnotify_put_inode_ref(objp);
    > +
    > + if (update_flags & FSNOTIFY_CONN_FLAG_WATCHES_CHILDREN)
    > + /*.

    Stray .

    > + * At this point, we've already detached the connector from the
    > + * inode. It's entirely possible that another connector has been
    > + * attached, and that connector would assume that the children's
    > + * flags are all clear. There are two possibilities:
    > + * (a) The connector has not yet attached a mark that watches its
    > + * children. In this case, we will properly clear out the flags,
    > + * and the connector's flags will be consistent with the
    > + * children.
    > + * (b) The connector attaches a mark that watches its children.
    > + * It may have even already altered i_fsnotify_mask and/or
    > + * altered the child dentry flags. In this case, our call here
    > + * will read the correct value of i_fsnotify_mask and apply it
    > + * to the children, which duplicates some work, but isn't
    > + * harmful.
    > + */

    Multi line section after if should have {} even if there is only a
    single line of
    code (checkpatch should complain?).

    But I personally dislike large and deeply indented comments,
    so if it were me, I would change the wording a bit
    "...When detaching a connector from an inode that watches children,
    there are two possibilities:..."
    and move the huge comment above the if ().

    Thanks,
    Amir.

    \
     
     \ /
      Last update: 2022-10-28 11:11    [W:6.193 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site