Re: [PATCH 1/5] xfs_repair: check filesystem geometry before allowing upgrades

From: "Darrick J. Wong" <djwong@kernel.org>
To: Eric Sandeen <sandeen@sandeen.net>
Cc: Chandan Babu R <chandan.babu@oracle.com>,
	linux-xfs@vger.kernel.org, david@fromorbit.com
Subject: Re: [PATCH 1/5] xfs_repair: check filesystem geometry before allowing upgrades
Date: Wed, 22 Jun 2022 15:12:31 -0700	[thread overview]
Message-ID: <YrOTz236fFQqx59l@magnolia> (raw)
In-Reply-To: <66855f31-32a0-7ccf-4cc2-ab56e39fe4f2@sandeen.net>

On Wed, Jun 22, 2022 at 01:57:52PM -0500, Eric Sandeen wrote:
> On 5/25/22 12:36 AM, Chandan Babu R wrote:
> > From: "Darrick J. Wong" <djwong@kernel.org>
> > 
> > Currently, the two V5 feature upgrades permitted by xfs_repair do not
> > affect filesystem space usage, so we haven't needed to verify the
> > geometry.
> > 
> > However, this will change once we start to allow the sysadmin to add new
> > metadata indexes to existing filesystems.  Add all the infrastructure we
> > need to ensure that the log will still be large enough, that there's
> > enough space for metadata space reservations, and the root inode will
> > still be where we expect it to be after the upgrade.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> > [Recompute transaction reservation values; Exit with error if upgrade fails]
> 
> This is a lot to digest; I'd like to go ahead and merge 3 patches out of
> this 5 patch series and leave this + the upgrade patch until I get a chance
> to digest it a bit more.
> 
> One thing at least, though:
> 
> 
> > +	/*
> > +	 * Would we have at least 10% free space in the data device after all
> > +	 * the upgrades?
> > +	 */
> > +	if (mp->m_sb.sb_fdblocks < mp->m_sb.sb_dblocks / 10)
> > +		printf(_("Filesystem will be low on space after upgrade.\n"));
> > +
> 
> This should be removed, IMHO; what is the point? The user can't do anything about
> it, it proceeds anyway, and for all we know they started with less than 10% free.
> So why bother printing something that might generate questions and support
> calls? I don't think it's useful or actionable information.

Would you rather this exit(1)'d afterwards?  i.e. refuse the upgrade if
the fs doesn't have enough free space?

--D

> Thanks,
> -Eric