Message-ID: <cunesvp5k37ocmz2nbkdov7ssu3djqvdii26d4gn6sj7sgtnca@b5mokxhvneay>
Date: Mon, 22 Dec 2025 16:36:14 +0100
From: Jan Kara <jack@...e.cz>
To: "Darrick J. Wong" <djwong@...nel.org>
Cc: brauner@...nel.org, hch@....de, linux-ext4@...r.kernel.org, 
	jack@...e.cz, linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org, 
	gabriel@...sman.be, amir73il@...il.com
Subject: Re: [PATCH 2/6] fs: report filesystem and file I/O errors to fsnotify

On Wed 17-12-25 18:03:11, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@...nel.org>
> 
> Create some wrapper code around struct super_block so that filesystems
> have a standard way to queue filesystem metadata and file I/O error
> reports to have them sent to fsnotify.
> 
> If a filesystem wants to provide an error number, it must supply only
> negative error numbers.  These are stored internally as negative
> numbers, but they are converted to positive error numbers before being
> passed to fanotify, per the fanotify(7) manpage.  Implementations of
> super_operations::report_error are passed the raw internal event data.
> 
> Note that we have to play some shenanigans with mempools and queue_work
> so that the error handling doesn't happen outside of process context,
> and the event handler functions (both ->report_error and fsnotify) can
> handle file I/O error messages without having to worry about whatever
> locks might be held.  This asynchronicity requires that unmount wait for
> pending events to clear.
> 
> Add a new callback to the superblock operations structure so that
> filesystem drivers can themselves respond to file I/O errors if they so
> desire.  This will be used for an upcoming self-healing patchset for
> XFS.
> 
> Suggested-by: Christoph Hellwig <hch@....de>
> Signed-off-by: "Darrick J. Wong" <djwong@...nel.org>

Looks good to me. Besides the nits Christoph commented on just two comments:

> +static inline struct fserror_event *fserror_alloc_event(struct super_block *sb,
> +							gfp_t gfp_flags)
> +{
> +	struct fserror_event *event = NULL;
> +
> +	/*
> +	 * If pending_errors already reached zero or is no longer active,
> +	 * the superblock is being deactivated so there's no point in
> +	 * continuing.
> +	 */
> +	if (!refcount_inc_not_zero(&sb->s_pending_errors))
> +		return NULL;

It would be good here or in the above comment explicitely mention that the
ordering of s_pending_errors check and SB_ACTIVE check is mandated by the
ordering in generic_shutdown_super() and that the barriers are implicitely
provided by the refcount manipulations here and in fserror_unmount().

> +	if (!(sb->s_flags & SB_ACTIVE))
> +		goto out_pending;
> +
> +	event = mempool_alloc(&fserror_events_pool, gfp_flags);
> +	if (!event)
> +		goto out_pending;
> +
> +	/* mempool_alloc doesn't support GFP_ZERO */
> +	memset(event, 0, sizeof(*event));
> +	event->sb = sb;
> +	INIT_WORK(&event->work, fserror_worker);
> +
> +	return event;
> +
> +out_pending:
> +	fserror_pending_dec(sb);
> +	return NULL;
> +}
> +
> +/**
> + * fserror_report - report a filesystem error of some kind
> + *
> + * Report details of a filesystem error to the super_operations::report_error
> + * callback if present; and to fsnotify for distribution to userspace.  @sb,
> + * @gfp, @type, and @error must all be specified.  For file I/O errors, the
> + * @inode, @pos, and @len fields must also be specified.  For file metadata
> + * errors, @inode must be specified.  If @inode is not NULL, then @inode->i_sb
> + * must point to @sb.
> + *
> + * Reporting work is deferred to a workqueue to ensure that ->report_error is
> + * called from process context without any locks held.  An active reference to
> + * the inode is maintained until event handling is complete, and unmount will
> + * wait for queued events to drain.
> + *
> + * @sb:		superblock of the filesystem
> + * @inode:	inode within that filesystem, if applicable
> + * @type:	type of error encountered
> + * @pos:	start of inode range affected, if applicable
> + * @len:	length of inode range affected, if applicable
> + * @error:	error number encountered, must be negative
> + * @gfp:	memory allocation flags for conveying the event to a worker,
> + *		since this function can be called from atomic contexts
> + */
> +void fserror_report(struct super_block *sb, struct inode *inode,
> +		    enum fserror_type type, loff_t pos, u64 len, int error,
> +		    gfp_t gfp)
> +{
> +	struct fserror_event *event;
> +
> +	/* sb and inode must be from the same filesystem */
> +	WARN_ON_ONCE(inode && inode->i_sb != sb);
> +
> +	/* error number must be negative */
> +	WARN_ON_ONCE(error >= 0);

Since the error reporting is kind of expensive now (allocation & queueing
work) it would be nice to check somebody actually cares about the error
events at all. We can provide a helper from fsnotify for that, I'm not sure
about ->report_error hook since it didn't get used in this series at all in
the end...

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists