Re: [PATCH RFC 7/7] xfs: error tag to force zeroing on debug kernels

[Brian Foster <bfoster@redhat.com>]

On Tue, Jun 10, 2025 at 10:20:28AM -0400, Brian Foster wrote:
> On Tue, Jun 10, 2025 at 06:30:29AM -0700, Christoph Hellwig wrote:
> > On Tue, Jun 10, 2025 at 08:26:45AM -0400, Brian Foster wrote:
> > > Well that is kind of the question.. ;) My preference was to either add
> > > something to fstests to enable select errortags by default on every
> > > mount (or do the same in-kernel via XFS_DEBUG[_ERRTAGS] or some such)
> > > over just creating a one-off test that runs fsx or whatever with this
> > > error tag turned on. [1].
> > > 
> > > That said, I wouldn't be opposed to just doing both if folks prefer
> > > that. It just bugs me to add yet another test that only runs a specific
> > > fsx test when we get much more coverage by running the full suite of
> > > tests. IOW, whenever somebody is testing a kernel that would actually
> > > run a custom test (XFS_DEBUG plus specific errortag support), we could
> > > in theory be running the whole suite with the same errortag turned on
> > > (albeit perhaps at a lesser frequency than a custom test would use). So
> > > from that perspective I'm not sure it makes a whole lot of sense to do
> > > both.
> > > 
> > > So any thoughts from anyone on a custom test vs. enabling errortag
> > > defaults (via fstests or kernel) vs. some combination of both?
> > 
> > I definitively like a targeted test to exercise it.  If you want
> > additional knows to turn on error tags that's probably fine if it
> > works out.  I'm worried about adding more flags to xfstests because
> > it makes it really hard to figure out what runs are need for good
> > test coverage.
> > 
> > 
> 
> Yeah, an fstests variable would add yet another configuration to test,
> which maybe defeats the point. But we could still turn on certain tags
> by default in the kernel. For example, see the couple of open coded
> get_random_u32_below() callsites in XFS where we already effectively do
> this for XFS_DEBUG, they just aren't implemented as proper errortags.
> 
> I think the main thing that would need to change is to not xfs_warn() on
> those knobs when they are enabled by default. I think there are a few
> different ways that could possibly be done, ideally so we go back to
> default/warn behavior when userspace makes an explicit errortag change,
> but I'd have to play around with it a little bit. Hm?
> 
> Anyways, given the fstests config matrix concern I'm inclined to at
> least give something like that a try first and then fall back to a
> custom test if that fails or is objectionable for some other reason..
> 
> Brian
> 
> 

Here's a prototype for 1. an errtag quiet mode and 2. on-by-default
tags. The alternative to a per-mount flag would be to hack a new struct
into m_errortag that holds the current randfactor as well as a per-tag
quiet flag, though I'm not sure how much people care about that. I
didn't really plan on exposing this to userspace or anything for per-tag
support, but this does mean all tags would start to warn once userspace
changes any tag. I suppose that could become noisy if some day we end up
with a bunch more default enabled tags. *shrug* I could go either way.

Otherwise I think this would allow conversion of the two open coded
get_random_u32_below() cases and the new force zero tag into
on-by-default errortags. Any thoughts?

--- 8< ---

 diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index dbd87e137694..54b38143a7a6 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -69,6 +69,7 @@ static unsigned int xfs_errortag_random_default[] = {
 struct xfs_errortag_attr {
 	struct attribute	attr;
 	unsigned int		tag;
+	bool			enable_default;
 };
 
 static inline struct xfs_errortag_attr *
@@ -129,12 +130,15 @@ static const struct sysfs_ops xfs_errortag_sysfs_ops = {
 	.store = xfs_errortag_attr_store,
 };
 
-#define XFS_ERRORTAG_ATTR_RW(_name, _tag) \
+#define __XFS_ERRORTAG_ATTR_RW(_name, _tag, enable) \
 static struct xfs_errortag_attr xfs_errortag_attr_##_name = {		\
 	.attr = {.name = __stringify(_name),				\
 		 .mode = VERIFY_OCTAL_PERMISSIONS(S_IWUSR | S_IRUGO) },	\
 	.tag	= (_tag),						\
+	.enable_default = enable,					\
 }
+#define XFS_ERRORTAG_ATTR_RW(_name, _tag) \
+	__XFS_ERRORTAG_ATTR_RW(_name, _tag, false)
 
 #define XFS_ERRORTAG_ATTR_LIST(_name) &xfs_errortag_attr_##_name.attr
 
@@ -240,6 +244,25 @@ static const struct kobj_type xfs_errortag_ktype = {
 	.default_groups = xfs_errortag_groups,
 };
 
+static void
+xfs_errortag_init_enable_defaults(
+	struct xfs_mount	*mp)
+{
+	int i;
+
+	for (i = 0; xfs_errortag_attrs[i]; i++) {
+		struct xfs_errortag_attr *xfs_attr =
+				to_attr(xfs_errortag_attrs[i]);
+
+		if (!xfs_attr->enable_default)
+			continue;
+
+		xfs_set_quiet_errtag(mp);
+		mp->m_errortag[xfs_attr->tag] =
+			xfs_errortag_random_default[xfs_attr->tag];
+	}
+}
+
 int
 xfs_errortag_init(
 	struct xfs_mount	*mp)
@@ -251,6 +274,8 @@ xfs_errortag_init(
 	if (!mp->m_errortag)
 		return -ENOMEM;
 
+	xfs_errortag_init_enable_defaults(mp);
+
 	ret = xfs_sysfs_init(&mp->m_errortag_kobj, &xfs_errortag_ktype,
 				&mp->m_kobj, "errortag");
 	if (ret)
@@ -320,9 +345,11 @@ xfs_errortag_test(
 	if (!randfactor || get_random_u32_below(randfactor))
 		return false;
 
-	xfs_warn_ratelimited(mp,
+	if (!xfs_is_quiet_errtag(mp)) {
+		xfs_warn_ratelimited(mp,
 "Injecting error (%s) at file %s, line %d, on filesystem \"%s\"",
 			expression, file, line, mp->m_super->s_id);
+	}
 	return true;
 }
 
@@ -346,6 +373,7 @@ xfs_errortag_set(
 	if (!xfs_errortag_valid(error_tag))
 		return -EINVAL;
 
+	xfs_clear_quiet_errtag(mp);
 	mp->m_errortag[error_tag] = tag_value;
 	return 0;
 }
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index d85084f9f317..44b02728056f 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -558,6 +558,8 @@ __XFS_HAS_FEAT(nouuid, NOUUID)
  */
 #define XFS_OPSTATE_BLOCKGC_ENABLED	6
 
+/* Debug kernel skips warning on errtag event triggers */
+#define XFS_OPSTATE_QUIET_ERRTAG	7
 /* Kernel has logged a warning about shrink being used on this fs. */
 #define XFS_OPSTATE_WARNED_SHRINK	9
 /* Kernel has logged a warning about logged xattr updates being used. */
@@ -600,6 +602,7 @@ __XFS_IS_OPSTATE(inode32, INODE32)
 __XFS_IS_OPSTATE(readonly, READONLY)
 __XFS_IS_OPSTATE(inodegc_enabled, INODEGC_ENABLED)
 __XFS_IS_OPSTATE(blockgc_enabled, BLOCKGC_ENABLED)
+__XFS_IS_OPSTATE(quiet_errtag, QUIET_ERRTAG)
 #ifdef CONFIG_XFS_QUOTA
 __XFS_IS_OPSTATE(quotacheck_running, QUOTACHECK_RUNNING)
 __XFS_IS_OPSTATE(resuming_quotaon, RESUMING_QUOTAON)