From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman) Subject: Re: [PATCH review 3/8] dcache: Clearly separate the two directory rename cases in d_splice_alias Date: Sat, 15 Aug 2015 13:25:55 -0500 Message-ID: <874mk08l3g.fsf@x220.int.ebiederm.org> References: <871tncuaf6.fsf@x220.int.ebiederm.org> <87mw5xq7lt.fsf@x220.int.ebiederm.org> <87a8yqou41.fsf_-_@x220.int.ebiederm.org> <874moq9oyb.fsf_-_@x220.int.ebiederm.org> <871tfkawu9.fsf_-_@x220.int.ebiederm.org> <87egjk9i61.fsf_-_@x220.int.ebiederm.org> <20150810043637.GC14139@ZenIV.linux.org.uk> <877foymrwt.fsf@x220.int.ebiederm.org> <87wpwyjxwc.fsf_-_@x220.int.ebiederm.org> <87fv3mjxsc.fsf_-_@x220.int.ebiederm.org> <20150815061617.GG14139@ZenIV.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: Andrey Vagin , Miklos Szeredi , Richard Weinberger , Linux Containers , Andy Lutomirski , "J. Bruce Fields" , linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jann Horn , Linus Torvalds , Willy Tarreau To: Al Viro Return-path: In-Reply-To: <20150815061617.GG14139-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org> (Al Viro's message of "Sat, 15 Aug 2015 07:16:17 +0100") List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org List-Id: linux-fsdevel.vger.kernel.org Al Viro writes: > On Thu, Aug 13, 2015 at 11:31:47PM -0500, Eric W. Biederman wrote: > >> The problem is that as the lookup locking stands today grabbing the >> s_vfs_rename_mutex must use mutex_trylock which can fail, so for reliability >> reasons we need to avoid using the rename_mutex as much as possible. > > I really don't like it. For one thing, you *still* are taking it - have to, > really. So this argument is moot anyway. Not moot. We don't take s_vfs_rename_mutex when we are connecting a disconnected dentry alias. If d_splice_alias could sleep and take s_vfs_rename_mutex then we could have a single path through the code. Unfortunately the code can't sleep when taking s_vfs_rename_mutex so attempting to take s_vfs_rename_mutex for both paths will introduce unnecessary -ESTALE failures into d_splice_alias. > ESTALE can happen here. For > another, I'm not convinced that this "we don't need no stinkin' > extra locks for attaching a detached subtree" is correct. E.g. what's > to protect that IS_ROOT(new) from changing right under you? You are quite correct that I missed that nothing protects the result of IS_ROOT(new). So my change does introduce a case where we don't hold the appropriate inode mutexes when renaming a dentry and that introduces races elsewhere in the code so it is not acceptable. But it is true that we only take rename_lock and don't take any additional mutex when connecting a disconnected dentry. Aka "we don't need no stinkin' extra locks". We clearly can not take the new->d_parent->d_inode->i_mutex when IS_ROOT(new) as that is meaningless. Further I do not see a point in taking s_vfs_rename_mutex in that case. Not for this round but if you can see any reason why our not taking s_vfs_rename_mutex when connecting disconnected dentries is wrong and we need to take it and risk -ESTALE. I would love to know because I would love to clean up that mess. > I'm not saying that it wouldn't benefit from cleaner locking - it sure > as hell would. But it's in a really incestous relationship with a lot > of other pieces, both in fs/dcache.c and elsewhere. Let's not mix that > into anything else - driveby cleanups in that place are very likely to > cause serious trouble. Fair enough. I do have to touch d_splice_alias and change the locking, so unfortunately I have to do some of the nasty locking analysis anyway. I am keeping the i_lock cleanup because it is trivial and removes the need to figure out if there is any existing ordering between i_lock and mount_lock, and if taking mount_lock could induce a deadlock. That audit would not be trivial. Eric