[PATCH 0/4] xfsprogs: remove custom dir2 sf fork verifier from repair

[PATCH 0/4] xfsprogs: remove custom dir2 sf fork verifier from repair

[Brian Foster]

Hi all,

These are some tweaks to eliminate the custom sf dir verifier code in
phase 6 of xfs_repair. Note that I don't quite recall how thoroughly I
reviewed the first couple patches, but Christoph was asking for this
recently for some libxfs sync work and I'm running out of time before I
disappear for a few weeks, so I wanted to get it on the list. Thoughts,
reviews, flames appreciated.

Brian

Brian Foster (4):
  repair: set the in-core inode parent in phase 3
  repair: don't double check dir2 sf parent in phase 4
  repair: use fs root ino for dummy parent value instead of zero
  repair: remove custom dir2 sf fork verifier from phase6

 repair/dino_chunks.c |  9 +------
 repair/dir2.c        |  8 ++++---
 repair/phase6.c      | 56 ++------------------------------------------
 3 files changed, 8 insertions(+), 65 deletions(-)

-- 
2.21.3

[PATCH 1/4] repair: set the in-core inode parent in phase 3

[Brian Foster]

The inode processing code checks and resets invalid parent values on
physical inodes in phase 3 but waits to update the parent value in
the in-core tracking until phase 4. There doesn't appear to be any
specific reason for the latter beyond caution. In reality, the only
reason this doesn't cause problems is that phase 3 replaces an
invalid on-disk parent with another invalid value, so the in-core
parent returned by phase 4 translates to NULLFSINO.

This is subtle and fragile. To eliminate this duplicate processing
behavior and break the subtle dependency of requiring an invalid
dummy value in physical directory inodes, update the in-core parent
tracking structure at the same point in phase 3 that physical inodes
are updated. Invalid on-disk parent values will still translate to
NULLFSINO for the in-core tracking to be identified by later phases.
This ensures that if a valid dummy value is placed in a physical
inode (such as rootino) with an invalid parent in phase 3, phase 4
won't mistakenly return the valid dummy value to be incorrectly set
in the in-core tracking over the NULLFSINO value that represents the
broken on-disk state.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 repair/dino_chunks.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
index 6685a4d2..96ed6a5b 100644
--- a/repair/dino_chunks.c
+++ b/repair/dino_chunks.c
@@ -859,14 +859,7 @@ next_readbuf:
 		 */
 		if (isa_dir)  {
 			set_inode_isadir(ino_rec, irec_offset);
-			/*
-			 * we always set the parent but
-			 * we may as well wait until
-			 * phase 4 (no inode discovery)
-			 * because the parent info will
-			 * be solid then.
-			 */
-			if (!ino_discovery)  {
+			if (ino_discovery)  {
 				ASSERT(parent != 0);
 				set_inode_parent(ino_rec, irec_offset, parent);
 				ASSERT(parent ==
-- 
2.21.3

[PATCH 2/4] repair: don't double check dir2 sf parent in phase 4

[Brian Foster]

The shortform parent ino verification code runs once in phase 3
(ino_discovery == true) and once in phase 4 (ino_discovery ==
false). This is unnecessary and leads to duplicate error messages if
repair replaces an invalid parent value with zero because zero is
still an invalid value. Skip the check in phase 4.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 repair/dir2.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/repair/dir2.c b/repair/dir2.c
index cbbce601..caf6963d 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -480,6 +480,9 @@ _("corrected entry offsets in directory %" PRIu64 "\n"),
 	 * check parent (..) entry
 	 */
 	*parent = libxfs_dir2_sf_get_parent_ino(sfp);
+	if (!ino_discovery)
+		return 0;
+
 
 	/*
 	 * if parent entry is bogus, null it out.  we'll fix it later .
-- 
2.21.3

[PATCH 3/4] repair: use fs root ino for dummy parent value instead of zero

[Brian Foster]

If a directory inode has an invalid parent ino on disk, repair
replaces the invalid value with a dummy value of zero in the buffer
and NULLFSINO in the in-core parent tracking. The zero value serves
no functional purpose as it is still an invalid value and the parent
must be repaired by phase 6 based on the in-core state before the
buffer can be written out.  Instead, use the root fs inode number as
a catch all for invalid parent values so phase 6 doesn't have to
create custom verifier infrastructure just to work around this
behavior.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 repair/dir2.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/repair/dir2.c b/repair/dir2.c
index caf6963d..9c789b4a 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -165,7 +165,6 @@ process_sf_dir2(
 	int			tmp_elen;
 	int			tmp_len;
 	xfs_dir2_sf_entry_t	*tmp_sfep;
-	xfs_ino_t		zero = 0;
 
 	sfp = (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip);
 	max_size = XFS_DFORK_DSIZE(dip, mp);
@@ -497,7 +496,7 @@ _("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);
+			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
 			*dino_dirty = 1;
 			*repair = 1;
 		} else  {
@@ -532,7 +531,7 @@ _("bad .. entry in directory inode %" PRIu64 ", points to self, "),
 		if (!no_modify)  {
 			do_warn(_("clearing inode number\n"));
 
-			libxfs_dir2_sf_put_parent_ino(sfp, zero);
+			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
 			*dino_dirty = 1;
 			*repair = 1;
 		} else  {
-- 
2.21.3

[PATCH 4/4] repair: remove custom dir2 sf fork verifier from phase6

[Brian Foster]

The custom verifier exists to catch the case of repair setting a
dummy parent value of zero on directory inodes and temporarily
replace it with a valid inode number so the rest of the directory
verification can proceed. The custom verifier is no longer needed
now that the rootino is used as a dummy value for invalid on-disk
parent values.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 repair/phase6.c | 56 ++-----------------------------------------------
 1 file changed, 2 insertions(+), 54 deletions(-)

diff --git a/repair/phase6.c b/repair/phase6.c
index beceea9a..43bcea50 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -26,58 +26,6 @@ static struct xfs_name		xfs_name_dot = {(unsigned char *)".",
 						1,
 						XFS_DIR3_FT_DIR};
 
-/*
- * When we're checking directory inodes, we're allowed to set a directory's
- * dotdot entry to zero to signal that the parent needs to be reconnected
- * during phase 6.  If we're handling a shortform directory the ifork
- * verifiers will fail, so temporarily patch out this canary so that we can
- * verify the rest of the fork and move on to fixing the dir.
- */
-static xfs_failaddr_t
-phase6_verify_dir(
-	struct xfs_inode		*ip)
-{
-	struct xfs_mount		*mp = ip->i_mount;
-	struct xfs_ifork		*ifp;
-	struct xfs_dir2_sf_hdr		*sfp;
-	xfs_failaddr_t			fa;
-	xfs_ino_t			old_parent;
-	bool				parent_bypass = false;
-	int				size;
-
-	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
-	sfp = (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data;
-	size = ifp->if_bytes;
-
-	/*
-	 * If this is a shortform directory, phase4 may have set the parent
-	 * inode to zero to indicate that it must be fixed.  Temporarily
-	 * set a valid parent so that the directory verifier will pass.
-	 */
-	if (size > offsetof(struct xfs_dir2_sf_hdr, parent) &&
-	    size >= xfs_dir2_sf_hdr_size(sfp->i8count)) {
-		old_parent = libxfs_dir2_sf_get_parent_ino(sfp);
-		if (old_parent == 0) {
-			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
-			parent_bypass = true;
-		}
-	}
-
-	fa = libxfs_default_ifork_ops.verify_dir(ip);
-
-	/* Put it back. */
-	if (parent_bypass)
-		libxfs_dir2_sf_put_parent_ino(sfp, old_parent);
-
-	return fa;
-}
-
-static struct xfs_ifork_ops phase6_ifork_ops = {
-	.verify_attr	= xfs_attr_shortform_verify,
-	.verify_dir	= phase6_verify_dir,
-	.verify_symlink	= xfs_symlink_shortform_verify,
-};
-
 /*
  * Data structures used to keep track of directories where the ".."
  * entries are updated. These must be rebuilt after the initial pass
@@ -1104,7 +1052,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 +2823,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(
-- 
2.21.3

Re: [PATCH 1/4] repair: set the in-core inode parent in phase 3

[Christoph Hellwig]

On Wed, Jul 15, 2020 at 10:08:33AM -0400, Brian Foster wrote:
> The inode processing code checks and resets invalid parent values on
> physical inodes in phase 3 but waits to update the parent value in
> the in-core tracking until phase 4. There doesn't appear to be any
> specific reason for the latter beyond caution. In reality, the only
> reason this doesn't cause problems is that phase 3 replaces an
> invalid on-disk parent with another invalid value, so the in-core
> parent returned by phase 4 translates to NULLFSINO.
> 
> This is subtle and fragile. To eliminate this duplicate processing
> behavior and break the subtle dependency of requiring an invalid
> dummy value in physical directory inodes, update the in-core parent
> tracking structure at the same point in phase 3 that physical inodes
> are updated. Invalid on-disk parent values will still translate to
> NULLFSINO for the in-core tracking to be identified by later phases.
> This ensures that if a valid dummy value is placed in a physical
> inode (such as rootino) with an invalid parent in phase 3, phase 4
> won't mistakenly return the valid dummy value to be incorrectly set
> in the in-core tracking over the NULLFSINO value that represents the
> broken on-disk state.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/4] repair: don't double check dir2 sf parent in phase 4

[Christoph Hellwig]

On Wed, Jul 15, 2020 at 10:08:34AM -0400, Brian Foster wrote:
> The shortform parent ino verification code runs once in phase 3
> (ino_discovery == true) and once in phase 4 (ino_discovery ==
> false). This is unnecessary and leads to duplicate error messages if
> repair replaces an invalid parent value with zero because zero is
> still an invalid value. Skip the check in phase 4.

This looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

As far as the existing code is concerned:  Does anyone else find the
ino_discovery booleand passed as int as annoying as I do?  An
"enum repair_phase phase" would be much more descriptive in my opinion.

Re: [PATCH 3/4] repair: use fs root ino for dummy parent value instead of zero

[Christoph Hellwig]

On Wed, Jul 15, 2020 at 10:08:35AM -0400, Brian Foster wrote:
> If a directory inode has an invalid parent ino on disk, repair
> replaces the invalid value with a dummy value of zero in the buffer
> and NULLFSINO in the in-core parent tracking. The zero value serves
> no functional purpose as it is still an invalid value and the parent
> must be repaired by phase 6 based on the in-core state before the
> buffer can be written out.  Instead, use the root fs inode number as
> a catch all for invalid parent values so phase 6 doesn't have to
> create custom verifier infrastructure just to work around this
> behavior.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 4/4] repair: remove custom dir2 sf fork verifier from phase6

[Christoph Hellwig]

On Wed, Jul 15, 2020 at 10:08:36AM -0400, Brian Foster wrote:
> The custom verifier exists to catch the case of repair setting a
> dummy parent value of zero on directory inodes and temporarily
> replace it with a valid inode number so the rest of the directory
> verification can proceed. The custom verifier is no longer needed
> now that the rootino is used as a dummy value for invalid on-disk
> parent values.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/4] repair: use fs root ino for dummy parent value instead of zero

[Dave Chinner]

On Wed, Jul 15, 2020 at 10:08:35AM -0400, Brian Foster wrote:
> If a directory inode has an invalid parent ino on disk, repair
> replaces the invalid value with a dummy value of zero in the buffer
> and NULLFSINO in the in-core parent tracking. The zero value serves
> no functional purpose as it is still an invalid value and the parent
> must be repaired by phase 6 based on the in-core state before the
> buffer can be written out.  Instead, use the root fs inode number as
> a catch all for invalid parent values so phase 6 doesn't have to
> create custom verifier infrastructure just to work around this
> behavior.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Reasonale, but wouldn't it be better to use lost+found as the dummy
parent inode (i.e. the orphanage inode)? Because if the parent can't
be found and the inode reconnected correctly, we're going to put it
in lost+found, anyway?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/4] repair: don't double check dir2 sf parent in phase 4

[Darrick J. Wong]

On Wed, Jul 15, 2020 at 07:43:50PM +0100, Christoph Hellwig wrote:
> On Wed, Jul 15, 2020 at 10:08:34AM -0400, Brian Foster wrote:
> > The shortform parent ino verification code runs once in phase 3
> > (ino_discovery == true) and once in phase 4 (ino_discovery ==
> > false). This is unnecessary and leads to duplicate error messages if
> > repair replaces an invalid parent value with zero because zero is
> > still an invalid value. Skip the check in phase 4.
> 
> This looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> As far as the existing code is concerned:  Does anyone else find the
> ino_discovery booleand passed as int as annoying as I do?  An
> "enum repair_phase phase" would be much more descriptive in my opinion.

I can never remember what "ino_discovery" actually means.  true means
phase2 (looking at inodes for the first time) and false means phase4
(looking for crosslinked data and whatnot)?

--D

Re: [PATCH 2/4] repair: don't double check dir2 sf parent in phase 4

[Brian Foster]

On Wed, Jul 15, 2020 at 04:54:14PM -0700, Darrick J. Wong wrote:
> On Wed, Jul 15, 2020 at 07:43:50PM +0100, Christoph Hellwig wrote:
> > On Wed, Jul 15, 2020 at 10:08:34AM -0400, Brian Foster wrote:
> > > The shortform parent ino verification code runs once in phase 3
> > > (ino_discovery == true) and once in phase 4 (ino_discovery ==
> > > false). This is unnecessary and leads to duplicate error messages if
> > > repair replaces an invalid parent value with zero because zero is
> > > still an invalid value. Skip the check in phase 4.
> > 
> > This looks good,
> > 
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > 
> > As far as the existing code is concerned:  Does anyone else find the
> > ino_discovery booleand passed as int as annoying as I do?  An
> > "enum repair_phase phase" would be much more descriptive in my opinion.
> 
> I can never remember what "ino_discovery" actually means.  true means
> phase2 (looking at inodes for the first time) and false means phase4
> (looking for crosslinked data and whatnot)?
> 

Same.. I agree with Christoph on not just ino_discovery, but the various
boolean parameters to some of the common scanning functions that are
reused across multiple phases. It's confusing to track when reading the
code as well as to identify intent and whether certain hunks of
idempotent code are running multiple times, etc.

That said, I'm not necessarily convinced that replacing the booleans
with a phase enum is a huge benefit. That just changes the interface so
it's easier to determine what phase we're in vs. why certain bits of
logic are executed. I.e., 'if (ino_discovery) { do_discovery_stuff(); }'
confuses what phase we're in when reading the lower level code, but 'if
(phase == PHASE_3) { do_discovery_stuff() }' doesn't really clarify how
do_discovery_stuff() might interact with other behaviors that are part
of the same phase.

I'm not sure what the right answer is but if we're going to look at
refactoring things, I'd rather see us start with considering something
more fundamental. For example, could we perhaps factor out the the phase
specific functionality into phase specific functions via an approach
like Darrick's recent log recovery rework?  That way it might be easier
to read through each phase and understand the core repair logic vs.
having to troll through scanning infrastructure multiple times trying to
keep track of where you are in the grand scheme of things. Of course if
that is too much of a mess then perhaps the phase enum thing makes more
sense..

Brian

> --D
> 

Re: [PATCH 3/4] repair: use fs root ino for dummy parent value instead of zero

[Brian Foster]

On Thu, Jul 16, 2020 at 08:22:16AM +1000, Dave Chinner wrote:
> On Wed, Jul 15, 2020 at 10:08:35AM -0400, Brian Foster wrote:
> > If a directory inode has an invalid parent ino on disk, repair
> > replaces the invalid value with a dummy value of zero in the buffer
> > and NULLFSINO in the in-core parent tracking. The zero value serves
> > no functional purpose as it is still an invalid value and the parent
> > must be repaired by phase 6 based on the in-core state before the
> > buffer can be written out.  Instead, use the root fs inode number as
> > a catch all for invalid parent values so phase 6 doesn't have to
> > create custom verifier infrastructure just to work around this
> > behavior.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> 
> Reasonale, but wouldn't it be better to use lost+found as the dummy
> parent inode (i.e. the orphanage inode)? Because if the parent can't
> be found and the inode reconnected correctly, we're going to put it
> in lost+found, anyway?
> 

That was my first thought when I originally wrote this, but there's
several reasons I didn't end up doing that. The orphanage isn't created
until much later in repair and only if we end up with orphaned inodes.
We'd have to change that in order to use a dummy parent inode number
that corresponds to a valid orphanage, and TBH I'm not even sure if it's
always going to be safe to expect an inode allocation to work at this
point in repair.

Further, it's still too early to tell whether these directories are
orphaned because the directory scan in phase 6 can easily repair
missing/broken parent information. The scenarios I used to test this
functionality didn't involve the orphanage at all, so now we not only
need to change when/how the orphanage is created, but need to free it if
it ends up unused before we exit (which could be via any number of
do_error() calls before we ever get close to phase 6).

If you consider all of that with the facts that this is a dummy value
and so has no real functional effect on repair, and that the purpose of
this series is simply to remove some custom verifier code to facilitate
libxfs synchronization, it seems to me this just adds a bunch of code
and testing complexity for no tangible benefit.

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

Re: [PATCH 3/4] repair: use fs root ino for dummy parent value instead of zero

[Dave Chinner]

On Thu, Jul 16, 2020 at 06:41:03AM -0400, Brian Foster wrote:
> On Thu, Jul 16, 2020 at 08:22:16AM +1000, Dave Chinner wrote:
> > On Wed, Jul 15, 2020 at 10:08:35AM -0400, Brian Foster wrote:
> > > If a directory inode has an invalid parent ino on disk, repair
> > > replaces the invalid value with a dummy value of zero in the buffer
> > > and NULLFSINO in the in-core parent tracking. The zero value serves
> > > no functional purpose as it is still an invalid value and the parent
> > > must be repaired by phase 6 based on the in-core state before the
> > > buffer can be written out.  Instead, use the root fs inode number as
> > > a catch all for invalid parent values so phase 6 doesn't have to
> > > create custom verifier infrastructure just to work around this
> > > behavior.
> > > 
> > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > 
> > Reasonale, but wouldn't it be better to use lost+found as the dummy
> > parent inode (i.e. the orphanage inode)? Because if the parent can't
> > be found and the inode reconnected correctly, we're going to put it
> > in lost+found, anyway?
> > 
> 
> That was my first thought when I originally wrote this, but there's
> several reasons I didn't end up doing that. The orphanage isn't created
> until much later in repair and only if we end up with orphaned inodes.
> We'd have to change that in order to use a dummy parent inode number
> that corresponds to a valid orphanage, and TBH I'm not even sure if it's
> always going to be safe to expect an inode allocation to work at this
> point in repair.
> 
> Further, it's still too early to tell whether these directories are
> orphaned because the directory scan in phase 6 can easily repair
> missing/broken parent information. The scenarios I used to test this
> functionality didn't involve the orphanage at all, so now we not only
> need to change when/how the orphanage is created, but need to free it if
> it ends up unused before we exit (which could be via any number of
> do_error() calls before we ever get close to phase 6).

Fair enough - can you please capture all this in the commit message
to preserve the explanation of why the root inode was chosen and
not lost+found?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/4] repair: use fs root ino for dummy parent value instead of zero

[Brian Foster]

On Fri, Jul 17, 2020 at 08:06:05AM +1000, Dave Chinner wrote:
> On Thu, Jul 16, 2020 at 06:41:03AM -0400, Brian Foster wrote:
> > On Thu, Jul 16, 2020 at 08:22:16AM +1000, Dave Chinner wrote:
> > > On Wed, Jul 15, 2020 at 10:08:35AM -0400, Brian Foster wrote:
> > > > If a directory inode has an invalid parent ino on disk, repair
> > > > replaces the invalid value with a dummy value of zero in the buffer
> > > > and NULLFSINO in the in-core parent tracking. The zero value serves
> > > > no functional purpose as it is still an invalid value and the parent
> > > > must be repaired by phase 6 based on the in-core state before the
> > > > buffer can be written out.  Instead, use the root fs inode number as
> > > > a catch all for invalid parent values so phase 6 doesn't have to
> > > > create custom verifier infrastructure just to work around this
> > > > behavior.
> > > > 
> > > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > 
> > > Reasonale, but wouldn't it be better to use lost+found as the dummy
> > > parent inode (i.e. the orphanage inode)? Because if the parent can't
> > > be found and the inode reconnected correctly, we're going to put it
> > > in lost+found, anyway?
> > > 
> > 
> > That was my first thought when I originally wrote this, but there's
> > several reasons I didn't end up doing that. The orphanage isn't created
> > until much later in repair and only if we end up with orphaned inodes.
> > We'd have to change that in order to use a dummy parent inode number
> > that corresponds to a valid orphanage, and TBH I'm not even sure if it's
> > always going to be safe to expect an inode allocation to work at this
> > point in repair.
> > 
> > Further, it's still too early to tell whether these directories are
> > orphaned because the directory scan in phase 6 can easily repair
> > missing/broken parent information. The scenarios I used to test this
> > functionality didn't involve the orphanage at all, so now we not only
> > need to change when/how the orphanage is created, but need to free it if
> > it ends up unused before we exit (which could be via any number of
> > do_error() calls before we ever get close to phase 6).
> 
> Fair enough - can you please capture all this in the commit message
> to preserve the explanation of why the root inode was chosen and
> not lost+found?
> 

Sure, v2 incoming...

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

[PATCH v2] repair: use fs rootino for dummy parent value instead of zero

[Brian Foster]

If a directory inode has an invalid parent ino on disk, repair
replaces the invalid value with a dummy value of zero in the buffer
and NULLFSINO in the in-core parent tracking. The zero value serves
no functional purpose as it is still an invalid value and the parent
must be repaired by phase 6 based on the in-core state before the
buffer can be written out. A consequence of using an invalid dummy
value is that phase 6 requires custom verifier infrastructure to
detect the invalid parent inode and temporarily replace it while the
core fork verifier runs. If we use a valid inode number as a dummy
value earlier in repair, this workaround can be removed.

An obvious choice for a valid dummy parent inode value is the
orphanage inode. However, the orphanage inode is not allocated until
much later in repair when the filesystem structure is established as
sound and placement of orphaned inodes is imminent. In this case, it
is too early to know for sure whether the associated inodes are
orphaned because a directory traversal later in repair can locate
references to the inode and repair the parent value based on the
structure of the directory tree.

Given all of this, escalate the preexisting workaround from the
custom verifier in phase 6 and set the root inode value as a dummy
parent for shortform directories with an invalid on-disk parent. The
in-core parent is still tracked as NULLFSINO and so forces repair to
either update the parent or orphan the inode before repair
completes.

Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---

v2:
- Update patch subject and commit log.

 repair/dir2.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/repair/dir2.c b/repair/dir2.c
index caf6963d..9c789b4a 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -165,7 +165,6 @@ process_sf_dir2(
 	int			tmp_elen;
 	int			tmp_len;
 	xfs_dir2_sf_entry_t	*tmp_sfep;
-	xfs_ino_t		zero = 0;
 
 	sfp = (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip);
 	max_size = XFS_DFORK_DSIZE(dip, mp);
@@ -497,7 +496,7 @@ _("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);
+			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
 			*dino_dirty = 1;
 			*repair = 1;
 		} else  {
@@ -532,7 +531,7 @@ _("bad .. entry in directory inode %" PRIu64 ", points to self, "),
 		if (!no_modify)  {
 			do_warn(_("clearing inode number\n"));
 
-			libxfs_dir2_sf_put_parent_ino(sfp, zero);
+			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
 			*dino_dirty = 1;
 			*repair = 1;
 		} else  {
-- 
2.21.3

Re: [PATCH v2] repair: use fs rootino for dummy parent value instead of zero

[Dave Chinner]

On Fri, Jul 17, 2020 at 07:59:20AM -0400, Brian Foster wrote:
> If a directory inode has an invalid parent ino on disk, repair
> replaces the invalid value with a dummy value of zero in the buffer
> and NULLFSINO in the in-core parent tracking. The zero value serves
> no functional purpose as it is still an invalid value and the parent
> must be repaired by phase 6 based on the in-core state before the
> buffer can be written out. A consequence of using an invalid dummy
> value is that phase 6 requires custom verifier infrastructure to
> detect the invalid parent inode and temporarily replace it while the
> core fork verifier runs. If we use a valid inode number as a dummy
> value earlier in repair, this workaround can be removed.
> 
> An obvious choice for a valid dummy parent inode value is the
> orphanage inode. However, the orphanage inode is not allocated until
> much later in repair when the filesystem structure is established as
> sound and placement of orphaned inodes is imminent. In this case, it
> is too early to know for sure whether the associated inodes are
> orphaned because a directory traversal later in repair can locate
> references to the inode and repair the parent value based on the
> structure of the directory tree.
> 
> Given all of this, escalate the preexisting workaround from the
> custom verifier in phase 6 and set the root inode value as a dummy
> parent for shortform directories with an invalid on-disk parent. The
> in-core parent is still tracked as NULLFSINO and so forces repair to
> either update the parent or orphan the inode before repair
> completes.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> 
> v2:
> - Update patch subject and commit log.

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] repair: use fs rootino for dummy parent value instead of zero

[Darrick J. Wong]

On Fri, Jul 17, 2020 at 07:59:20AM -0400, Brian Foster wrote:
> If a directory inode has an invalid parent ino on disk, repair
> replaces the invalid value with a dummy value of zero in the buffer
> and NULLFSINO in the in-core parent tracking. The zero value serves
> no functional purpose as it is still an invalid value and the parent
> must be repaired by phase 6 based on the in-core state before the
> buffer can be written out. A consequence of using an invalid dummy
> value is that phase 6 requires custom verifier infrastructure to
> detect the invalid parent inode and temporarily replace it while the
> core fork verifier runs. If we use a valid inode number as a dummy
> value earlier in repair, this workaround can be removed.
> 
> An obvious choice for a valid dummy parent inode value is the
> orphanage inode. However, the orphanage inode is not allocated until
> much later in repair when the filesystem structure is established as
> sound and placement of orphaned inodes is imminent. In this case, it
> is too early to know for sure whether the associated inodes are
> orphaned because a directory traversal later in repair can locate
> references to the inode and repair the parent value based on the
> structure of the directory tree.
> 
> Given all of this, escalate the preexisting workaround from the
> custom verifier in phase 6 and set the root inode value as a dummy
> parent for shortform directories with an invalid on-disk parent. The
> in-core parent is still tracked as NULLFSINO and so forces repair to
> either update the parent or orphan the inode before repair
> completes.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Finally this stupid dragon dies!

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> v2:
> - Update patch subject and commit log.
> 
>  repair/dir2.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/repair/dir2.c b/repair/dir2.c
> index caf6963d..9c789b4a 100644
> --- a/repair/dir2.c
> +++ b/repair/dir2.c
> @@ -165,7 +165,6 @@ process_sf_dir2(
>  	int			tmp_elen;
>  	int			tmp_len;
>  	xfs_dir2_sf_entry_t	*tmp_sfep;
> -	xfs_ino_t		zero = 0;
>  
>  	sfp = (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip);
>  	max_size = XFS_DFORK_DSIZE(dip, mp);
> @@ -497,7 +496,7 @@ _("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);
> +			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
>  			*dino_dirty = 1;
>  			*repair = 1;
>  		} else  {
> @@ -532,7 +531,7 @@ _("bad .. entry in directory inode %" PRIu64 ", points to self, "),
>  		if (!no_modify)  {
>  			do_warn(_("clearing inode number\n"));
>  
> -			libxfs_dir2_sf_put_parent_ino(sfp, zero);
> +			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
>  			*dino_dirty = 1;
>  			*repair = 1;
>  		} else  {
> -- 
> 2.21.3
> 

Re: [PATCH 4/4] repair: remove custom dir2 sf fork verifier from phase6

[Darrick J. Wong]

On Wed, Jul 15, 2020 at 10:08:36AM -0400, Brian Foster wrote:
> The custom verifier exists to catch the case of repair setting a
> dummy parent value of zero on directory inodes and temporarily
> replace it with a valid inode number so the rest of the directory
> verification can proceed. The custom verifier is no longer needed
> now that the rootino is used as a dummy value for invalid on-disk
> parent values.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

More or less the same code that's in my tree...

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  repair/phase6.c | 56 ++-----------------------------------------------
>  1 file changed, 2 insertions(+), 54 deletions(-)
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index beceea9a..43bcea50 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -26,58 +26,6 @@ static struct xfs_name		xfs_name_dot = {(unsigned char *)".",
>  						1,
>  						XFS_DIR3_FT_DIR};
>  
> -/*
> - * When we're checking directory inodes, we're allowed to set a directory's
> - * dotdot entry to zero to signal that the parent needs to be reconnected
> - * during phase 6.  If we're handling a shortform directory the ifork
> - * verifiers will fail, so temporarily patch out this canary so that we can
> - * verify the rest of the fork and move on to fixing the dir.
> - */
> -static xfs_failaddr_t
> -phase6_verify_dir(
> -	struct xfs_inode		*ip)
> -{
> -	struct xfs_mount		*mp = ip->i_mount;
> -	struct xfs_ifork		*ifp;
> -	struct xfs_dir2_sf_hdr		*sfp;
> -	xfs_failaddr_t			fa;
> -	xfs_ino_t			old_parent;
> -	bool				parent_bypass = false;
> -	int				size;
> -
> -	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> -	sfp = (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data;
> -	size = ifp->if_bytes;
> -
> -	/*
> -	 * If this is a shortform directory, phase4 may have set the parent
> -	 * inode to zero to indicate that it must be fixed.  Temporarily
> -	 * set a valid parent so that the directory verifier will pass.
> -	 */
> -	if (size > offsetof(struct xfs_dir2_sf_hdr, parent) &&
> -	    size >= xfs_dir2_sf_hdr_size(sfp->i8count)) {
> -		old_parent = libxfs_dir2_sf_get_parent_ino(sfp);
> -		if (old_parent == 0) {
> -			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
> -			parent_bypass = true;
> -		}
> -	}
> -
> -	fa = libxfs_default_ifork_ops.verify_dir(ip);
> -
> -	/* Put it back. */
> -	if (parent_bypass)
> -		libxfs_dir2_sf_put_parent_ino(sfp, old_parent);
> -
> -	return fa;
> -}
> -
> -static struct xfs_ifork_ops phase6_ifork_ops = {
> -	.verify_attr	= xfs_attr_shortform_verify,
> -	.verify_dir	= phase6_verify_dir,
> -	.verify_symlink	= xfs_symlink_shortform_verify,
> -};
> -
>  /*
>   * Data structures used to keep track of directories where the ".."
>   * entries are updated. These must be rebuilt after the initial pass
> @@ -1104,7 +1052,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 +2823,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(
> -- 
> 2.21.3
> 

Re: [PATCH 2/4] repair: don't double check dir2 sf parent in phase 4

[Darrick J. Wong]

On Wed, Jul 15, 2020 at 10:08:34AM -0400, Brian Foster wrote:
> The shortform parent ino verification code runs once in phase 3
> (ino_discovery == true) and once in phase 4 (ino_discovery ==
> false). This is unnecessary and leads to duplicate error messages if
> repair replaces an invalid parent value with zero because zero is
> still an invalid value. Skip the check in phase 4.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  repair/dir2.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/repair/dir2.c b/repair/dir2.c
> index cbbce601..caf6963d 100644
> --- a/repair/dir2.c
> +++ b/repair/dir2.c
> @@ -480,6 +480,9 @@ _("corrected entry offsets in directory %" PRIu64 "\n"),
>  	 * check parent (..) entry
>  	 */
>  	*parent = libxfs_dir2_sf_get_parent_ino(sfp);
> +	if (!ino_discovery)
> +		return 0;

I feel like this ought to have a comment explaining why we skip only the
parent check in phase 4:

/*
 * If this function is called during inode discovery (phase 3), it will
 * set a bad sf dir parent pointer to the root directory.  This fixes
 * the directory enough to pass the inode fork verifier in phase 6 when
 * we try to reset the parent pointer to the correct value.  There is no
 * need to re-check the parent pointer during phase 4.
 */

With that added,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> +
>  
>  	/*
>  	 * if parent entry is bogus, null it out.  we'll fix it later .
> -- 
> 2.21.3
> 

Re: [PATCH 1/4] repair: set the in-core inode parent in phase 3

[Darrick J. Wong]

On Wed, Jul 15, 2020 at 10:08:33AM -0400, Brian Foster wrote:
> The inode processing code checks and resets invalid parent values on
> physical inodes in phase 3 but waits to update the parent value in
> the in-core tracking until phase 4. There doesn't appear to be any
> specific reason for the latter beyond caution. In reality, the only
> reason this doesn't cause problems is that phase 3 replaces an
> invalid on-disk parent with another invalid value, so the in-core
> parent returned by phase 4 translates to NULLFSINO.
> 
> This is subtle and fragile. To eliminate this duplicate processing
> behavior and break the subtle dependency of requiring an invalid
> dummy value in physical directory inodes, update the in-core parent
> tracking structure at the same point in phase 3 that physical inodes
> are updated. Invalid on-disk parent values will still translate to
> NULLFSINO for the in-core tracking to be identified by later phases.
> This ensures that if a valid dummy value is placed in a physical
> inode (such as rootino) with an invalid parent in phase 3, phase 4
> won't mistakenly return the valid dummy value to be incorrectly set
> in the in-core tracking over the NULLFSINO value that represents the
> broken on-disk state.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks fine to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  repair/dino_chunks.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
> index 6685a4d2..96ed6a5b 100644
> --- a/repair/dino_chunks.c
> +++ b/repair/dino_chunks.c
> @@ -859,14 +859,7 @@ next_readbuf:
>  		 */
>  		if (isa_dir)  {
>  			set_inode_isadir(ino_rec, irec_offset);
> -			/*
> -			 * we always set the parent but
> -			 * we may as well wait until
> -			 * phase 4 (no inode discovery)
> -			 * because the parent info will
> -			 * be solid then.
> -			 */
> -			if (!ino_discovery)  {
> +			if (ino_discovery)  {
>  				ASSERT(parent != 0);
>  				set_inode_parent(ino_rec, irec_offset, parent);
>  				ASSERT(parent ==
> -- 
> 2.21.3
>