Re: [PATCH 11/16] xfs: split inode AG walking into separate code for reclaim

[Alex Elder <aelder@sgi.com>]

On Wed, 2010-09-22 at 16:44 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The reclaim walk requires different locking and has a slightly
> different walk algorithm, so separate it out so that it can be
> optimised separately.

You assert this above xfs_reclaim_inodes_ag():
 Walk the AGs and reclaim the inodes in them. Even if the filesystem is
 corrupted, we still want to try to reclaim all the inodes.

I don't refute that statement, but if I'm not mistaken
this is new handling.  I just felt it should not slip in
without being mentioned.  I also suggest a short sentence
along with that comment to justify it might be in order.

I have one other minor comment below, but otherwise this
looks good.

Reviewed-by: Alex Elder <aelder@sgi.com>

> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/linux-2.6/xfs_sync.c    |  200 ++++++++++++++++++----------------------
>  fs/xfs/linux-2.6/xfs_sync.h    |    2 +-
>  fs/xfs/linux-2.6/xfs_trace.h   |    2 +-
>  fs/xfs/quota/xfs_qm_syscalls.c |    3 +-
>  fs/xfs/xfs_mount.c             |   26 +++++
>  fs/xfs/xfs_mount.h             |    2 +
>  6 files changed, 120 insertions(+), 115 deletions(-)
> 
> diff --git a/fs/xfs/linux-2.6/xfs_sync.c b/fs/xfs/linux-2.6/xfs_sync.c
> index ddeaff9..7737a13 100644
> --- a/fs/xfs/linux-2.6/xfs_sync.c
> +++ b/fs/xfs/linux-2.6/xfs_sync.c

. . .

> @@ -859,13 +781,70 @@ reclaim:
>  
>  }
>  
> +/*
> + * Walk the AGs and reclaim the inodes in them. Even if the filesystem is
> + * corrupted, we still want to try to reclaim all the inodes.
> + */
> +int
> +xfs_reclaim_inodes_ag(
> +	struct xfs_mount	*mp,
> +	int			flags,
> +	int			*nr_to_scan)
> +{
> +	struct xfs_perag	*pag;
> +	int			error = 0;
> +	int			last_error = 0;
> +	xfs_agnumber_t		ag;
> +
> +	ag = 0;
> +	while ((pag = xfs_perag_get_tag(mp, ag, XFS_ICI_RECLAIM_TAG))) {
> +		unsigned long	first_index = 0;
> +		int		done = 0;
> +
> +		ag = pag->pag_agno + 1;
> +
> +		do {
> +			struct xfs_inode *ip;
> +			int	nr_found;
> +
> +			write_lock(&pag->pag_ici_lock);
> +			nr_found = radix_tree_gang_lookup_tag(&pag->pag_ici_root,
> +					(void **)&ip, first_index, 1,
> +					XFS_ICI_RECLAIM_TAG);
> +			if (!nr_found) {
> +				write_unlock(&pag->pag_ici_lock);
> +				break;
> +			}
> +
> +			/*
> +			 * Update the index for the next lookup. Catch overflows
> +			 * into the next AG range which can occur if we have inodes
> +			 * in the last block of the AG and we are currently
> +			 * pointing to the last inode.
> +			 */
> +			first_index = XFS_INO_TO_AGINO(mp, ip->i_ino + 1);
> +			if (first_index < XFS_INO_TO_AGINO(mp, ip->i_ino))
> +				done = 1;
> +
> +			error = xfs_reclaim_inode(ip, pag, flags);
> +			if (error && last_error != EFSCORRUPTED)
> +				last_error = error;
> +
> +		} while (!done && (*nr_to_scan)--);

I know you guys disagreed with me before on this point.
But since there is no reason anybody outside this function
has a need to see the intermediate values of *nr_to_scan,
you should use a local variable to keep track of it instead,
and then assign its final value to *nr_to_scan before returning.
It allows registers to be used rather than updating real
memory at each iteration.

> +
> +		xfs_perag_put(pag);
> +	}
> +	return XFS_ERROR(last_error);
> +}
> +
>  int
>  xfs_reclaim_inodes(
>  	xfs_mount_t	*mp,
>  	int		mode)
>  {

. . .

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