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

[Brian Foster <bfoster@redhat.com>]

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..

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,
> > > 
> > 
>