Re: [PATCH 5/9] xfs_db: add inobtcnt upgrade path

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Brian Foster <bfoster@redhat.com>
Cc: sandeen@sandeen.net, linux-xfs@vger.kernel.org
Subject: Re: [PATCH 5/9] xfs_db: add inobtcnt upgrade path
Date: Thu, 29 Oct 2020 08:42:50 -0700	[thread overview]
Message-ID: <20201029154250.GL1061252@magnolia> (raw)
In-Reply-To: <20201029120905.GA1657878@bfoster>

On Thu, Oct 29, 2020 at 08:09:05AM -0400, Brian Foster wrote:
> On Wed, Oct 28, 2020 at 05:03:32PM -0700, Darrick J. Wong wrote:
> > On Wed, Oct 28, 2020 at 01:29:25PM -0400, Brian Foster wrote:
> > > On Mon, Oct 26, 2020 at 04:33:38PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > Enable users to upgrade their filesystems to support inode btree block
> > > > counters.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> > > >  db/sb.c              |   76 +++++++++++++++++++++++++++++++++++++++++++++++++-
> > > >  db/xfs_admin.sh      |    4 ++-
> > > >  man/man8/xfs_admin.8 |   16 +++++++++++
> > > >  3 files changed, 94 insertions(+), 2 deletions(-)
> > > > 
> > > > 
> ...
> > > > diff --git a/man/man8/xfs_admin.8 b/man/man8/xfs_admin.8
> > > > index 8afc873fb50a..65ca6afc1e12 100644
> > > > --- a/man/man8/xfs_admin.8
> > > > +++ b/man/man8/xfs_admin.8
> ...
> > > > @@ -103,6 +105,20 @@ The filesystem label can be cleared using the special "\c
> > > >  " value for
> > > >  .IR label .
> > > >  .TP
> > > > +.BI \-O " feature"
> > > > +Add a new feature to the filesystem.
> > > > +Only one feature can be specified at a time.
> > > > +Features are as follows:
> > > > +.RS 0.7i
> > > > +.TP
> > > > +.B inobtcount
> > > > +Upgrade the 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.
> > > 
> > > Any reason for not allowing the downgrade path? It seems like we're
> > > mostly there implementation wise and that might facilitate enabling the
> > > feature by default.
> > 
> > Downgrading will not be easy for bigtime, since we'd have to scan the
> > whole fs to see if there are any timestamps past 2038.  For other
> > features it might not be such a big deal, but in general I don't want to
> > increase our testing burden even further.
> > 
> 
> Well it's not something I'd say we should necessarily support for every
> feature. TBH, I wasn't expecting an upgrade mechanism for inobtcount in
> the first place since I thought we didn't tend to do that for v5
> filesystems.

We've not been especially consistent with that -- I think you can
"upgrade" to meta_uuid; at one point Christoph had a series to make it
possible to upgrade to reflink; but all the other features don't support
upgrades.

I picked inobtcount for the initial v5 upgrade support because it seemed
like a small enough feature that people could theoretically backport the
feature to old kernels.  That was supposed to go in ahead of y2038
support (which is my real motivation for allowing some upgrades) but
then they both landed at the same time. ;)

> ISTM it's simple enough to support and perhaps that's good
> enough reason to do so, but if we're going to move the "test burden"
> line anyways it seems rather arbitrary to me to support one direction
> and not the other when they are presumably of comparable complexity.
> Just my .02 of course, and I don't feel strongly about whether we
> support upgrade, downgrade, or neither...

I think the only features that we've supported downgrading are attr2 ->
attr1 and (in the old days) unwritten extents.

--D

> Brian
> 
> > I'll ask the ext4 folks if they know of people downgrading filesystems
> > with tune2fs, but AFAIK it's not generally done.
> > 
> > --D
> > 
> > > Brian
> > > 
> > > > +.RE
> > > > +.TP
> > > >  .BI \-U " uuid"
> > > >  Set the UUID of the filesystem to
> > > >  .IR uuid .
> > > > 
> > > 
> > 
> 