Re: [PATCH V1.1] xfs_repair: Search for conflicts in inode_tree_ptrs[] when processing uncertain inodes

[Dave Chinner <david@fromorbit.com>]

On Mon, May 23, 2022 at 01:28:47PM -0700, Darrick J. Wong wrote:
> On Mon, May 23, 2022 at 02:04:10PM +0530, Chandan Babu R wrote:
> > When processing an uncertain inode chunk record, if we lose 2 blocks worth of
> > inodes or 25% of the chunk, xfs_repair decides to ignore the chunk. Otherwise,
> > xfs_repair adds a new chunk record to inode_tree_ptrs[agno], marking each
> > inode as either free or used. However, before adding the new chunk record,
> > xfs_repair has to check for the existance of a conflicting record.
> > 
> > The existing code incorrectly checks for the conflicting record in
> > inode_uncertain_tree_ptrs[agno]. This check will succeed since the inode chunk
> > record being processed was originally obtained from
> > inode_uncertain_tree_ptrs[agno].
> > 
> > This commit fixes the bug by changing xfs_repair to search
> > inode_tree_ptrs[agno] for conflicts.
> 
> Just out of curiosity -- how did you come across this bug?  I /think/ it
> looks reasonable, but want to know more context...
> 
> > Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> > ---
> > Changelog:
> > V1 -> V1.1:
> >    1. Fix commit message.
> >    
> >  repair/dino_chunks.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
> > index 11b0eb5f..80c52a43 100644
> > --- a/repair/dino_chunks.c
> > +++ b/repair/dino_chunks.c
> > @@ -229,8 +229,7 @@ verify_inode_chunk(xfs_mount_t		*mp,
> >  		/*
> >  		 * ok, put the record into the tree, if no conflict.
> >  		 */
> > -		if (find_uncertain_inode_rec(agno,
> > -				XFS_AGB_TO_AGINO(mp, start_agbno)))
> > +		if (find_inode_rec(mp, agno, XFS_AGB_TO_AGINO(mp, start_agbno)))
> 
> ...because the big question I have is: why not check both the certain
> and the uncertain records for confliects?

Yeah, that was my question, too.

WHile I'm here, Chandan, a small patch admin note: tools like b4
don't handle patch versions like "V1.1" properly.

If you are replying in line with a new patch, just call it "V2" or
"V3" - the version of the entire patchset (in the [PATCH 0/N V2]
header) doesn't matter in this case, what matters is that it the
second version of the patch in this thread. Us humans are smart
enough to tell the difference between "series version" and "patch
within series version", and it turns out if you use the right
version formats the tools are smart enough, too. :)

As such, b4 will automatically pick up the V2 patch as a newer
version of the patch in the current series rather than miss it
entirely because it doesn't understand the V1.1 version numbering
you've used...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com