From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 30 Aug 2018 11:43:04 +1000 From: Dave Chinner To: Waiman Long Cc: Alexander Viro , Jonathan Corbet , linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-doc@vger.kernel.org, "Luis R. Rodriguez" , Kees Cook , Linus Torvalds , Jan Kara , "Paul E. McKenney" , Andrew Morton , Ingo Molnar , Miklos Szeredi , Matthew Wilcox , Larry Woodman , James Bottomley , "Wangkai (Kevin C)" , Michal Hocko Subject: Re: [PATCH 1/2] fs/dcache: Track & report number of negative dentries Message-ID: <20180830014304.GD5631@dastard> References: <1535476780-5773-1-git-send-email-longman@redhat.com> <1535476780-5773-2-git-send-email-longman@redhat.com> <20180829001153.GD1572@dastard> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: owner-linux-mm@kvack.org List-ID: On Wed, Aug 29, 2018 at 01:11:08PM -0400, Waiman Long wrote: > On 08/28/2018 08:11 PM, Dave Chinner wrote: > > On Tue, Aug 28, 2018 at 01:19:39PM -0400, Waiman Long wrote: > >> The current dentry number tracking code doesn't distinguish between > >> positive & negative dentries. It just reports the total number of > >> dentries in the LRU lists. > >> > >> As excessive number of negative dentries can have an impact on system > >> performance, it will be wise to track the number of positive and > >> negative dentries separately. > >> > >> This patch adds tracking for the total number of negative dentries in > >> the system LRU lists and reports it in the /proc/sys/fs/dentry-state > >> file. The number, however, does not include negative dentries that are > >> in flight but not in the LRU yet. > >> > >> The number of positive dentries in the LRU lists can be roughly found > >> by subtracting the number of negative dentries from the total. > >> > >> Signed-off-by: Waiman Long > >> --- > >> Documentation/sysctl/fs.txt | 19 +++++++++++++------ > >> fs/dcache.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > >> include/linux/dcache.h | 7 ++++--- > >> 3 files changed, 62 insertions(+), 9 deletions(-) > >> > >> diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt > >> index 819caf8..118bb93 100644 > >> --- a/Documentation/sysctl/fs.txt > >> +++ b/Documentation/sysctl/fs.txt > >> @@ -63,19 +63,26 @@ struct { > >> int nr_unused; > >> int age_limit; /* age in seconds */ > >> int want_pages; /* pages requested by system */ > >> - int dummy[2]; > >> + int nr_negative; /* # of unused negative dentries */ > >> + int dummy; > >> } dentry_stat = {0, 0, 45, 0,}; > > That's not a backwards compatible ABI change. Those dummy fields > > used to represent some metric we no longer calculate, and there are > > probably still monitoring apps out there that think they still have > > the old meaning. i.e. they are still visible to userspace: > > > > $ cat /proc/sys/fs/dentry-state > > 83090 67661 45 0 0 0 > > $ > > > > IOWs, you can add new fields for new metrics to the end of the > > structure, but you can't re-use existing fields even if they > > aren't calculated anymore. > > > > [....] > > I looked up the git history and the state of the dentry_stat structure > hadn't changed since it was first put into git in 2.6.12-rc2 on Apr 16, > 2005. That was over 13 years ago. Even adding an extra argument can have > the potential of breaking old applications depending on how the parsing > code was written. I'm pretty we've had this discussion many times before w.r.t. /proc/self/mount* and other multi-field proc files. IIRC, The answer has always been that it's OK to extend lines with new fields as existing apps /should/ ignore them, but it's not OK to remove or redefine existing fields in the line because existing apps /will/ misinterpret what that field means. > Given that systems that are still using some very old tools are not > likely to upgrade to the latest kernel anyway. I don't see that as a big > problem. I don't think that matters when it comes to changing what information we expose in proc files. Cheers, Dave. -- Dave Chinner david@fromorbit.com