[RFC,13/15] ext4: Send notifications on error

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

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.

On Mon, Apr 26, 2021 at 02:41:59PM -0400, Gabriel Krisman Bertazi 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);
>  }
>  
> @@ -765,10 +786,10 @@ void __ext4_error_inode(struct inode *inode, const char *function,
>  		return;
>  
>  	trace_ext4_error(inode->i_sb, function, line);
> +	va_start(args, fmt);
> +	vaf.fmt = fmt;
> +	vaf.va = &args;
>  	if (ext4_error_ratelimit(inode->i_sb)) {
> -		va_start(args, fmt);
> -		vaf.fmt = fmt;
> -		vaf.va = &args;
>  		if (block)
>  			printk(KERN_CRIT "EXT4-fs error (device %s): %s:%d: "
>  			       "inode #%lu: block %llu: comm %s: %pV\n",
> @@ -779,8 +800,11 @@ void __ext4_error_inode(struct inode *inode, const char *function,
>  			       "inode #%lu: comm %s: %pV\n",
>  			       inode->i_sb->s_id, function, line, inode->i_ino,
>  			       current->comm, &vaf);
> -		va_end(args);
>  	}
> +
> +	ext4_fsnotify_error(error, inode, block, function, line, NULL, &vaf);
> +	va_end(args);
> +
>  	ext4_handle_error(inode->i_sb, false, error, inode->i_ino, block,
>  			  function, line);
>  }
> @@ -798,13 +822,16 @@ void __ext4_error_file(struct file *file, const char *function,
>  		return;
>  
>  	trace_ext4_error(inode->i_sb, function, line);
> +
> +	path = file_path(file, pathname, sizeof(pathname));
> +	if (IS_ERR(path))
> +		path = "(unknown)";
> +
> +	va_start(args, fmt);
> +	vaf.fmt = fmt;
> +	vaf.va = &args;
> +
>  	if (ext4_error_ratelimit(inode->i_sb)) {
> -		path = file_path(file, pathname, sizeof(pathname));
> -		if (IS_ERR(path))
> -			path = "(unknown)";
> -		va_start(args, fmt);
> -		vaf.fmt = fmt;
> -		vaf.va = &args;
>  		if (block)
>  			printk(KERN_CRIT
>  			       "EXT4-fs error (device %s): %s:%d: inode #%lu: "
> @@ -817,8 +844,10 @@ void __ext4_error_file(struct file *file, const char *function,
>  			       "comm %s: path %s: %pV\n",
>  			       inode->i_sb->s_id, function, line, inode->i_ino,
>  			       current->comm, path, &vaf);
> -		va_end(args);
>  	}
> +	ext4_fsnotify_error(EFSCORRUPTED, inode, block, function, line, NULL, &vaf);
> +	va_end(args);
> +
>  	ext4_handle_error(inode->i_sb, false, EFSCORRUPTED, inode->i_ino, block,
>  			  function, line);
>  }
> @@ -886,6 +915,7 @@ void __ext4_std_error(struct super_block *sb, const char *function,
>  		printk(KERN_CRIT "EXT4-fs error (device %s) in %s:%d: %s\n",
>  		       sb->s_id, function, line, errstr);
>  	}
> +	ext4_fsnotify_error(errno, NULL, -1L, function, line, errstr, NULL);
>  
>  	ext4_handle_error(sb, false, -errno, 0, 0, function, line);
>  }
> diff --git a/include/uapi/linux/ext4-notify.h b/include/uapi/linux/ext4-notify.h
> new file mode 100644
> index 000000000000..31a3bbcafd13
> --- /dev/null
> +++ b/include/uapi/linux/ext4-notify.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> +/*
> + * Copyright 2021, Collabora Ltd.
> + */
> +
> +#ifndef EXT4_NOTIFY_H
> +#define EXT4_NOTIFY_H
> +
> +#define EXT4_FSN_DESC_LEN	256
> +
> +struct ext4_error_inode_report {
> +	u64 inode;

I don't have much to contribute this time, other than suggesting that
you might want to encode the inode generation here so that forensics
tools won't waste their time if the inode has been deleted and recreated
in between when the error happens and when the fs gets pulled offline
for analysis.

(...and maybe add a u32 flags field that can remain zero for now)

> +	u64 block;

...and maybe call this "lblk" (assuming this is the logical block offset
within the file?) since that's already in wide use around e2fsprogs and
fs/ext4/.

--D

> +	char desc[EXT4_FSN_DESC_LEN];
> +};
> +
> +#endif
> -- 
> 2.31.0
>