* [PATCH v2] xfs: allow setting full range of panic tags
@ 2023-01-26 5:29 Donald Douwsma
2023-01-27 16:27 ` Darrick J. Wong
0 siblings, 1 reply; 4+ messages in thread
From: Donald Douwsma @ 2023-01-26 5:29 UTC (permalink / raw)
To: linux-xfs; +Cc: Donald Douwsma
xfs will not allow combining other panic masks with
XFS_PTAG_VERIFIER_ERROR.
sysctl fs.xfs.panic_mask=511
sysctl: setting key "fs.xfs.panic_mask": Invalid argument
fs.xfs.panic_mask = 511
Update to the maximum value that can be set to allow the full range of
masks.
Fixes: d519da41e2b7 ("xfs: Introduce XFS_PTAG_VERIFIER_ERROR panic mask")
Signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
---
Documentation/admin-guide/xfs.rst | 2 +-
fs/xfs/xfs_error.h | 13 ++++++++++++-
fs/xfs/xfs_globals.c | 3 ++-
3 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst
index 8de008c0c5ad..e2561416391c 100644
--- a/Documentation/admin-guide/xfs.rst
+++ b/Documentation/admin-guide/xfs.rst
@@ -296,7 +296,7 @@ The following sysctls are available for the XFS filesystem:
XFS_ERRLEVEL_LOW: 1
XFS_ERRLEVEL_HIGH: 5
- fs.xfs.panic_mask (Min: 0 Default: 0 Max: 256)
+ fs.xfs.panic_mask (Min: 0 Default: 0 Max: 511)
Causes certain error conditions to call BUG(). Value is a bitmask;
OR together the tags which represent errors which should cause panics:
diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
index dbe6c37dc697..a015f7b370dc 100644
--- a/fs/xfs/xfs_error.h
+++ b/fs/xfs/xfs_error.h
@@ -75,7 +75,7 @@ extern int xfs_errortag_clearall(struct xfs_mount *mp);
/*
* XFS panic tags -- allow a call to xfs_alert_tag() be turned into
- * a panic by setting xfs_panic_mask in a sysctl.
+ * a panic by setting fs.xfs.panic_mask in a sysctl.
*/
#define XFS_NO_PTAG 0u
#define XFS_PTAG_IFLUSH (1u << 0)
@@ -88,6 +88,17 @@ extern int xfs_errortag_clearall(struct xfs_mount *mp);
#define XFS_PTAG_FSBLOCK_ZERO (1u << 7)
#define XFS_PTAG_VERIFIER_ERROR (1u << 8)
+#define XFS_MAX_PTAG ( \
+ XFS_PTAG_IFLUSH | \
+ XFS_PTAG_LOGRES | \
+ XFS_PTAG_AILDELETE | \
+ XFS_PTAG_ERROR_REPORT | \
+ XFS_PTAG_SHUTDOWN_CORRUPT | \
+ XFS_PTAG_SHUTDOWN_IOERROR | \
+ XFS_PTAG_SHUTDOWN_LOGERROR | \
+ XFS_PTAG_FSBLOCK_ZERO | \
+ XFS_PTAG_VERIFIER_ERROR)
+
#define XFS_PTAG_STRINGS \
{ XFS_NO_PTAG, "none" }, \
{ XFS_PTAG_IFLUSH, "iflush" }, \
diff --git a/fs/xfs/xfs_globals.c b/fs/xfs/xfs_globals.c
index 4d0a98f920ca..ff129acce8e6 100644
--- a/fs/xfs/xfs_globals.c
+++ b/fs/xfs/xfs_globals.c
@@ -4,6 +4,7 @@
* All Rights Reserved.
*/
#include "xfs.h"
+#include "xfs_error.h"
/*
* Tunable XFS parameters. xfs_params is required even when CONFIG_SYSCTL=n,
@@ -15,7 +16,7 @@ xfs_param_t xfs_params = {
/* MIN DFLT MAX */
.sgid_inherit = { 0, 0, 1 },
.symlink_mode = { 0, 0, 1 },
- .panic_mask = { 0, 0, 256 },
+ .panic_mask = { 0, 0, XFS_MAX_PTAG},
.error_level = { 0, 3, 11 },
.syncd_timer = { 1*100, 30*100, 7200*100},
.stats_clear = { 0, 0, 1 },
--
2.31.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] xfs: allow setting full range of panic tags
2023-01-26 5:29 [PATCH v2] xfs: allow setting full range of panic tags Donald Douwsma
@ 2023-01-27 16:27 ` Darrick J. Wong
2023-01-30 22:20 ` Eric Sandeen
0 siblings, 1 reply; 4+ messages in thread
From: Darrick J. Wong @ 2023-01-27 16:27 UTC (permalink / raw)
To: Donald Douwsma; +Cc: linux-xfs
On Thu, Jan 26, 2023 at 04:29:10PM +1100, Donald Douwsma wrote:
> xfs will not allow combining other panic masks with
> XFS_PTAG_VERIFIER_ERROR.
>
> sysctl fs.xfs.panic_mask=511
> sysctl: setting key "fs.xfs.panic_mask": Invalid argument
> fs.xfs.panic_mask = 511
>
> Update to the maximum value that can be set to allow the full range of
> masks.
>
> Fixes: d519da41e2b7 ("xfs: Introduce XFS_PTAG_VERIFIER_ERROR panic mask")
> Signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
> ---
> Documentation/admin-guide/xfs.rst | 2 +-
> fs/xfs/xfs_error.h | 13 ++++++++++++-
> fs/xfs/xfs_globals.c | 3 ++-
> 3 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst
> index 8de008c0c5ad..e2561416391c 100644
> --- a/Documentation/admin-guide/xfs.rst
> +++ b/Documentation/admin-guide/xfs.rst
> @@ -296,7 +296,7 @@ The following sysctls are available for the XFS filesystem:
> XFS_ERRLEVEL_LOW: 1
> XFS_ERRLEVEL_HIGH: 5
>
> - fs.xfs.panic_mask (Min: 0 Default: 0 Max: 256)
> + fs.xfs.panic_mask (Min: 0 Default: 0 Max: 511)
> Causes certain error conditions to call BUG(). Value is a bitmask;
> OR together the tags which represent errors which should cause panics:
>
> diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
> index dbe6c37dc697..a015f7b370dc 100644
> --- a/fs/xfs/xfs_error.h
> +++ b/fs/xfs/xfs_error.h
> @@ -75,7 +75,7 @@ extern int xfs_errortag_clearall(struct xfs_mount *mp);
>
> /*
> * XFS panic tags -- allow a call to xfs_alert_tag() be turned into
> - * a panic by setting xfs_panic_mask in a sysctl.
> + * a panic by setting fs.xfs.panic_mask in a sysctl.
> */
> #define XFS_NO_PTAG 0u
> #define XFS_PTAG_IFLUSH (1u << 0)
> @@ -88,6 +88,17 @@ extern int xfs_errortag_clearall(struct xfs_mount *mp);
> #define XFS_PTAG_FSBLOCK_ZERO (1u << 7)
> #define XFS_PTAG_VERIFIER_ERROR (1u << 8)
>
> +#define XFS_MAX_PTAG ( \
The ptag values are a bitmask, not a continuous integer range, so the
name should have "MASK" in it, e.g.
#define XFS_PTAG_MASK (XFS_PTAG_IFLUSH | \
XFS_PTAG_LOGRES | \
...
and follow the customary style where the macro definition lines are
indented from the name.
Otherwise this looks fine.
--D
> + XFS_PTAG_IFLUSH | \
> + XFS_PTAG_LOGRES | \
> + XFS_PTAG_AILDELETE | \
> + XFS_PTAG_ERROR_REPORT | \
> + XFS_PTAG_SHUTDOWN_CORRUPT | \
> + XFS_PTAG_SHUTDOWN_IOERROR | \
> + XFS_PTAG_SHUTDOWN_LOGERROR | \
> + XFS_PTAG_FSBLOCK_ZERO | \
> + XFS_PTAG_VERIFIER_ERROR)
> +
> #define XFS_PTAG_STRINGS \
> { XFS_NO_PTAG, "none" }, \
> { XFS_PTAG_IFLUSH, "iflush" }, \
> diff --git a/fs/xfs/xfs_globals.c b/fs/xfs/xfs_globals.c
> index 4d0a98f920ca..ff129acce8e6 100644
> --- a/fs/xfs/xfs_globals.c
> +++ b/fs/xfs/xfs_globals.c
> @@ -4,6 +4,7 @@
> * All Rights Reserved.
> */
> #include "xfs.h"
> +#include "xfs_error.h"
>
> /*
> * Tunable XFS parameters. xfs_params is required even when CONFIG_SYSCTL=n,
> @@ -15,7 +16,7 @@ xfs_param_t xfs_params = {
> /* MIN DFLT MAX */
> .sgid_inherit = { 0, 0, 1 },
> .symlink_mode = { 0, 0, 1 },
> - .panic_mask = { 0, 0, 256 },
> + .panic_mask = { 0, 0, XFS_MAX_PTAG},
> .error_level = { 0, 3, 11 },
> .syncd_timer = { 1*100, 30*100, 7200*100},
> .stats_clear = { 0, 0, 1 },
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] xfs: allow setting full range of panic tags
2023-01-27 16:27 ` Darrick J. Wong
@ 2023-01-30 22:20 ` Eric Sandeen
2023-01-31 19:28 ` Darrick J. Wong
0 siblings, 1 reply; 4+ messages in thread
From: Eric Sandeen @ 2023-01-30 22:20 UTC (permalink / raw)
To: Darrick J. Wong, Donald Douwsma; +Cc: linux-xfs
On 1/27/23 10:27 AM, Darrick J. Wong wrote:
> On Thu, Jan 26, 2023 at 04:29:10PM +1100, Donald Douwsma wrote:
>> xfs will not allow combining other panic masks with
>> XFS_PTAG_VERIFIER_ERROR.
>>
>> sysctl fs.xfs.panic_mask=511
>> sysctl: setting key "fs.xfs.panic_mask": Invalid argument
>> fs.xfs.panic_mask = 511
>>
>> Update to the maximum value that can be set to allow the full range of
>> masks.
>>
>> Fixes: d519da41e2b7 ("xfs: Introduce XFS_PTAG_VERIFIER_ERROR panic mask")
whoops :)
I wonder ...
>> Signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
...
> The ptag values are a bitmask, not a continuous integer range, so the
> name should have "MASK" in it, e.g.
>
> #define XFS_PTAG_MASK (XFS_PTAG_IFLUSH | \
> XFS_PTAG_LOGRES | \
> ...
>
> and follow the customary style where the macro definition lines are
> indented from the name.
>
> Otherwise this looks fine.
>
> --D
>
>> +#define XFS_MAX_PTAG ( \
>> + XFS_PTAG_IFLUSH | \
>> + XFS_PTAG_LOGRES | \
>> + XFS_PTAG_AILDELETE | \
>> + XFS_PTAG_ERROR_REPORT | \
>> + XFS_PTAG_SHUTDOWN_CORRUPT | \
>> + XFS_PTAG_SHUTDOWN_IOERROR | \
>> + XFS_PTAG_SHUTDOWN_LOGERROR | \
>> + XFS_PTAG_FSBLOCK_ZERO | \
>> + XFS_PTAG_VERIFIER_ERROR)
>> +
...
>> + .panic_mask = { 0, 0, XFS_MAX_PTAG},
>> .error_level = { 0, 3, 11 },
>> .syncd_timer = { 1*100, 30*100, 7200*100},
>> .stats_clear = { 0, 0, 1 },
Do we really gain anything by carefully crafting the max bit that can be set here?
Nothing stops someone from forgetting to update XFS_MAX_PTAG (or whatever it
may be named) in the future, and I think nothing bad happens if you try to turn
on a PTAG that doesn't exist. Should we just set it to LONG_MAX and be done with
it?
(I guess it's maybe nice to tell the user that they're out of range, but it is
a debug knob after all. Just a thought, I'm ot super picky about this.)
-Eric
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] xfs: allow setting full range of panic tags
2023-01-30 22:20 ` Eric Sandeen
@ 2023-01-31 19:28 ` Darrick J. Wong
0 siblings, 0 replies; 4+ messages in thread
From: Darrick J. Wong @ 2023-01-31 19:28 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Donald Douwsma, linux-xfs
On Mon, Jan 30, 2023 at 04:20:00PM -0600, Eric Sandeen wrote:
> On 1/27/23 10:27 AM, Darrick J. Wong wrote:
> > On Thu, Jan 26, 2023 at 04:29:10PM +1100, Donald Douwsma wrote:
> >> xfs will not allow combining other panic masks with
> >> XFS_PTAG_VERIFIER_ERROR.
> >>
> >> sysctl fs.xfs.panic_mask=511
> >> sysctl: setting key "fs.xfs.panic_mask": Invalid argument
> >> fs.xfs.panic_mask = 511
> >>
> >> Update to the maximum value that can be set to allow the full range of
> >> masks.
> >>
> >> Fixes: d519da41e2b7 ("xfs: Introduce XFS_PTAG_VERIFIER_ERROR panic mask")
>
> whoops :)
>
> I wonder ...
>
> >> Signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
>
> ...
>
> > The ptag values are a bitmask, not a continuous integer range, so the
> > name should have "MASK" in it, e.g.
> >
> > #define XFS_PTAG_MASK (XFS_PTAG_IFLUSH | \
> > XFS_PTAG_LOGRES | \
> > ...
> >
> > and follow the customary style where the macro definition lines are
> > indented from the name.
> >
> > Otherwise this looks fine.
> >
> > --D
> >
> >> +#define XFS_MAX_PTAG ( \
> >> + XFS_PTAG_IFLUSH | \
> >> + XFS_PTAG_LOGRES | \
> >> + XFS_PTAG_AILDELETE | \
> >> + XFS_PTAG_ERROR_REPORT | \
> >> + XFS_PTAG_SHUTDOWN_CORRUPT | \
> >> + XFS_PTAG_SHUTDOWN_IOERROR | \
> >> + XFS_PTAG_SHUTDOWN_LOGERROR | \
> >> + XFS_PTAG_FSBLOCK_ZERO | \
> >> + XFS_PTAG_VERIFIER_ERROR)
> >> +
>
> ...
>
> >> + .panic_mask = { 0, 0, XFS_MAX_PTAG},
> >> .error_level = { 0, 3, 11 },
> >> .syncd_timer = { 1*100, 30*100, 7200*100},
> >> .stats_clear = { 0, 0, 1 },
>
> Do we really gain anything by carefully crafting the max bit that can be set here?
> Nothing stops someone from forgetting to update XFS_MAX_PTAG (or whatever it
> may be named) in the future,
That's true, but every other _ALL and _MASK define in the xfs codebase
also have that problem. *Most* of the time people remember to update
it.
> and I think nothing bad happens if you try to turn
> on a PTAG that doesn't exist. Should we just set it to LONG_MAX and be done with
> it?
That would break the existing input validation:
# echo 1023 > /proc/sys/fs/xfs/panic_mask
-bash: echo: write error: Invalid argument
--D
> (I guess it's maybe nice to tell the user that they're out of range, but it is
> a debug knob after all. Just a thought, I'm ot super picky about this.)
> -Eric
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-31 19:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-26 5:29 [PATCH v2] xfs: allow setting full range of panic tags Donald Douwsma
2023-01-27 16:27 ` Darrick J. Wong
2023-01-30 22:20 ` Eric Sandeen
2023-01-31 19:28 ` Darrick J. Wong
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.