From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751177AbcGFEfp (ORCPT ); Wed, 6 Jul 2016 00:35:45 -0400 Received: from linuxhacker.ru ([217.76.32.60]:55620 "EHLO fiona.linuxhacker.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750832AbcGFEfn convert rfc822-to-8bit (ORCPT ); Wed, 6 Jul 2016 00:35:43 -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=windows-1252 From: Oleg Drokin In-Reply-To: <6334F6B6-807C-4B36-B785-7DD502E0D70B@linuxhacker.ru> Date: Wed, 6 Jul 2016 00:35:30 -0400 Cc: Mailing List , "" Content-Transfer-Encoding: 8BIT Message-Id: References: <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> <20160705200826.GP14480@ZenIV.linux.org.uk> <1587788B-C7F6-4D19-BDAD-29846231CD6C@linuxhacker.ru> <6188A470-DF7F-44CE-89F0-117EC2FA1677@linuxhacker.ru> <20160706032055.GQ14480@ZenIV.linux.org.uk> <6334F6B6-807C-4B36-B785-7DD502E0D70B@linuxhacker.ru> 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 On Jul 5, 2016, at 11:25 PM, Oleg Drokin wrote: > > On Jul 5, 2016, at 11:20 PM, Al Viro wrote: > >> On Tue, Jul 05, 2016 at 08:29:37PM -0400, Oleg Drokin wrote: >>>> + /* Otherwise we just unhash it to be rehashed afresh via >>>> + * lookup if necessary >>>> + */ >>>> + d_drop(dentry); >>> >>> So we can even drop this part and retain the top condition as it was. >>> d_add does not care if the dentry we are feeding it was hashed or not, >>> so do you see any downsides to doing that I wonder? >> >> d_add() on hashed dentry will end up reaching this: >> static void __d_rehash(struct dentry * entry, struct hlist_bl_head *b) >> { >> BUG_ON(!d_unhashed(entry)); > > Ah, ok. Yes, I remember about it now from the older issue with nfs. > > It's still puzzling why I did not hit it yet, but oh well. > > I wonder if handling of negative dentries broke… Time for more investigations. Ah, the reason was just looking me in the eyes as the first thing we do in ll_revalidate_dentry(): if ((lookup_flags & (LOOKUP_OPEN | LOOKUP_CREATE)) == (LOOKUP_OPEN | LOOKUP_CREATE)) return 0; Apparently we are trying to protect from the case where dentry is valid when we check it, but gets pulled under us as soon as we are done (no lustre locking held around it). So if we don't do this, we fall into ll_file_open/ll_intent_file_open and form a request to open the file based on the inode. If this inode is already gone server-side, we might get ESTALE, since we don't really send the name to the server in this case so it does not know what is it we are after anyway. On the userspace side of things the picture is not pretty then, we are doing open(O_CREAT) and get ESTALE back. Looking at do_filp_open(), there's actually a retry: if (unlikely(filp == ERR_PTR(-ESTALE))) filp = path_openat(&nd, op, flags | LOOKUP_REVAL); But it's only once so for a contended file we can still have some other thread do a lookup, provide us with a new cached dentry that's similarly pulled under us next time around. But I guess the whole idea of this is to let us know the file is contended and we can just only fail revalidate then. Hm.. looking at the server - it's even worse, we'd get ENOENT from the server if the desired inode no longer exist, so a perfect opportunity for the client to turn that ENOENT into ESTALE if the create flag was set. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from linuxhacker.ru ([217.76.32.60]:55620 "EHLO fiona.linuxhacker.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750832AbcGFEfn convert rfc822-to-8bit (ORCPT ); Wed, 6 Jul 2016 00:35:43 -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=windows-1252 From: Oleg Drokin In-Reply-To: <6334F6B6-807C-4B36-B785-7DD502E0D70B@linuxhacker.ru> Date: Wed, 6 Jul 2016 00:35:30 -0400 Cc: Mailing List , "" Content-Transfer-Encoding: 8BIT Message-Id: References: <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> <20160705200826.GP14480@ZenIV.linux.org.uk> <1587788B-C7F6-4D19-BDAD-29846231CD6C@linuxhacker.ru> <6188A470-DF7F-44CE-89F0-117EC2FA1677@linuxhacker.ru> <20160706032055.GQ14480@ZenIV.linux.org.uk> <6334F6B6-807C-4B36-B785-7DD502E0D70B@linuxhacker.ru> To: Al Viro Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Jul 5, 2016, at 11:25 PM, Oleg Drokin wrote: > > On Jul 5, 2016, at 11:20 PM, Al Viro wrote: > >> On Tue, Jul 05, 2016 at 08:29:37PM -0400, Oleg Drokin wrote: >>>> + /* Otherwise we just unhash it to be rehashed afresh via >>>> + * lookup if necessary >>>> + */ >>>> + d_drop(dentry); >>> >>> So we can even drop this part and retain the top condition as it was. >>> d_add does not care if the dentry we are feeding it was hashed or not, >>> so do you see any downsides to doing that I wonder? >> >> d_add() on hashed dentry will end up reaching this: >> static void __d_rehash(struct dentry * entry, struct hlist_bl_head *b) >> { >> BUG_ON(!d_unhashed(entry)); > > Ah, ok. Yes, I remember about it now from the older issue with nfs. > > It's still puzzling why I did not hit it yet, but oh well. > > I wonder if handling of negative dentries brokeďż˝ Time for more investigations. Ah, the reason was just looking me in the eyes as the first thing we do in ll_revalidate_dentry(): if ((lookup_flags & (LOOKUP_OPEN | LOOKUP_CREATE)) == (LOOKUP_OPEN | LOOKUP_CREATE)) return 0; Apparently we are trying to protect from the case where dentry is valid when we check it, but gets pulled under us as soon as we are done (no lustre locking held around it). So if we don't do this, we fall into ll_file_open/ll_intent_file_open and form a request to open the file based on the inode. If this inode is already gone server-side, we might get ESTALE, since we don't really send the name to the server in this case so it does not know what is it we are after anyway. On the userspace side of things the picture is not pretty then, we are doing open(O_CREAT) and get ESTALE back. Looking at do_filp_open(), there's actually a retry: if (unlikely(filp == ERR_PTR(-ESTALE))) filp = path_openat(&nd, op, flags | LOOKUP_REVAL); But it's only once so for a contended file we can still have some other thread do a lookup, provide us with a new cached dentry that's similarly pulled under us next time around. But I guess the whole idea of this is to let us know the file is contended and we can just only fail revalidate then. Hm.. looking at the server - it's even worse, we'd get ENOENT from the server if the desired inode no longer exist, so a perfect opportunity for the client to turn that ENOENT into ESTALE if the create flag was set.