[PATCH fs/locks 2 of 3] Use proper tgid in locks_remove_posix

[PATCH fs/locks 2 of 3] Use proper tgid in locks_remove_posix

[Olaf Kirch]

# Subject: [NFS] Use proper tgid in locks_remove_posix
# 
# This patch fixes a problem with lost unlock calls in multithreaded
# applications. locks_remove_posix would send a single unlock call with
# the task group ID of the current process, which may not be the same as
# the tgid of the lock. This patch ensures that we send separate unlock
# calls for different fl_pid values.
# 
# Signed-off-by: Olaf Kirch <okir@suse.de>

Index: linux-2.6.12/fs/locks.c
===================================================================
--- linux-2.6.12.orig/fs/locks.c	2005-07-11 12:16:41.000000000 +0200
+++ linux-2.6.12/fs/locks.c	2005-07-11 12:17:15.000000000 +0200
@@ -1811,47 +1811,60 @@ out:
 void locks_remove_posix(struct file *filp, fl_owner_t owner)
 {
 	struct file_lock lock, **before;
+	pid_t tgid;
+	int more;
 
-	/*
-	 * If there are no locks held on this file, we don't need to call
-	 * posix_lock_file().  Another process could be setting a lock on this
-	 * file at the same time, but we wouldn't remove that lock anyway.
-	 */
-	before = &filp->f_dentry->d_inode->i_flock;
-	if (*before == NULL)
-		return;
-
-	lock.fl_type = F_UNLCK;
-	lock.fl_flags = FL_POSIX;
-	lock.fl_start = 0;
-	lock.fl_end = OFFSET_MAX;
-	lock.fl_owner = owner;
-	lock.fl_pid = current->tgid;
-	lock.fl_file = filp;
-	lock.fl_ops = NULL;
-	lock.fl_lmops = NULL;
-
-	if (filp->f_op && filp->f_op->lock != NULL) {
-		filp->f_op->lock(filp, F_SETLK, &lock);
-		goto out;
-	}
+	do {
+		/*
+		 * If there are no locks held on this file, we don't need to call
+		 * posix_lock_file().  Another process could be setting a lock on this
+		 * file at the same time, but we wouldn't remove that lock anyway.
+		 */
+		before = &filp->f_dentry->d_inode->i_flock;
+		if (*before == NULL)
+			return;
+
+		more = 0;
+		tgid = 0;
+
+		/* Can't use posix_lock_file here; we need to remove it no matter
+		 * which pid we have.
+		 */
+		lock_kernel();
+		while (*before != NULL) {
+			struct file_lock *fl = *before;
+			if (IS_POSIX(fl) && posix_same_owner(fl, &lock)) {
+				if (tgid == 0 || tgid == fl->fl_pid) {
+					tgid = fl->fl_pid;
+					locks_delete_lock(before);
+					continue;
+				}
+				/* We have locks owned by a different tgid.
+				 * Need to come back. */
+				more = 1;
+			}
+			before = &fl->fl_next;
+		}
+		unlock_kernel();
 
-	/* Can't use posix_lock_file here; we need to remove it no matter
-	 * which pid we have.
-	 */
-	lock_kernel();
-	while (*before != NULL) {
-		struct file_lock *fl = *before;
-		if (IS_POSIX(fl) && posix_same_owner(fl, &lock)) {
-			locks_delete_lock(before);
-			continue;
+		if (tgid != 0) {
+			lock.fl_type = F_UNLCK;
+			lock.fl_flags = FL_POSIX;
+			lock.fl_start = 0;
+			lock.fl_end = OFFSET_MAX;
+			lock.fl_owner = owner;
+			lock.fl_pid = tgid;
+			lock.fl_file = filp;
+			lock.fl_ops = NULL;
+			lock.fl_lmops = NULL;
+
+			if (filp->f_op && filp->f_op->lock != NULL) {
+				filp->f_op->lock(filp, F_SETLK, &lock);
+				if (lock.fl_ops && lock.fl_ops->fl_release_private)
+					lock.fl_ops->fl_release_private(&lock);
+			}
 		}
-		before = &fl->fl_next;
-	}
-	unlock_kernel();
-out:
-	if (lock.fl_ops && lock.fl_ops->fl_release_private)
-		lock.fl_ops->fl_release_private(&lock);
+	} while (more);
 }
 
 EXPORT_SYMBOL(locks_remove_posix);

-- 
Olaf Kirch   |  --- o --- Nous sommes du soleil we love when we play
okir@suse.de |    / | \   sol.dhoop.naytheet.ah kin.ir.samse.qurax


-------------------------------------------------------
This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening
July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual
core and dual graphics technology at this free one hour event hosted by HP, 
AMD, and NVIDIA.  To register visit http://www.hp.com/go/dualwebinar
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH fs/locks 2 of 3] Use proper tgid in locks_remove_posix

[Trond Myklebust]

m=C3=A5 den 11.07.2005 Klokka 12:32 (+0200) skreiv Olaf Kirch:
> # Subject: [NFS] Use proper tgid in locks_remove_posix
> #=20
> # This patch fixes a problem with lost unlock calls in multithreaded
> # applications. locks_remove_posix would send a single unlock call with
> # the task group ID of the current process, which may not be the same as
> # the tgid of the lock. This patch ensures that we send separate unlock
> # calls for different fl_pid values.

This doesn't look right. Why should a thread be killing locks that
belong to a different process?

Cheers,
  Trond



-------------------------------------------------------
This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening
July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual
core and dual graphics technology at this free one hour event hosted by HP, 
AMD, and NVIDIA.  To register visit http://www.hp.com/go/dualwebinar
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH fs/locks 2 of 3] Use proper tgid in locks_remove_posix

[Olaf Kirch]

On Mon, Jul 11, 2005 at 08:16:57AM -0400, Trond Myklebust wrote:
> m=E5 den 11.07.2005 Klokka 12:32 (+0200) skreiv Olaf Kirch:
> > # Subject: [NFS] Use proper tgid in locks_remove_posix
> > #=20
> > # This patch fixes a problem with lost unlock calls in multithreaded
> > # applications. locks_remove_posix would send a single unlock call wi=
th
> > # the task group ID of the current process, which may not be the same=
 as
> > # the tgid of the lock. This patch ensures that we send separate unlo=
ck
> > # calls for different fl_pid values.
>=20
> This doesn't look right. Why should a thread be killing locks that
> belong to a different process?

locks_remove_posix is called when we close a file, i.e. when it is remove=
d
from current->files. The problem arises when the files struct is shared
by several threads with different tgid (which seems to be the default
when using pthreads) - in this case we must make sure we send the
unlock call with the right tgid. Previously we would send the unlock call
using fl_pid =3D current->tgid.

Olaf
--=20
Olaf Kirch   |  --- o --- Nous sommes du soleil we love when we play
okir@suse.de |    / | \   sol.dhoop.naytheet.ah kin.ir.samse.qurax


-------------------------------------------------------
This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening
July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual
core and dual graphics technology at this free one hour event hosted by HP, 
AMD, and NVIDIA.  To register visit http://www.hp.com/go/dualwebinar
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH fs/locks 2 of 3] Use proper tgid in locks_remove_posix

[Trond Myklebust]

m=C3=A5 den 11.07.2005 Klokka 14:48 (+0200) skreiv Olaf Kirch:
> On Mon, Jul 11, 2005 at 08:16:57AM -0400, Trond Myklebust wrote:
> > m=C3=A5 den 11.07.2005 Klokka 12:32 (+0200) skreiv Olaf Kirch:
> > > # Subject: [NFS] Use proper tgid in locks_remove_posix
> > > #=20
> > > # This patch fixes a problem with lost unlock calls in multithreaded
> > > # applications. locks_remove_posix would send a single unlock call wi=
th
> > > # the task group ID of the current process, which may not be the same=
 as
> > > # the tgid of the lock. This patch ensures that we send separate unlo=
ck
> > > # calls for different fl_pid values.
> >=20
> > This doesn't look right. Why should a thread be killing locks that
> > belong to a different process?
>=20
> locks_remove_posix is called when we close a file, i.e. when it is remove=
d
> from current->files. The problem arises when the files struct is shared
> by several threads with different tgid (which seems to be the default
> when using pthreads) - in this case we must make sure we send the
> unlock call with the right tgid. Previously we would send the unlock call
> using fl_pid =3D current->tgid.
>=20

For posix locks we should pretty much be ignoring the value of fl_pid.
It gets returned to the user in a F_GETLK call, but all the low-level
functions (such as posix_same_owner) should be testing the value of
fl_owner instead.

The exception here is nlmsvc_same_owner(), which has to follow the NLM
byte range spec but doesn't rely on locks_remove_posix. The NFSv4 server
code may do something cranky too, but that again won't rely on
locks_remove_posix.

Cheers,
  Trond



-------------------------------------------------------
This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening
July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual
core and dual graphics technology at this free one hour event hosted by HP, 
AMD, and NVIDIA.  To register visit http://www.hp.com/go/dualwebinar
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH fs/locks 2 of 3] Use proper tgid in locks_remove_posix

[Olaf Kirch]

On Mon, Jul 11, 2005 at 09:02:21AM -0400, Trond Myklebust wrote:
> For posix locks we should pretty much be ignoring the value of fl_pid.
> It gets returned to the user in a F_GETLK call, but all the low-level
> functions (such as posix_same_owner) should be testing the value of
> fl_owner instead.
> 
> The exception here is nlmsvc_same_owner(), which has to follow the NLM
> byte range spec but doesn't rely on locks_remove_posix.

Let me try to explain this a little better. What I believe is happening
is that on the client, thread A with tgid 0xA establishes a lock,
and when thread B closes the file. locks_remove_posix sends an unlock request
to the server with tgid 0xB.

On the server, posix_lock_file walks the list of all existing locks,
comparing the lock owner via nlmsvc_same_owner - and since the fl_pid
value of the request is 0xB, this misses the established lock with
fl_pid == 0xA.

Olaf
-- 
Olaf Kirch   |  --- o --- Nous sommes du soleil we love when we play
okir@suse.de |    / | \   sol.dhoop.naytheet.ah kin.ir.samse.qurax


-------------------------------------------------------
This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening
July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual
core and dual graphics technology at this free one hour event hosted by HP, 
AMD, and NVIDIA.  To register visit http://www.hp.com/go/dualwebinar
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH fs/locks 2 of 3] Use proper tgid in locks_remove_posix

[Peter Staubach]

Olaf Kirch wrote:

>On Mon, Jul 11, 2005 at 09:02:21AM -0400, Trond Myklebust wrote:
>  
>
>>For posix locks we should pretty much be ignoring the value of fl_pid.
>>It gets returned to the user in a F_GETLK call, but all the low-level
>>functions (such as posix_same_owner) should be testing the value of
>>fl_owner instead.
>>
>>The exception here is nlmsvc_same_owner(), which has to follow the NLM
>>byte range spec but doesn't rely on locks_remove_posix.
>>    
>>
>
>Let me try to explain this a little better. What I believe is happening
>is that on the client, thread A with tgid 0xA establishes a lock,
>and when thread B closes the file. locks_remove_posix sends an unlock request
>to the server with tgid 0xB.
>
>On the server, posix_lock_file walks the list of all existing locks,
>comparing the lock owner via nlmsvc_same_owner - and since the fl_pid
>value of the request is 0xB, this misses the established lock with
>fl_pid == 0xA.
>

I am not particularly following this logic.  It sounds reasonable, but I 
would
feel better if you had some way of reproducing and showing the problem.

    Thanx...

       ps


-------------------------------------------------------
This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening
July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual
core and dual graphics technology at this free one hour event hosted by HP, 
AMD, and NVIDIA.  To register visit http://www.hp.com/go/dualwebinar
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH fs/locks 2 of 3] Use proper tgid in locks_remove_posix

[Trond Myklebust]

m=C3=A5 den 11.07.2005 Klokka 15:16 (+0200) skreiv Olaf Kirch:
> On Mon, Jul 11, 2005 at 09:02:21AM -0400, Trond Myklebust wrote:
> > For posix locks we should pretty much be ignoring the value of fl_pid.
> > It gets returned to the user in a F_GETLK call, but all the low-level
> > functions (such as posix_same_owner) should be testing the value of
> > fl_owner instead.
> >=20
> > The exception here is nlmsvc_same_owner(), which has to follow the NLM
> > byte range spec but doesn't rely on locks_remove_posix.
>=20
> Let me try to explain this a little better. What I believe is happening
> is that on the client, thread A with tgid 0xA establishes a lock,
> and when thread B closes the file. locks_remove_posix sends an unlock req=
uest
> to the server with tgid 0xB.
>=20
> On the server, posix_lock_file walks the list of all existing locks,
> comparing the lock owner via nlmsvc_same_owner - and since the fl_pid
> value of the request is 0xB, this misses the established lock with
> fl_pid =3D=3D 0xA.

Then let me be more precise. The server should never see any value
fl_pid =3D=3D 0xA or 0xB.

In the case of NLM, we use the function nlm_find_lockowner() to create a
mapping between fl_owner and a 32-bit numerical value which we call
"pid", but which is really just a unique number. This mapping is
preserved at least for the lifetime of the lock, and is shared between
all locks that reference the same fl_owner.
The "pid" of this mapping is the number that is seen by the server.

Aside from ensuring that the current locks_remove_posix works properly,
it also ensures that the client and server have the same behaviour
w.r.t. merging locks. Recall that the client will merge locks with the
same fl_owner, and the server merges locks with the same pid. If the two
were to differ...

Note that the NFSv4 protocol uses a string called a "lockowner" instead
of the 32-bit "pid" value, so we don't need to maintain a special
mapping there.

Cheers,
  Trond



-------------------------------------------------------
This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening
July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual
core and dual graphics technology at this free one hour event hosted by HP, 
AMD, and NVIDIA.  To register visit http://www.hp.com/go/dualwebinar
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH fs/locks 2 of 3] Use proper tgid in locks_remove_posix

[Olaf Kirch]

[-- Attachment #1: Type: text/plain, Size: 1027 bytes --]

On Mon, Jul 11, 2005 at 09:52:17AM -0400, Trond Myklebust wrote:
> In the case of NLM, we use the function nlm_find_lockowner() to create a
> mapping between fl_owner and a 32-bit numerical value which we call
> "pid", but which is really just a unique number. This mapping is
> preserved at least for the lifetime of the lock, and is shared between
> all locks that reference the same fl_owner.
> The "pid" of this mapping is the number that is seen by the server.

Alright. I stand corrected.

I originally reproduced this problem on SLES (ie 2.6.5), where the
lockowner stuff didn't exist yet. I then retested on 2.6.12, and found
the same problem - but didn't notice the mechanism is entirely different.
So whatever I'm seeing on 2.6.12 is different from what I saw on 2.6.5.

I'll do some more testing - in case anyone else would like to do so, too,
the test app is attached.

Olaf
-- 
Olaf Kirch   |  --- o --- Nous sommes du soleil we love when we play
okir@suse.de |    / | \   sol.dhoop.naytheet.ah kin.ir.samse.qurax

[-- Attachment #2: two-thread-unlock.c --]
[-- Type: text/plain, Size: 1414 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <signal.h>
#include <unistd.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/wait.h>
#include <sys/param.h>
#include <fcntl.h>
#include <pthread.h>
#include <string.h>

int	fd;
int	running = 1;
int	locked = 0;

int
get_lock (int fd)
{
	struct flock    lock_it;
	int		res = 0;

	lock_it.l_type = F_WRLCK;
	lock_it.l_whence = SEEK_SET;
	lock_it.l_start = 0;
	lock_it.l_len = 0;

	if (fcntl (fd, F_SETLK, &lock_it) < 0) {
		perror ("fcntl(F_WRLCK)");
		res = -1;
	}

	return res;
}


void *
do_lock(void *arg)
{
	if (get_lock(fd) >= 0) {
		printf("Locked file\n");
		sleep(1);
		locked = 1;
	}

	while (running)
		sleep(1);
	return NULL;
}

void *
do_close(void *arg)
{
	int i;

	for (i = 0; !locked && i < 10; ++i)
		sleep(1);
	if (locked)
		close(fd);
	else
		printf("Never obtained the lock\n");
	running = 0;
	return NULL;
}

int
main(int argc, char** argv)
{
#define THREADS 1

	pthread_t	locker_thread;
	pthread_t	close_thread;

	if (argc != 2) {
		fprintf(stderr, "%s <path>\n", argv[0]);
		exit(-1);
	}

	if ((fd = open (argv[1], O_RDWR | O_CREAT, S_IRUSR | S_IWUSR)) < 0) {
		perror ("open");
		return 1;
	}

	pthread_create(&locker_thread, NULL, do_lock, NULL);
	pthread_create(&close_thread, NULL, do_close, NULL);
	sleep(2);

	running = 0;
	pthread_join(locker_thread, NULL);
	pthread_join(close_thread, NULL);

	return 0;
}

Re: [PATCH fs/locks 2 of 3] Use proper tgid in locks_remove_posix

[Olaf Hering]

 On Mon, Jul 11, Olaf Kirch wrote:

> # Subject: [NFS] Use proper tgid in locks_remove_posix
> # 
> # This patch fixes a problem with lost unlock calls in multithreaded
> # applications. locks_remove_posix would send a single unlock call with
> # the task group ID of the current process, which may not be the same as
> # the tgid of the lock. This patch ensures that we send separate unlock
> # calls for different fl_pid values.
> # 
> # Signed-off-by: Olaf Kirch <okir@suse.de>
> 
> Index: linux-2.6.12/fs/locks.c

This leads to glibc make check errors on ppc64:

cat /usr/src/packages/BUILD/glibc-2.3/cc-nptl/nptl/tst-cancel16.out
Timed out: killed the child process
1

tested with 2.6.13-rc3 and 'powerpc32 make check'


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH fs/locks 2 of 3] Use proper tgid in locks_remove_posix

[Olaf Kirch]

On Sat, Jul 16, 2005 at 05:15:27PM +0200, Olaf Hering wrote:
> This leads to glibc make check errors on ppc64:
> 
> cat /usr/src/packages/BUILD/glibc-2.3/cc-nptl/nptl/tst-cancel16.out
> Timed out: killed the child process
> 1
> 
> tested with 2.6.13-rc3 and 'powerpc32 make check'

I thought we had fixed those. 

Anyway, as Trond pointed out, this patch is is irrelevant on 2.6.12.

Olaf
-- 
Olaf Kirch   |  --- o --- Nous sommes du soleil we love when we play
okir@suse.de |    / | \   sol.dhoop.naytheet.ah kin.ir.samse.qurax


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs