From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from cuda.sgi.com (cuda3.sgi.com [192.48.176.15]) by oss.sgi.com (8.14.3/8.14.3/SuSE Linux 0.8) with ESMTP id o4RMcFmD169176 for ; Thu, 27 May 2010 17:38:15 -0500 Received: from mail.internode.on.net (localhost [127.0.0.1]) by cuda.sgi.com (Spam Firewall) with ESMTP id 225751D531A3 for ; Thu, 27 May 2010 15:40:38 -0700 (PDT) Received: from mail.internode.on.net (bld-mail15.adl6.internode.on.net [150.101.137.100]) by cuda.sgi.com with ESMTP id YFHuQwfBC4TLCg3I for ; Thu, 27 May 2010 15:40:38 -0700 (PDT) Date: Fri, 28 May 2010 08:40:34 +1000 From: Dave Chinner Subject: Re: [PATCH 3/5] superblock: introduce per-sb cache shrinker infrastructure Message-ID: <20100527224034.GO12087@dastard> References: <1274777588-21494-1-git-send-email-david@fromorbit.com> <1274777588-21494-4-git-send-email-david@fromorbit.com> <20100527063523.GJ22536@laptop> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20100527063523.GJ22536@laptop> List-Id: XFS Filesystem from SGI List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Sender: xfs-bounces@oss.sgi.com Errors-To: xfs-bounces@oss.sgi.com To: Nick Piggin Cc: linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, xfs@oss.sgi.com T24gVGh1LCBNYXkgMjcsIDIwMTAgYXQgMDQ6MzU6MjNQTSArMTAwMCwgTmljayBQaWdnaW4gd3Jv dGU6Cj4gT24gVHVlLCBNYXkgMjUsIDIwMTAgYXQgMDY6NTM6MDZQTSArMTAwMCwgRGF2ZSBDaGlu bmVyIHdyb3RlOgo+ID4gLS0tIGEvZnMvc3VwZXIuYwo+ID4gKysrIGIvZnMvc3VwZXIuYwo+ID4g QEAgLTM3LDYgKzM3LDUwIEBACj4gPiAgTElTVF9IRUFEKHN1cGVyX2Jsb2Nrcyk7Cj4gPiAgREVG SU5FX1NQSU5MT0NLKHNiX2xvY2spOwo+ID4gIAo+ID4gK3N0YXRpYyBpbnQgcHJ1bmVfc3VwZXIo c3RydWN0IHNocmlua2VyICpzaHJpbmssIGludCBucl90b19zY2FuLCBnZnBfdCBnZnBfbWFzaykK PiA+ICt7Cj4gPiArCXN0cnVjdCBzdXBlcl9ibG9jayAqc2I7Cj4gPiArCWludCBjb3VudDsKPiA+ ICsKPiA+ICsJc2IgPSBjb250YWluZXJfb2Yoc2hyaW5rLCBzdHJ1Y3Qgc3VwZXJfYmxvY2ssIHNf c2hyaW5rKTsKPiA+ICsKPiA+ICsJLyoKPiA+ICsJICogRGVhZGxvY2sgYXZvaWRhbmNlLiAgV2Ug bWF5IGhvbGQgdmFyaW91cyBGUyBsb2NrcywgYW5kIHdlIGRvbid0IHdhbnQKPiA+ICsJICogdG8g cmVjdXJzZSBpbnRvIHRoZSBGUyB0aGF0IGNhbGxlZCB1cyBpbiBjbGVhcl9pbm9kZSgpIGFuZCBm cmllbmRzLi4KPiA+ICsJICovCj4gPiArCWlmICghKGdmcF9tYXNrICYgX19HRlBfRlMpKQo+ID4g KwkJcmV0dXJuIC0xOwo+ID4gKwo+ID4gKwkvKgo+ID4gKwkgKiBpZiB3ZSBjYW4ndCBnZXQgdGhl IHVtb3VudCBsb2NrLCB0aGVuIHRoZXJlJ3Mgbm8gcG9pbnQgaGF2aW5nIHRoZQo+ID4gKwkgKiBz aHJpbmtlciB0cnkgYWdhaW4gYmVjYXVzZSB0aGUgc2IgaXMgYmVpbmcgdG9ybiBkb3duLgo+ID4g KwkgKi8KPiA+ICsJaWYgKCFkb3duX3JlYWRfdHJ5bG9jaygmc2ItPnNfdW1vdW50KSkKPiA+ICsJ CXJldHVybiAtMTsKPiA+ICsKPiA+ICsJaWYgKCFzYi0+c19yb290KSB7Cj4gPiArCQl1cF9yZWFk KCZzYi0+c191bW91bnQpOwo+ID4gKwkJcmV0dXJuIC0xOwo+ID4gKwl9Cj4gPiArCj4gPiArCWlm IChucl90b19zY2FuKSB7Cj4gPiArCQkvKiBwcm9wb3J0aW9uIHRoZSBzY2FuIGJldHdlZW4gdGhl IHR3byBjYWNoZdGVICovCj4gPiArCQlpbnQgdG90YWw7Cj4gPiArCj4gPiArCQl0b3RhbCA9IHNi LT5zX25yX2RlbnRyeV91bnVzZWQgKyBzYi0+c19ucl9pbm9kZXNfdW51c2VkICsgMTsKPiA+ICsJ CWNvdW50ID0gKG5yX3RvX3NjYW4gKiBzYi0+c19ucl9kZW50cnlfdW51c2VkKSAvIHRvdGFsOwo+ ID4gKwo+ID4gKwkJLyogcHJ1bmUgZGNhY2hlIGZpcnN0IGFzIGljYWNoZSBpcyBwaW5uZWQgYnkg aXQgKi8KPiA+ICsJCXBydW5lX2RjYWNoZV9zYihzYiwgY291bnQpOwo+ID4gKwkJcHJ1bmVfaWNh Y2hlX3NiKHNiLCBucl90b19zY2FuIC0gY291bnQpOwo+IAo+IEhtbSwgYW4gaW50ZXJlc3Rpbmcg ZHluYW1pYyB0aGF0IHlvdSd2ZSBjaGFuZ2VkIGlzIHRoYXQgcHJldmlvdXNseQo+IHdlJ2Qgc2Nh biBkY2FjaGUgTFJVIHByb3BvcnRpb25hdGVseSB0byBwYWdlY2FjaGUsIGFuZCB0aGVuIHNjYW4K PiBpbm9kZSBMUlUgaW4gcHJvcG9ydGlvbiB0byB0aGUgY3VycmVudCBudW1iZXIgb2YgdW51c2Vk IGlub2Rlcy4KPiAKPiBCdXQgd2UgY2FuIHRoaW5rIG9mIGlub2RlcyB0aGF0IGFyZSBvbmx5IGlu IHVzZSBieSB1bnVzZWQgKGFuZCBhZ2VkKQo+IGRlbnRyaWVzIGFzIGVmZmVjdGl2ZWx5IHVudXNl ZCB0aGVtc2VsdmVzLiBTbyB0aGlzIHNlcXVlbmNlIHVuZGVyCj4gZXN0aW1hdGVzIGhvdyBtYW55 IGlub2RlcyB0byBzY2FuLiBUaGlzIGNvdWxkIGJpYXMgcHJlc3N1cmUgYWdhaW5zdAo+IGRjYWNo ZSBJJ2QgdGhpbmssIGVzcGVjaWFsbHkgY29uc2lkZXJpbmcgaW5vZGVzIGFyZSBmYXIgbGFyZ2Vy IHRoYW4KPiBkZW50cmllcy4gTWF5YmUgcmVxdWlyZSAyIHBhc3NlcyB0byBnZXQgdGhlIGlub2Rl cyB1bnVzZWQgaW50aGUKPiBmaXJzdCBwYXNzLgoKSXQncyBzZWxmLWJhbGFuY2luZyAtIGl0IHRy ZW5kcyB0b3dhcmRzIGFuIGVxdWFsIG51bWJlciBvZiB1bnVzZWQKZGVudHJpZXMgYW5kIGlub2Rl cyBpbiB0aGUgY2FjaGVzLiBZZXMsIGl0IHdpbGwgdGVhciBkb3duIG1vcmUKZGVudHJpZXMgYXQg Zmlyc3QsIGJ1dCB3ZSBuZWVkIHRvIGRvIHRoYXQgdG8gYmUgYWJsZSB0byByZWNsYWltCmlub2Rl cy4g4oSrcyByZWNsYWltIHByb2dyZXNzZXMgdGhlIHByb3BvdGlvbiBvZiBpbm9kZXMgaW5jcmVh c2VzLCBzbwp0aGUgYW1vdW50IG9mIGlub2RlcyByZWNsYWltZWQgaW5jcmVhc2VzLiAKCkJhc2lj YWxseSB0aGlzIGlzIGEgcmVjb2duaXRpb24gdGhhdCB0aGUgaW1wb3J0YW50IGNhY2hlIGZvcgph dm9pZGluZyBJTyBpcyB0aGUgaW5vZGUgY2FjaGUsIG5vdCBoZSBkZW50cnkgY2FjaGUuIE9uY2Ug dGhlIGlub2RlCmNhY2hlIGlzIGZyZWVkIHRoYXQgd2UgbmVlZCB0byBkbyBJTyB0byByZXBvcHVs YXRlIGl0LCBidXQKcmVidWlsZGluZyBkZW50cmllcyBmcm9tdGVoIGlub2RlIGNhY2hlIG9ubHkg Y29zdHMgQ1BVIHRpbWUuIEhlbmNlCnVuZGVyIGxpZ2h0IHJlY2xhaW0sIGlub2RlcyBhcmUgbW9z dGx5IGxlZnQgaW4gY2FjaGUgYnV0IHdlIGZyZWUgdXAKbWVtb3J5IHRoYXQgb25seSBjb3N0cyBD UFUgdG8gcmVidWlsZC4gVW5kZXIgaGVhdnksIHN1c3RhaW5lZApyZWNsYWltLCB3ZSB0cmVuZCB0 b3dhcmRzIGZyZWVpbmcgZXF1YWwgYW1vdW50cyBvZiBvYmplY3RzIGZyb20gYm90aApjYWNoZXMu CgpUaGlzIGlzIHByZXR0eSBtdWNoIHdoYXQgdGhlIGN1cnJlbnQgY29kZSBhdHRlbXB0cyB0byBk byAtIGZyZWUgYQpsb3Qgb2YgZGVudHJpZXMsIHRoZW4gZnJlZSBhIHNtYWxsZXIgYW1vdW50IG9m IHRoZSBpbm9kZXMgdGhhdCB3ZXJlCnVzZWQgYnkgdGhlIGZyZWVkIGRlbnRyaWVzLiBPbmNlIGFn YWluIGl0IGlzIGEgZGlyZWN0IGVuY29kaW5nIG9mCndoYXQgaXMgY3VycmVudGx5IGFuIGltcGxp Y2l0IGRlc2lnbiBmZWF0dXJlIC0gaXQgbWFrZXMgaXQgKm9idmlvdXMqCmhvdyB3ZSBhcmUgdHJ5 aW5nIHRvIGJhbGFuY2UgdGhlIGNhY2hlcy4KCkFub3RoZXIgcmVhc29uIGZvciB0aGlzIGlzIHRo YXQgdGhlIGNhbGN1bGF0aW9uIGNoYW5nZXMgYWdhaW4gdG8KYWxsb3cgZmlsZXN5c3RlbSBjYWNo ZXMgdG8gbW9kaXkgdGhpcyBwcm9wb3J0aW9uaW5nIGluIHRoZSBuZXh0CnBhdGNoLi4uLgoKRldJ VywgdGhpcyBhbHNvIG1ha2VzIHdvcmtsb2FkcyB0aGF0IGdlbmVyYXRlIGh1bmRyZWRzIG9mIHRo b3VzYW5kcwpvZiBuZXZlci10by1iZS11c2VkIGFnYWluIG5lZ2F0aXZlIGRlbnRyaWVzIGZyZWUg ZGNhY2hlIG1lbW9yeSByZWFsbHkKcXVpY2tseSBvbiBtZW1vcnkgcHJlc3N1cmUuLi4KCj4gUGFy dCBvZiB0aGUgcHJvYmxlbSBpcyB0aGUgZnVubnkgc2hyaW5rZXIgQVBJLgo+IAo+IFRoZSByaWdo dCB3YXkgdG8gZG8gaXQgaXMgdG8gY2hhbmdlIHRoZSBzaHJpbmtlciBBUEkgc28gdGhhdCBpdCBw YXNzZXMKPiBkb3duIHRoZSBscnVfcGFnZXMgYW5kIHNjYW5uZWQgaW50byB0aGUgY2FsbGJhY2su IEZyb20gdGhlcmUsIHRoZQo+IHNocmlua2VycyBjYW4gY2FsY3VsYXRlIHRoZSBhcHByb3ByaWF0 ZSByYXRpbyBvZiBvYmplY3RzIHRvIHNjYW4uCj4gTm8gbmVlZCBmb3IgMi1jYWxsIHNjaGVtZSwg bm8gbmVlZCBmb3Igc2hyaW5rZXItPnNlZWtzLCBhbmQgdGhlCj4gYWJpbGl0eSB0byBjYWxjdWxh dGUgYW4gYXBwcm9wcmlhdGUgcmF0aW8gZmlyc3QgZm9yIGRjYWNoZSwgYW5kICp0aGVuKgo+IGZv ciBpY2FjaGUuCgpNeSBvbmx5IGNvbmNlcm4gYWJvdXQgdGhpcyBpcyB0aGF0IGV4cG9zZXMgdGhl IGlubmVyIHdvcmtpbmdzIG9mIHRoZQpzaHJpbmtlciBhbmQgbW0gc3Vic3lzdGVtIHRvIGNvZGUg dGhhdCBzaW1wbHkgZG9lc24ndCBuZWVkIHRvIGtub3cKYWJvdXQgaXQuCgoKPiBBIGhlbHBlciBv ZiBjb3Vyc2UgY2FuIGRvIHRoZSBjYWxjdWxhdGlvbiAoY29uc2lkZXJpbmcgdGhhdCBldmVyeQo+ IGRyaXZlciBhbmQgdGhlaXIgZG9nIHdpbGwgZG8gdGhlIHdyb25nIHRoaW5nIGlmIHdlIGxldCB0 aGVtIDopKS4KPiAKPiB1bnNpZ25lZCBsb25nIHNocmlua2VyX3NjYW4odW5zaWduZWQgbG9uZyBs cnVfcGFnZXMsCj4gCQkJdW5zaWduZWQgbG9uZyBscnVfc2Nhbm5lZCwKPiAJCQl1bnNpZ25lZCBs b25nIG5yX29iamVjdHMsCj4gCQkJdW5zaWduZWQgbG9uZyBzY2FuX3JhdGlvKQo+IHsKPiAJdW5z aWduZWQgbG9uZyBsb25nIHRtcCA9IG5yX29iamVjdHM7Cj4gCj4gCXRtcCAqPSBscnVfc2Nhbm5l ZCAqIDEwMDsKPiAJZG9fZGl2KHRtcCwgKGxydV9wYWdlcyAqIHNjYW5fcmF0aW8pICsgMSk7Cj4g Cj4gCXJldHVybiAodW5zaWduZWQgbG9uZyl0bXA7Cj4gfQo+IAo+IFRoZW4gdGhlIHNocmlua2Vy IGNhbGxiYWNrIHdpbGwgZ286Cj4gCXNiLT5zX25yX2RlbnRyeV9zY2FuICs9IHNocmlua2VyX3Nj YW4obHJ1X3BhZ2VzLCBscnVfc2Nhbm5lZCwKPiAJCQkJc2ItPnNfbnJfZGVudHJ5X3VudXNlZCwK PiAJCQkJdmZzX2NhY2hlX3ByZXNzdXJlICogU0VFS1NfUEVSX0RFTlRSWSk7Cj4gCWlmIChzYi0+ c19ucl9kZW50cnlfc2NhbiA+IFNIUklOS19CQVRDSCkKPiAJCXBydW5lX2RjYWNoZSgpCj4gCj4g CXNiLT5zX25yX2lub2RlX3NjYW4gKz0gc2hyaW5rZXJfc2NhbihscnVfcGFnZXMsIGxydV9zY2Fu bmVkLAo+IAkJCQlzYi0+c19ucl9pbm9kZXNfdW51c2VkLAo+IAkJCQl2ZnNfY2FjaGVfcHJlc3N1 cmUgKiBTRUVLU19QRVJfSU5PREUpOwo+IAkuLi4KPiAKPiBXaGF0IGRvIHlvdSB0aGluayBvZiB0 aGF0PyBTZWVpbmcgYXMgd2UncmUgY2hhbmdpbmcgdGhlIHNocmlua2VyIEFQSQo+IGFueXdheSwg SSdkIHRoaW5rIGl0IGlzIGhpZ2ggdGltZSB0byBkbyBzb210aGluZyBsaWtlIHRoaXMuCgpJZ25v cmluZyB0aGUgZGNhY2hlL2ljYWNoZSByZWNsYWltIHJhdGlvIGlzc3VlcywgSSdkIHByZWZlciBh IHR3bwpjYWxsIEFQSSB0aGF0IG1hdGNoZXMgdGhlIGN1cnJlbnQgYmVoYXZpb3VyLCBsZWF2aW5n IHRoZSBjYWNsdWxhdGlvbgpvZiBob3cgbXVjaCB0byByZWNsYWltIGluIHNocmlua19zbGFiKCku IEVuY29kaW5nIGl0IHRoaXMgd2F5IG1ha2VzCml0IG1vcmUgZGlmZmljdWx0IHRvIGNoYW5nZSB0 aGUgaGlnaCBsZXZlbCBiZWhhdmlvdXIgZS5nLiBpZiB3ZSB3YW50CnRvIG1vZGlmeSB0aGUgYW1v dW50IG9mIHNsYWIgcmVjbGFpbSBiYXNlZCBvbiByZWNsYWltIHByaW9yaXR5LCB3ZSdkCmhhdmUg dG8gY2FobmdlIGV2ZXJ5IHNocmlua2VyIGluc3RlYWQgb2YganVzdCBzaHJpbmtfc2xhYigpLgoK Q2hlZXJzLAoKRGF2ZS4KLS0gCkRhdmUgQ2hpbm5lcgpkYXZpZEBmcm9tb3JiaXQuY29tCgpfX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwp4ZnMgbWFpbGluZyBs aXN0Cnhmc0Bvc3Muc2dpLmNvbQpodHRwOi8vb3NzLnNnaS5jb20vbWFpbG1hbi9saXN0aW5mby94 ZnMK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932632Ab0E0Wku (ORCPT ); Thu, 27 May 2010 18:40:50 -0400 Received: from bld-mail15.adl6.internode.on.net ([150.101.137.100]:43520 "EHLO mail.internode.on.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932511Ab0E0Wkq (ORCPT ); Thu, 27 May 2010 18:40:46 -0400 Date: Fri, 28 May 2010 08:40:34 +1000 From: Dave Chinner To: Nick Piggin Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, xfs@oss.sgi.com Subject: Re: [PATCH 3/5] superblock: introduce per-sb cache shrinker infrastructure Message-ID: <20100527224034.GO12087@dastard> References: <1274777588-21494-1-git-send-email-david@fromorbit.com> <1274777588-21494-4-git-send-email-david@fromorbit.com> <20100527063523.GJ22536@laptop> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20100527063523.GJ22536@laptop> User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 27, 2010 at 04:35:23PM +1000, Nick Piggin wrote: > On Tue, May 25, 2010 at 06:53:06PM +1000, Dave Chinner wrote: > > --- a/fs/super.c > > +++ b/fs/super.c > > @@ -37,6 +37,50 @@ > > LIST_HEAD(super_blocks); > > DEFINE_SPINLOCK(sb_lock); > > > > +static int prune_super(struct shrinker *shrink, int nr_to_scan, gfp_t gfp_mask) > > +{ > > + struct super_block *sb; > > + int count; > > + > > + sb = container_of(shrink, struct super_block, s_shrink); > > + > > + /* > > + * Deadlock avoidance. We may hold various FS locks, and we don't want > > + * to recurse into the FS that called us in clear_inode() and friends.. > > + */ > > + if (!(gfp_mask & __GFP_FS)) > > + return -1; > > + > > + /* > > + * if we can't get the umount lock, then there's no point having the > > + * shrinker try again because the sb is being torn down. > > + */ > > + if (!down_read_trylock(&sb->s_umount)) > > + return -1; > > + > > + if (!sb->s_root) { > > + up_read(&sb->s_umount); > > + return -1; > > + } > > + > > + if (nr_to_scan) { > > + /* proportion the scan between the two cacheѕ */ > > + int total; > > + > > + total = sb->s_nr_dentry_unused + sb->s_nr_inodes_unused + 1; > > + count = (nr_to_scan * sb->s_nr_dentry_unused) / total; > > + > > + /* prune dcache first as icache is pinned by it */ > > + prune_dcache_sb(sb, count); > > + prune_icache_sb(sb, nr_to_scan - count); > > Hmm, an interesting dynamic that you've changed is that previously > we'd scan dcache LRU proportionately to pagecache, and then scan > inode LRU in proportion to the current number of unused inodes. > > But we can think of inodes that are only in use by unused (and aged) > dentries as effectively unused themselves. So this sequence under > estimates how many inodes to scan. This could bias pressure against > dcache I'd think, especially considering inodes are far larger than > dentries. Maybe require 2 passes to get the inodes unused inthe > first pass. It's self-balancing - it trends towards an equal number of unused dentries and inodes in the caches. Yes, it will tear down more dentries at first, but we need to do that to be able to reclaim inodes. Ås reclaim progresses the propotion of inodes increases, so the amount of inodes reclaimed increases. Basically this is a recognition that the important cache for avoiding IO is the inode cache, not he dentry cache. Once the inode cache is freed that we need to do IO to repopulate it, but rebuilding dentries fromteh inode cache only costs CPU time. Hence under light reclaim, inodes are mostly left in cache but we free up memory that only costs CPU to rebuild. Under heavy, sustained reclaim, we trend towards freeing equal amounts of objects from both caches. This is pretty much what the current code attempts to do - free a lot of dentries, then free a smaller amount of the inodes that were used by the freed dentries. Once again it is a direct encoding of what is currently an implicit design feature - it makes it *obvious* how we are trying to balance the caches. Another reason for this is that the calculation changes again to allow filesystem caches to modiy this proportioning in the next patch.... FWIW, this also makes workloads that generate hundreds of thousands of never-to-be-used again negative dentries free dcache memory really quickly on memory pressure... > Part of the problem is the funny shrinker API. > > The right way to do it is to change the shrinker API so that it passes > down the lru_pages and scanned into the callback. From there, the > shrinkers can calculate the appropriate ratio of objects to scan. > No need for 2-call scheme, no need for shrinker->seeks, and the > ability to calculate an appropriate ratio first for dcache, and *then* > for icache. My only concern about this is that exposes the inner workings of the shrinker and mm subsystem to code that simply doesn't need to know about it. > A helper of course can do the calculation (considering that every > driver and their dog will do the wrong thing if we let them :)). > > unsigned long shrinker_scan(unsigned long lru_pages, > unsigned long lru_scanned, > unsigned long nr_objects, > unsigned long scan_ratio) > { > unsigned long long tmp = nr_objects; > > tmp *= lru_scanned * 100; > do_div(tmp, (lru_pages * scan_ratio) + 1); > > return (unsigned long)tmp; > } > > Then the shrinker callback will go: > sb->s_nr_dentry_scan += shrinker_scan(lru_pages, lru_scanned, > sb->s_nr_dentry_unused, > vfs_cache_pressure * SEEKS_PER_DENTRY); > if (sb->s_nr_dentry_scan > SHRINK_BATCH) > prune_dcache() > > sb->s_nr_inode_scan += shrinker_scan(lru_pages, lru_scanned, > sb->s_nr_inodes_unused, > vfs_cache_pressure * SEEKS_PER_INODE); > ... > > What do you think of that? Seeing as we're changing the shrinker API > anyway, I'd think it is high time to do somthing like this. Ignoring the dcache/icache reclaim ratio issues, I'd prefer a two call API that matches the current behaviour, leaving the caclulation of how much to reclaim in shrink_slab(). Encoding it this way makes it more difficult to change the high level behaviour e.g. if we want to modify the amount of slab reclaim based on reclaim priority, we'd have to cahnge every shrinker instead of just shrink_slab(). Cheers, Dave. -- Dave Chinner david@fromorbit.com From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Chinner Subject: Re: [PATCH 3/5] superblock: introduce per-sb cache shrinker infrastructure Date: Fri, 28 May 2010 08:40:34 +1000 Message-ID: <20100527224034.GO12087@dastard> References: <1274777588-21494-1-git-send-email-david@fromorbit.com> <1274777588-21494-4-git-send-email-david@fromorbit.com> <20100527063523.GJ22536@laptop> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, xfs@oss.sgi.com To: Nick Piggin Return-path: Content-Disposition: inline In-Reply-To: <20100527063523.GJ22536@laptop> Sender: owner-linux-mm@kvack.org List-Id: linux-fsdevel.vger.kernel.org On Thu, May 27, 2010 at 04:35:23PM +1000, Nick Piggin wrote: > On Tue, May 25, 2010 at 06:53:06PM +1000, Dave Chinner wrote: > > --- a/fs/super.c > > +++ b/fs/super.c > > @@ -37,6 +37,50 @@ > > LIST_HEAD(super_blocks); > > DEFINE_SPINLOCK(sb_lock); > > =20 > > +static int prune_super(struct shrinker *shrink, int nr_to_scan, gfp_= t gfp_mask) > > +{ > > + struct super_block *sb; > > + int count; > > + > > + sb =3D container_of(shrink, struct super_block, s_shrink); > > + > > + /* > > + * Deadlock avoidance. We may hold various FS locks, and we don't = want > > + * to recurse into the FS that called us in clear_inode() and frien= ds.. > > + */ > > + if (!(gfp_mask & __GFP_FS)) > > + return -1; > > + > > + /* > > + * if we can't get the umount lock, then there's no point having th= e > > + * shrinker try again because the sb is being torn down. > > + */ > > + if (!down_read_trylock(&sb->s_umount)) > > + return -1; > > + > > + if (!sb->s_root) { > > + up_read(&sb->s_umount); > > + return -1; > > + } > > + > > + if (nr_to_scan) { > > + /* proportion the scan between the two cache=D1=95 */ > > + int total; > > + > > + total =3D sb->s_nr_dentry_unused + sb->s_nr_inodes_unused + 1; > > + count =3D (nr_to_scan * sb->s_nr_dentry_unused) / total; > > + > > + /* prune dcache first as icache is pinned by it */ > > + prune_dcache_sb(sb, count); > > + prune_icache_sb(sb, nr_to_scan - count); >=20 > Hmm, an interesting dynamic that you've changed is that previously > we'd scan dcache LRU proportionately to pagecache, and then scan > inode LRU in proportion to the current number of unused inodes. >=20 > But we can think of inodes that are only in use by unused (and aged) > dentries as effectively unused themselves. So this sequence under > estimates how many inodes to scan. This could bias pressure against > dcache I'd think, especially considering inodes are far larger than > dentries. Maybe require 2 passes to get the inodes unused inthe > first pass. It's self-balancing - it trends towards an equal number of unused dentries and inodes in the caches. Yes, it will tear down more dentries at first, but we need to do that to be able to reclaim inodes. =E2=84=ABs reclaim progresses the propotion of inodes increases, = so the amount of inodes reclaimed increases.=20 Basically this is a recognition that the important cache for avoiding IO is the inode cache, not he dentry cache. Once the inode cache is freed that we need to do IO to repopulate it, but rebuilding dentries fromteh inode cache only costs CPU time. Hence under light reclaim, inodes are mostly left in cache but we free up memory that only costs CPU to rebuild. Under heavy, sustained reclaim, we trend towards freeing equal amounts of objects from both caches. This is pretty much what the current code attempts to do - free a lot of dentries, then free a smaller amount of the inodes that were used by the freed dentries. Once again it is a direct encoding of what is currently an implicit design feature - it makes it *obvious* how we are trying to balance the caches. Another reason for this is that the calculation changes again to allow filesystem caches to modiy this proportioning in the next patch.... FWIW, this also makes workloads that generate hundreds of thousands of never-to-be-used again negative dentries free dcache memory really quickly on memory pressure... > Part of the problem is the funny shrinker API. >=20 > The right way to do it is to change the shrinker API so that it passes > down the lru_pages and scanned into the callback. From there, the > shrinkers can calculate the appropriate ratio of objects to scan. > No need for 2-call scheme, no need for shrinker->seeks, and the > ability to calculate an appropriate ratio first for dcache, and *then* > for icache. My only concern about this is that exposes the inner workings of the shrinker and mm subsystem to code that simply doesn't need to know about it. > A helper of course can do the calculation (considering that every > driver and their dog will do the wrong thing if we let them :)). >=20 > unsigned long shrinker_scan(unsigned long lru_pages, > unsigned long lru_scanned, > unsigned long nr_objects, > unsigned long scan_ratio) > { > unsigned long long tmp =3D nr_objects; >=20 > tmp *=3D lru_scanned * 100; > do_div(tmp, (lru_pages * scan_ratio) + 1); >=20 > return (unsigned long)tmp; > } >=20 > Then the shrinker callback will go: > sb->s_nr_dentry_scan +=3D shrinker_scan(lru_pages, lru_scanned, > sb->s_nr_dentry_unused, > vfs_cache_pressure * SEEKS_PER_DENTRY); > if (sb->s_nr_dentry_scan > SHRINK_BATCH) > prune_dcache() >=20 > sb->s_nr_inode_scan +=3D shrinker_scan(lru_pages, lru_scanned, > sb->s_nr_inodes_unused, > vfs_cache_pressure * SEEKS_PER_INODE); > ... >=20 > What do you think of that? Seeing as we're changing the shrinker API > anyway, I'd think it is high time to do somthing like this. Ignoring the dcache/icache reclaim ratio issues, I'd prefer a two call API that matches the current behaviour, leaving the caclulation of how much to reclaim in shrink_slab(). Encoding it this way makes it more difficult to change the high level behaviour e.g. if we want to modify the amount of slab reclaim based on reclaim priority, we'd have to cahnge every shrinker instead of just shrink_slab(). Cheers, Dave. --=20 Dave Chinner david@fromorbit.com -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail203.messagelabs.com (mail203.messagelabs.com [216.82.254.243]) by kanga.kvack.org (Postfix) with SMTP id B21B86B01C3 for ; Thu, 27 May 2010 18:40:43 -0400 (EDT) Date: Fri, 28 May 2010 08:40:34 +1000 From: Dave Chinner Subject: Re: [PATCH 3/5] superblock: introduce per-sb cache shrinker infrastructure Message-ID: <20100527224034.GO12087@dastard> References: <1274777588-21494-1-git-send-email-david@fromorbit.com> <1274777588-21494-4-git-send-email-david@fromorbit.com> <20100527063523.GJ22536@laptop> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20100527063523.GJ22536@laptop> Sender: owner-linux-mm@kvack.org To: Nick Piggin Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, xfs@oss.sgi.com List-ID: On Thu, May 27, 2010 at 04:35:23PM +1000, Nick Piggin wrote: > On Tue, May 25, 2010 at 06:53:06PM +1000, Dave Chinner wrote: > > --- a/fs/super.c > > +++ b/fs/super.c > > @@ -37,6 +37,50 @@ > > LIST_HEAD(super_blocks); > > DEFINE_SPINLOCK(sb_lock); > > > > +static int prune_super(struct shrinker *shrink, int nr_to_scan, gfp_t gfp_mask) > > +{ > > + struct super_block *sb; > > + int count; > > + > > + sb = container_of(shrink, struct super_block, s_shrink); > > + > > + /* > > + * Deadlock avoidance. We may hold various FS locks, and we don't want > > + * to recurse into the FS that called us in clear_inode() and friends.. > > + */ > > + if (!(gfp_mask & __GFP_FS)) > > + return -1; > > + > > + /* > > + * if we can't get the umount lock, then there's no point having the > > + * shrinker try again because the sb is being torn down. > > + */ > > + if (!down_read_trylock(&sb->s_umount)) > > + return -1; > > + > > + if (!sb->s_root) { > > + up_read(&sb->s_umount); > > + return -1; > > + } > > + > > + if (nr_to_scan) { > > + /* proportion the scan between the two cacheN? */ > > + int total; > > + > > + total = sb->s_nr_dentry_unused + sb->s_nr_inodes_unused + 1; > > + count = (nr_to_scan * sb->s_nr_dentry_unused) / total; > > + > > + /* prune dcache first as icache is pinned by it */ > > + prune_dcache_sb(sb, count); > > + prune_icache_sb(sb, nr_to_scan - count); > > Hmm, an interesting dynamic that you've changed is that previously > we'd scan dcache LRU proportionately to pagecache, and then scan > inode LRU in proportion to the current number of unused inodes. > > But we can think of inodes that are only in use by unused (and aged) > dentries as effectively unused themselves. So this sequence under > estimates how many inodes to scan. This could bias pressure against > dcache I'd think, especially considering inodes are far larger than > dentries. Maybe require 2 passes to get the inodes unused inthe > first pass. It's self-balancing - it trends towards an equal number of unused dentries and inodes in the caches. Yes, it will tear down more dentries at first, but we need to do that to be able to reclaim inodes. a?< Part of the problem is the funny shrinker API. > > The right way to do it is to change the shrinker API so that it passes > down the lru_pages and scanned into the callback. From there, the > shrinkers can calculate the appropriate ratio of objects to scan. > No need for 2-call scheme, no need for shrinker->seeks, and the > ability to calculate an appropriate ratio first for dcache, and *then* > for icache. My only concern about this is that exposes the inner workings of the shrinker and mm subsystem to code that simply doesn't need to know about it. > A helper of course can do the calculation (considering that every > driver and their dog will do the wrong thing if we let them :)). > > unsigned long shrinker_scan(unsigned long lru_pages, > unsigned long lru_scanned, > unsigned long nr_objects, > unsigned long scan_ratio) > { > unsigned long long tmp = nr_objects; > > tmp *= lru_scanned * 100; > do_div(tmp, (lru_pages * scan_ratio) + 1); > > return (unsigned long)tmp; > } > > Then the shrinker callback will go: > sb->s_nr_dentry_scan += shrinker_scan(lru_pages, lru_scanned, > sb->s_nr_dentry_unused, > vfs_cache_pressure * SEEKS_PER_DENTRY); > if (sb->s_nr_dentry_scan > SHRINK_BATCH) > prune_dcache() > > sb->s_nr_inode_scan += shrinker_scan(lru_pages, lru_scanned, > sb->s_nr_inodes_unused, > vfs_cache_pressure * SEEKS_PER_INODE); > ... > > What do you think of that? Seeing as we're changing the shrinker API > anyway, I'd think it is high time to do somthing like this. Ignoring the dcache/icache reclaim ratio issues, I'd prefer a two call API that matches the current behaviour, leaving the caclulation of how much to reclaim in shrink_slab(). Encoding it this way makes it more difficult to change the high level behaviour e.g. if we want to modify the amount of slab reclaim based on reclaim priority, we'd have to cahnge every shrinker instead of just shrink_slab(). Cheers, Dave. -- Dave Chinner david@fromorbit.com -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org