From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752642AbcGEUIc (ORCPT ); Tue, 5 Jul 2016 16:08:32 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:43470 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751105AbcGEUI2 (ORCPT ); Tue, 5 Jul 2016 16:08:28 -0400 Date: Tue, 5 Jul 2016 21:08:26 +0100 From: Al Viro To: Oleg Drokin Cc: Mailing List , "" Subject: Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes. Message-ID: <20160705200826.GP14480@ZenIV.linux.org.uk> References: <20160617042914.GD14480@ZenIV.linux.org.uk> <20160703062917.GG14480@ZenIV.linux.org.uk> <94F1587A-7AFC-4B48-A0FC-F4CE152F18CC@linuxhacker.ru> <20160705123110.GL14480@ZenIV.linux.org.uk> <20160705135149.GM14480@ZenIV.linux.org.uk> <6D633478-6B94-465E-84D7-C0BA59C5E5F5@linuxhacker.ru> <20160705180841.GO14480@ZenIV.linux.org.uk> <1C63FC85-848B-486F-8223-F5CEB5C39848@linuxhacker.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1C63FC85-848B-486F-8223-F5CEB5C39848@linuxhacker.ru> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jul 05, 2016 at 03:12:44PM -0400, Oleg Drokin wrote: > > When d_in_lookup was introduced, it was described as: > New primitives: d_in_lookup() (a predicate checking if dentry is in > the in-lookup state) and d_lookup_done() (tells the system that > we are done with lookup and if it's still marked as in-lookup, it > should cease to be such). > > I don't see where it mentions anything about exclusive vs parallel lookup > that probably led to some confusion. In the same commit: #define DCACHE_PAR_LOOKUP 0x10000000 /* being looked up (with parent locked shared) */ static inline int d_in_lookup(struct dentry *dentry) { return dentry->d_flags & DCACHE_PAR_LOOKUP; } Sure, we could use d_alloc_parallel() for all lookups, but it wouldn't buy us anything in terms of exclusion (parent locked exclusive => no other lookup attempts on that parent/name pair anyway) and it would cost extra searches both in the primary and in-lookup hashes, as well as insertions and removals from the latter. Hell knows - perhaps teaching d_alloc_parallel() that NULL wq => just allocate and mark in-lookup, without touching either primary or in-lookup hash (and scream bloody murder if the parent isn't locked exclusive) would be a good idea. A few places in fs/dcache.c would need to check for ->d_wait being non-NULL (__d_lookup_done(), __d_add() an __d_move()); We could use that for lookups when parent is locked exclusive; then d_in_lookup() would be true for *all* dentries passed to ->lookup(). I'll look into that, but it's obviously the next cycle fodder. > So with Lustre the dentry can be in three states, really: > > 1. hashed dentry that's all A-ok to reuse. > 2. hashed dentry that's NOT valid (dlm lock went away) - this is distinguished in d_compare by looking at a bit in the fs_data > 3. unhashed dentry ( I guess could be both valid and invalid lustre-wise). > > So the logic in ll_lookup_it_finish() (part of regular lookup) is this: > > If the dentry we have is not hashed - this is a new lookup, so we need to > call into ll_splice_alias() to see if there's a better dentry we need to > reuse that was already rejected by VFS before since we did not have necessary locks, > but we do have them now. > The comment at the top of ll_dcompare() explains why we don't just unhash the > dentry on lock-loss - that apparently leads to a loop around real_lookup for > real-contended dentries. > This is also why we cannot use d_splice_alias here - such cases are possible > for regular files and directories. > > Anyway, I guess additional point of confusion here is then why does > ll_lookup_it_finish() need to check for hashedness of the dentry since it's in > lookup, so we should be unhashed here. > I checked the commit history and this check was added along with atomic_open > support, so I imagine we can just move it up into ll_atomic_open and then your > change starts to make sense along with a few other things. So basically this } else if (!it_disposition(it, DISP_LOOKUP_NEG) && !it_disposition(it, DISP_OPEN_CREATE)) { /* With DISP_OPEN_CREATE dentry will be * instantiated in ll_create_it. */ LASSERT(!d_inode(*de)); d_instantiate(*de, inode); } is something we should do only for negative hashed fed to it by ->atomic_open(), and that - only if we have no O_CREAT in flags? Then, since 3/3 eliminates that case completely, we could just rip that else-if, along with the check for d_unhashed itself, making the call of ll_splice_alias() unconditional there. Or am I misreading what you said? Do you see any problems with what's in #for-linus now (head at 11f0083)?