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, 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
