On Wed, Oct 16, 2024 at 02:48:23PM +1100, Dave Chinner wrote:
> On Tue, Oct 15, 2024 at 05:20:51PM -0700, Darrick J. Wong wrote:
> > On Wed, Oct 16, 2024 at 10:03:24AM +1100, Dave Chinner wrote:
> > > On Tue, Oct 15, 2024 at 11:25:41AM -0700, Darrick J. Wong wrote:
> > > > > > + if (xfs_has_metadir(mp))
> > > > > > + xfs_warn(mp,
> > > > > > +"EXPERIMENTAL metadata directory feature in use. Use at your own risk!");
> > > > > > +
> > > > >
> > > > > We really need a 'xfs_mark_experimental(mp, "Metadata directory")'
> > > > > function to format all these experimental feature warnings the same
> > > > > way....
> > > >
> > > > We already have xfs_warn_mount for functionality that isn't sb feature
> > > > bits. Maybe xfs_warn_feat?
> > >
> > > xfs_warn_mount() is only used for experimental warnings, so maybe we
> > > should simply rename that xfs_mark_experiental(). Then we can use
> > > it's inherent "warn once" behaviour for all the places where we
> > > issue an experimental warning regardless of how the experimental
> > > feature is enabled/detected.
> > >
> > > This means we'd have a single location that formats all experimental
> > > feature warnings the same way. Having a single function explicitly
> > > for this makes it trivial to audit and manage all the experimental
> > > features supported by a given kernel version because we are no
> > > longer reliant on grepping for custom format strings to find
> > > experimental features.
> > >
> > > It also means that adding a kernel taint flag indicating that the
> > > kernel is running experimental code is trivial to do...
> >
> > ...and I guess this means you can discover which forbidden features are
> > turned on from crash dumps. Ok, sounds good to me.
>
> Yes, though I don't consider experimental features as "forbidden".
>
> This is more about enabling experimental filesystem features to be
> shipped under tech preview constraints(*). Knowing that an
> experimental feature is in use will help manage support expectations
> and workload. This, in turn, will also allow us to stay closer to
> the upstream XFS code base and behaviour....
<nod>
> > Do you want it to return an int so that you (as a distributor, not you
> > personally) can decide that nobody gets to use the experimental
> > features?
>
> I considered suggesting that earlier, but if we want to disable
> specific experimental features we'll have to patch the kernel
> anyway. Hence I don't think there's any reason for having the
> upstream code doing anything other than tracking what experimental
> features are in use.
Here's what I have now:
enum xfs_experimental_feat {
XFS_EXPERIMENTAL_SCRUB,
XFS_EXPERIMENTAL_SHRINK,
XFS_EXPERIMENTAL_LARP,
XFS_EXPERIMENTAL_LBS,
XFS_EXPERIMENTAL_EXCHRANGE,
XFS_EXPERIMENTAL_PPTR,
XFS_EXPERIMENTAL_METADIR,
XFS_EXPERIMENTAL_MAX,
};
void
xfs_warn_experimental(
struct xfs_mount *mp,
enum xfs_experimental_feat feat)
{
static const struct {
const char *name;
long opstate;
} features[] = {
[XFS_EXPERIMENTAL_SCRUB] = {
.opstate = XFS_OPSTATE_WARNED_SCRUB,
.name = "online scrub",
},
[XFS_EXPERIMENTAL_SHRINK] = {
.opstate = XFS_OPSTATE_WARNED_SHRINK,
.name = "online shrink",
},
[XFS_EXPERIMENTAL_LARP] = {
.opstate = XFS_OPSTATE_WARNED_LARP,
.name = "logged extended attributes",
},
[XFS_EXPERIMENTAL_LBS] = {
.opstate = XFS_OPSTATE_WARNED_LBS,
.name = "large block size",
},
[XFS_EXPERIMENTAL_EXCHRANGE] = {
.opstate = XFS_OPSTATE_WARNED_EXCHRANGE,
.name = "exchange range",
},
[XFS_EXPERIMENTAL_PPTR] = {
.opstate = XFS_OPSTATE_WARNED_PPTR,
.name = "parent pointer",
},
[XFS_EXPERIMENTAL_METADIR] = {
.opstate = XFS_OPSTATE_WARNED_METADIR,
.name = "metadata directory tree",
},
};
ASSERT(feat >= 0 && feat < XFS_EXPERIMENTAL_MAX);
BUILD_BUG_ON(ARRAY_SIZE(features) != XFS_EXPERIMENTAL_MAX);
if (xfs_should_warn(mp, features[feat].opstate))
xfs_warn(mp,
"EXPERIMENTAL %s feature enabled. Use at your own risk!",
features[feat].name);
}
and the callsite:
if (xfs_has_parent(mp))
xfs_warn_experimental(mp, XFS_EXPERIMENTAL_PPTR);
> -Dave.
>
> (*) https://access.redhat.com/solutions/21101
>
> "Technology Preview features are not fully supported, may not be
> functionally complete, and are not suitable for deployment in
> production. However, these features are provided to the customer as
> a courtesy and the primary goal is for the feature to gain wider
> exposure with the goal of full support in the future."
Similar here, though we usually massage the dmesg text to mention that
only certain customers are supported.
--D
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
