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!
