From: ebiederm@xmission.com (Eric W. Biederman) Subject: Re: [PATCH] vfs: Speed up deactivate_super for non-modular filesystems Date: Tue, 15 May 2012 17:34:50 -0700 Message-ID: <87k40d2cb9.fsf@xmission.com> References: <1336142995.25479.49.camel@marge.simpson.net> <1336150643.7502.4.camel@marge.simpson.net> <1336197362.7346.9.camel@marge.simpson.net> <1336198093.7346.11.camel@marge.simpson.net> <1336201977.7346.22.camel@marge.simpson.net> <87r4uv64oj.fsf_-_@xmission.com> <20120507221706.GI22082@ZenIV.linux.org.uk> <20120507235647.GH21152@linux.vnet.ibm.com> <87zk9jzdi5.fsf@xmission.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: paulmck@linux.vnet.ibm.com, Al Viro , Andrew Morton , Oleg Nesterov , LKML , Pavel Emelyanov , Cyrill Gorcunov , Louis Rilling , Mike Galbraith , Christoph Hellwig , linux-fsdevel@vger.kernel.org To: Nick Piggin Return-path: Received: from out02.mta.xmission.com ([166.70.13.232]:37229 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756963Ab2EPAfM convert rfc822-to-8bit (ORCPT ); Tue, 15 May 2012 20:35:12 -0400 In-Reply-To: (Nick Piggin's message of "Tue, 15 May 2012 18:40:46 +1000") Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Nick Piggin writes: > On 9 May 2012 21:02, Eric W. Biederman wrote: >> Nick Piggin writes: >> >>> On 8 May 2012 11:07, Eric W. Biederman wrot= e: >>>> "Paul E. McKenney" writes: >>>> >>>>> On Mon, May 07, 2012 at 11:17:06PM +0100, Al Viro wrote: >>>>>> On Mon, May 07, 2012 at 02:51:08PM -0700, Eric W. Biederman wrot= e: >>>>>> >>>>>> > /proc and similar non-modular filesystems do not need a rcu_ba= rrier >>>>>> > in deactivate_locked_super. =C2=A0Being non-modular there is n= o danger >>>>>> > of the rcu callback running after the module is unloaded. >>>>>> >>>>>> There's more than just a module unload there, though - actual fr= eeing >>>>>> =C2=A0struct super_block also happens past that rcu_barrier()... >>>> >>>> Al. =C2=A0I have not closely audited the entire code path but at a= quick >>>> sample I see no evidence that anything depends on inode->i_sb bein= g >>>> rcu safe. =C2=A0Do you know of any such location? >>>> >>>> It has only been a year and a half since Nick added this code whic= h >>>> isn't very much time to have grown strange dependencies like that. >>> >>> No, it has always depended on this. >>> >>> Look at ncp_compare_dentry(), for example. >> >> Interesting. ncp_compare_dentry this logic is broken. >> >> Accessing i_sb->s_fs_info for parameters does seem reasonable. >> Unfortunately ncp_put_super frees server directly. >> >> Meaning if we are depending on only rcu protections a badly timed >> ncp_compare_dentry will oops the kernel. >> >> I am going to go out on a limb and guess that every other filesystem >> with a similar dependency follows the same pattern and is likely >> broken as well. > > But ncp_put_super should be called after the rcu_barrier(), no? > > How is it broken? The interesting hunk of code from deactivate_locked_super is: > cleancache_invalidate_fs(s); > fs->kill_sb(s); ^^^^^^^^^^^^^^ This is where ncp_put_super() is called. > > /* caches are now gone, we can safely kill the shrinker now */ > unregister_shrinker(&s->s_shrink); > > /* > * We need to call rcu_barrier so all the delayed rcu free > * inodes are flushed before we release the fs module. > */ > rcu_barrier(); > put_filesystem(fs); > put_super(s); Which guarantees ncp_put_super() happens before the rcu_barrier. >> Taking that observation farther we have a mount reference count, tha= t >> pins the super block. =C2=A0So at first glance the super block looks= safe >> without any rcu protections. > > Well yes, that's what I'm getting at. But I don't think it's quite co= mplete... > >> >> I'm not certain what pins the inodes. Let's see: >> >> mnt->d_mnt_root has the root dentry of the dentry tree, and that >> dentry count is protected by the vfsmount_lock. > > If the mount is already detached from the namespace when we start > to do a path walk, AFAIKS it can be freed up from underneath us at > that point. > > This would require cycling vfsmount_lock for write in such path. It's > better than rcu_barrier probably, but not terribly nice. Where do you see the possibility of a mount detached from a namespace causing problems? Simply having any count on a mount ensures we cycle the vfsmount in mntput_no_expire. Or if you want to see what I am seeing: The rcu_path_walk starts at one of. "." "/" or file->f_path, all of which hold a reference on a struct vfsmount. We perform an rcu_path_walk with the locking. br_read_lock(vfsmount_lock); rcu_read_lock(); We can transition to another vfs mount via follow_mount_rcu=20 which consults the mount hash table which can only be modified under the br_write_lock(vfsmount_lock); We can also transition to another vfs mount via follow_up_rcu which simply goes to mnt->mnt_parent. Where our starting vfsmount holds a reference to the target vfsmount. When we complete the rcu_path_walk we do: rcu_read_unlock() br_write_lock(vfsmount_lock) mntput_no_expire, which decrements mount counts takes and releases br_write_lock before we put the final mount reference. Which means that it is impossible for the final mntput on a mount to complete while we are in the middle of an rcu path walk. Once we have take and released br_write_lock(vfsmount_lock) in mntput_no_expire we call mntfree. mntfree calls deactivate_super. And deactivate_super calls deactivate_locked_super. Which is a long winded way of saying we always call deactivate_locked_super after we put our final mount count. I don't possibly see how a mount can be freed while we are in the middle of a rcu path walk. Not while we hold the br_read_lock(vfsmount_lock), and the final mntput takes br_write_lock(vfsmount_lock). >> Documentation/filesystems/vfs.txt seems to duplicate this reasoning >> of why the superblock is safe. =C2=A0Because we hold a real referenc= e to it >> from the vfsmount. > > rcu walk does not hold a reference to the vfsmount, however. It can > go away. This is why functions which can be called from rcu-walk > must go through synchronize_rcu() before they go away, also before > the superblock goes away. Not at all. The rcu walk itself does not hold a reference to the vfsmount, but something holds a reference to the vfsmount and to drop the final reference on a vfsmount we must hold the vfsmount_lock for write. The rcu walk holds the vfsmount_lock for read which prevents us from grabbing the vfsmount_lock for write. We need to wait an rcu grace period before freeing dentries and inodes becuase for dentries and inodes we only have rcu protection for them. =46or vfsmounts and the superblock we have a lock protected reference count. > The other way we could change the rule is to require barrier only for > those filesystems which access superblock or other info from rcu-walk= =2E > I would prefer not to have such a rule, but it could be pragmatic. I don't see that we need to change a rule. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel= " in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html