From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi0-f67.google.com ([209.85.218.67]:38558 "EHLO mail-oi0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727985AbeHVMFR (ORCPT ); Wed, 22 Aug 2018 08:05:17 -0400 Received: by mail-oi0-f67.google.com with SMTP id z185-v6so1445609oia.5 for ; Wed, 22 Aug 2018 01:41:22 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: From: Miklos Szeredi Date: Wed, 22 Aug 2018 10:41:21 +0200 Message-ID: Subject: Re: ovl: set I_CREATING on inode being created To: Amir Goldstein Cc: Al Viro , overlayfs , linux-fsdevel Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Fri, Aug 17, 2018 at 1:16 PM, Amir Goldstein wrote: > Miklos, > > I have 2 questions/comments w.r.t. commit 9475938ce8cf > ("ovl: set I_CREATING on inode being created") in overlayfs-next. > > 1. insert_inode_locked4() sets I_CREATING not inside > spinlock and your patch sets it inside spinlock. > technically, I guess the spinlock taken inside inode_insert5() > to set I_NEW and insert to hash probably provides the needed > barriers. I was thinking along the same lines. However, the spinlock is already in L1 cache (due to new_inode_pseudo() pulling it in) and is obviously uncontended, so it's basically free (at least compared to the actual creation of a new object, which follows). The lock here is for documentation purposes. Using the common pattern also makes possible future changes to i_state rules simpler. > Do you think we should choose one of the practices > and stick with it?? The only lockless modification of i_state is insert_inode_locked4(), AFAICS. I think it should either be converted to being locked or a comment added explaining why that is not done. > 2. I find it nicer if ovl_new_inode() would return an I_CREATING > inode, to conform with the users of inode_insert_locked4() (e.g. > btrfs_new_inode()) this will conform with the pattern: > inode = XXXfs_new_inode(...); > d_instantiate_new(dentry, inode); > The call site d_make_root(ovl_new_inode(sb, S_IFDIR, 0)); > could either use a variant of ovl_new_inode() or we could > let ovl_inode_init() clear I_CREATING. Normal filesystems already know the hash key when allocating the in-core inode. That's not the case for overlay, that's why we need the unusual sequence of calls. Basically before the inode is inserted it just doesn't matter when I_CREATING is set, we could just as well set it right before calling inode_insert5(). Thanks, Miklos