From: Amir Goldstein <amir73il@gmail.com>
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
Subject: Re: [PATCH v3 12/15] fanotify: Introduce FAN_FS_ERROR event
Date: Wed, 30 Jun 2021 17:03:32 +0300 [thread overview]
Message-ID: <CAOQ4uxiQc+g5dRzTOVzesZTffVkX7o-bedd_MB9SmG4Unex6wg@mail.gmail.com> (raw)
In-Reply-To: <20210629191035.681913-13-krisman@collabora.com>
On Tue, Jun 29, 2021 at 10:13 PM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> The FAN_FS_ERROR event is a new inode event used by filesystem wide
> monitoring tools to receive notifications of type FS_ERROR_EVENT,
> emitted directly by filesystems when a problem is detected. The error
> notification includes a generic error descriptor and a FID identifying
> the file affected.
>
> FID is sent for every FAN_FS_ERROR. Errors not linked to a regular inode
> are reported against the root inode.
>
> An error reporting structure is attached per-mark, and only a single
> error can be stored at a time. This is ok, since once an error occurs,
> it is common for a stream of related errors to be reported. We only log
> accumulate the total of errors occurred since the last notification.
>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
>
> ---
> Changes since v2:
> - Support and equire FID mode (amir)
> - Goto error path instead of early return (amir)
> - Simplify get_one_event (me)
About that...
> - Base merging on error_count
> - drop fanotify_queue_error_event
[...]
> +static int fanotify_merge_error_event(struct fsnotify_group *group,
> + struct fsnotify_event *event)
> +{
> + struct fanotify_event *fae = FANOTIFY_E(event);
> +
> + /*
> + * When err_count > 0, the reporting slot is full. Just account
> + * the additional error and abort the insertion.
> + */
> + if (atomic_fetch_inc(&FANOTIFY_EE(fae)->err_count) != 0)
> + return 1;
It feels a bit unsafe to bump err_count to 1 in merge() and set the error
info in insert(). feels like looking for trouble.
Maybe atomic_inc_not_zero() would have been better,
but since I am going to argue against modifying err_count outside
the notification_lock, it does not need to be atomic at all.
> +
> + return 0;
> +}
> +
> +static void fanotify_insert_error_event(struct fsnotify_group *group,
> + struct fsnotify_event *event,
> + const void *data)
> +{
> + const struct fsnotify_event_info *ei =
> + (struct fsnotify_event_info *) data;
> + struct fanotify_event *fae = FANOTIFY_E(event);
> + const struct fs_error_report *report;
> + struct fanotify_error_event *fee;
> + struct inode *inode;
> + int fh_len;
> +
> + /* This might be an unexpected type of event (i.e. overflow). */
> + if (!fanotify_is_error_event(fae->mask))
> + return;
> +
> + report = (struct fs_error_report *) ei->data;
> + inode = report->inode ?: ei->sb->s_root->d_inode;
As I commented on the cover letter, I think it would be better to
encode a NULL-FID in case of NULL inode, e.g.:
static int fanotify_encode_null_fh(struct fanotify_fh *fh)
{
fh->type = FILEID_ROOT;
fh->len = 8;
fh->flags = 0;
memset(fh->buf, 0, 8);
}
But that API may be controversial, so wait for other voices
before changing that.
> +
> + fee = FANOTIFY_EE(fae);
> + fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
> + fee->error = report->error;
> + fee->fsid = fee->mark->connector->fsid;
> +
> + fsnotify_get_mark(fee->mark);
> +
> + /*
> + * Error reporting needs to happen in atomic context. If this
> + * inode's file handler is more than we initially predicted,
> + * there is nothing better we can do than report the error with
> + * a bad FH.
> + */
> + fh_len = fanotify_encode_fh_len(inode);
> + if (WARN_ON(fh_len > fee->max_fh_len))
> + return;
> +
> + fanotify_encode_fh(&fee->object_fh, inode, fh_len, NULL, 0);
> +}
> +
[...]
> +static size_t copy_error_info_to_user(struct fanotify_event *event,
> + char __user *buf, int count)
> +{
> + struct fanotify_event_info_error info;
> + struct fanotify_error_event *fee = FANOTIFY_EE(event);
> +
> + info.hdr.info_type = FAN_EVENT_INFO_TYPE_ERROR;
> + info.hdr.pad = 0;
> + info.hdr.len = sizeof(struct fanotify_event_info_error);
> +
> + if (WARN_ON(count < info.hdr.len))
> + return -EFAULT;
> +
> + info.error = fee->error;
> +
> + /* This effectively releases the event for logging another error */
> + info.error_count = atomic_xchg(&fee->err_count, 0);
> +
it's a shame because the code is really simpler, but
I am afraid it may not be correct, even without mentioning breaking
the queue abstraction.
While the read/insert of err_count is "atomic", read of FID is done
after this point, not to mention that read of fee->error could be reordered
without barriers. FID and error could be set by insert event after reading
fee->err_count.
You can either go back to copying the error report to stack on dequeue
(though it was a bit ugly) under the group notification_lock
or you can allocate a new initialized error event on dequeue and
exchange it with the mark's event, without having to change the
calling semantics of get_one_event(), e.g.:
fsnotify_remove_first_event(group);
if (fanotify_is_perm_event(event->mask))
FANOTIFY_PERM(event)->state = FAN_EVENT_REPORTED;
if (fanotify_is_hashed_event(event->mask))
fanotify_unhash_event(group, event);
+ if (fanotify_is_error_event(event->mask))
+ event = fanotify_recreate_fs_error_event(event);
If allocation fails, this helper returns ERR_PTR(-EINVAL)
and resets the info and err_count of the event in the mark.
Thanks,
Amir.
next prev parent reply other threads:[~2021-06-30 14:05 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-29 19:10 [PATCH v3 00/15] File system wide monitoring Gabriel Krisman Bertazi
2021-06-29 19:10 ` [PATCH v3 01/15] fsnotify: Don't insert unmergeable events in hashtable Gabriel Krisman Bertazi
2021-06-30 3:12 ` Amir Goldstein
2021-07-07 19:21 ` Jan Kara
2021-06-29 19:10 ` [PATCH v3 02/15] fanotify: Fold event size calculation to its own function Gabriel Krisman Bertazi
2021-07-07 19:22 ` Jan Kara
2021-06-29 19:10 ` [PATCH v3 03/15] fanotify: Split fsid check from other fid mode checks Gabriel Krisman Bertazi
2021-06-30 3:14 ` Amir Goldstein
2021-07-07 19:24 ` Jan Kara
2021-06-29 19:10 ` [PATCH v3 04/15] fanotify: Split superblock marks out to a new cache Gabriel Krisman Bertazi
2021-06-30 3:17 ` Amir Goldstein
2021-07-07 20:13 ` Jan Kara
2021-07-08 6:16 ` Amir Goldstein
2021-06-29 19:10 ` [PATCH v3 05/15] inotify: Don't force FS_IN_IGNORED Gabriel Krisman Bertazi
2021-07-07 19:37 ` Jan Kara
2021-06-29 19:10 ` [PATCH v3 06/15] fsnotify: Add helper to detect overflow_event Gabriel Krisman Bertazi
2021-07-07 20:14 ` Jan Kara
2021-06-29 19:10 ` [PATCH v3 07/15] fsnotify: pass arguments of fsnotify() in struct fsnotify_event_info Gabriel Krisman Bertazi
2021-06-29 20:39 ` kernel test robot
2021-06-30 0:10 ` kernel test robot
2021-06-30 3:29 ` Amir Goldstein
2021-06-30 8:11 ` Dan Carpenter
2021-06-30 8:35 ` Amir Goldstein
2021-06-30 8:45 ` Dan Carpenter
2021-06-30 9:32 ` Amir Goldstein
2021-06-30 9:34 ` Amir Goldstein
2021-06-30 10:49 ` Dan Carpenter
2021-06-30 12:51 ` Amir Goldstein
2021-07-08 10:43 ` Jan Kara
2021-07-08 11:09 ` Amir Goldstein
2021-07-08 11:37 ` Jan Kara
2021-06-29 19:10 ` [PATCH v3 08/15] fsnotify: Support passing argument to insert callback on add_event Gabriel Krisman Bertazi
2021-06-30 3:40 ` Amir Goldstein
2021-07-08 10:48 ` Jan Kara
2021-06-29 19:10 ` [PATCH v3 09/15] fsnotify: Always run the merge hook Gabriel Krisman Bertazi
2021-06-30 3:42 ` Amir Goldstein
2021-06-29 19:10 ` [PATCH v3 10/15] fsnotify: Support FS_ERROR event type Gabriel Krisman Bertazi
2021-07-08 10:53 ` Jan Kara
2021-06-29 19:10 ` [PATCH v3 11/15] fsnotify: Introduce helpers to send error_events Gabriel Krisman Bertazi
2021-06-30 3:44 ` Amir Goldstein
2021-07-08 11:02 ` Jan Kara
2021-06-29 19:10 ` [PATCH v3 12/15] fanotify: Introduce FAN_FS_ERROR event Gabriel Krisman Bertazi
2021-06-30 10:26 ` Amir Goldstein
2021-06-30 17:43 ` Gabriel Krisman Bertazi
2021-07-01 6:37 ` Amir Goldstein
2021-06-30 14:03 ` Amir Goldstein [this message]
2021-06-29 19:10 ` [PATCH v3 13/15] ext4: Send notifications on error Gabriel Krisman Bertazi
2021-06-29 19:10 ` [PATCH v3 14/15] samples: Add fs error monitoring example Gabriel Krisman Bertazi
2021-06-30 2:42 ` kernel test robot
2021-07-19 14:36 ` Gabriel Krisman Bertazi
2021-07-20 19:49 ` Dan Carpenter
2021-07-22 12:54 ` Chen, Rong A
2021-07-22 16:15 ` Gabriel Krisman Bertazi
2021-07-23 1:35 ` Chen, Rong A
2021-06-30 3:46 ` kernel test robot
2021-06-30 3:58 ` Amir Goldstein
2021-06-29 19:10 ` [PATCH v3 15/15] docs: Document the FAN_FS_ERROR event Gabriel Krisman Bertazi
2021-06-30 4:18 ` Amir Goldstein
2021-06-30 5:10 ` [PATCH v3 00/15] File system wide monitoring Amir Goldstein
2021-07-08 11:32 ` Jan Kara
2021-07-08 12:25 ` Amir Goldstein
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOQ4uxiQc+g5dRzTOVzesZTffVkX7o-bedd_MB9SmG4Unex6wg@mail.gmail.com \
--to=amir73il@gmail.com \
--cc=david@fromorbit.com \
--cc=dhowells@redhat.com \
--cc=djwong@kernel.org \
--cc=jack@suse.com \
--cc=kernel@collabora.com \
--cc=khazhy@google.com \
--cc=krisman@collabora.com \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=tytso@mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).