Re: [PATCH 08/12] xfs: remove xfs_ifork_ops

[Brian Foster <bfoster@redhat.com>]

On Thu, May 07, 2020 at 09:43:55AM -0400, Brian Foster wrote:
> On Thu, May 07, 2020 at 02:34:11PM +0200, Christoph Hellwig wrote:
> > On Fri, May 01, 2020 at 02:23:16PM -0400, Brian Foster wrote:
> > > Can we use another dummy parent inode value in xfs_repair? It looks to
> > > me that we set it to zero in phase 4 if it fails verification and set
> > > the parent to NULLFSINO (i.e. unknown) in repair's in-core tracking.
> > > Phase 6 walks the directory entries and explicitly sets the parent inode
> > > number of entries with an unknown parent (according to the in-core
> > > tracking). IOW, I don't see where we actually rely on the directory
> > > header having a parent inode of zero outside of detecting it in the
> > > custom verifier. If that's the only functional purpose, I wonder if we
> > > could do something like set the bogus parent field of a sf dir to the
> > > root inode or to itself, that way the default verifier wouldn't trip
> > > over it..
> > 
> > I don't think we need a dummy parent at all - we can just skip the
> > parent validation entirely, which is what my incremental patch does.
> > 
> 
> xfs_repair already skips the parent validation, this patch just
> refactors it. What I was considering above is whether repair uses the
> current dummy value of zero for any functional reason. If not, it kind
> of looks like the earlier phase of repair checks the parent, sees that
> it would fail a verifier, replaces it with zero (which would also fail
> the verifier) and then eventually replaces zero with a valid parent or
> ditches the entry in phase 6. If we placed a temporary parent value in
> the early phase that wouldn't explicitly fail a verifier by being an
> invalid inode number (instead of using 0 to notify the verifier to skip
> the validation), then we wouldn't need to skip the parent validation in
> phase 6 when we look up the inode again.
> 
...

To demonstrate, I hacked on repair a bit using an fs with an
intentionally corrupted shortform parent inode and had to make the
following tweaks to work around the custom fork verifier. The
ino_discovery checks were added because phases 3 and 4 toggle that flag
such that the former clears the parent value in the inode, but the
latter actually updates the external parent tracking. IOW, setting a
"valid" inode in phase 3 would otherwise trick phase 4 into using it.
I'd probably try to think of something cleaner for that issue if we were
to take such an approach.

Brian

diff --git a/repair/dir2.c b/repair/dir2.c
index cbbce601..c30ccb37 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -165,7 +165,7 @@ process_sf_dir2(
 	int			tmp_elen;
 	int			tmp_len;
 	xfs_dir2_sf_entry_t	*tmp_sfep;
-	xfs_ino_t		zero = 0;
+	xfs_ino_t		zero = mp->m_sb.sb_rootino;
 
 	sfp = (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip);
 	max_size = XFS_DFORK_DSIZE(dip, mp);
@@ -494,7 +494,8 @@ _("bogus .. inode number (%" PRIu64 ") in directory inode %" PRIu64 ", "),
 		if (!no_modify)  {
 			do_warn(_("clearing inode number\n"));
 
-			libxfs_dir2_sf_put_parent_ino(sfp, zero);
+			if (!ino_discovery)
+				libxfs_dir2_sf_put_parent_ino(sfp, zero);
 			*dino_dirty = 1;
 			*repair = 1;
 		} else  {
@@ -528,8 +529,8 @@ _("bad .. entry in directory inode %" PRIu64 ", points to self, "),
 			ino);
 		if (!no_modify)  {
 			do_warn(_("clearing inode number\n"));
-
-			libxfs_dir2_sf_put_parent_ino(sfp, zero);
+			if (!ino_discovery)
+				libxfs_dir2_sf_put_parent_ino(sfp, zero);
 			*dino_dirty = 1;
 			*repair = 1;
 		} else  {
diff --git a/repair/phase6.c b/repair/phase6.c
index beceea9a..613ca578 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1104,7 +1104,7 @@ mv_orphanage(
 					(unsigned long long)ino, ++incr);
 
 	/* Orphans may not have a proper parent, so use custom ops here */
-	err = -libxfs_iget(mp, NULL, ino, 0, &ino_p, &phase6_ifork_ops);
+	err = -libxfs_iget(mp, NULL, ino, 0, &ino_p, &xfs_default_ifork_ops);
 	if (err)
 		do_error(_("%d - couldn't iget disconnected inode\n"), err);
 
@@ -2875,7 +2875,7 @@ process_dir_inode(
 
 	ASSERT(!is_inode_refchecked(irec, ino_offset) || dotdot_update);
 
-	error = -libxfs_iget(mp, NULL, ino, 0, &ip, &phase6_ifork_ops);
+	error = -libxfs_iget(mp, NULL, ino, 0, &ip, &xfs_default_ifork_ops);
 	if (error) {
 		if (!no_modify)
 			do_error(