Re: [PATCH 1/2] xfs_db: add inobtcnt upgrade path

From: "Darrick J. Wong" <djwong@kernel.org>
To: Brian Foster <bfoster@redhat.com>
Cc: sandeen@sandeen.net, darrick.wong@oracle.com, linux-xfs@vger.kernel.org
Subject: Re: [PATCH 1/2] xfs_db: add inobtcnt upgrade path
Date: Fri, 15 Jan 2021 12:40:22 -0800	[thread overview]
Message-ID: <20210115204022.GB3134581@magnolia> (raw)
In-Reply-To: <20210114094057.GC1333929@bfoster>

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@kernel.org>
> > > > 
> > > > Enable users to upgrade their filesystems to support inode btree block
> > > > counters.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > 
> > > 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
> > > > 
> > > 
> > 
> 