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: <20221028001016.332663-1-stephen.s.brennan@oracle.com>
 <20221111220614.991928-1-stephen.s.brennan@oracle.com> <20221111220614.991928-4-stephen.s.brennan@oracle.com>
In-Reply-To: <20221111220614.991928-4-stephen.s.brennan@oracle.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Sat, 12 Nov 2022 11:06:42 +0200
Message-ID: <CAOQ4uxgK6H_zaCRZG3FvUhD7-28-P79qPTTmLUD4t0XY3LakbQ@mail.gmail.com>
Subject: Re: [PATCH v4 3/5] dnotify: move fsnotify_recalc_mask() outside spinlock
To:     Stephen Brennan <stephen.s.brennan@oracle.com>
Cc:     Jan Kara <jack@suse.cz>, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, Al Viro <viro@zeniv.linux.org.uk>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Nov 12, 2022 at 12:06 AM Stephen Brennan
<stephen.s.brennan@oracle.com> wrote:
>
> In order to allow sleeping during fsnotify_recalc_mask(), we need to
> ensure no callers are holding a spinlock.
>
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>

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

small suggestion below.

> ---
>  fs/notify/dnotify/dnotify.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
> index 190aa717fa32..a9f05b3cf5ea 100644
> --- a/fs/notify/dnotify/dnotify.c
> +++ b/fs/notify/dnotify/dnotify.c
> @@ -58,10 +58,10 @@ struct dnotify_mark {
>   * dnotify cares about for that inode may change.  This function runs the
>   * list of everything receiving dnotify events about this directory and calculates
>   * the set of all those events.  After it updates what dnotify is interested in
> - * it calls the fsnotify function so it can update the set of all events relevant
> - * to this inode.
> + * it returns true if fsnotify_recalc_mask() should be called to update the set
> + * of all events related to this inode.
>   */
> -static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
> +static bool dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
>  {
>         __u32 new_mask = 0;
>         struct dnotify_struct *dn;
> @@ -74,10 +74,9 @@ static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
>         for (dn = dn_mark->dn; dn != NULL; dn = dn->dn_next)
>                 new_mask |= (dn->dn_mask & ~FS_DN_MULTISHOT);
>         if (fsn_mark->mask == new_mask)
> -               return;
> +               return false;
>         fsn_mark->mask = new_mask;
> -
> -       fsnotify_recalc_mask(fsn_mark->connector);
> +       return true;
>  }
>
>  /*
> @@ -97,6 +96,7 @@ static int dnotify_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
>         struct dnotify_struct **prev;
>         struct fown_struct *fown;
>         __u32 test_mask = mask & ~FS_EVENT_ON_CHILD;
> +       bool recalc = false;
>
>         /* not a dir, dnotify doesn't care */
>         if (!dir && !(mask & FS_ISDIR))
> @@ -118,12 +118,15 @@ static int dnotify_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
>                 else {
>                         *prev = dn->dn_next;
>                         kmem_cache_free(dnotify_struct_cache, dn);
> -                       dnotify_recalc_inode_mask(inode_mark);
> +                       recalc = dnotify_recalc_inode_mask(inode_mark);
>                 }
>         }
>
>         spin_unlock(&inode_mark->lock);
>
> +       if (recalc)
> +               fsnotify_recalc_mask(inode_mark->connector);
> +
>         return 0;
>  }
>
> @@ -158,6 +161,7 @@ void dnotify_flush(struct file *filp, fl_owner_t id)
>         struct dnotify_struct **prev;
>         struct inode *inode;
>         bool free = false;
> +       bool recalc = false;
>
>         inode = file_inode(filp);
>         if (!S_ISDIR(inode->i_mode))
> @@ -176,7 +180,7 @@ void dnotify_flush(struct file *filp, fl_owner_t id)
>                 if ((dn->dn_owner == id) && (dn->dn_filp == filp)) {
>                         *prev = dn->dn_next;
>                         kmem_cache_free(dnotify_struct_cache, dn);
> -                       dnotify_recalc_inode_mask(fsn_mark);
> +                       recalc = dnotify_recalc_inode_mask(fsn_mark);
>                         break;
>                 }
>                 prev = &dn->dn_next;
> @@ -184,6 +188,9 @@ void dnotify_flush(struct file *filp, fl_owner_t id)
>
>         spin_unlock(&fsn_mark->lock);
>
> +       if (recalc)
> +               fsnotify_recalc_mask(fsn_mark->connector);
> +
>         /* nothing else could have found us thanks to the dnotify_groups
>            mark_mutex */
>         if (dn_mark->dn == NULL) {
> @@ -268,6 +275,7 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
>         struct file *f;
>         int destroy = 0, error = 0;
>         __u32 mask;
> +       bool recalc = false;
>
>         /* we use these to tell if we need to kfree */
>         new_fsn_mark = NULL;
> @@ -377,9 +385,11 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
>         else if (error == -EEXIST)
>                 error = 0;
>
> -       dnotify_recalc_inode_mask(fsn_mark);
> +       recalc = dnotify_recalc_inode_mask(fsn_mark);
>  out:
>         spin_unlock(&fsn_mark->lock);
> +       if (recalc)
> +               fsnotify_recalc_mask(fsn_mark->connector);
>
>         if (destroy)
>                 fsnotify_detach_mark(fsn_mark);

I'd do else if (recalc)

just to emphasise that destroy and recalc are mutually exclusive.

Thanks,
Amir.