Use proper tgid in locks_remove_posix

# 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 <[email protected]>

Index: linux-2.6.12/fs/locks.c
===================================================================
@@ -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
[email protected] |    / | \   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  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

m=C3=A5 den 11.07.2005 Klokka 12:32 (+0200) skreiv Olaf Kirch:

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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

th

as

ck

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
[email protected] | / | \ 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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

m=C3=A5 den 11.07.2005 Klokka 14:48 (+0200) skreiv Olaf Kirch:

th

as

ck

d

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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

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
[email protected] |    / | \   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  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

m=C3=A5 den 11.07.2005 Klokka 15:16 (+0200) skreiv Olaf Kirch:

uest

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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

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
[email protected] |    / | \   sol.dhoop.naytheet.ah kin.ir.samse.qurax

#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;
}

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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

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
[email protected] |    / | \   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  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs