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

[Eric Sandeen <sandeen@sandeen.net>]

Christoph Hellwig wrote:

> From: Dave Chinner <david@fromorbit.com>
> 
> Given that we walk across the per-ag inode lists so often, it makes sense to
> introduce an iterator for this.
> 
> Convert the sync and reclaim code to use this new iterator, quota code will
> follow in the next patch.
> 
> [hch: merged the lookup and execute callbacks back into one to get the
>  pag_ici_lock locking correct and simplify the code flow]
> 
> Signed-off-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Somehow I'm finding this hard to review, but...

> Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:20:37.012658983 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:22:26.321659103 +0200

...

> +STATIC int
> +xfs_inode_ag_walk(
> +	struct xfs_mount	*mp,
> +	xfs_agnumber_t		ag,
> +	int			(*execute)(struct xfs_inode *ip,
> +					   struct xfs_perag *pag, int flags),
> +	int			flags,
> +	int			tag)
> +{
> +	struct xfs_perag	*pag = &mp->m_perag[ag];
> +	uint32_t		first_index;
> +	int			last_error = 0;
> +	int			skipped;
> +
> +restart:
> +	skipped = 0;
> +	first_index = 0;
> +	do {
> +		int		error = 0;
> +		xfs_inode_t	*ip;
> +
> +		ip = xfs_inode_ag_lookup(mp, pag, &first_index, tag);
> +		if (!ip)
> +			break;
> +
> +		error = execute(ip, pag, flags);
> +		if (error == EAGAIN) {
> +			skipped++;
> +			continue;
> +		}
> +		if (error)
> +			last_error = error;
> +		/*
> +		 * bail out if the filesystem is corrupted.
> +		 */
> +		if (error == EFSCORRUPTED)
> +			break;

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

> +
> +	} while (1);

...

> @@ -85,12 +201,17 @@ xfs_sync_inode_valid(
>  STATIC int
>  xfs_sync_inode_data(
>  	struct xfs_inode	*ip,
> +	struct xfs_perag	*pag,
>  	int			flags)
>  {
>  	struct inode	*inode = VFS_I(ip);
>  	struct address_space *mapping = inode->i_mapping;
>  	int		error = 0;
>  
> +	error = xfs_sync_inode_valid(ip, pag);
> +	if (error)
> +		return 0;xfs_sync_inode_attr(
> +

xfs_sync_inode_valid can return 0, ENOENT, or EFSCORRUPTED.

Aren't we losing the error here...

>  	if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
>  		if (!xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED)) {
>  			if (flags & SYNC_TRYLOCK)
> @@ -106,16 +227,22 @@ xfs_sync_inode_data(
>   out_wait:
>  	if (flags & SYNC_IOWAIT)
>  		xfs_ioend_wait(ip);
> +	IRELE(ip);
>  	return error;
>  }
>  
>  STATIC int
>  xfs_sync_inode_attr(
>  	struct xfs_inode	*ip,
> +	struct xfs_perag	*pag,
>  	int			flags)
>  {
>  	int			error = 0;
>  
> +	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():

> +		error = execute(ip, pag, flags);
> +		if (error == EAGAIN) {
> +			skipped++;
> +			continue;
> +		}
> +		if (error)
> +			last_error = error;

above, and I think they're ignoring the return from
xfs_sync_inode_valid(), therefore xfs_inode_ag_walk won't see
EFSCORRUPTED from it either ... right?

-Eric

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