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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox