| Message ID | 20200715140836.10197-4-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfsprogs: remove custom dir2 sf fork verifier from repair | expand |
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>
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.
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 >
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.
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 >
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 {
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(-)