Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210629191035.681913-1-krisman@collabora.com> <20210629191035.681913-9-krisman@collabora.com>
In-Reply-To: <20210629191035.681913-9-krisman@collabora.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Wed, 30 Jun 2021 06:40:00 +0300
Message-ID: <CAOQ4uxhbRvZXTQ9c2V7KFFDs0pYw05tgKruJTXr8AmJCKKTauw@mail.gmail.com>
Subject: Re: [PATCH v3 08/15] fsnotify: Support passing argument to insert
 callback on add_event
To:     Gabriel Krisman Bertazi <krisman@collabora.com>
Cc:     "Darrick J. Wong" <djwong@kernel.org>,
        Theodore Tso <tytso@mit.edu>,
        Dave Chinner <david@fromorbit.com>, Jan Kara <jack@suse.com>,
        David Howells <dhowells@redhat.com>,
        Khazhismel Kumykov <khazhy@google.com>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Ext4 <linux-ext4@vger.kernel.org>, kernel@collabora.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jun 29, 2021 at 10:12 PM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> FAN_FS_ERROR requires some initialization to happen from inside the
> insert hook.  This allows a user of fanotify_add_event to pass an
> argument to be sent to the insert callback.
>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>

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

with optional suggestion for improvement...

> ---
>  fs/notify/fanotify/fanotify.c        | 5 +++--
>  fs/notify/inotify/inotify_fsnotify.c | 2 +-
>  fs/notify/notification.c             | 6 ++++--
>  include/linux/fsnotify_backend.h     | 7 +++++--
>  4 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 4f2febb15e94..aba06b84da91 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -695,7 +695,8 @@ static __kernel_fsid_t fanotify_get_fsid(struct fsnotify_iter_info *iter_info)
>   * Add an event to hash table for faster merge.
>   */
>  static void fanotify_insert_event(struct fsnotify_group *group,
> -                                 struct fsnotify_event *fsn_event)
> +                                 struct fsnotify_event *fsn_event,
> +                                 const void *data)
>  {
>         struct fanotify_event *event = FANOTIFY_E(fsn_event);
>         unsigned int bucket = fanotify_event_hash_bucket(group, event);
> @@ -779,7 +780,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,
>
>         fsn_event = &event->fse;
>         ret = fsnotify_add_event(group, fsn_event, fanotify_merge,
> -                                fanotify_insert_event);
> +                                fanotify_insert_event, NULL);
>         if (ret) {
>                 /* Permission events shouldn't be merged */
>                 BUG_ON(ret == 1 && mask & FANOTIFY_PERM_EVENTS);
> diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
> index d1a64daa0171..a003a64ff8ee 100644
> --- a/fs/notify/inotify/inotify_fsnotify.c
> +++ b/fs/notify/inotify/inotify_fsnotify.c
> @@ -116,7 +116,7 @@ int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask,
>         if (len)
>                 strcpy(event->name, name->name);
>
> -       ret = fsnotify_add_event(group, fsn_event, inotify_merge, NULL);
> +       ret = fsnotify_add_event(group, fsn_event, inotify_merge, NULL, NULL);
>         if (ret) {
>                 /* Our event wasn't used in the end. Free it. */
>                 fsnotify_destroy_event(group, fsn_event);
> diff --git a/fs/notify/notification.c b/fs/notify/notification.c
> index 32f45543b9c6..0d9ba592d725 100644
> --- a/fs/notify/notification.c
> +++ b/fs/notify/notification.c
> @@ -83,7 +83,9 @@ int fsnotify_add_event(struct fsnotify_group *group,
>                        int (*merge)(struct fsnotify_group *,
>                                     struct fsnotify_event *),
>                        void (*insert)(struct fsnotify_group *,
> -                                     struct fsnotify_event *))
> +                                     struct fsnotify_event *,
> +                                     const void *),
> +                      const void *insert_data)
>  {
>         int ret = 0;
>         struct list_head *list = &group->notification_list;
> @@ -121,7 +123,7 @@ int fsnotify_add_event(struct fsnotify_group *group,
>         group->q_len++;
>         list_add_tail(&event->list, list);
>         if (insert)
> -               insert(group, event);
> +               insert(group, event, insert_data);
>         spin_unlock(&group->notification_lock);
>
>         wake_up(&group->notification_waitq);
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index b1590f654ade..8222fe12a6c9 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -526,11 +526,14 @@ extern int fsnotify_add_event(struct fsnotify_group *group,
>                               int (*merge)(struct fsnotify_group *,
>                                            struct fsnotify_event *),
>                               void (*insert)(struct fsnotify_group *,
> -                                            struct fsnotify_event *));
> +                                            struct fsnotify_event *,
> +                                            const void *),
> +                             const void *insert_data);
> +
>  /* Queue overflow event to a notification group */
>  static inline void fsnotify_queue_overflow(struct fsnotify_group *group)
>  {
> -       fsnotify_add_event(group, group->overflow_event, NULL, NULL);
> +       fsnotify_add_event(group, group->overflow_event, NULL, NULL, NULL);
>  }
>

Looking back, when I added the insert() callback, it might have been wiser to
rename fsnotify_add_event() to fsnotify_insert_event() add a wrapper:

static inline int fsnotify_add_event(group, event, merge) {
    return fsnotify_insert_event(group, event, merge, NULL);
}

But the two call sites did not seem to justify it.
Perhaps with the growing list of NULL arguments it is time to
reconsider this small tidy up?
Not critical though.

Thanks,
Amir.