Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753221AbbHAAVu (ORCPT ); Fri, 31 Jul 2015 20:21:50 -0400 Received: from mail-pd0-f180.google.com ([209.85.192.180]:36787 "EHLO mail-pd0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751180AbbHAAVr (ORCPT ); Fri, 31 Jul 2015 20:21:47 -0400 Date: Fri, 31 Jul 2015 17:20:58 -0700 (PDT) From: Hugh Dickins X-X-Sender: hugh@eggly.anvils To: Linus Torvalds cc: Dominique Martinet , Hugh Dickins , "J. Bruce Fields" , Dominique Martinet , Al Viro , Linux Kernel Mailing List , linux-fsdevel , David Howells Subject: Re: v4.2-rc dcache regression, probably 75a6f82a0d10 In-Reply-To: Message-ID: References: <20150731205036.GA3752@nautica> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2126 Lines: 55 On Fri, 31 Jul 2015, Linus Torvalds wrote: > > I'd be more suspicious about other effects. For example, iot's not at > all obvious that the commit in question just changes the order of the > flags/inode field accesses, there are potentialy bigger changes there. > For example, this part (in __d_obtain_alias): > > - tmp->d_inode = inode; > - tmp->d_flags |= add_flags; > + __d_set_inode_and_type(tmp, inode, add_flags); > > looks a bit off, because it *used* to just add those flags, but now, > through __d_set_inode_and_type, it does > > + dentry->d_inode = inode; > + smp_wmb(); > + flags = READ_ONCE(dentry->d_flags); > + flags &= ~(DCACHE_ENTRY_TYPE | DCACHE_FALLTHRU); > + flags |= type_flags; > + WRITE_ONCE(dentry->d_flags, flags); > > so it clears DCACHE_ENTRY_TYPE | DCACHE_FALLTHRU. > > Is that correct? Maybe, I haven't checked. And maybe it's a big bad > bug. Regardless, it sure as hell isn't just changing the order of the > access to those fields. That "DCACHE_ENTRY_TYPE | DCACHE_FALLTHRU" > clearing came from __d_instantiate(), but now it hits __d_obtain_alias > too. > > There may be other changes like that for all I know. I didn't look Yes, the one which grabbed my attention is: @@ -311,7 +346,7 @@ static void dentry_iput(struct dentry * dentry) { struct inode *inode = dentry->d_inode; if (inode) { - dentry->d_inode = NULL; + __d_clear_type_and_inode(dentry); hlist_del_init(&dentry->d_u.d_alias); spin_unlock(&dentry->d_lock); spin_unlock(&inode->i_lock); which I think clears the DCACHE_ENTRY_TYPE i.e. makes it DCACHE_MISS_TYPE, when it was left as is before. While there might be an RCU lookup in progress, suddenly finding this to be a negative dentry. Perhaps - this is not an area I've visited for years, and I've not followed up the sequence count protection. Hugh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/