| Message ID | 20230126052910.588098-1-ddouwsma@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] xfs: allow setting full range of panic tags | expand |
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 >
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
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
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 },
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(-)