Re: [PATCH 5/7] xfs: introduce a per-ag inode iterator

[Christoph Hellwig <hch@infradead.org>]

On Wed, Jun 03, 2009 at 05:01:13PM -0500, Eric Sandeen wrote:
> Ok so here we are looking for EFSCORRUPTED from the "execute" function.
>  This might be xfs_sync_inode_data, xfs_sync_inode_attr, or
> xfs_reclaim_inode_now.  But ...

> 
> xfs_sync_inode_valid can return 0, ENOENT, or EFSCORRUPTED.
> 
> Aren't we losing the error here...

We can get the EFSCORRUPTED from xfs_iflush.

> >  
> > +	error = xfs_sync_inode_valid(ip, pag);
> > +	if (error)
> > +		return 0;
> 
> and here?
> 
> so xfs_sync_inode_data / xfs_sync_inode_attr are the "execute" in
> xfs_inode_ag_walk():

If you look at the old code we return early with 0 for the
XFS_FORCED_SHUTDOWN case, which is the only reason xfs_sync_inode_valid 
return xfs_sync_inode_valid in the new code.  We don't actually break
out of the loop in the new code, but don't do any action so the
behaviour is as similar as it gets.  An EFSCORRUPTED later in the
execute function (which AFAICS can only come from xfs_iflush) will
end up pssed down to xfs_inode_ag_iterator.

I can't say that I like this too much.  And in the end only
xfs_fs_quota_sync actually every propagates the return value from
xfs_sync_inodes, and then just directly to userspace.

So I think we are safe just propagating the EFSCORRUPTED down and
make all this a lot more logical.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs