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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20101008073141.GQ4681@dastard>
Date:	Fri, 8 Oct 2010 18:31:41 +1100
From:	Dave Chinner <david@...morbit.com>
To:	Christoph Hellwig <hch@....de>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 04/18] fs: Implement lazy LRU updates for inodes.

On Fri, Oct 08, 2010 at 03:08:56AM -0400, Christoph Hellwig wrote:
> Looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@....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@...morbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/