From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751597AbcFYQit (ORCPT ); Sat, 25 Jun 2016 12:38:49 -0400 Received: from linuxhacker.ru ([217.76.32.60]:37346 "EHLO fiona.linuxhacker.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750872AbcFYQir convert rfc822-to-8bit (ORCPT ); Sat, 25 Jun 2016 12:38:47 -0400 Subject: Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes. Mime-Version: 1.0 (Apple Message framework v1283) Content-Type: text/plain; charset=us-ascii From: Oleg Drokin In-Reply-To: <20160617042914.GD14480@ZenIV.linux.org.uk> Date: Sat, 25 Jun 2016 12:38:40 -0400 Cc: Mailing List , "" Content-Transfer-Encoding: 8BIT Message-Id: References: <20160617042914.GD14480@ZenIV.linux.org.uk> To: Al Viro X-Mailer: Apple Mail (2.1283) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello! On Jun 17, 2016, at 12:29 AM, Al Viro wrote: > On Fri, Jun 17, 2016 at 12:09:19AM -0400, Oleg Drokin wrote: > >> So they both do d_drop(), the dentry is now unhashed, and they both >> dive into nfs_lookup(). >> There eventually they both call >> >> res = d_splice_alias(inode, dentry); >> >> And so the first lucky one continues on it's merry way with a hashed dentry, >> but the other less lucky one ends up calling into d_splice_alias() with >> dentry that's already hashed and hits the very familiar assertion. >> >> I took a brief look into ceph and it looks like a very similar thing >> might happen there with handle_reply() for two parallel replies calling into >> ceph_fill_trace() and then splice_alias()->d_splice_alias(), since the >> unhashed check it does is not under any locks, it's unsafe, so the problem >> might be more generic than just NFS too. >> >> So I wonder how to best fix this? Holding some sort of dentry lock across a call >> into atomic_open in VFS? We cannot just make d_splice_alias() callers call with >> inode->i_lock held because dentry might be negative. > > Oh, lovely... So basically the problem is that we violate the "no lookups on > the same name in parallel" rule on those fallbacks from foo_atomic_open() to > foo_lookup(). The thing is, a lot of ->atomic_open() instances have such > fallbacks and I wonder if that's a sign that we need to lift some of that > to fs/namei.c... > > Hell knows; alternative is to have that d_drop() followed by d_alloc_parallel() > and feeding that dentry to lookup. I'll play with that a bit and see what's > better; hopefully I'll have something by tomorrow. Sorry to nag you about this, but did any of those pan out? d_alloc_parallel() sounds like a bit too heavy there, esp. considering we came in with a dentry already (though a potentially shared one, I understand). Would not it be better to try and establish some dentry locking rule for calling into d_splice_alias() instead? At least then the callers can make sure the dentry does not change under them? Though I guess if there's dentry locking like that, we might as well do all the checking in d_splice_alias(), but that means the unhashed dentries would no longer be disallowed which is a change of semantic from now.