Re: [PATCH] repair: fix wrong logic when validating node magic number

[Brian Foster <bfoster@xxxxxxxxxx>]

On Thu, Aug 13, 2015 at 03:15:24PM +0800, Eryu Guan wrote:
> On Thu, Aug 13, 2015 at 03:01:16PM +0800, Eryu Guan wrote:
> > Magic number is wrong only when != XFS_DA_NODE_MAGIC and
> > != XFS_DA3_NODE_MAGIC.
> > 
> > This is triggered by shared/002 when testing 512 block size XFS.
> > 
> >   Phase 1 - find and verify superblock...
> >   Phase 2 - using internal log
> >           - scan filesystem freespace and inode maps...
> >           - found root inode chunk
> >   Phase 3 - for each AG...
> >           - scan (but don't clear) agi unlinked lists...
> >           - process known inodes and perform inode discovery...
> >           - agno = 0
> >   bad magic number febe in block 64 (108) for directory inode 35
> >   ......
> > 
> > Fix it by changing "||" to "&&".
> > 
> > Signed-off-by: Eryu Guan <eguan@xxxxxxxxxx>
> 
> With this patch applied, shared/002 still fails on 512 block size XFS,
> full xfs_repair -n output is                                                  
>                                                                 
>                                                                               
>                                                                 
> *** xfs_repair -n output ***                                                  
>                                                                 
> Phase 1 - find and verify superblock...                                       
>                                                                 
> Phase 2 - using internal log                                                  
>                                                                 
>         - scan filesystem freespace and inode maps...                         
>                                                                 
>         - found root inode chunk                                              
>                                                                 
> Phase 3 - for each AG...                                                      
>                                                                 
>         - scan (but don't clear) agi unlinked lists...                        
>                                                                 
>         - process known inodes and perform inode discovery...                 
>                                                                 
>         - agno = 0                                                            
>                                                                 
> problem with attribute contents in inode 35                                   
>                                                                 
> would clear attr fork                                                         
>                                                                 
> bad nblocks 67 for inode 35, would reset to 0                                 
>                                                                 
> bad anextents 5 for inode 35, would reset to 0                                
>                                                                 
>         - agno = 1                                                            
>                                                                 
>         - agno = 2                                                            
>                                                                 
>         - agno = 3                                                            
>                                                                 
>         - process newly discovered inodes...                                  
>                                                                 
> Phase 4 - check for duplicate blocks...                                       
>                                                                 
>         - setting up duplicate extent list...                                 
>                                                                 
>         - check for inodes claiming duplicate blocks...                       
>                                                                 
>         - agno = 0                                                            
>                                                                 
>         - agno = 1                                                            
>                                                                 
>         - agno = 2                                                            
>                                                                 
>         - agno = 3                                                            
>                                                                 
> No modify flag set, skipping phase 5                                          
>                                                                 
> Phase 6 - check inode connectivity...                                         
>                                                                 
>         - traversing filesystem ...                                           
>                                                                 
>         - traversal finished ...                                              
>                                                                 
>         - moving disconnected inodes to lost+found ...                        
>                                                                 
> Phase 7 - verify link counts...                                               
>                                                                 
> No modify flag set, skipping filesystem flush and exiting.                    
>                                                                 
> *** end xfs_repair output
> 
> And a simplified reproducer is just adding >= 577 xattrs to file foo on
> 512 block size XFS, no dmflaky is needed.
> 
> num_xattrs=577
> for ((i = 1; i <= $num_xattrs; i++)); do
>         name="user.attr_$(printf "%04d" $i)"
>         $SETFATTR_PROG -n $name -v "val_$(printf "%04d" $i)" $SCRATCH_MNT/foo
> done
> 
> And it's easily reproduced.
> 

Thanks for the reproducer. This looks like a bug in xfs_repair. Care to
test the appended hunk?

Brian

---8<---

diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index 83a07a8..b76618a 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -562,7 +562,7 @@ verify_da_path(xfs_mount_t  *mp,
                }
 
                newnode = (xfs_da_intnode_t *)XFS_BUF_PTR(bp);
-               btree = M_DIROPS(mp)->node_tree_p(node);
+               btree = M_DIROPS(mp)->node_tree_p(newnode);
                M_DIROPS(mp)->node_hdr_from_disk(&nodehdr, newnode);
 
                /*