From patchwork Tue Feb 17 20:12:24 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 5841391 Return-Path: X-Original-To: patchwork-linux-fsdevel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id E13ABBF440 for ; Tue, 17 Feb 2015 20:12:50 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id EE86920166 for ; Tue, 17 Feb 2015 20:12:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0EF8520155 for ; Tue, 17 Feb 2015 20:12:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752868AbbBQUMd (ORCPT ); Tue, 17 Feb 2015 15:12:33 -0500 Received: from mail-yh0-f41.google.com ([209.85.213.41]:39685 "EHLO mail-yh0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752760AbbBQUMc (ORCPT ); Tue, 17 Feb 2015 15:12:32 -0500 Received: by yhab6 with SMTP id b6so343046yha.6 for ; Tue, 17 Feb 2015 12:12:31 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-type; bh=ZX88mqxS4x4qJk+ZK9bm5BqjnlW4WjOkUYS0Ce/L9j4=; b=JUYDvxOG5fyWOS9CWADp9wVvZPoqYpiQYWiwi2PzN5fscZs5j5E9FmdooA3e0wknQA +T9Mq6BslTOae0x6OPV4tGVqc9+rm1ESYk+v9LJ2FaVh19N2Kc27r1OiJbleDP6YsOE/ k5+0DUbXqEfirAMmImeh+wVlcfdsHPlPKCqXVk/kHZhpqORbB2Y5PgmK5KgFxgHbtYES ivugkGdbppTovHP2k0ZpIJNdwSZfYKpOv6BgBVAxTPSstu5yEgJhbXqBU4ouiZzFoMSX 4Pu3hNTZfm+OFhO/Jdxw4qzJ0yXV/qTaiOY6Z4P8sPBueZeOy6l0GJ4Dn/3VvFmmzWrB vwoA== X-Gm-Message-State: ALoCoQnIF1D4ykytMXqVxDe8770Ut6WaVU71LrkZBh4mVRqC88JXGiiEYz3zI72rQtP3uWgycV3E X-Received: by 10.236.202.233 with SMTP id d69mr228493yho.22.1424203951408; Tue, 17 Feb 2015 12:12:31 -0800 (PST) Received: from tlielax.poochiereds.net ([2606:a000:1105:207d:3a60:77ff:fe93:a95d]) by mx.google.com with ESMTPSA id c43sm14643255yha.40.2015.02.17.12.12.30 (version=TLSv1.2 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 17 Feb 2015 12:12:30 -0800 (PST) Date: Tue, 17 Feb 2015 15:12:24 -0500 From: Jeff Layton To: Linus Torvalds Cc: "J. Bruce Fields" , "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: <20150217151224.2dc31ad8@tlielax.poochiereds.net> In-Reply-To: References: <20150209055540.2f2a3689@tlielax.poochiereds.net> <20150216133200.GB3270@node.dhcp.inet.fi> <20150216090054.62455465@tlielax.poochiereds.net> <20150217190844.GC27900@fieldses.org> <20150217142714.36ed9ddb@tlielax.poochiereds.net> X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.25; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, T_TVD_MIME_EPI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, 17 Feb 2015 11:41:40 -0800 Linus Torvalds wrote: > On Tue, Feb 17, 2015 at 11:27 AM, Jeff Layton 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. > > 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 I agree that there's no deadlock. I also agree that allowing two LOCK_EX's (or a LOCK_SH + LOCK_EX) on the file is broken. I'm just leery on making a user-visible change at this point. I'd prefer to let something like that soak in linux-next for a while. Another possibility is to keep dropping the spinlock, but check to see if someone set a new lock on the same filp in the loop after that. If they have, then we could just remove that lock before adding the new one. I don't think that would violate anything since there are no atomicity guarantees here. If you're setting locks on the same filp from multiple tasks then you're simply asking for trouble. I don't expect that most apps do that though, but rather work on their own set of open file descriptions. Those might get bitten however if we stop dropping the spinlock there since we'll be changing how flock's fairness works. See the attached (untested) patch for what I'm thinking. If you still think that removing the "if (found)" clause is the right thing to do, I'll go with that, but I do worry that we might break some (fragile) app that might rely on the way that flock works today. From 3212be05d47300fbb5718932f92b33acde3d219c Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Tue, 17 Feb 2015 15:08:06 -0500 Subject: [PATCH] locks: ensure that we can't set multiple flock locks for the same filp Currently, we'll drop the spinlock in the middle of flock_lock_file in the event that we found an lock that needed to be removed prior to an upgrade or downgrade. It's possible however for another task to race in and set a lock on the same filp. If that happens, then we don't want to set an additional lock, so just remove the one that raced in and set our own. Signed-off-by: Jeff Layton --- fs/locks.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/fs/locks.c b/fs/locks.c index fe8f9f46445b..099b60a46ccc 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -864,7 +864,7 @@ static int posix_locks_deadlock(struct file_lock *caller_fl, static int flock_lock_file(struct file *filp, struct file_lock *request) { struct file_lock *new_fl = NULL; - struct file_lock *fl; + struct file_lock *fl, *tmp; struct file_lock_context *ctx; struct inode *inode = file_inode(filp); int error = 0; @@ -912,7 +912,12 @@ static int flock_lock_file(struct file *filp, struct file_lock *request) } find_conflict: - list_for_each_entry(fl, &ctx->flc_flock, fl_list) { + list_for_each_entry_safe(fl, tmp, &ctx->flc_flock, fl_list) { + /* did someone set a lock on the same filp? */ + if (fl->fl_file == filp) { + locks_delete_lock_ctx(fl, &dispose); + continue; + } if (!flock_locks_conflict(request, fl)) continue; error = -EAGAIN; -- 2.1.0