[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/4] XFS: Use the inode tree for finding dirty inodes
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Tue, 22 Jul 2008 00:28:29 -0400
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1216556394-17529-3-git-send-email-david@xxxxxxxxxxxxx>
References: <1216556394-17529-1-git-send-email-david@xxxxxxxxxxxxx> <1216556394-17529-3-git-send-email-david@xxxxxxxxxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.5.18 (2008-05-17)
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.

<Prev in Thread] Current Thread [Next in Thread>