Message-ID: <20260106173513.GD191481@frogsfrogsfrogs>
Date: Tue, 6 Jan 2026 09:35:13 -0800
From: "Darrick J. Wong" <djwong@...nel.org>
To: Jan Kara <jack@...e.cz>
Cc: brauner@...nel.org, hch@....de, linux-ext4@...r.kernel.org,
	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 Mon, Dec 22, 2025 at 04:36:14PM +0100, Jan Kara wrote:
> 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().

Ok.  I'll send a follow-on patch, though I don't see vfs-7.0.fserror on
vfs.git so I'm confused about where things are right now.

> > +	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...

I didn't quite get to posting that patchset before vacation, but it's
posted now in "xfs: convey file I/O errors to the health monitor":

https://lore.kernel.org/linux-fsdevel/176766637421.774337.94510884010750487.stgit@frogsfrogsfrogs/T/#Z2e.:..:176766637421.774337.94510884010750487.stgit::40frogsfrogsfrogs:1fs:xfs:xfs_super.c

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

Powered by blists - more mailing lists