[PATCH] fs/locks.c: Fix posix locking for threaded tasks

[PATCH] fs/locks.c: Fix posix locking for threaded tasks

[Matthew Wilcox]

Saurabh Desai believes that locks created by threads should not conflict
with each other.  I'm inclined to agree; I don't know why the test for
->fl_pid was added, but the comment suggests that whoever added it wasn't
sure either.

Frankly, I have no clue about the intended semantics for threads, and
SUS v3 does not offer any enlightenment.  But it seems reasonable that
processes which share a files_struct should share locks.  After all,
if one process closes the fd, they'll remove locks belonging to the
other process.

Here's a patch generated against 2.4; it also applies to 2.5.
Please apply.

===== fs/locks.c 1.9 vs edited =====
--- 1.9/fs/locks.c	Mon Jun  3 18:49:43 2002
+++ edited/fs/locks.c	Fri Jun  7 21:24:12 2002
@@ -380,15 +380,12 @@
 }
 
 /*
- * Check whether two locks have the same owner
- * N.B. Do we need the test on PID as well as owner?
- * (Clone tasks should be considered as one "owner".)
+ * Locks are deemed to have the same owner if the tasks share files_struct.
  */
 static inline int
 locks_same_owner(struct file_lock *fl1, struct file_lock *fl2)
 {
-	return (fl1->fl_owner == fl2->fl_owner) &&
-	       (fl1->fl_pid   == fl2->fl_pid);
+	return (fl1->fl_owner == fl2->fl_owner);
 }
 
 /* Remove waiter from blocker's block list.

-- 
Revolutions do not require corporate support.

Re: [PATCH] fs/locks.c: Fix posix locking for threaded tasks

[Andreas Dilger]

On Jun 10, 2002  03:48 +0100, Matthew Wilcox wrote:
> Saurabh Desai believes that locks created by threads should not conflict
> with each other.  I'm inclined to agree; I don't know why the test for
> ->fl_pid was added, but the comment suggests that whoever added it wasn't
> sure either.
> 
> Frankly, I have no clue about the intended semantics for threads, and
> SUS v3 does not offer any enlightenment.  But it seems reasonable that
> processes which share a files_struct should share locks.  After all,
> if one process closes the fd, they'll remove locks belonging to the
> other process.

I would pass this by the Samba folks first.  I seem to recall them
complaining about this area.  The "one thread closing a file removes
locks from another thread" is one area that I'm sure they don't like.
Making it so that multiple threads ignore file locks is probably
not going to make them happy either.  I believe one of the issues is
that Samba is running server threads for many remote processes, and
it needs to be able to enforce these locks.

Otherwise, this change makes it impossible to write a multi-threaded
program that _does_ allow you use locking between threads.  If anything,
this PID check could be conditional on some extra lock flag (e.g.
THREADS_SHARE_LOCKS or whatever).

Cheers, Andreas
--
Andreas Dilger
http://www-mddsp.enel.ucalgary.ca/People/adilger/
http://sourceforge.net/projects/ext2resize/

Re: [PATCH] fs/locks.c: Fix posix locking for threaded tasks

[Matthew Wilcox]

On Mon, Jun 10, 2002 at 12:41:20AM -0600, Andreas Dilger wrote:
> I would pass this by the Samba folks first.  I seem to recall them
> complaining about this area.  The "one thread closing a file removes
> locks from another thread" is one area that I'm sure they don't like.

I know they don't; they've complained about it in the past.  However, this
_is_ mandated by POSIX so we can't really fix that (Turns out everyone
thinks POSIX locks are broken in a different way, so my definition of
sane locking semantics is very different from yours).

> Making it so that multiple threads ignore file locks is probably
> not going to make them happy either.  I believe one of the issues is
> that Samba is running server threads for many remote processes, and
> it needs to be able to enforce these locks.

Looking at samba-2.2.4/source/locking/posix.c, it seems to me that samba
trusts the OS file locking so little that this change will have no effect.

> Otherwise, this change makes it impossible to write a multi-threaded
> program that _does_ allow you use locking between threads.  If anything,
> this PID check could be conditional on some extra lock flag (e.g.
> THREADS_SHARE_LOCKS or whatever).

if you're locking between threads, you should be using posix thread
mutexes, not file locks, IMO.  There's nothing in SUS v3 which says
you can do what you've described.

-- 
Revolutions do not require corporate support.

Re: [PATCH] fs/locks.c: Fix posix locking for threaded tasks

[Matthew Wilcox]

On Mon, Jun 10, 2002 at 01:41:19PM +0100, Matthew Wilcox wrote:
> On Mon, Jun 10, 2002 at 12:41:20AM -0600, Andreas Dilger wrote:
> > Otherwise, this change makes it impossible to write a multi-threaded
> > program that _does_ allow you use locking between threads.  If anything,
> > this PID check could be conditional on some extra lock flag (e.g.
> > THREADS_SHARE_LOCKS or whatever).
> 
> if you're locking between threads, you should be using posix thread
> mutexes, not file locks, IMO.  There's nothing in SUS v3 which says
> you can do what you've described.

the light dawned... of course it doesn't say.  If you have userlevel
threads (or some M:N system), either you force the threading library
to reimplement the fcntl lock interface, or they have to share locks.
I can't imagine that even the posix threading people require a
reimplementation of the grotesque solaris file locking scheme in
userspace.

-- 
Revolutions do not require corporate support.

Re: [PATCH] fs/locks.c: Fix posix locking for threaded tasks

[Alan Cox]

> SUS v3 does not offer any enlightenment.  But it seems reasonable that
> processes which share a files_struct should share locks.  After all,
> if one process closes the fd, they'll remove locks belonging to the
> other process.
> 
> Here's a patch generated against 2.4; it also applies to 2.5.
> Please apply.

This seems horribly inappropriate for 2.4 as it may break apps

Re: [PATCH] fs/locks.c: Fix posix locking for threaded tasks

[Matthew Wilcox]

On Wed, Jun 12, 2002 at 10:40:07AM +0100, Alan Cox wrote:
> > SUS v3 does not offer any enlightenment.  But it seems reasonable that
> > processes which share a files_struct should share locks.  After all,
> > if one process closes the fd, they'll remove locks belonging to the
> > other process.
> > 
> > Here's a patch generated against 2.4; it also applies to 2.5.
> > Please apply.
> 
> This seems horribly inappropriate for 2.4 as it may break apps

I have no problem with withdrawing the request for 2.4.  It does mean that
it's almost impossible to write an M:N threading library implementation.
This doesn't concern me too much; I just want you to be aware this is
the tradeoff you're making.

I would still like to see it in 2.5.

-- 
Revolutions do not require corporate support.

Re: [PATCH] fs/locks.c: Fix posix locking for threaded tasks

[Saurabh Desai]

Matthew Wilcox wrote:
> 
> On Wed, Jun 12, 2002 at 10:40:07AM +0100, Alan Cox wrote:
> > > SUS v3 does not offer any enlightenment.  But it seems reasonable that
> > > processes which share a files_struct should share locks.  After all,
> > > if one process closes the fd, they'll remove locks belonging to the
> > > other process.
> > >
> > > Here's a patch generated against 2.4; it also applies to 2.5.
> > > Please apply.
> >
> > This seems horribly inappropriate for 2.4 as it may break apps
> 
> I have no problem with withdrawing the request for 2.4.  It does mean that
> it's almost impossible to write an M:N threading library implementation.
> This doesn't concern me too much; I just want you to be aware this is
> the tradeoff you're making.
> 
> I would still like to see it in 2.5.

Yes, it's needed for M:N threading library. Here is scenario: Task A
holds a lock and waiting for some event in library, now task B tries
to acquire that lock and waits in kernel and this can create a deadlock.
These tasks are created with CLONE_THREAD (for M:N) flag. 
This change (removing pid check) may cause problem for 1:1 (linuxthreads),
where each task has unique pid and tgid. Again, whether that's a right 
behavior or not is questionable. 
However, with CLONE_THREAD flag, all tasks shares "tgid" value with unique
pid and that's why I suggested earlier to change the "fl_pid" from "pid" 
to "tgid" and it works for both the cases (M:N and 1:1).

Re: [PATCH] fs/locks.c: Fix posix locking for threaded tasks

[Matthew Wilcox]

On Wed, Jun 12, 2002 at 05:18:56PM -0500, Saurabh Desai wrote:
> Yes, it's needed for M:N threading library. Here is scenario: Task A
> holds a lock and waiting for some event in library, now task B tries
> to acquire that lock and waits in kernel and this can create a deadlock.
> These tasks are created with CLONE_THREAD (for M:N) flag. 
> This change (removing pid check) may cause problem for 1:1 (linuxthreads),
> where each task has unique pid and tgid. Again, whether that's a right 
> behavior or not is questionable. 
> However, with CLONE_THREAD flag, all tasks shares "tgid" value with unique
> pid and that's why I suggested earlier to change the "fl_pid" from "pid" 
> to "tgid" and it works for both the cases (M:N and 1:1).

But then we have different behaviour for applications which are linked
against a 1:1 library and an M:N library.  That makes no sense.

-- 
Revolutions do not require corporate support.