Re: xfs_clear_incompat_log_features considered harmful?

[Christoph Hellwig <hch@infradead.org>]

On Wed, Jan 31, 2024 at 03:00:43PM -0800, Darrick J. Wong wrote:
> Hi everyone,
> 
> Christoph spied the xfs_swapext_can_use_without_log_assistance
> function[0] in the atomic file updates patchset[1] 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 xfs_attri_can_use_without_log_assistance
> from the since-merged LARP series.)

xfs_attri_can_use_without_log_assistance actually is new in your
patch stack.

Not that my biggest issue is that this check actually is broken.
The point of the compat,incompat,log_incompat features is that they
move away from a linear version model where a new version implies all
previous version to one where we have a bit designating exactly what
is supported.  Optimizing away the need to set a bit just because other
bits are sit brings back this linear versining in a sneaky, undocumented
and dangerous way.

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

Yes.

> Given that this set/clear dance imposes continuous runtime costs on all
> the users, I want to remove xfs_clear_incompat_log_features.  Log
> incompat bits get set once, and they never go away.  This eliminates the
> need for the rwsem, all the extra incompat-clearing bits in the log
> code, and fixes the performance problems I see.

I mostly agree.  I don't think we have to strictly say they don't
go away, but makign them go away should be explicit and we should
only do that if someone has a clear use case for it.

> Going forward, I'd make mkfs set the log incompat features during a
> fresh format if any of the currently-undefined feature bits are set,
> which means that they'll be enabled by default on any filesystem with
> directory parent pointers and/or metadata directories.  I'd also add
> mkfs -l options so that sysadmins can turn it on at format time.

Yes.

> We can discuss whether we want to allow people to set the log incompat
> features at runtime -- allowing it at least for recent filesystems (e.g.
> v5 + rmap) is easier for users, but only if we decide that we don't
> really care about the "recover with old Knoppix" surprise.  If we decide
> against online upgrades, we /could/ at least allow upgrades via
> xfs_admin like we have for bigtime/inobtcnt.  Or we could decide that
> new functionality requires a reformat.

I think a runtime add (for recent enough file systems) would be really
useful.  But it should be an explicit opt-in and not a silent upgrade,
which avoids any surprices with reocvery tools.

> Thoughts?  I vote for removing xfs_clear_incompat_log_features and
> letting people turn on log incompat features at runtime.

Please do!