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

From: Dave Chinner <david@fromorbit.com>
To: Brian Foster <bfoster@redhat.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 3/4] repair: use fs root ino for dummy parent value instead of zero
Date: Fri, 17 Jul 2020 08:06:05 +1000	[thread overview]
Message-ID: <20200716220605.GM2005@dread.disaster.area> (raw)
In-Reply-To: <20200716104103.GB26218@bfoster>

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