Re: [PATCH 3/7] xfs: allow scrub to hook metadata updates in other writers

Christoph Hellwig
2024-01-02T11:30:17Z

On Sun, Dec 31, 2023 at 12:05:13PM -0800, Darrick J. Wong wrote:
> On the author's computer, calling an empty srcu notifier chain was
> observed to have an overhead averaging ~40ns with a maximum of 60ns.
> Adding a no-op notifier function increased the average to ~58ns and
> 66ns. When the quotacheck live update notifier is attached, the average
> increases to ~322ns with a max of 372ns to update scrub's in-memory
> observation data, assuming no lock contention.
>
> With jump labels enabled, calls to empty srcu notifier chains are elided
> from the call sites when there are no hooks registered, which means that
> the overhead is 0.36ns when fsck is not running. For compilers that do
> not support jump labels (all major architectures do), the overhead of a
> no-op notifier call is less bad (on a many-cpu system) than the atomic
> counter ops, so we make the hook switch itself a nop.

Based on the next patch it seems like blocking notifier are the way
to go and thus this patch should switch to using them and the above
needs updates.

> Note: This new code is also split out as a separate patch from its
> initial user so that the author can move patches around his tree with
> ease.

For the final merge candidate at least this comment should go away,
and maybe also the split..

> +config XFS_LIVE_HOOKS
> + bool
> + select JUMP_LABEL if HAVE_ARCH_JUMP_LABEL
> +
> config XFS_ONLINE_SCRUB
> bool "XFS online metadata check support"
> default n
> depends on XFS_FS
> depends on TMPFS && SHMEM
> + select XFS_LIVE_HOOKS
> select XFS_DRAIN_INTENTS

I'm a bit confused by all the extra Kconfig options here.
Why do we need XFS_LIVE_HOOKS, or the existing XFS_DRAIN_INTENTS
instead of just switching the ifdefs to XFS_ONLINE_SCRUB and
selecting JUMP_LABEL if HAVE_ARCH_JUMP_LABEL from
XFS_ONLINE_SCRUB?

Also while I'm at random Kconfig critique, n is the default default
and the default n here can be dropped. I might just send a patch for
that instead of bothering you once this series is in, though.

Otherwise this looks good except for the choice of which notifier
type to use.