Re: [PATCH 2/4] XFS: Use the inode tree for finding dirty inodes

Christoph Hellwig
2008-07-22T04:27:25Z

On Sun, Jul 20, 2008 at 10:19:52PM +1000, Dave Chinner wrote:
> Update xfs_sync_inodes to walk the inode radix tree cache to find
> dirty inodes. This removes a huge bunch of nasty, messy code for
> traversing the mount inode list safely and removes another user of
> the mount inode list.

Looks good, some minor nits below:

> xfs_inode_t *ip = NULL;
> bhv_vnode_t *vp = NULL;

As you're touching most uses of vp what about just turning it
into a plain Linux inode?

> + if (!pag->pag_ici_init)
> + return 0;

I think it would be cleaner to move this into the caller and not even
call into this function for uninitialized AGs.

> + read_lock(&pag->pag_ici_lock);
> + nr_found = radix_tree_gang_lookup(&pag->pag_ici_root,
> + (void**)&ip, first_index, 1);

This needs a big comment on why you're using the gang lookup for
a single lookup. I guess that's because the gang lookup skips to
the next actually existing entry instead of returning NULL, but that's
not really obvious to the reader.

> + VN_RELE(vp);

This should either be an iput or IRELE, VN_RELE is on it's way out.

Btw, all these also apply to the next patch, I won't comment on them
there again.