diff mbox series

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

Message ID 20200717115920.59986-1-bfoster@redhat.com (mailing list archive)
State Accepted
Headers show
Series [v2] repair: use fs rootino for dummy parent value instead of zero | expand

Commit Message

Brian Foster July 17, 2020, 11:59 a.m. UTC 
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(-)

Comments

Dave Chinner July 20, 2020, 3:21 a.m. UTC | #1 
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>
Darrick J. Wong July 21, 2020, 12:47 a.m. UTC | #2 
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
>
diff mbox series

Patch

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  {