From: "Darrick J. Wong" <djwong@kernel.org>
To: Carlos Maiolino <cem@kernel.org>
Cc: hchc@last.de, sandeen@sandeen.net, linux-xfs@vger.kernel.org
Subject: Re: [PATCH v2 1/3] xfs: add xfs_error_report the ability to display an error code
Date: Mon, 14 Sep 2026 08:08:27 -0700 [thread overview]
Message-ID: <20260914150827.GP6265@frogsfrogsfrogs> (raw)
In-Reply-To: <aqgLft0IEhUocC8R@cronus.toxiclabs.cc>
On Mon, Sep 14, 2026 at 05:00:51PM +0200, Carlos Maiolino wrote:
> On Mon, Sep 14, 2026 at 07:42:24AM -0700, Darrick J. Wong wrote:
> > On Mon, Sep 14, 2026 at 01:57:39PM +0200, cem@kernel.org wrote:
> > > From: Carlos Maiolino <cem@kernel.org>
> > >
> > > Users could opt to request an error code to be printed now.
> > > This relies on errname() and CONFIG_SYMBOLIC_ERRNAME without requiring it
> >
> > Eh? No it doesn't...
> >
> > > to be enabled.
> > > Report 'unkown error' If CONFIG_SYMBOLIC_ERRNAME is not enabled or the
> > > user passes 0 as an error.
> > >
> > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > >
> > > ---
> > > V2:
> > > - Fix commit description s/_NAME/_ERRNAME
> > > - xfs_corruption_error should call xfs_error_report with
> > > -EFSCORRUPTED
> > > - Modify xfs_error_report to only print out the error number if
> > > it received an error from the caller.
> > > - Don't use errname() anymore.
> >
> > ...right?
>
> Of course I'd forget something... I'll fix that for next version...
>
>
> >
> > > fs/xfs/xfs_error.c | 16 +++++++++++-----
> > > fs/xfs/xfs_error.h | 9 +++++----
> > > fs/xfs/xfs_exchmaps_item.c | 9 ++++++---
> > > fs/xfs/xfs_inode_item.c | 3 ++-
> > > fs/xfs/xfs_log_recover.c | 3 ++-
> > > fs/xfs/xfs_trans.c | 2 +-
> > > 6 files changed, 27 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> > > index 9ece70fd94ca..c943717f1dd7 100644
> > > --- a/fs/xfs/xfs_error.c
> > > +++ b/fs/xfs/xfs_error.c
> > > @@ -241,16 +241,21 @@ void
> > > xfs_error_report(
> > > const char *tag,
> > > int level,
> > > + int error,
> > > struct xfs_mount *mp,
> > > const char *filename,
> > > int linenum,
> > > xfs_failaddr_t failaddr)
> > > {
> > > if (level <= xfs_error_level) {
> > > - xfs_alert_tag(mp, XFS_PTAG_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,
> > > +"Internal error %s (%d) at line %d of file %s. Caller %pS",
> > > + tag, error, linenum, filename, failaddr);
> > > + else
> > > + xfs_alert_tag(mp, XFS_PTAG_ERROR_REPORT,
> > > +"Internal error %s at line %d of file %s. Caller %pS",
> > > + tag, linenum, filename, failaddr);
> >
> > /me wonders if we should add a tracepoint to these error reporting
> > functions so that one isn't totally reliant on scraping dmesg, but maybe
> > adding fserror_report_metadata is more appropriate for that? Will have
> > to ruminate on that for a while.
>
> Hmmm, what you think about controlling with the tracepoint? Spitting out
> the error? *If* that's the case, the problem I see is when users hit the
> error we can't enable a tracepoint on a past error :P so I'd rather have
> it logged in dmesg without needing to rely on having tracepoints enabled
> or not.
>
> Or perhaps I missed your point here?
I wasn't going to delete anything out of these functions, just add a new
fserror_report_metadata() call. dmesg scraping would continue working
as it always has, but now a monitoring program could watch for events
via fsnotify.
--D
> >
> > In the meantime, this patch looks fine to me,
> > Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
>
> Thanks!
>
>
> >
> > --D
> >
> > > xfs_stack_trace();
> > > }
> > > }
> > > @@ -268,7 +273,8 @@ 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, -EFSCORRUPTED, 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 05fc1d1cf521..3c76d0dad51c 100644
> > > --- a/fs/xfs/xfs_error.h
> > > +++ b/fs/xfs/xfs_error.h
> > > @@ -8,8 +8,9 @@
> > >
> > > struct xfs_mount;
> > >
> > > -void xfs_error_report(const char *tag, int level, struct xfs_mount *mp,
> > > - const char *filename, int linenum, xfs_failaddr_t failaddr);
> > > +void xfs_error_report(const char *tag, int level, int error,
> > > + struct xfs_mount *mp, const char *filename,
> > > + int linenum, xfs_failaddr_t failaddr);
> > > void xfs_corruption_error(const char *tag, int level, struct xfs_mount *mp,
> > > const void *buf, size_t bufsize, const char *filename,
> > > int linenum, xfs_failaddr_t failaddr);
> > > @@ -20,8 +21,8 @@ void xfs_verifier_error(struct xfs_buf *bp, int error, xfs_failaddr_t failaddr);
> > > 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 dd5d92ca1010..9f499a80038c 100644
> > > --- a/fs/xfs/xfs_exchmaps_item.c
> > > +++ b/fs/xfs/xfs_exchmaps_item.c
> > > @@ -573,13 +573,15 @@ xlog_recover_xmi_commit_pass2(
> > >
> > > len = sizeof(struct xfs_xmi_log_format);
> > > if (item->ri_buf[0].iov_len != len) {
> > > - XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
> > > + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW,
> > > + -EFSCORRUPTED, log->l_mp);
> > > return -EFSCORRUPTED;
> > > }
> > >
> > > xmi_formatp = item->ri_buf[0].iov_base;
> > > if (xmi_formatp->__pad != 0) {
> > > - XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
> > > + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW,
> > > + -EFSCORRUPTED, log->l_mp);
> > > return -EFSCORRUPTED;
> > > }
> > >
> > > @@ -614,7 +616,8 @@ xlog_recover_xmd_commit_pass2(
> > >
> > > xmd_formatp = item->ri_buf[0].iov_base;
> > > if (item->ri_buf[0].iov_len != sizeof(struct xfs_xmd_log_format)) {
> > > - XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
> > > + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW,
> > > + -EFSCORRUPTED, log->l_mp);
> > > return -EFSCORRUPTED;
> > > }
> > >
> > > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> > > index 99d6ecccdaa7..a29c90f0650f 100644
> > > --- a/fs/xfs/xfs_inode_item.c
> > > +++ b/fs/xfs/xfs_inode_item.c
> > > @@ -1229,7 +1229,8 @@ xfs_inode_item_format_convert(
> > > struct xfs_inode_log_format_32 *in_f32 = buf->iov_base;
> > >
> > > if (buf->iov_len != sizeof(*in_f32)) {
> > > - XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> > > + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW,
> > > + -EFSCORRUPTED, NULL);
> > > return -EFSCORRUPTED;
> > > }
> > >
> > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > > index e7e49529658b..e92afc455d0f 100644
> > > --- a/fs/xfs/xfs_log_recover.c
> > > +++ b/fs/xfs/xfs_log_recover.c
> > > @@ -2945,7 +2945,8 @@ 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,
> > > + -EFSCORRUPTED, log->l_mp);
> > > return -EFSCORRUPTED;
> > > }
> > > }
> > > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> > > index 1b36cf12d4e3..5c522790e5be 100644
> > > --- a/fs/xfs/xfs_trans.c
> > > +++ b/fs/xfs/xfs_trans.c
> > > @@ -971,7 +971,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, 0, mp);
> > > xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> > > }
> > > #ifdef DEBUG
> > > --
> > > 2.55.0
> > >
> >
>
next prev parent reply other threads:[~2026-09-14 15:08 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-14 11:57 [PATCH v2 0/3] Enable xfs error reporting to print out error codes cem
2026-09-14 11:57 ` [PATCH v2 1/3] xfs: add xfs_error_report the ability to display an error code cem
2026-09-14 14:42 ` Darrick J. Wong
2026-09-14 15:00 ` Carlos Maiolino
2026-09-14 15:08 ` Darrick J. Wong [this message]
2026-09-17 10:22 ` Carlos Maiolino
2026-09-14 11:57 ` [PATCH v2 2/3] xfs: enable xfs_trans_cancel() to report and " cem
2026-09-14 14:38 ` Darrick J. Wong
2026-09-14 11:57 ` [PATCH v2 3/3] xfs: make xfs_iomap_write_direct() report an error to xfs_trans_cancel cem
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260914150827.GP6265@frogsfrogsfrogs \
--to=djwong@kernel.org \
--cc=cem@kernel.org \
--cc=hchc@last.de \
--cc=linux-xfs@vger.kernel.org \
--cc=sandeen@sandeen.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.