On Tue, Feb 17, 2015 at 02:11:36PM -0500, J. Bruce Fields wrote:
> On Tue, Feb 17, 2015 at 12:56:49PM -0500, Jeff Layton wrote:
> > On Tue, 17 Feb 2015 12:10:17 -0500
> > "J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote:
> >
> > > On Tue, Feb 17, 2015 at 07:46:28AM -0500, Jeff Layton wrote:
> > > > As Linus pointed out:
> > > >
> > > > 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.
> > > >
> > > > ...remove the "drop the spinlock" code in the middle of this function as
> > > > it has always been suspicious.
> > >
> > > Looking back through a historical git repo, purely out of curiosity--the
> > > cond_resched was previously a yield, and then I *think* bug #2 was
> > > introduced in 2002 by a "[PATCH] fs/locks.c: more cleanup". Before that
> > > it retried the first loop after the yield.
> > >
> > > Before that the yield was in locks_wake_up_blocks, removed by:
> > >
> > > commit 7962ad19e6300531784722c16849837864304d84
> > > Author: Matthew Wilcox <willy@xxxxxxxxxx>
> > > Date: Sat Jun 8 02:09:24 2002 -0700
> > >
> > > [PATCH] fs/locks.c: Only yield once for flocks
> > >
> > > This patch removes the annoying and confusing `wait' argument
> > > from many places. The only change in behaviour is that we now
> > > yield once when unblocking other BSD-style flocks instead of
> > > once for each lock.
> > >
> > > This slightly improves the semantics for userspace. Before,
> > > when we had two tasks waiting on a lock, the first one would
> > > receive the lock. Now, the one with the highest priority
> > > receives the lock.
> > >
> > > So this really was intended to guarantee other waiters the lock before
> > > allowing an upgrade. Could that actually have worked?
> > >
> > > The non-atomic behavior is documented in flock(2), which says it's
> > > "original BSD behavior".
> > >
> > > A comment at the top of locks.c says this is to avoid deadlock. Hm, so,
> > > are we introducing a deadlock?:
> > >
> > > 1. Two processes both get shared lock on different filps.
> > > 2. Both request exclusive lock.
> > >
> > > Now they're stuck, whereas previously they might have recovered?
> > >
> > > --b.
> > >
> >
> >
> > Yes, I see that now. It might be best to preserve the existing behavior
> > for that reason then. I'd rather not introduce any behavioral changes in this
> > set if we can help it, particularly if there are userland apps that
> > might rely on it.
> >
> > It may be best then to keep this patch, but drop patch #3.
>
> Unfortunately it's this patch that I'm worried about.
(Urp, never mind, you and Linus were right here.)
> Also these patches are introducing some failure in my locking tests
> (probably unrelated, I doubt I ever wrote a test for this case). I'll
> investigate.
I didn't try to figure out, but after dropping patch 3 everything does
pass.
--b.
>
> --b.
>
> > That should
> > be enough to ensure that we don't end up with two different locks on
> > the same file description, which is clearly a bug.
> >
> > It might also not hurt to follow HCH's earlier advice and make
> > locks_remove_flock just iterate over the list and just unconditionally
> > delete any lock that in the case where the ->flock operation isn't set.
> >
> > In principle we shouldn't need that once apply patch #2, but it would
> > probably be simpler than dealing with flock_lock_file in that case.
> >
> > > >
> > > > Reported-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > > > Signed-off-by: Jeff Layton <jeff.layton@xxxxxxxxxxxxxxx>
> > > > ---
> > > > fs/locks.c | 10 ----------
> > > > 1 file changed, 10 deletions(-)
> > > >
> > > > diff --git a/fs/locks.c b/fs/locks.c
> > > > index 7998f670812c..00c105f499a2 100644
> > > > --- a/fs/locks.c
> > > > +++ b/fs/locks.c
> > > > @@ -901,16 +901,6 @@ static int flock_lock_file(struct file *filp, struct file_lock *request)
> > > > goto out;
> > > > }
> > > >
> > > > - /*
> > > > - * If a higher-priority process was blocked on the old file lock,
> > > > - * give it the opportunity to lock the file.
> > > > - */
> > > > - if (found) {
> > > > - spin_unlock(&ctx->flc_lock);
> > > > - cond_resched();
> > > > - spin_lock(&ctx->flc_lock);
> > > > - }
> > > > -
> > > > find_conflict:
> > > > list_for_each_entry(fl, &ctx->flc_flock, fl_list) {
> > > > if (!flock_locks_conflict(request, fl))
> > > > --
> > > > 2.1.0
> >
> >
> > --
> > Jeff Layton <jeff.layton@xxxxxxxxxxxxxxx>
--
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
