On Thu, Jan 14, 2021 at 04:40:57AM -0500, Brian Foster wrote:
> On Wed, Jan 13, 2021 at 05:08:35PM -0800, Darrick J. Wong wrote:
> > On Wed, Jan 13, 2021 at 01:35:05PM -0500, Brian Foster wrote:
> > > On Fri, Jan 08, 2021 at 10:28:25PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> > > >
> > > > Enable users to upgrade their filesystems to support inode btree block
> > > > counters.
> > > >
> > > > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > > > ---
> > >
> > > These two look Ok to me, but I noticed that both commands have weird
> > > error semantics when run through xfs_admin and the associated features
> > > are already set. E.g.:
> > >
> > > # xfs_admin -O inobtcount /dev/test/tmp
> > > Upgrading V5 filesystem
> > > Upgraded V5 filesystem. Please run xfs_repair.
> > > versionnum [0xb4a5+0x18a] = V5,NLINK,DIRV2,ALIGN,LOGV2,EXTFLG,MOREBITS,ATTR2,LAZYSBCOUNT,PROJID32BIT,CRC,FTYPE,FINOBT,SPARSE_INODES,REFLINK,INOBTCNT
> > > Running xfs_repair to ensure filesystem consistency.
> > > # xfs_admin -O inobtcount /dev/test/tmp
> > > inode btree counter feature is already enabled
> > > Running xfs_repair to ensure filesystem consistency.
> > > Conversion failed, is the filesystem unmounted?
> > > # mount /dev/test/tmp /mnt/
> > > # umount /mnt/
> > >
> > > So it looks like we run repair again the second time around even though
> > > the bit was already set, which is probably unnecessary, but then also
> > > for some reason report the result as failed.
> >
> > Hm. I guess I could define a second exitcode that means "no action
> > taken", and have xfs_admin exit.
> >
>
> It's not totally clear to me what the expected flow is supposed to be if
> a particular feature upgrade fails or is otherwise interrupted partway
> through.
Me neither. But let me try to work through some outcomes:
Upgrade succeeds -- xfs_db returns 0, needsrepair is set, repair runs
Cannot upgrade -- xfs_db returns 2, fs untouched
Feature already set -- xfs_db returns 2, fs untouched
FS corrupt -- xfs_db returns 1, fs untouched, repair runs
primary super write fails -- xfs_db returns 1, fs untouched, repair runs
secondary sb write fails -- xfs_db returns 1, primary super has
needsrepair set, repair runs
Does that seem thorough enough? That's, uh, what the code in my dev
tree does now. Will send out a v++ in a bit.
> If the expectation is for the user to rerun the xfs_admin
> command, it might be worth making sure that we somehow or another fall
> back through to repair when appropriate (whether it be unconditionally,
> by checking if NEEDSREPAIR is still set, etc.)..
My expectation is that if xfs_db fails, the admin should run xfs_repair
to fix anything that's wrong with the fs. Maybe they can skip that if
the primary super write fails with EIO, but that's up to the sysadmin's
judgment.
--D
> Brian
>
> > > (I also realized that repair is fixing up some agi metadata in this
> > > case, so my previous thought around a special repair verify mode is
> > > probably not relevant..).
> >
> > <nod>
> >
> > --D
> >
> > > Brian
> > >
> > > > db/sb.c | 22 ++++++++++++++++++++++
> > > > man/man8/xfs_admin.8 | 7 +++++++
> > > > man/man8/xfs_db.8 | 3 +++
> > > > 3 files changed, 32 insertions(+)
> > > >
> > > >
> > > > diff --git a/db/sb.c b/db/sb.c
> > > > index 93e4c405..b89ccdbe 100644
> > > > --- a/db/sb.c
> > > > +++ b/db/sb.c
> > > > @@ -597,6 +597,7 @@ version_help(void)
> > > > " 'version attr2' - enable v2 inline extended attributes\n"
> > > > " 'version log2' - enable v2 log format\n"
> > > > " 'version needsrepair' - flag filesystem as requiring repair\n"
> > > > +" 'version inobtcount' - enable inode btree counters\n"
> > > > "\n"
> > > > "The version function prints currently enabled features for a filesystem\n"
> > > > "according to the version field of its primary superblock.\n"
> > > > @@ -857,6 +858,27 @@ version_f(
> > > > }
> > > >
> > > > v5features.incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> > > > + } else if (!strcasecmp(argv[1], "inobtcount")) {
> > > > + if (xfs_sb_version_hasinobtcounts(&mp->m_sb)) {
> > > > + dbprintf(
> > > > + _("inode btree counter feature is already enabled\n"));
> > > > + exitcode = 1;
> > > > + return 1;
> > > > + }
> > > > + if (!xfs_sb_version_hasfinobt(&mp->m_sb)) {
> > > > + dbprintf(
> > > > + _("inode btree counter feature cannot be enabled on filesystems lacking free inode btrees\n"));
> > > > + exitcode = 1;
> > > > + return 1;
> > > > + }
> > > > + if (!xfs_sb_version_hascrc(&mp->m_sb)) {
> > > > + dbprintf(
> > > > + _("inode btree counter feature cannot be enabled on pre-V5 filesystems\n"));
> > > > + exitcode = 1;
> > > > + return 1;
> > > > + }
> > > > +
> > > > + v5features.ro_compat |= XFS_SB_FEAT_RO_COMPAT_INOBTCNT;
> > > > } else if (!strcasecmp(argv[1], "extflg")) {
> > > > switch (XFS_SB_VERSION_NUM(&mp->m_sb)) {
> > > > case XFS_SB_VERSION_1:
> > > > diff --git a/man/man8/xfs_admin.8 b/man/man8/xfs_admin.8
> > > > index b423981d..a776b375 100644
> > > > --- a/man/man8/xfs_admin.8
> > > > +++ b/man/man8/xfs_admin.8
> > > > @@ -116,6 +116,13 @@ If this is a V5 filesystem, flag the filesystem as needing repairs.
> > > > Until
> > > > .BR xfs_repair (8)
> > > > is run, the filesystem will not be mountable.
> > > > +.TP
> > > > +.B inobtcount
> > > > +Upgrade a V5 filesystem to support the inode btree counters feature.
> > > > +This reduces mount time by caching the size of the inode btrees in the
> > > > +allocation group metadata.
> > > > +Once enabled, the filesystem will not be writable by older kernels.
> > > > +The filesystem cannot be downgraded after this feature is enabled.
> > > > .RE
> > > > .TP
> > > > .BI \-U " uuid"
> > > > diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
> > > > index 7331cf19..1b826e5d 100644
> > > > --- a/man/man8/xfs_db.8
> > > > +++ b/man/man8/xfs_db.8
> > > > @@ -976,6 +976,9 @@ The filesystem can be flagged as requiring a run through
> > > > if the
> > > > .B needsrepair
> > > > option is specified and the filesystem is formatted with the V5 format.
> > > > +Support for the inode btree counters feature can be enabled by using the
> > > > +.B inobtcount
> > > > +option if the filesystem is formatted with the V5 format.
> > > > .IP
> > > > If no argument is given, the current version and feature bits are printed.
> > > > With one argument, this command will write the updated version number
> > > >
> > >
> >
>
