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@xxxxxxxxxx>
>
> ...
>
> > 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
