Re: [patch 0/5] [PATCH,RFC] vfs: per-superblock unused dentries list (2nd version)

[David Chinner <dgc@sgi.com>]

On Thu, Jun 01, 2006 at 06:06:59PM -0700, Andrew Morton wrote:
> On Thu, 01 Jun 2006 11:51:25 +0200
> jblunck@suse.de wrote:
> 
> > This is an attempt to have per-superblock unused dentry lists.
> 
> Fairly significant clashes with
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.17-rc5/2.6.17-rc5-mm2/broken-out/fix-dcache-race-during-umount.patch 
> 
> I guess Neil's patch will go into the 2.6.18 tree, so you'd be best off
> working against that.

Though this patch series fixes the same problem in a much cleaner
way. It effectively obsoletes Neil's fix.

> Also, you're making what appears to be a quite deep design change to a
> pretty important part of the memory reclaim code and all the info we have
> is this:
> 
> 
> +				/*
> +				 * Try to be fair to the unused lists:
> +				 *  sb_count/sb_unused ~ count/global_unused
> +				 *
> +				 * Additionally, if the age_limit of the
> +				 * superblock is expired shrink at least one
> +				 * dentry from the superblock
> +				 */
> +				tmp = sb->s_dentry_stat.nr_unused /
> +					((unused / count) + 1);
> +				if (!tmp && time_after(jiffies,
> +						       sb->s_dentry_unused_age))
> +					tmp = 1;
> 
> 
> Please, we'll need much much more description of what this is trying to
> achieve, why it exists, analysis, testing results, etc, etc.  Coz my
> immediate reaction is "wtf is that, and what will that do to my computer?".

Discussed in this thread:

http://marc.theaimsgroup.com/?l=linux-fsdevel&m=114890371801114&w=2

Short summary of the problem: due to SHRINK_BATCH resolution, a proportional
reclaim based on "count" across all superblocks will not shrink anything on
lists 2 orders of magnitude smaller than the longest list as tmp will evaluate
as zero.  Hence to prevent small unused lists from never being reclaimed and
pinning memory until >90% of the dentry cache has been reclaimed we need to
turn them over slowly. However, if we turn them over too quickly, the dentry
cache does no caching for small filesystems.

This is not a problem a single global unused list has...

> In particular, `jiffies' has near-to-zero correlation with the rate of
> creation and reclaim of these objects, so it looks highly inappropriate
> that it's in there.  If anything can be used to measure "time" in this code
> it is the number of scanned entries, not jiffies.

Sure, but SHRINK_BATCH resolution basically makes it impossible to reconcile
lists of vastly different lengths. If the shrinker simply called us
with the entire count it now hands us in batches, I doubt that this would be
an issue.

In the mean time, we need some other method to ensure we do eventually free
up these items on small lists. The above implements an idle timer used to
determine when we start to turn over a small cache. Maybe if we wrap it in:

> +				if (!tmp && dentry_lru_idle(sb))
> +					tmp = 1;

with a more appropriate comment it would make more sense?

Suggestions on other ways to resolve the problem are welcome....

Cheers,

Dave.
-- 
Dave Chinner
R&D Software Enginner
SGI Australian Software Group