Re: [PATCH 6/6] xfs_repair: check plausibility of root dir pointer before trashing it\

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Dec 13, 2019 at 06:19:08AM -0500, Brian Foster wrote:
> On Thu, Dec 12, 2019 at 02:46:18PM -0800, Darrick J. Wong wrote:
> > On Thu, Dec 05, 2019 at 09:38:58AM -0500, Brian Foster wrote:
> > > On Wed, Dec 04, 2019 at 09:05:02AM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > If sb_rootino doesn't point to where we think mkfs should have allocated
> > > > the root directory, check to see if the alleged root directory actually
> > > > looks like a root directory.  If so, we'll let it live because someone
> > > > could have changed sunit since formatting time, and that changes the
> > > > root directory inode estimate.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> > > >  repair/xfs_repair.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 45 insertions(+)
> > > > 
> > > > 
> > > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > > > index abd568c9..b0407f4b 100644
> > > > --- a/repair/xfs_repair.c
> > > > +++ b/repair/xfs_repair.c
> > > > @@ -426,6 +426,37 @@ _("would reset superblock %s inode pointer to %"PRIu64"\n"),
> > > >  	*ino = expected_ino;
> > > >  }
> > > >  
> > > > +/* Does the root directory inode look like a plausible root directory? */
> > > > +static bool
> > > > +has_plausible_rootdir(
> > > > +	struct xfs_mount	*mp)
> > > > +{
> > > > +	struct xfs_inode	*ip;
> > > > +	xfs_ino_t		ino;
> > > > +	int			error;
> > > > +	bool			ret = false;
> > > > +
> > > > +	error = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &ip,
> > > > +			&xfs_default_ifork_ops);
> > > > +	if (error)
> > > > +		goto out;
> > > > +	if (!S_ISDIR(VFS_I(ip)->i_mode))
> > > > +		goto out_rele;
> > > > +
> > > > +	error = -libxfs_dir_lookup(NULL, ip, &xfs_name_dotdot, &ino, NULL);
> > > > +	if (error)
> > > > +		goto out_rele;
> > > > +
> > > > +	/* The root directory '..' entry points to the directory. */
> > > > +	if (ino == mp->m_sb.sb_rootino)
> > > > +		ret = true;
> > > > +
> > > > +out_rele:
> > > > +	libxfs_irele(ip);
> > > > +out:
> > > > +	return ret;
> > > > +}
> > > > +
> > > >  /*
> > > >   * Make sure that the first 3 inodes in the filesystem are the root directory,
> > > >   * the realtime bitmap, and the realtime summary, in that order.
> > > > @@ -436,6 +467,20 @@ calc_mkfs(
> > > >  {
> > > >  	xfs_ino_t		rootino = libxfs_ialloc_calc_rootino(mp, -1);
> > > >  
> > > > +	/*
> > > > +	 * If the root inode isn't where we think it is, check its plausibility
> > > > +	 * as a root directory.  It's possible that somebody changed sunit
> > > > +	 * since the filesystem was created, which can change the value of the
> > > > +	 * above computation.  Don't blow up the root directory if this is the
> > > > +	 * case.
> > > > +	 */
> > > > +	if (mp->m_sb.sb_rootino != rootino && has_plausible_rootdir(mp)) {
> > > > +		do_warn(
> > > > +_("sb root inode value %" PRIu64 " inconsistent with alignment (expected %"PRIu64")\n"),
> > > > +			mp->m_sb.sb_rootino, rootino);
> > > > +		rootino = mp->m_sb.sb_rootino;
> > > > +	}
> > > > +
> > > 
> > > A slightly unfortunate side effect of this is that there's seemingly no
> > > straightforward way for a user to "clear" this state/warning. We've
> > > solved the major problem by allowing repair to handle this condition,
> > > but AFAICT this warning will persist unless the stripe unit is changed
> > > back to its original value.
> > 
> > Heh, I apparently never replied to this. :(
> > 
> > > IOW, what if this problem exists simply because a user made a mistake
> > > and wants to undo it? It's probably easy enough for us to say "use
> > > whatever you did at mkfs time," but what if that's unknown or was set
> > > automatically? I feel like that is the type of thing that in practice
> > > could result in unnecessary bugs or error reports unless the tool can
> > > make a better suggestion to the end user. For example, could we check
> > > the geometry on secondary supers (if they exist) against the current
> > > rootino and use that as a secondary form of verification and/or suggest
> > > the user reset to that geometry (if desired)?
> > 
> > That sounds reasonable.
> > 
> > > OTOH, I guess we'd have to consider what happens if the filesystem was
> > > grown in that scenario too..  :/
> > 
> > I think it would be fine, so long as we're careful with the if-then
> > chain.  Specifically:
> > 
> > a. If we dislike the rootino that we compute with the ondisk sunit value,
> > and...
> > 
> > b. The thing sb_rootino points to actually does look like the root
> > directory, and...
> > 
> > c. One of the secondary supers has an sunit value that gives us a
> > rootino calculation that matches the sb_rootino that we just checked
> > out...
> > 
> > ...then we'll propose correcting the primary sb_unit to the value we
> > found in (c).
> > 
> 
> Yeah, that makes sense. My broader concern was addressing the situation
> where we aren't lucky enough to glean original alignment from the fs.
> Perhaps we could 1.) update the warning message to unconditionally
> recommend an alignment and 2.) if nothing is gleaned from secondary
> supers (and all your above conditions apply), calculate and recommend
> the max alignment that accommodates the root inode chunk..? It might not
> be the original value, but at least guides the user to a solution to
> quiet the warning..

Hmm, I suppose if the secondary sb scan didn't produce any usable values
then we could just try increasing powers of two until the computed
rootino value >= sb_rootino in the hopes of finding one.

I'm not sure how I feel about repair guessing values until it finds one
that shuts off the warning light, though.  Is doing so foolishness, or
is it AI? :)

--D

> Brian
> 
> > > 
> > > (Actually on a quick test, it looks like growfs updates every super,
> > > even preexisting..).
> > 
> > I'll throw that onto the V3 series.
> > 
> > --D
> > 
> > > 
> > > Brian
> > > 
> > > >  	ensure_fixed_ino(&mp->m_sb.sb_rootino, rootino,
> > > >  			_("root"));
> > > >  	ensure_fixed_ino(&mp->m_sb.sb_rbmino, rootino + 1,
> > > > 
> > > 
> > 
>