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

Eric Sandeen
2009-06-03T22:18:26Z

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>

And a similar error handling question...

> 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;
> + }

Ok, it's looking for EAGAIN here, I'm assuming this is for when we are
calling xfs_reclaim_inode_now, because...

...

> -STATIC void
> -xfs_reclaim_inodes_ag(
> - xfs_mount_t *mp,
> - int ag,
> - int mode)
> +STATIC int
> +xfs_reclaim_inode_now(
> + struct xfs_inode *ip,
> + struct xfs_perag *pag,
> + int flags)
> {
> - xfs_inode_t *ip = NULL;
> - xfs_perag_t *pag = &mp->m_perag[ag];
> - int nr_found;
> - uint32_t first_index;
> - int skipped;
> -
> -restart:
> - first_index = 0;
> - skipped = 0;
> - do {

...

> -
> - /*
> - * hmmm - this is an inode already in reclaim. Do
> - * we even bother catching it here?
> - */
> - if (xfs_reclaim_inode(ip, 0, mode))
> - skipped++;
> - } while (nr_found);

... because before, that's what we did above, after testing for a non-0
return from xfs_reclaim_inode.

But xfs_reclaim_inode_now() returns 0 or the result of
xfs_reclaim_inode, which is 0/1, so above:

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

isn't going to see EAGAIN from xfs_reclaim_inode_now... am I following
this right?

-Eric