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