On Fri, 2 Jun 2006 12:23:39 +1000 David Chinner <dgc@xxxxxxx> wrote: > On Thu, Jun 01, 2006 at 06:06:59PM -0700, Andrew Morton wrote: > > On Thu, 01 Jun 2006 11:51:25 +0200 > > jblunck@xxxxxxx 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. OK. > > 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... Reasonable. Whatever we do needs to be fully communicated in the comment text please. > > 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.... Don't do a divide? sb->s_scan_count += count; ... tmp = sb->s_dentry_stat.nr_unused / (global_dentry_stat.nr_unused / sb->s_scan_count + 1); if (tmp) { sb->s_scan_count -= <can't be bothered doing the arith ;)>; prune_dcache_sb(sb, tmp); } That could go weird on us if there are sudden swings in sb->s_dentry_stat.nr_unused or global_dentry_stat.nr_unused, but appropriate boundary checking should fix that? - 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
