Re: [PATCH 04/18] fs: Implement lazy LRU updates for inodes.

[Dave Chinner <david@fromorbit.com>]

On Fri, Oct 08, 2010 at 03:08:56AM -0400, Christoph Hellwig wrote:
> Looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> 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@fromorbit.com