Re: [PATCH 1/2] xfs: only clear log incompat flags at clean unmount

Dave Chinner
2024-04-07T22:48:05Z

On Tue, Mar 26, 2024 at 06:50:44PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> While reviewing the online fsck patchset, someone spied the
> xfs_swapext_can_use_without_log_assistance function and wondered why we
> go through this inverted-bitmask dance to avoid setting the
> XFS_SB_FEAT_INCOMPAT_LOG_SWAPEXT feature.
>
> (The same principles apply to the logged extended attribute update
> feature bit in the since-merged LARP series.)
>
> The reason for this dance is that xfs_add_incompat_log_feature is an
> expensive operation -- it forces the log, pushes the AIL, and then if
> nobody's beaten us to it, sets the feature bit and issues a synchronous
> write of the primary superblock. That could be a one-time cost
> amortized over the life of the filesystem, but the log quiesce and cover
> operations call xfs_clear_incompat_log_features to remove feature bits
> opportunistically. On a moderately loaded filesystem this leads to us
> cycling those bits on and off over and over, which hurts performance.
>
> Why do we clear the log incompat bits? Back in ~2020 I think Dave and I
> had a conversation on IRC[2] about what the log incompat bits represent.
> IIRC in that conversation we decided that the log incompat bits protect
> unrecovered log items so that old kernels won't try to recover them and
> barf. Since a clean log has no protected log items, we could clear the
> bits at cover/quiesce time.
>
> As Dave Chinner pointed out in the thread, clearing log incompat bits at
> unmount time has positive effects for golden root disk image generator
> setups, since the generator could be running a newer kernel than what
> gets written to the golden image -- if there are log incompat fields set
> in the golden image that was generated by a newer kernel/OS image
> builder then the provisioning host cannot mount the filesystem even
> though the log is clean and recovery is unnecessary to mount the
> filesystem.
>
> Given that it's expensive to set log incompat bits, we really only want
> to do that once per bit per mount. Therefore, I propose that we only
> clear log incompat bits as part of writing a clean unmount record. Do
> this by adding an operational state flag to the xfs mount that guards
> whether or not the feature bit clearing can actually take place.
>
> This eliminates the l_incompat_users rwsem that we use to protect a log
> cleaning operation from clearing a feature bit that a frontend thread is
> trying to set -- this lock adds another way to fail w.r.t. locking. For
> the swapext series, I shard that into multiple locks just to work around
> the lockdep complaints, and that's fugly.
>
> Link: https://lore.kernel.org/linux-xfs/20240131230043.GA6180@frogsfrogsfrogs/
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> .../filesystems/xfs/xfs-online-fsck-design.rst | 3 -
> fs/xfs/xfs_log.c | 28 -------------
> fs/xfs/xfs_log.h | 2 -
> fs/xfs/xfs_log_priv.h | 3 -
> fs/xfs/xfs_log_recover.c | 15 -------
> fs/xfs/xfs_mount.c | 8 +++-
> fs/xfs/xfs_mount.h | 6 ++-
> fs/xfs/xfs_xattr.c | 42 +++-----------------
> 8 files changed, 19 insertions(+), 88 deletions(-)

Looks fine.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com