Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221018041233.376977-1-stephen.s.brennan@oracle.com>
 <20221021010310.29521-1-stephen.s.brennan@oracle.com> <20221021010310.29521-2-stephen.s.brennan@oracle.com>
In-Reply-To: <20221021010310.29521-2-stephen.s.brennan@oracle.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Fri, 21 Oct 2022 12:25:57 +0300
Message-ID: <CAOQ4uxgxN+-Q+C7fHJ7PwEbM0GVH9mVKiAzdpCb+wFd6M=zsuQ@mail.gmail.com>
Subject: Re: [PATCH v2 1/3] fsnotify: Use d_find_any_alias to get dentry
 associated with inode
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, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Oct 21, 2022 at 4:03 AM Stephen Brennan
<stephen.s.brennan@oracle.com> wrote:
>
> Rather than iterating over the inode's i_dentry (requiring holding the
> i_lock for the entire duration of the function), we know that there
> should be only one item in the list. Use d_find_any_alias() and no
> longer hold i_lock.
>
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>

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

one nit below

> ---
>  fs/notify/fsnotify.c | 41 ++++++++++++++++++-----------------------
>  1 file changed, 18 insertions(+), 23 deletions(-)
>
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 7974e91ffe13..6c338322f0c3 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -105,7 +105,7 @@ void fsnotify_sb_delete(struct super_block *sb)
>   */
>  void __fsnotify_update_child_dentry_flags(struct inode *inode)
>  {
> -       struct dentry *alias;
> +       struct dentry *alias, *child;
>         int watched;
>
>         if (!S_ISDIR(inode->i_mode))
> @@ -114,30 +114,25 @@ void __fsnotify_update_child_dentry_flags(struct inode *inode)
>         /* determine if the children should tell inode about their events */
>         watched = fsnotify_inode_watches_children(inode);
>
> -       spin_lock(&inode->i_lock);
> -       /* run all of the dentries associated with this inode.  Since this is a
> -        * directory, there damn well better only be one item on this list */
> -       hlist_for_each_entry(alias, &inode->i_dentry, d_u.d_alias) {
> -               struct dentry *child;
> -
> -               /* run all of the children of the original inode and fix their
> -                * d_flags to indicate parental interest (their parent is the
> -                * original inode) */
> -               spin_lock(&alias->d_lock);
> -               list_for_each_entry(child, &alias->d_subdirs, d_child) {
> -                       if (!child->d_inode)
> -                               continue;
> +       /* Since this is a directory, there damn well better only be one child */
> +       alias = d_find_any_alias(inode);
>
> -                       spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
> -                       if (watched)
> -                               child->d_flags |= DCACHE_FSNOTIFY_PARENT_WATCHED;
> -                       else
> -                               child->d_flags &= ~DCACHE_FSNOTIFY_PARENT_WATCHED;
> -                       spin_unlock(&child->d_lock);
> -               }
> -               spin_unlock(&alias->d_lock);
> +       /* run all of the children of the original inode and fix their
> +        * d_flags to indicate parental interest (their parent is the
> +        * original inode) */

Please add new lines at the beginning and end of this multi line comment.

Thanks,
Amir.