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.
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.
--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
