Re: [PATCH v2] xfs: allow setting full range of panic tags

["Darrick J. Wong" <djwong@kernel.org>]

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