[RFC PATCH 0/3] Log transaction cancel error codes in the kernel log buffer

[RFC PATCH 0/3] Log transaction cancel error codes in the kernel log buffer

[cem]

From: Carlos Maiolino <cem@kernel.org>

Hi, this is a big RFC which is just compiled tested and has the only
goal to get opinions, please don't use it...

Once in a while we face some sort of failure where a transaction gets
cancelled in the middle of an IO operation and it is not always easy to
understand why.
One of these reasons is when xfs_bmapi_write() fails. It can fail for
several reasons and sometimes we get to start a guessing game on why it
actually failed.

So I thought it would be useful to have xfs_trans_cancel() to log the
exact -ERRORCODE that caused the transaction to cancel.

The reason I used a variadic function was mostly to make it a smaller
patch instead of a long sed-generated patch which would not make my
point, although I kind'a liked it :P

Another approach I thought would be to add a t_error field to the
transaction descriptor and report it via the tracepoint, so callers
could opt to set an error code or not.

Does this seem useful to anybody else or should I just abort the idea?

Cheers.

Carlos Maiolino (3):
  xfs: add xfs_error_report the ability to display an error code
  xfs: enable xfs_trans_cancel() to report and error code
  xfs: make xfs_iomap_write_direct() report an error to xfs_trans_cancel

 fs/xfs/xfs_error.c         |  7 ++++++-
 fs/xfs/xfs_error.h         | 10 +++++-----
 fs/xfs/xfs_exchmaps_item.c |  6 +++---
 fs/xfs/xfs_inode_item.c    |  2 +-
 fs/xfs/xfs_iomap.c         |  2 +-
 fs/xfs/xfs_log_recover.c   |  2 +-
 fs/xfs/xfs_trans.c         | 12 +++++++++---
 fs/xfs/xfs_trans.h         |  8 +++++++-
 8 files changed, 33 insertions(+), 16 deletions(-)

-- 
2.55.0

[RFC PATCH 1/3] xfs: add xfs_error_report the ability to display an error code

[cem]

From: Carlos Maiolino <cem@kernel.org>

Users could opt to request an error code to be printed

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
Signed-off-by: Carlos Maiolino <cem@kernel.org>
---
 fs/xfs/xfs_error.c         |  7 ++++++-
 fs/xfs/xfs_error.h         | 10 +++++-----
 fs/xfs/xfs_exchmaps_item.c |  6 +++---
 fs/xfs/xfs_inode_item.c    |  2 +-
 fs/xfs/xfs_log_recover.c   |  2 +-
 5 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index dbd87e137694..1c882b910ffd 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -377,6 +377,7 @@ void
 xfs_error_report(
 	const char		*tag,
 	int			level,
+	int			error,
 	struct xfs_mount	*mp,
 	const char		*filename,
 	int			linenum,
@@ -387,6 +388,10 @@ xfs_error_report(
 		"Internal error %s at line %d of file %s.  Caller %pS",
 			    tag, linenum, filename, failaddr);
 
+		if (error)
+			xfs_alert_tag(mp, XFS_PTAG_ERROR_REPORT,
+				      "Error Code: %d", error);
+
 		xfs_stack_trace();
 	}
 }
@@ -404,7 +409,7 @@ xfs_corruption_error(
 {
 	if (buf && level <= xfs_error_level)
 		xfs_hex_dump(buf, bufsize);
-	xfs_error_report(tag, level, mp, filename, linenum, failaddr);
+	xfs_error_report(tag, level, 0, mp, filename, linenum, failaddr);
 	xfs_alert(mp, "Corruption detected. Unmount and run xfs_repair");
 }
 
diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
index 0b9c5ba8a598..1a1aa60e92f2 100644
--- a/fs/xfs/xfs_error.h
+++ b/fs/xfs/xfs_error.h
@@ -8,9 +8,9 @@
 
 struct xfs_mount;
 
-extern void xfs_error_report(const char *tag, int level, struct xfs_mount *mp,
-			const char *filename, int linenum,
-			xfs_failaddr_t failaddr);
+extern void xfs_error_report(const char *tag, int level, int error,
+			     struct xfs_mount *mp, const char *filename,
+			     int linenum, xfs_failaddr_t failaddr);
 extern void xfs_corruption_error(const char *tag, int level,
 			struct xfs_mount *mp, const void *buf, size_t bufsize,
 			const char *filename, int linenum,
@@ -25,8 +25,8 @@ extern void xfs_inode_verifier_error(struct xfs_inode *ip, int error,
 			const char *name, const void *buf, size_t bufsz,
 			xfs_failaddr_t failaddr);
 
-#define	XFS_ERROR_REPORT(e, lvl, mp)	\
-	xfs_error_report(e, lvl, mp, __FILE__, __LINE__, __return_address)
+#define	XFS_ERROR_REPORT(e, lvl, error, mp)	\
+	xfs_error_report(e, lvl, error, mp, __FILE__, __LINE__, __return_address)
 #define	XFS_CORRUPTION_ERROR(e, lvl, mp, buf, bufsize)	\
 	xfs_corruption_error(e, lvl, mp, buf, bufsize, \
 			     __FILE__, __LINE__, __return_address)
diff --git a/fs/xfs/xfs_exchmaps_item.c b/fs/xfs/xfs_exchmaps_item.c
index 264a121c5e16..b94c5e5e6549 100644
--- a/fs/xfs/xfs_exchmaps_item.c
+++ b/fs/xfs/xfs_exchmaps_item.c
@@ -559,13 +559,13 @@ xlog_recover_xmi_commit_pass2(
 
 	len = sizeof(struct xfs_xmi_log_format);
 	if (item->ri_buf[0].i_len != len) {
-		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);
 		return -EFSCORRUPTED;
 	}
 
 	xmi_formatp = item->ri_buf[0].i_addr;
 	if (xmi_formatp->__pad != 0) {
-		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);
 		return -EFSCORRUPTED;
 	}
 
@@ -600,7 +600,7 @@ xlog_recover_xmd_commit_pass2(
 
 	xmd_formatp = item->ri_buf[0].i_addr;
 	if (item->ri_buf[0].i_len != sizeof(struct xfs_xmd_log_format)) {
-		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);
 		return -EFSCORRUPTED;
 	}
 
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index c6cb0b6b9e46..862b7d7d3ce4 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -1185,7 +1185,7 @@ xfs_inode_item_format_convert(
 	struct xfs_inode_log_format_32	*in_f32 = buf->i_addr;
 
 	if (buf->i_len != sizeof(*in_f32)) {
-		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, NULL);
 		return -EFSCORRUPTED;
 	}
 
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 2f76531842f8..083e4bf61e2b 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2932,7 +2932,7 @@ xlog_recover_process(
 		 * fatal log corruption failure.
 		 */
 		if (xfs_has_crc(log->l_mp)) {
-			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
+			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);
 			return -EFSCORRUPTED;
 		}
 	}
-- 
2.55.0

[RFC PATCH 2/3] xfs: enable xfs_trans_cancel() to report and error code

[cem]

From: Carlos Maiolino <cem@kernel.org>

Once in a while it's useful to know exactly what kind of error caused a
transaction to be cancelled.
Enable xfs_trans_cancel() to receive an error code to be reported.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
Signed-off-by: Carlos Maiolino <cem@kernel.org>
---
 fs/xfs/xfs_trans.c | 12 +++++++++---
 fs/xfs/xfs_trans.h |  8 +++++++-
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index c6657072361a..f6004590565b 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -971,12 +971,18 @@ xfs_trans_commit(
  * xfs_trans_commit().
  */
 void
-xfs_trans_cancel(
-	struct xfs_trans	*tp)
+_xfs_trans_cancel(
+	struct xfs_trans	*tp, ...)
 {
 	struct xfs_mount	*mp = tp->t_mountp;
 	struct xlog		*log = mp->m_log;
 	bool			dirty = (tp->t_flags & XFS_TRANS_DIRTY);
+	int			error = 0;
+	va_list			args;
+
+	va_start(args, tp);
+	error = va_arg(args, int);
+	va_end(args);
 
 	trace_xfs_trans_cancel(tp, _RET_IP_);
 
@@ -999,7 +1005,7 @@ xfs_trans_cancel(
 	 * here.
 	 */
 	if (dirty && !xfs_is_shutdown(mp)) {
-		XFS_ERROR_REPORT("xfs_trans_cancel", XFS_ERRLEVEL_LOW, mp);
+		XFS_ERROR_REPORT("xfs_trans_cancel", XFS_ERRLEVEL_LOW, error, mp);
 		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
 	}
 #ifdef DEBUG
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 2b366851e9a4..05cc7464eb3b 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -213,6 +213,13 @@ xfs_trans_read_buf(
 				      flags, bpp, ops);
 }
 
+void _xfs_trans_cancel(xfs_trans_t *, ...);
+
+#define xfs_trans_cancel(tp, ...)		\
+({						\
+	_xfs_trans_cancel(tp, ##__VA_ARGS__);	\
+})
+
 struct xfs_buf	*xfs_trans_getsb(struct xfs_trans *);
 struct xfs_buf	*xfs_trans_getrtsb(struct xfs_trans *tp);
 
@@ -237,7 +244,6 @@ void		xfs_trans_log_inode(xfs_trans_t *, struct xfs_inode *, uint);
 int		xfs_trans_commit(struct xfs_trans *);
 int		xfs_trans_roll(struct xfs_trans **);
 int		xfs_trans_roll_inode(struct xfs_trans **, struct xfs_inode *);
-void		xfs_trans_cancel(xfs_trans_t *);
 int		xfs_trans_ail_init(struct xfs_mount *);
 void		xfs_trans_ail_destroy(struct xfs_mount *);
 
-- 
2.55.0

[RFC PATCH 3/3] xfs: make xfs_iomap_write_direct() report an error to xfs_trans_cancel

[cem]

From: Carlos Maiolino <cem@kernel.org>

I've been in the situation a few times where the transaction got
cancelled by some reason. Most of time because bmapi_write failed and I
start a guessing game trying to understand why.

So if xfs_iomap_write_direct fails and it causes the transaction to be
cancelled, send the error to xfs_trans_cancel so the error gets logged
into the kernel log buffer.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
Signed-off-by: Carlos Maiolino <cem@kernel.org>
---
 fs/xfs/xfs_iomap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index ff05e6b1b0bb..3d1a52757697 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -344,7 +344,7 @@ xfs_iomap_write_direct(
 	return error;
 
 out_trans_cancel:
-	xfs_trans_cancel(tp);
+	xfs_trans_cancel(tp, error);
 	goto out_unlock;
 }
 
-- 
2.55.0

Re: [RFC PATCH 1/3] xfs: add xfs_error_report the ability to display an error code

[Darrick J. Wong]

On Fri, Sep 04, 2026 at 01:32:21PM +0200, cem@kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
> 
> Users could opt to request an error code to be printed
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> Signed-off-by: Carlos Maiolino <cem@kernel.org>
> ---
>  fs/xfs/xfs_error.c         |  7 ++++++-
>  fs/xfs/xfs_error.h         | 10 +++++-----
>  fs/xfs/xfs_exchmaps_item.c |  6 +++---
>  fs/xfs/xfs_inode_item.c    |  2 +-
>  fs/xfs/xfs_log_recover.c   |  2 +-
>  5 files changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> index dbd87e137694..1c882b910ffd 100644
> --- a/fs/xfs/xfs_error.c
> +++ b/fs/xfs/xfs_error.c
> @@ -377,6 +377,7 @@ void
>  xfs_error_report(
>  	const char		*tag,
>  	int			level,
> +	int			error,
>  	struct xfs_mount	*mp,
>  	const char		*filename,
>  	int			linenum,
> @@ -387,6 +388,10 @@ xfs_error_report(
>  		"Internal error %s at line %d of file %s.  Caller %pS",
>  			    tag, linenum, filename, failaddr);
>  
> +		if (error)
> +			xfs_alert_tag(mp, XFS_PTAG_ERROR_REPORT,
> +				      "Error Code: %d", error);

/me wonders, should the error erport just go in the "Internal error..."
message above?

"Internal error $tag ($error) at line..."

> +
>  		xfs_stack_trace();
>  	}
>  }
> @@ -404,7 +409,7 @@ xfs_corruption_error(
>  {
>  	if (buf && level <= xfs_error_level)
>  		xfs_hex_dump(buf, bufsize);
> -	xfs_error_report(tag, level, mp, filename, linenum, failaddr);
> +	xfs_error_report(tag, level, 0, mp, filename, linenum, failaddr);
>  	xfs_alert(mp, "Corruption detected. Unmount and run xfs_repair");
>  }
>  
> diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
> index 0b9c5ba8a598..1a1aa60e92f2 100644
> --- a/fs/xfs/xfs_error.h
> +++ b/fs/xfs/xfs_error.h
> @@ -8,9 +8,9 @@
>  
>  struct xfs_mount;
>  
> -extern void xfs_error_report(const char *tag, int level, struct xfs_mount *mp,
> -			const char *filename, int linenum,
> -			xfs_failaddr_t failaddr);
> +extern void xfs_error_report(const char *tag, int level, int error,
> +			     struct xfs_mount *mp, const char *filename,
> +			     int linenum, xfs_failaddr_t failaddr);
>  extern void xfs_corruption_error(const char *tag, int level,
>  			struct xfs_mount *mp, const void *buf, size_t bufsize,
>  			const char *filename, int linenum,
> @@ -25,8 +25,8 @@ extern void xfs_inode_verifier_error(struct xfs_inode *ip, int error,
>  			const char *name, const void *buf, size_t bufsz,
>  			xfs_failaddr_t failaddr);
>  
> -#define	XFS_ERROR_REPORT(e, lvl, mp)	\
> -	xfs_error_report(e, lvl, mp, __FILE__, __LINE__, __return_address)
> +#define	XFS_ERROR_REPORT(e, lvl, error, mp)	\
> +	xfs_error_report(e, lvl, error, mp, __FILE__, __LINE__, __return_address)
>  #define	XFS_CORRUPTION_ERROR(e, lvl, mp, buf, bufsize)	\
>  	xfs_corruption_error(e, lvl, mp, buf, bufsize, \
>  			     __FILE__, __LINE__, __return_address)
> diff --git a/fs/xfs/xfs_exchmaps_item.c b/fs/xfs/xfs_exchmaps_item.c
> index 264a121c5e16..b94c5e5e6549 100644
> --- a/fs/xfs/xfs_exchmaps_item.c
> +++ b/fs/xfs/xfs_exchmaps_item.c
> @@ -559,13 +559,13 @@ xlog_recover_xmi_commit_pass2(
>  
>  	len = sizeof(struct xfs_xmi_log_format);
>  	if (item->ri_buf[0].i_len != len) {
> -		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
> +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);

These should be -EFSCORRUPTED, not 0, right?

--D

>  		return -EFSCORRUPTED;
>  	}
>  
>  	xmi_formatp = item->ri_buf[0].i_addr;
>  	if (xmi_formatp->__pad != 0) {
> -		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
> +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);
>  		return -EFSCORRUPTED;
>  	}
>  
> @@ -600,7 +600,7 @@ xlog_recover_xmd_commit_pass2(
>  
>  	xmd_formatp = item->ri_buf[0].i_addr;
>  	if (item->ri_buf[0].i_len != sizeof(struct xfs_xmd_log_format)) {
> -		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
> +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);
>  		return -EFSCORRUPTED;
>  	}
>  
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index c6cb0b6b9e46..862b7d7d3ce4 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -1185,7 +1185,7 @@ xfs_inode_item_format_convert(
>  	struct xfs_inode_log_format_32	*in_f32 = buf->i_addr;
>  
>  	if (buf->i_len != sizeof(*in_f32)) {
> -		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, NULL);
>  		return -EFSCORRUPTED;
>  	}
>  
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 2f76531842f8..083e4bf61e2b 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2932,7 +2932,7 @@ xlog_recover_process(
>  		 * fatal log corruption failure.
>  		 */
>  		if (xfs_has_crc(log->l_mp)) {
> -			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
> +			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);
>  			return -EFSCORRUPTED;
>  		}
>  	}
> -- 
> 2.55.0
> 
> 

Re: [RFC PATCH 2/3] xfs: enable xfs_trans_cancel() to report and error code

[Darrick J. Wong]

On Fri, Sep 04, 2026 at 01:32:22PM +0200, cem@kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
> 
> Once in a while it's useful to know exactly what kind of error caused a
> transaction to be cancelled.
> Enable xfs_trans_cancel() to receive an error code to be reported.

Stylistically I think I'd rather have an explicit @error and some macros
to avoid the va_args machinery.


void __xfs_trans_cancel(struct xfs_trans *tp, int error) {...}

#define xfs_trans_cancel(tp) __xfs_trans_cancel(tp, 0)
#define xfs_trans_cancel_with(tp, error) __xfs_trans_cancel(tp, error)

--D

> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> Signed-off-by: Carlos Maiolino <cem@kernel.org>
> ---
>  fs/xfs/xfs_trans.c | 12 +++++++++---
>  fs/xfs/xfs_trans.h |  8 +++++++-
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index c6657072361a..f6004590565b 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -971,12 +971,18 @@ xfs_trans_commit(
>   * xfs_trans_commit().
>   */
>  void
> -xfs_trans_cancel(
> -	struct xfs_trans	*tp)
> +_xfs_trans_cancel(
> +	struct xfs_trans	*tp, ...)
>  {
>  	struct xfs_mount	*mp = tp->t_mountp;
>  	struct xlog		*log = mp->m_log;
>  	bool			dirty = (tp->t_flags & XFS_TRANS_DIRTY);
> +	int			error = 0;
> +	va_list			args;
> +
> +	va_start(args, tp);
> +	error = va_arg(args, int);
> +	va_end(args);
>  
>  	trace_xfs_trans_cancel(tp, _RET_IP_);
>  
> @@ -999,7 +1005,7 @@ xfs_trans_cancel(
>  	 * here.
>  	 */
>  	if (dirty && !xfs_is_shutdown(mp)) {
> -		XFS_ERROR_REPORT("xfs_trans_cancel", XFS_ERRLEVEL_LOW, mp);
> +		XFS_ERROR_REPORT("xfs_trans_cancel", XFS_ERRLEVEL_LOW, error, mp);
>  		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
>  	}
>  #ifdef DEBUG
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 2b366851e9a4..05cc7464eb3b 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -213,6 +213,13 @@ xfs_trans_read_buf(
>  				      flags, bpp, ops);
>  }
>  
> +void _xfs_trans_cancel(xfs_trans_t *, ...);
> +
> +#define xfs_trans_cancel(tp, ...)		\
> +({						\
> +	_xfs_trans_cancel(tp, ##__VA_ARGS__);	\
> +})
> +
>  struct xfs_buf	*xfs_trans_getsb(struct xfs_trans *);
>  struct xfs_buf	*xfs_trans_getrtsb(struct xfs_trans *tp);
>  
> @@ -237,7 +244,6 @@ void		xfs_trans_log_inode(xfs_trans_t *, struct xfs_inode *, uint);
>  int		xfs_trans_commit(struct xfs_trans *);
>  int		xfs_trans_roll(struct xfs_trans **);
>  int		xfs_trans_roll_inode(struct xfs_trans **, struct xfs_inode *);
> -void		xfs_trans_cancel(xfs_trans_t *);
>  int		xfs_trans_ail_init(struct xfs_mount *);
>  void		xfs_trans_ail_destroy(struct xfs_mount *);
>  
> -- 
> 2.55.0
> 
> 

Re: [RFC PATCH 3/3] xfs: make xfs_iomap_write_direct() report an error to xfs_trans_cancel

[Darrick J. Wong]

On Fri, Sep 04, 2026 at 01:32:23PM +0200, cem@kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
> 
> I've been in the situation a few times where the transaction got
> cancelled by some reason. Most of time because bmapi_write failed and I
> start a guessing game trying to understand why.
> 
> So if xfs_iomap_write_direct fails and it causes the transaction to be
> cancelled, send the error to xfs_trans_cancel so the error gets logged
> into the kernel log buffer.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> Signed-off-by: Carlos Maiolino <cem@kernel.org>

Seems reasonable to me.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_iomap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index ff05e6b1b0bb..3d1a52757697 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -344,7 +344,7 @@ xfs_iomap_write_direct(
>  	return error;
>  
>  out_trans_cancel:
> -	xfs_trans_cancel(tp);
> +	xfs_trans_cancel(tp, error);
>  	goto out_unlock;
>  }
>  
> -- 
> 2.55.0
> 
> 

Re: [RFC PATCH 1/3] xfs: add xfs_error_report the ability to display an error code

[Christoph Hellwig]

On Fri, Sep 04, 2026 at 06:08:51PM -0700, Darrick J. Wong wrote:
> > @@ -387,6 +388,10 @@ xfs_error_report(
> >  		"Internal error %s at line %d of file %s.  Caller %pS",
> >  			    tag, linenum, filename, failaddr);
> >  
> > +		if (error)
> > +			xfs_alert_tag(mp, XFS_PTAG_ERROR_REPORT,
> > +				      "Error Code: %d", error);
> 
> /me wonders, should the error erport just go in the "Internal error..."
> message above?
> 
> "Internal error $tag ($error) at line..."

Yeah, that seems much easier to follow.  Maybe make it conditional on
an error being passed.

> > +extern void xfs_error_report(const char *tag, int level, int error,
> > +			     struct xfs_mount *mp, const char *filename,
> > +			     int linenum, xfs_failaddr_t failaddr);

.. and let's drop the pointless extern here while we touch this.

> > -			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
> > +			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);

Overly long line here now.

Re: [RFC PATCH 2/3] xfs: enable xfs_trans_cancel() to report and error code

[Christoph Hellwig]

On Fri, Sep 04, 2026 at 06:10:54PM -0700, Darrick J. Wong wrote:
> On Fri, Sep 04, 2026 at 01:32:22PM +0200, cem@kernel.org wrote:
> > From: Carlos Maiolino <cem@kernel.org>
> > 
> > Once in a while it's useful to know exactly what kind of error caused a
> > transaction to be cancelled.
> > Enable xfs_trans_cancel() to receive an error code to be reported.
> 
> Stylistically I think I'd rather have an explicit @error and some macros
> to avoid the va_args machinery.
> 
> 
> void __xfs_trans_cancel(struct xfs_trans *tp, int error) {...}
> 
> #define xfs_trans_cancel(tp) __xfs_trans_cancel(tp, 0)
> #define xfs_trans_cancel_with(tp, error) __xfs_trans_cancel(tp, error)

Agreed.

Re: [RFC PATCH 1/3] xfs: add xfs_error_report the ability to display an error code

[Carlos Maiolino]

On Sun, Sep 06, 2026 at 11:04:44PM -0700, Christoph Hellwig wrote:
> On Fri, Sep 04, 2026 at 06:08:51PM -0700, Darrick J. Wong wrote:
> > > @@ -387,6 +388,10 @@ xfs_error_report(
> > >  		"Internal error %s at line %d of file %s.  Caller %pS",
> > >  			    tag, linenum, filename, failaddr);
> > >  
> > > +		if (error)
> > > +			xfs_alert_tag(mp, XFS_PTAG_ERROR_REPORT,
> > > +				      "Error Code: %d", error);
> > 
> > /me wonders, should the error erport just go in the "Internal error..."
> > message above?
> > 
> > "Internal error $tag ($error) at line..."
> 
> Yeah, that seems much easier to follow.  Maybe make it conditional on
> an error being passed.
> 
> > > +extern void xfs_error_report(const char *tag, int level, int error,
> > > +			     struct xfs_mount *mp, const char *filename,
> > > +			     int linenum, xfs_failaddr_t failaddr);
> 
> .. and let's drop the pointless extern here while we touch this.
> 
> > > -			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
> > > +			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);
> 
> Overly long line here now.
> 
> 

Thanks both for the review. I'll work on it and send a proper patch
later on, I need to finish a case for xfstests first, then I'll move to
this one...

Re: [RFC PATCH 1/3] xfs: add xfs_error_report the ability to display an error code

[Carlos Maiolino]

On Fri, Sep 04, 2026 at 06:08:51PM -0700, Darrick J. Wong wrote:
> On Fri, Sep 04, 2026 at 01:32:21PM +0200, cem@kernel.org wrote:
> > From: Carlos Maiolino <cem@kernel.org>
> > 
> > Users could opt to request an error code to be printed
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > Signed-off-by: Carlos Maiolino <cem@kernel.org>
> > ---
> >  fs/xfs/xfs_error.c         |  7 ++++++-
> >  fs/xfs/xfs_error.h         | 10 +++++-----
> >  fs/xfs/xfs_exchmaps_item.c |  6 +++---
> >  fs/xfs/xfs_inode_item.c    |  2 +-
> >  fs/xfs/xfs_log_recover.c   |  2 +-
> >  5 files changed, 16 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> > index dbd87e137694..1c882b910ffd 100644
> > --- a/fs/xfs/xfs_error.c
> > +++ b/fs/xfs/xfs_error.c
> > @@ -377,6 +377,7 @@ void
> >  xfs_error_report(
> >  	const char		*tag,
> >  	int			level,
> > +	int			error,
> >  	struct xfs_mount	*mp,
> >  	const char		*filename,
> >  	int			linenum,
> > @@ -387,6 +388,10 @@ xfs_error_report(
> >  		"Internal error %s at line %d of file %s.  Caller %pS",
> >  			    tag, linenum, filename, failaddr);
> >  
> > +		if (error)
> > +			xfs_alert_tag(mp, XFS_PTAG_ERROR_REPORT,
> > +				      "Error Code: %d", error);
> 
> /me wonders, should the error erport just go in the "Internal error..."
> message above?
> 
> "Internal error $tag ($error) at line..."

Sure, I initially didn't want a longer line, but I'd rather have a
single line here too.

> 
> > +
> >  		xfs_stack_trace();
> >  	}
> >  }
> > @@ -404,7 +409,7 @@ xfs_corruption_error(
> >  {
> >  	if (buf && level <= xfs_error_level)
> >  		xfs_hex_dump(buf, bufsize);
> > -	xfs_error_report(tag, level, mp, filename, linenum, failaddr);
> > +	xfs_error_report(tag, level, 0, mp, filename, linenum, failaddr);
> >  	xfs_alert(mp, "Corruption detected. Unmount and run xfs_repair");
> >  }
> >  
> > diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
> > index 0b9c5ba8a598..1a1aa60e92f2 100644
> > --- a/fs/xfs/xfs_error.h
> > +++ b/fs/xfs/xfs_error.h
> > @@ -8,9 +8,9 @@
> >  
> >  struct xfs_mount;
> >  
> > -extern void xfs_error_report(const char *tag, int level, struct xfs_mount *mp,
> > -			const char *filename, int linenum,
> > -			xfs_failaddr_t failaddr);
> > +extern void xfs_error_report(const char *tag, int level, int error,
> > +			     struct xfs_mount *mp, const char *filename,
> > +			     int linenum, xfs_failaddr_t failaddr);
> >  extern void xfs_corruption_error(const char *tag, int level,
> >  			struct xfs_mount *mp, const void *buf, size_t bufsize,
> >  			const char *filename, int linenum,
> > @@ -25,8 +25,8 @@ extern void xfs_inode_verifier_error(struct xfs_inode *ip, int error,
> >  			const char *name, const void *buf, size_t bufsz,
> >  			xfs_failaddr_t failaddr);
> >  
> > -#define	XFS_ERROR_REPORT(e, lvl, mp)	\
> > -	xfs_error_report(e, lvl, mp, __FILE__, __LINE__, __return_address)
> > +#define	XFS_ERROR_REPORT(e, lvl, error, mp)	\
> > +	xfs_error_report(e, lvl, error, mp, __FILE__, __LINE__, __return_address)
> >  #define	XFS_CORRUPTION_ERROR(e, lvl, mp, buf, bufsize)	\
> >  	xfs_corruption_error(e, lvl, mp, buf, bufsize, \
> >  			     __FILE__, __LINE__, __return_address)
> > diff --git a/fs/xfs/xfs_exchmaps_item.c b/fs/xfs/xfs_exchmaps_item.c
> > index 264a121c5e16..b94c5e5e6549 100644
> > --- a/fs/xfs/xfs_exchmaps_item.c
> > +++ b/fs/xfs/xfs_exchmaps_item.c
> > @@ -559,13 +559,13 @@ xlog_recover_xmi_commit_pass2(
> >  
> >  	len = sizeof(struct xfs_xmi_log_format);
> >  	if (item->ri_buf[0].i_len != len) {
> > -		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
> > +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);
> 
> These should be -EFSCORRUPTED, not 0, right?

This was a RFC and my goal here was just leave the messages unchanged by
now, I'll change it for a formal patch.

> 
> --D
> 
> >  		return -EFSCORRUPTED;
> >  	}
> >  
> >  	xmi_formatp = item->ri_buf[0].i_addr;
> >  	if (xmi_formatp->__pad != 0) {
> > -		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
> > +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);
> >  		return -EFSCORRUPTED;
> >  	}
> >  
> > @@ -600,7 +600,7 @@ xlog_recover_xmd_commit_pass2(
> >  
> >  	xmd_formatp = item->ri_buf[0].i_addr;
> >  	if (item->ri_buf[0].i_len != sizeof(struct xfs_xmd_log_format)) {
> > -		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
> > +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);
> >  		return -EFSCORRUPTED;
> >  	}
> >  
> > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> > index c6cb0b6b9e46..862b7d7d3ce4 100644
> > --- a/fs/xfs/xfs_inode_item.c
> > +++ b/fs/xfs/xfs_inode_item.c
> > @@ -1185,7 +1185,7 @@ xfs_inode_item_format_convert(
> >  	struct xfs_inode_log_format_32	*in_f32 = buf->i_addr;
> >  
> >  	if (buf->i_len != sizeof(*in_f32)) {
> > -		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> > +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, NULL);
> >  		return -EFSCORRUPTED;
> >  	}
> >  
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index 2f76531842f8..083e4bf61e2b 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -2932,7 +2932,7 @@ xlog_recover_process(
> >  		 * fatal log corruption failure.
> >  		 */
> >  		if (xfs_has_crc(log->l_mp)) {
> > -			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
> > +			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);
> >  			return -EFSCORRUPTED;
> >  		}
> >  	}
> > -- 
> > 2.55.0
> > 
> > 
> 

Re: [RFC PATCH 2/3] xfs: enable xfs_trans_cancel() to report and error code

[Carlos Maiolino]

On Sun, Sep 06, 2026 at 11:05:59PM -0700, Christoph Hellwig wrote:
> On Fri, Sep 04, 2026 at 06:10:54PM -0700, Darrick J. Wong wrote:
> > On Fri, Sep 04, 2026 at 01:32:22PM +0200, cem@kernel.org wrote:
> > > From: Carlos Maiolino <cem@kernel.org>
> > > 
> > > Once in a while it's useful to know exactly what kind of error caused a
> > > transaction to be cancelled.
> > > Enable xfs_trans_cancel() to receive an error code to be reported.
> > 
> > Stylistically I think I'd rather have an explicit @error and some macros
> > to avoid the va_args machinery.
> > 
> > 
> > void __xfs_trans_cancel(struct xfs_trans *tp, int error) {...}
> > 
> > #define xfs_trans_cancel(tp) __xfs_trans_cancel(tp, 0)
> > #define xfs_trans_cancel_with(tp, error) __xfs_trans_cancel(tp, error)
> 
> Agreed.
> 
> 

Thanks! This is easily doable, I just wanted to avoid that on an initial
patch to don't clobber the patch with the changes. I'll get them ready

Re: [RFC PATCH 1/3] xfs: add xfs_error_report the ability to display an error code

[Darrick J. Wong]

On Tue, Sep 08, 2026 at 07:36:34AM +0200, Carlos Maiolino wrote:
> On Fri, Sep 04, 2026 at 06:08:51PM -0700, Darrick J. Wong wrote:
> > On Fri, Sep 04, 2026 at 01:32:21PM +0200, cem@kernel.org wrote:
> > > From: Carlos Maiolino <cem@kernel.org>
> > > 
> > > Users could opt to request an error code to be printed
> > > 
> > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > > Signed-off-by: Carlos Maiolino <cem@kernel.org>
> > > ---
> > >  fs/xfs/xfs_error.c         |  7 ++++++-
> > >  fs/xfs/xfs_error.h         | 10 +++++-----
> > >  fs/xfs/xfs_exchmaps_item.c |  6 +++---
> > >  fs/xfs/xfs_inode_item.c    |  2 +-
> > >  fs/xfs/xfs_log_recover.c   |  2 +-
> > >  5 files changed, 16 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> > > index dbd87e137694..1c882b910ffd 100644
> > > --- a/fs/xfs/xfs_error.c
> > > +++ b/fs/xfs/xfs_error.c
> > > @@ -377,6 +377,7 @@ void
> > >  xfs_error_report(
> > >  	const char		*tag,
> > >  	int			level,
> > > +	int			error,
> > >  	struct xfs_mount	*mp,
> > >  	const char		*filename,
> > >  	int			linenum,
> > > @@ -387,6 +388,10 @@ xfs_error_report(
> > >  		"Internal error %s at line %d of file %s.  Caller %pS",
> > >  			    tag, linenum, filename, failaddr);
> > >  
> > > +		if (error)
> > > +			xfs_alert_tag(mp, XFS_PTAG_ERROR_REPORT,
> > > +				      "Error Code: %d", error);
> > 
> > /me wonders, should the error erport just go in the "Internal error..."
> > message above?
> > 
> > "Internal error $tag ($error) at line..."
> 
> Sure, I initially didn't want a longer line, but I'd rather have a
> single line here too.
> 
> > 
> > > +
> > >  		xfs_stack_trace();
> > >  	}
> > >  }
> > > @@ -404,7 +409,7 @@ xfs_corruption_error(
> > >  {
> > >  	if (buf && level <= xfs_error_level)
> > >  		xfs_hex_dump(buf, bufsize);
> > > -	xfs_error_report(tag, level, mp, filename, linenum, failaddr);
> > > +	xfs_error_report(tag, level, 0, mp, filename, linenum, failaddr);
> > >  	xfs_alert(mp, "Corruption detected. Unmount and run xfs_repair");
> > >  }
> > >  
> > > diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
> > > index 0b9c5ba8a598..1a1aa60e92f2 100644
> > > --- a/fs/xfs/xfs_error.h
> > > +++ b/fs/xfs/xfs_error.h
> > > @@ -8,9 +8,9 @@
> > >  
> > >  struct xfs_mount;
> > >  
> > > -extern void xfs_error_report(const char *tag, int level, struct xfs_mount *mp,
> > > -			const char *filename, int linenum,
> > > -			xfs_failaddr_t failaddr);
> > > +extern void xfs_error_report(const char *tag, int level, int error,
> > > +			     struct xfs_mount *mp, const char *filename,
> > > +			     int linenum, xfs_failaddr_t failaddr);
> > >  extern void xfs_corruption_error(const char *tag, int level,
> > >  			struct xfs_mount *mp, const void *buf, size_t bufsize,
> > >  			const char *filename, int linenum,
> > > @@ -25,8 +25,8 @@ extern void xfs_inode_verifier_error(struct xfs_inode *ip, int error,
> > >  			const char *name, const void *buf, size_t bufsz,
> > >  			xfs_failaddr_t failaddr);
> > >  
> > > -#define	XFS_ERROR_REPORT(e, lvl, mp)	\
> > > -	xfs_error_report(e, lvl, mp, __FILE__, __LINE__, __return_address)
> > > +#define	XFS_ERROR_REPORT(e, lvl, error, mp)	\
> > > +	xfs_error_report(e, lvl, error, mp, __FILE__, __LINE__, __return_address)
> > >  #define	XFS_CORRUPTION_ERROR(e, lvl, mp, buf, bufsize)	\
> > >  	xfs_corruption_error(e, lvl, mp, buf, bufsize, \
> > >  			     __FILE__, __LINE__, __return_address)
> > > diff --git a/fs/xfs/xfs_exchmaps_item.c b/fs/xfs/xfs_exchmaps_item.c
> > > index 264a121c5e16..b94c5e5e6549 100644
> > > --- a/fs/xfs/xfs_exchmaps_item.c
> > > +++ b/fs/xfs/xfs_exchmaps_item.c
> > > @@ -559,13 +559,13 @@ xlog_recover_xmi_commit_pass2(
> > >  
> > >  	len = sizeof(struct xfs_xmi_log_format);
> > >  	if (item->ri_buf[0].i_len != len) {
> > > -		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
> > > +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);
> > 
> > These should be -EFSCORRUPTED, not 0, right?
> 
> This was a RFC and my goal here was just leave the messages unchanged by
> now, I'll change it for a formal patch.

Oh, ok.  On some level it would be fun to wrap all that crap in a macro
so that all -EFSCORRUPTED returns could be a logging site:

#define XFS_EFSCORRUPTED(mp) \
(XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, -EFSCORRUPTED, (mp)),
-EFSCORRUPTED)

		return XFS_EFSCORRUPTED(mp);

Eh.  I hate preprocessor macros.  I'll just call the above a bad idea
and press <Send>.

--D

> 
> > 
> > --D
> > 
> > >  		return -EFSCORRUPTED;
> > >  	}
> > >  
> > >  	xmi_formatp = item->ri_buf[0].i_addr;
> > >  	if (xmi_formatp->__pad != 0) {
> > > -		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
> > > +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);
> > >  		return -EFSCORRUPTED;
> > >  	}
> > >  
> > > @@ -600,7 +600,7 @@ xlog_recover_xmd_commit_pass2(
> > >  
> > >  	xmd_formatp = item->ri_buf[0].i_addr;
> > >  	if (item->ri_buf[0].i_len != sizeof(struct xfs_xmd_log_format)) {
> > > -		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
> > > +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);
> > >  		return -EFSCORRUPTED;
> > >  	}
> > >  
> > > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> > > index c6cb0b6b9e46..862b7d7d3ce4 100644
> > > --- a/fs/xfs/xfs_inode_item.c
> > > +++ b/fs/xfs/xfs_inode_item.c
> > > @@ -1185,7 +1185,7 @@ xfs_inode_item_format_convert(
> > >  	struct xfs_inode_log_format_32	*in_f32 = buf->i_addr;
> > >  
> > >  	if (buf->i_len != sizeof(*in_f32)) {
> > > -		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> > > +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, NULL);
> > >  		return -EFSCORRUPTED;
> > >  	}
> > >  
> > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > > index 2f76531842f8..083e4bf61e2b 100644
> > > --- a/fs/xfs/xfs_log_recover.c
> > > +++ b/fs/xfs/xfs_log_recover.c
> > > @@ -2932,7 +2932,7 @@ xlog_recover_process(
> > >  		 * fatal log corruption failure.
> > >  		 */
> > >  		if (xfs_has_crc(log->l_mp)) {
> > > -			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
> > > +			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, 0, log->l_mp);
> > >  			return -EFSCORRUPTED;
> > >  		}
> > >  	}
> > > -- 
> > > 2.55.0
> > > 
> > > 
> > 
>