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

[Chandan Babu R <chandan.babu@oracle.com>]

On Tue, May 24, 2022 at 09:08:13 AM +1000, Dave Chinner wrote:
> 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.

I came across this issue while reading code in order to better understand
xfs_repair.

The following steps illustrate how the code flows from phase 2 and 3 of
xfs_repair.

During phase 2,
1. Scan inobt records.
2. For valid records, add corresponding entries to certain inode tree
   (i.e. inode_tree_ptrs[agno]).
3. For suspect records (e.g. Inobt leaf blocks which have a CRC mismatch), add
   entries to uncertain inode tree (i.e. inode_uncertain_tree_ptrs[agno]).

Uncertain inode chunk records are processed at the beginning of Phase 3
(please refer to check_uncertain_aginodes()). We pick one inode chunk at a
time from the uncertain inode tree and verify each inode's ondisk contents. If
most of the chunk's inodes turn out to be valid, we would want to treat the
chunk's inodes as certain i.e. move them over to the certain inode tree.

Existing code would check for the presence of the inode chunk in the uncertain
inode tree and when such an entry is found, we skip further processing of the
inode chunk. Since the inode chunk was obtained from the uncertain inode tree
in the first place, this check succeeds and the code ended up ignoring
uncertain inodes which were actually valid inodes.

I think checking uncertain inode tree for conflicts is a programming error. We
should actually be checking only the certain inode tree for conflicts before
moving the inode chunk to certain inode tree.

I wrote the script
(https://gist.github.com/chandanr/5ad2da06a7863c2918ad793636537536) to
illustrate the problem. This script create an inobt with two fully populated
leaves. It then changes 2nd leaf's lsn value to cause a CRC check
failure. This causes phase 2 of xfs_repair to add inodes in the 2nd leaf to
uncertain inode tree.

Without the fix provided by the patch, phase 3 will skip converting inodes
from the 2nd leaf into certain inodes and hence xfs_repair ends up trashing
these inodes.

>
> 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...

Sure, I will use integers for patch version numbers from now onwards.

-- 
chandan