From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 31F20C47097 for ; Thu, 3 Jun 2021 22:02:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 18B0E613E9 for ; Thu, 3 Jun 2021 22:02:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230342AbhFCWEi (ORCPT ); Thu, 3 Jun 2021 18:04:38 -0400 Received: from out02.mta.xmission.com ([166.70.13.232]:39304 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229576AbhFCWEh (ORCPT ); Thu, 3 Jun 2021 18:04:37 -0400 Received: from in01.mta.xmission.com ([166.70.13.51]) by out02.mta.xmission.com with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1lovQR-009J4M-1W; Thu, 03 Jun 2021 16:02:51 -0600 Received: from ip68-227-160-95.om.om.cox.net ([68.227.160.95] helo=email.xmission.com) by in01.mta.xmission.com with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1lovQB-0016WB-Cw; Thu, 03 Jun 2021 16:02:50 -0600 From: ebiederm@xmission.com (Eric W. Biederman) To: Miklos Szeredi Cc: Ian Kent , Greg Kroah-Hartman , Tejun Heo , Eric Sandeen , Fox Chen , Brice Goglin , Al Viro , Rick Lindsley , David Howells , Marcelo Tosatti , linux-fsdevel , Kernel Mailing List References: <162218354775.34379.5629941272050849549.stgit@web.messagingengine.com> <162218364554.34379.636306635794792903.stgit@web.messagingengine.com> <87czt2q2pl.fsf@disp2133> Date: Thu, 03 Jun 2021 17:02:28 -0500 In-Reply-To: (Miklos Szeredi's message of "Thu, 3 Jun 2021 20:06:38 +0200") Message-ID: <87y2bqli8b.fsf@disp2133> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1lovQB-0016WB-Cw;;;mid=<87y2bqli8b.fsf@disp2133>;;;hst=in01.mta.xmission.com;;;ip=68.227.160.95;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX1+G9lB5nC8jUvCdCdERpz90oQIIf4sP1Xk= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [REPOST PATCH v4 2/5] kernfs: use VFS negative dentry caching X-SA-Exim-Version: 4.2.1 (built Sat, 08 Feb 2020 21:53:50 +0000) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Miklos Szeredi writes: > On Thu, 3 Jun 2021 at 19:26, Eric W. Biederman wrote: >> >> Ian Kent writes: >> >> > If there are many lookups for non-existent paths these negative lookups >> > can lead to a lot of overhead during path walks. >> > >> > The VFS allows dentries to be created as negative and hashed, and caches >> > them so they can be used to reduce the fairly high overhead alloc/free >> > cycle that occurs during these lookups. >> > >> > Signed-off-by: Ian Kent >> > --- >> > fs/kernfs/dir.c | 55 +++++++++++++++++++++++++++++++++---------------------- >> > 1 file changed, 33 insertions(+), 22 deletions(-) >> > >> > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c >> > index 4c69e2af82dac..5151c712f06f5 100644 >> > --- a/fs/kernfs/dir.c >> > +++ b/fs/kernfs/dir.c >> > @@ -1037,12 +1037,33 @@ static int kernfs_dop_revalidate(struct dentry *dentry, unsigned int flags) >> > if (flags & LOOKUP_RCU) >> > return -ECHILD; >> > >> > - /* Always perform fresh lookup for negatives */ >> > - if (d_really_is_negative(dentry)) >> > - goto out_bad_unlocked; >> > + mutex_lock(&kernfs_mutex); >> > >> > kn = kernfs_dentry_node(dentry); >> > - mutex_lock(&kernfs_mutex); >> >> Why bring kernfs_dentry_node inside the mutex? >> >> The inode lock of the parent should protect negative to positive >> transitions not the kernfs_mutex. So moving the code inside >> the mutex looks unnecessary and confusing. > > Except that d_revalidate() may or may not be called with parent lock > held. I grant that this works because kernfs_io_lookup today holds kernfs_mutex over d_splice_alias. The problem is that the kernfs_mutex only should be protecting the kernfs data structures not the vfs data structures. Reading through the code history that looks like a hold over from when sysfs lived in the dcache before it was reimplemented as a distributed file system. So it was probably a complete over sight and something that did not matter. The big problem is that if the code starts depending upon the kernfs_mutex (or the kernfs_rwsem) to provide semantics the rest of the filesystems does not the code will diverge from the rest of the filesystems and maintenance will become much more difficult. Diverging from other filesystems and becoming a maintenance pain has already been seen once in the life of sysfs and I don't think we want to go back there. Further extending the scope of lock, when the problem is that the locking is causing problems seems like the opposite of the direction we want the code to grow. I really suspect all we want kernfs_dop_revalidate doing for negative dentries is something as simple as comparing the timestamp of the negative dentry to the timestamp of the parent dentry, and if the timestamp has changed perform the lookup. That is roughly what nfs does today with negative dentries. The dentry cache will always lag the kernfs_node data structures, and that is fundamental. We should take advantage of that to make the code as simple and as fast as we can not to perform lots of work that creates overhead. Plus the kernfs data structures should not change much so I expect there will be effectively 0 penalty in always performing the lookup of a negative dentry when the directory itself has changed. Eric