Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752830AbbBQTIq (ORCPT ); Tue, 17 Feb 2015 14:08:46 -0500 Received: from fieldses.org ([173.255.197.46]:47202 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751586AbbBQTIp (ORCPT ); Tue, 17 Feb 2015 14:08:45 -0500 Date: Tue, 17 Feb 2015 14:08:44 -0500 From: "J. Bruce Fields" To: Linus Torvalds Cc: Jeff Layton , "Kirill A. Shutemov" , linux-fsdevel , Linux Kernel Mailing List , Christoph Hellwig , Dave Chinner , Sasha Levin Subject: Re: [GIT PULL] please pull file-locking related changes for v3.20 Message-ID: <20150217190844.GC27900@fieldses.org> References: <20150209055540.2f2a3689@tlielax.poochiereds.net> <20150216133200.GB3270@node.dhcp.inet.fi> <20150216090054.62455465@tlielax.poochiereds.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2754 Lines: 67 On Mon, Feb 16, 2015 at 11:24:03AM -0800, Linus Torvalds wrote: > On Mon, Feb 16, 2015 at 10:46 AM, Linus Torvalds > wrote: > > > > This code is so broken that my initial reaction is "We need to just > > revert the crap". > > How the hell is flock_lock_file() supposed to work at all, btw? > > Say we have an existing flock, and now do a new one that conflicts. I > see what looks like three separate bugs. > > - We go through the first loop, find a lock of another type, and > delete it in preparation for replacing it > > - we *drop* the lock context spinlock. > > - BUG #1? So now there is no lock at all, and somebody can come in > and see that unlocked state. Is that really valid? > > - another thread comes in while the first thread dropped the lock > context lock, and wants to add its own lock. It doesn't see the > deleted or pending locks, so it just adds it > > - the first thread gets the context spinlock again, and adds the lock > that replaced the original > > - BUG #2? So now there are *two* locks on the thing, and the next > time you do an unlock (or when you close the file), it will only > remove/replace the first one. > > Both of those bugs are due to the whole "drop the lock in the middle", > which is pretty much always a mistake. BUG#2 could easily explain the > warning Kirill reports, afaik. > > BUG#3 seems to be independent, and is about somebody replacing an > existing lock, but the new lock conflicts. Again, the first loop will > remove the old lock, and then the second loop will see the conflict, > and return an error (and we may then end up waiting for it for the > FILE_LOCK_DEFERRED case). Now the original lock is gone. Is that > really right? That sounds bogus. *Failing* to insert a flock causing > the old flock to go away? >From flock(2): Converting a lock (shared to exclusive, or vice versa) is not guaranteed to be atomic: the existing lock is first removed, and then a new lock is established. Between these two steps, a pending lock request by another process may be granted, with the result that the conversion either blocks, or fails if LOCK_NB was specified. I also checked Michael Kerrisk's book quickly and see similar language plus "... the conversion will fail and the process will lose its original lock". I don't have a quick way to check BSD, but it looks to me like this is the way Linux has always behaved. I agree that it's weird, but I think it's what we're stuck with. --b. -- 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/