On Fri, Apr 05, 2019 at 01:19:52PM -0500, Eric Sandeen wrote: > > > On 4/5/19 1:17 PM, Eric Sandeen wrote: > > On 3/14/19 4:06 PM, Darrick J. Wong wrote: > >> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > >> > >> Retain the ifork ops used to validate the inode so that we can use the > >> same one to iflush it. xfs_repair phase 6 can use multiple transactions > >> to fix various inode problems, which means that the inode might not be > >> fully fixed when each transaction commits. > >> > >> This can be a particular problem if there's a shortform directory with > >> both invalid directory entries and incorrect i8count. Phase 3 will set > >> the parent inode to "0" to signal to phase 6 that it needs to reset the > >> parent and i8count, but phase 6 starts a transaction to junk the bad > >> entries which fail to commit because the parent is invalid: > >> > >> fixing i8count in inode 69022994673 > >> Invalid inode number 0x0 > >> xfs_dir_ino_validate: XFS_ERROR_REPORT > >> Metadata corruption detected at 0x464eb0, inode 0x10121750f1 data fork > >> xfs_repair: warning - iflush_int failed (-117) > >> > >> And thus the inode fixes never get written out. > > > > This feels like it would be better to use consistently by dropping the ops > > arg to libxfs_inode_verify_forks, using ->i_fork_ops inside it, and setting > > it prior to the call. Er, same for libxfs_iget? > > er, no not for libxfs_iget, sorry. But libxfs_iget can set the ops on > the inode if found and from then on stuff like libxfs_inode_verify_forks > uses what's currently set in the inode ....) > > > A mishmash of grabbing what was last set in the inode vs. explicitly pointing > > at a global ops structure seems a bit ad hoc. Thoughts? Yeah, there's no reason to have the second parameter when we've already got it in @ip, so I'll make a new patch to refactor it out. --D
