On Tue, Feb 17, 2015 at 11:41:40AM -0800, Linus Torvalds wrote: > On Tue, Feb 17, 2015 at 11:27 AM, Jeff Layton <jlayton@xxxxxxxxxxxxxxx> wrote: > > > > What about this instead then? > > No. Really. > > > - leave the "drop the spinlock" thing in place in flock_lock_file for > > v3.20 > > No. The whole concept of "drop the lock in the middle" is *BROKEN*. > It's seriously crap. It's not just a bug, it's a really fundamentally > wrong thing to do. > > > - change locks_remove_flock to just walk the list and delete any locks > > associated with the filp being closed > > No. That's still wrong. You can have two people holding a write-lock. > Seriously. That's *shit*. > > The "drop the spinlock in the middle" must go. There's not even any > reason for it. Just get rid of it. There can be no deadlock if you get > rid of it, because > > - we hold the flc_lock over the whole event, so we can never see any > half-way state > > - if we actually decide to sleep (due to conflicting locks) and > return FILE_LOCK_DEFERRED, we will drop the lock before actually > sleeping, so nobody else will be deadlocking on this file lock. So any > *other* person who tries to do an upgrade will not sleep, because the > pending upgrade will have moved to the blocking list (that whole > "locks_insert_block" part. Whoops, you're right, I was forgetting that wait happens up in flock_lock_file_wait(), OK. --b. > Ergo, either we'll upgrade the lock (atomically, within flc_lock), or > we will drop the lock (possibly moving it to the blocking list). I > don't see a deadlock. > > I think your (and mine - but mine had the more fundamental problem of > never setting "old_fl" correctly at all) patch had a deadlock because > you didn't actually remove the old lock when you returned > FILE_LOCK_DEFERRED. > > But I think the correct minimal patch is actually to just remove the > "if (found)" statement. > > Linus -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
