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