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: <20210426184201.4177978-1-krisman@collabora.com> <20210426184201.4177978-14-krisman@collabora.com>
In-Reply-To: <20210426184201.4177978-14-krisman@collabora.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Tue, 27 Apr 2021 07:32:58 +0300
Message-ID: <CAOQ4uxg8X3UNjYpbLy-gksc=SuR0z4RWc=ZXru-zFQdNt5RyEw@mail.gmail.com>
Subject: Re: [PATCH RFC 13/15] ext4: Send notifications on error
To:     Gabriel Krisman Bertazi <krisman@collabora.com>
Cc:     Theodore Tso <tytso@mit.edu>,
        "Darrick J. Wong" <djwong@kernel.org>,
        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 Mon, Apr 26, 2021 at 9:43 PM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> Send a FS_ERROR message via fsnotify to a userspace monitoring tool
> whenever a ext4 error condition is triggered.  This follows the existing
> error conditions in ext4, so it is hooked to the ext4_error* functions.
>
> It also follows the current dmesg reporting in the format.  The
> filesystem message is composed mostly by the string that would be
> otherwise printed in dmesg.
>
> A new ext4 specific record format is exposed in the uapi, such that a
> monitoring tool knows what to expect when listening errors of an ext4
> filesystem.
>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> ---
>  fs/ext4/super.c                  | 60 ++++++++++++++++++++++++--------
>  include/uapi/linux/ext4-notify.h | 17 +++++++++
>  2 files changed, 62 insertions(+), 15 deletions(-)
>  create mode 100644 include/uapi/linux/ext4-notify.h
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index b9693680463a..032e29e7ff6a 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -46,6 +46,8 @@
>  #include <linux/part_stat.h>
>  #include <linux/kthread.h>
>  #include <linux/freezer.h>
> +#include <linux/fsnotify.h>
> +#include <uapi/linux/ext4-notify.h>
>
>  #include "ext4.h"
>  #include "ext4_extents.h"      /* Needed for trace points definition */
> @@ -727,6 +729,22 @@ static void flush_stashed_error_work(struct work_struct *work)
>         ext4_commit_super(sbi->s_sb);
>  }
>
> +static void ext4_fsnotify_error(int error, struct inode *inode, __u64 block,
> +                               const char *func, int line,
> +                               const char *desc, struct va_format *vaf)
> +{
> +       struct ext4_error_inode_report report;
> +
> +       if (inode->i_sb->s_fsnotify_marks) {
> +               report.inode = inode ? inode->i_ino : -1L;
> +               report.block = block ? block : -1L;
> +
> +               snprintf(report.desc, EXT4_FSN_DESC_LEN, "%s%pV\n", desc?:"", vaf);
> +
> +               fsnotify_error_event(error, inode, func, line, &report, sizeof(report));
> +       }
> +}
> +
>  #define ext4_error_ratelimit(sb)                                       \
>                 ___ratelimit(&(EXT4_SB(sb)->s_err_ratelimit_state),     \
>                              "EXT4-fs error")
> @@ -742,15 +760,18 @@ void __ext4_error(struct super_block *sb, const char *function,
>                 return;
>
>         trace_ext4_error(sb, function, line);
> +
> +       va_start(args, fmt);
> +       vaf.fmt = fmt;
> +       vaf.va = &args;
>         if (ext4_error_ratelimit(sb)) {
> -               va_start(args, fmt);
> -               vaf.fmt = fmt;
> -               vaf.va = &args;
>                 printk(KERN_CRIT
>                        "EXT4-fs error (device %s): %s:%d: comm %s: %pV\n",
>                        sb->s_id, function, line, current->comm, &vaf);
> -               va_end(args);
> +
>         }
> +       ext4_fsnotify_error(error, sb->s_root->d_inode, block, function, line, NULL, &vaf);
> +       va_end(args);
>         ext4_handle_error(sb, force_ro, error, 0, block, function, line);
>  }
>

So error reporting to kernel log is ratelimited and error reporting to
fsnotify is limited by a fixed size ring buffer which may be filled by
report floods from another filesystem, so user can miss the first
important error report from this filesystem.

Not optimal.

With my proposal of keeping a single fsnotify_error_info in every
fsnotify_sb_mark, users will be guaranteed to get the first error
report from every filesystem and once they read that report they
will be guaranteed to also get the next report.

Thanks,
Amir.