Re: [PATCH 07/11] fsnotify: Introduce helpers to send error_events

From: Amir Goldstein <amir73il@gmail.com>
To: Gabriel Krisman Bertazi <krisman@collabora.com>
Cc: kernel@collabora.com, "Darrick J . Wong" <djwong@kernel.org>,
	"Theodore Ts'o" <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>
Subject: Re: [PATCH 07/11] fsnotify: Introduce helpers to send error_events
Date: Fri, 21 May 2021 12:32:22 +0300	[thread overview]
Message-ID: <CAOQ4uxi+xE7Obh8k=hpcf4eJcPrY2DKw1x-2+xe+ta7rvm5zZQ@mail.gmail.com> (raw)
In-Reply-To: <20210521024134.1032503-8-krisman@collabora.com>

On Fri, May 21, 2021 at 5:42 AM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>

Some maintainers are very strict about empty commit description...

> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
>
> ---
> Changes since v2:
>   - Use the inode argument (Amir)
> ---
>  include/linux/fsnotify.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index f8acddcf54fb..458e4feb5fe1 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -317,4 +317,17 @@ static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid)
>                 fsnotify_dentry(dentry, mask);
>  }
>
> +static inline void fsnotify_error_event(struct super_block *sb, struct inode *inode,
> +                                       int error)

The _event in the helper name is inconsistent with the rest of the helpers.
I would go with fsnotify_sb_error(), especially if you agree with me about
allowing FAN_ERROR only on sb marks.
I would also consider FAN_FS_ERROR (?) to reduce ambiguity in the future
with FAN_WB_ERROR.

> +{
> +       if (sb->s_fsnotify_marks) {
> +               struct fs_error_report report = {
> +                       .error = error,
> +                       .inode = inode,
> +               };
> +               fsnotify(FS_ERROR, &report, FSNOTIFY_EVENT_ERROR, NULL, NULL,
> +                        sb->s_root->d_inode, 0);

This is a bit hacky.
It *may* be acceptable if we allow FAN_ERROR only for sb marks, but if we
allow to set them on inode marks, this is wrong, because it will only report
events for all inodes only to an inode mark that was set on the root inode.

If you want a clean solution:
1) Take this cleanup patch from [1]
    "fsnotify: pass arguments of fsnotify() in struct fsnotify_event_info"
2) Add sb field to fsnotify_event_info
3) In fsnotify() do not assume that inode is non-NULL:
-       struct super_block *sb;
+       struct inode *inode = event_info->inode;
+       struct super_block *sb = event_info->sb ?: inode->i_sb;

There are too many args to fsnotify() already, so the cleanup patch
is due anyway.

Thanks,
Amir.

[1] https://github.com/amir73il/linux/commits/fanotify_idmapped