On Fri, Oct 08, 2010 at 03:08:56AM -0400, Christoph Hellwig wrote:
> Looks good,
>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
>
> A few nipicks on the comments or lack thereof below:
>
> > @@ -489,8 +484,15 @@ static void prune_icache(int nr_to_scan)
> >
> > inode = list_entry(inode_unused.prev, struct inode, i_list);
> >
> > - if (inode->i_state || atomic_read(&inode->i_count)) {
> > + if (atomic_read(&inode->i_count) ||
> > + (inode->i_state & ~I_REFERENCED)) {
> > + list_del_init(&inode->i_list);
> > + percpu_counter_dec(&nr_inodes_unused);
> > + continue;
> > + }
> > + if (inode->i_state & I_REFERENCED) {
> > list_move(&inode->i_list, &inode_unused);
> > + inode->i_state &= ~I_REFERENCED;
> > continue;
>
> I think this code could use some comments explaining the lazy LRU
> scheme.
Ok. I'll add some to it.
> > - if (inode != list_entry(inode_unused.next,
> > - struct inode, i_list))
> > - continue; /* wrong inode or list_empty */
> > - if (!can_unuse(inode))
> > + /*
> > + * if we can't reclaim this inod immediately, give it
> > + * another pass through the free list so we don't spin
> > + * on it.
>
> s/inod/inode/
Woops, missed that one again.
> > +
> > + /*
> > + * We avoid moving dirty inodes back onto the LRU now because I_FREEING
> > + * is set and hence writeback_single_inode() won't move the inode
> > + * around.
> > + */
> > + if (!list_empty(&inode->i_list)) {
> > + list_del_init(&inode->i_list);
> > + percpu_counter_dec(&nr_inodes_unused);
> > + }
> > +
>
> The comment is a bit misleading. We do not only avoid moving it to the
> LRU, but actively delete the inode from the LRU here.
Right. My intent was that "after the inode is deleted from the LRU,
we avoid moving dirty inodes....". I'll add that clarification to
the comment.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
