iget behaviour in xlog_recover_process_iunlinks

iget behaviour in xlog_recover_process_iunlinks

[Christoph Hellwig]

shouldn't we call xfs_iget with the XFS_IGET_CREATE flag here?

the code seems to be perfectly happy with zero-ed out inodes as long as
di_next_unlinked is valid.

Re: iget behaviour in xlog_recover_process_iunlinks

[Timothy Shimmin]

Christoph Hellwig wrote:
> shouldn't we call xfs_iget with the XFS_IGET_CREATE flag here?
> 
> the code seems to be perfectly happy with zero-ed out inodes as long as
> di_next_unlinked is valid.
> 


So looking at this code...

we look at each bucket in the AGI bucket list and
walk the di_next_unlinked list of inodes.
We do an xfs_iget with 0 for flags which will mean
in xfs_iget_core it will error
out if it finds an inode with zeroed mode
(which happens on inodes which have been zeroed b/c it is new
or inativated with the mode reset in xfs_ifree,
however, it wouldn't all be zeroed as di_next_unlinked is
non zero).

And yet in the xlog_recover_process_iunlinks() it
later checks for a zero mode or not.
>  if (ip->i_d.di_mode == 0)
>          xfs_iput_new(ip, 0);
>  else
>          IRELE(ip);
....
> void
> xfs_iput_new(xfs_inode_t        *ip,
>              uint               lock_flags)
> {
>         struct inode    *inode = ip->i_vnode;
> 
>         xfs_itrace_entry(ip);
> 
>         if ((ip->i_d.di_mode == 0)) {
>                 ASSERT(!xfs_iflags_test(ip, XFS_IRECLAIMABLE));
>                 make_bad_inode(inode);
>         }
>         if (inode->i_state & I_NEW)
>                 unlock_new_inode(inode);
>         if (lock_flags)
>                 xfs_iunlock(ip, lock_flags);
>         IRELE(ip);
> }

And if it does get an error from xfs_iget, it will ditch
the rest of the list and the bucket.

So yeah to me it looks like we need XFS_IGET_CREATE or we
have handling code for zero mode which never happens or
we are risking ditching some unlinked inodes.

Was that what you were noticing?

--Tim

Re: iget behaviour in xlog_recover_process_iunlinks

[David Chinner]

On Thu, Apr 10, 2008 at 09:04:53PM +0200, Christoph Hellwig wrote:
> shouldn't we call xfs_iget with the XFS_IGET_CREATE flag here?
> 
> the code seems to be perfectly happy with zero-ed out inodes as long as
> di_next_unlinked is valid.

Don't think so - di_mode is not zero'd until the inode is removed
from the unlinked list. Hence if it requires recovery from the unlinked list,
then INACTIVE transaction that removes it from the unlinked list and sets
di_mode to zero has not been replayed at all. XFS_IGET_CREATE is only
needed for inodes with a zero di_mode....

IOWs, I'm not sure how you'd get an inode with a zero mode on the unlinked
list at all, and certainly the current xfs_iget() call should not return
any inodes with a zero di_mode. So why is there special code to handle
this in recovery?

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group