Advisory locking fixes:

* Made the access strategy to vnode::advisory_locking consistent.
  get_advisory_locking() was guarding it with sVnodeMutex,
  create_advisory_locking() was using atomic_pointer_test_and_set(), and
  release_advisory_lock() just set it unguardedly. We do use sVnodeMutex
  consequently, now.
* Beautified create_advisory_locking() (got rid of the gotos,
  reorganized the control flow).
* Fixed race conditions in acquire_advisory_lock(). It was always
  unlocking and relocking the advisory_locking object when it didn't
  have to wait, but in the meantime someone else could have changed the
  locking situation. Reorganized the control flow, so that it only drops
  the lock when it has to fail or wait. Using create_advisory_locking()
  upfront simplifies the code quite a bit (and fixes another race
  condition).

APR's testprocmutex test seems happy now, at least.


git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@25407 a95241bf-73f2-0310-859d-f6bbb57e9c96

src/system/kernel/fs/vfs.cpp

@@ -27,6 +27,7 @@
 #include <OS.h>
 #include <StorageDefs.h>
 
+#include <AutoDeleter.h>
 #include <block_cache.h>
 #include <boot/kernel_args.h>
 #include <disk_device_manager/KDiskDevice.h>
@@ -151,6 +152,21 @@ struct advisory_locking {
 	sem_id			lock;
 	sem_id			wait_sem;
 	LockList		locks;
+
+	advisory_locking()
+		:
+		lock(-1),
+		wait_sem(-1)
+	{
+	}
+
+	~advisory_locking()
+	{
+		if (lock >= 0)
+			delete_sem(lock);
+		if (wait_sem >= 0)
+			delete_sem(wait_sem);
+	}
 };
 
 static mutex sFileSystemsMutex;
@@ -189,7 +205,7 @@ static mutex sVnodeCoveredByMutex;
 
 /*!	\brief Guards sVnodeTable.
 
-	The holder is allowed to read/write access sVnodeTable and to
+	The holder is allowed read/write access to sVnodeTable and to
 	any unbusy vnode in that table, save to the immutable fields (device, id,
 	private_node, mount) to which
 	only read-only access is allowed, and to the field covered_by, which is
@@ -1122,42 +1138,38 @@ create_advisory_locking(struct vnode *vnode)
 	if (vnode == NULL)
 		return B_FILE_ERROR;
 
-	struct advisory_locking *locking = new(std::nothrow) advisory_locking;
+	ObjectDeleter<advisory_locking> lockingDeleter;
-	if (locking == NULL)
+	struct advisory_locking *locking = NULL;
-		return B_NO_MEMORY;
 
-	status_t status;
+	while (get_advisory_locking(vnode) == NULL) {
+		// no locking object set on the vnode yet, create one
+		if (locking == NULL) {
+			locking = new(std::nothrow) advisory_locking;
+			if (locking == NULL)
+				return B_NO_MEMORY;
+			lockingDeleter.SetTo(locking);
 
-	locking->wait_sem = create_sem(0, "advisory lock");
+			locking->wait_sem = create_sem(0, "advisory lock");
-	if (locking->wait_sem < B_OK) {
+			if (locking->wait_sem < B_OK)
-		status = locking->wait_sem;
+				return locking->wait_sem;
-		goto err1;
-	}
 
-	locking->lock = create_sem(0, "advisory locking");
+			locking->lock = create_sem(0, "advisory locking");
-	if (locking->lock < B_OK) {
+			if (locking->lock < B_OK)
-		status = locking->lock;
+				return locking->lock;
-		goto err2;
+		}
-	}
 
-	// We need to set the locking structure atomically - someone
+		// set our newly created locking object
-	// else might set one at the same time
+		MutexLocker _(sVnodeMutex);
-	do {
+		if (vnode->advisory_locking == NULL) {
-		if (atomic_pointer_test_and_set(&vnode->advisory_locking, locking,
+			vnode->advisory_locking = locking;
-				(advisory_locking*)NULL) == NULL)
+			lockingDeleter.Detach();
 			return B_OK;
-	} while (get_advisory_locking(vnode) == NULL);
+		}
+	}
 
-	status = B_OK;
+	// The vnode already had a locking object. That's just as well.
-		// we delete the one we've just created, but nevertheless, the vnode
-		// does have a locking structure now
 
-	delete_sem(locking->lock);
+	return B_OK;
-err2:
-	delete_sem(locking->wait_sem);
-err1:
-	delete locking;
-	return status;
 }
 
 
@@ -1287,9 +1299,12 @@ release_advisory_lock(struct vnode *vnode, struct flock *flock)
 		// longer used
 		locking = get_advisory_locking(vnode);
 		if (locking != NULL) {
+			MutexLocker locker(sVnodeMutex);
+
 			// the locking could have been changed in the mean time
 			if (locking->locks.IsEmpty()) {
 				vnode->advisory_locking = NULL;
+				locker.Unlock();
 
 				// we've detached the locking from the vnode, so we can
 				// safely delete it
@@ -1298,6 +1313,7 @@ release_advisory_lock(struct vnode *vnode, struct flock *flock)
 				delete locking;
 			} else {
 				// the locking is in use again
+				locker.Unlock();
 				release_sem_etc(locking->lock, 1, B_DO_NOT_RESCHEDULE);
 			}
 		}
@@ -1328,19 +1344,25 @@ acquire_advisory_lock(struct vnode *vnode, pid_t session, struct flock *flock,
 
 	// TODO: do deadlock detection!
 
-restart:
+	struct advisory_locking *locking;
-	// if this vnode has an advisory_locking structure attached,
+	sem_id waitForLock;
-	// lock that one and search for any colliding file lock
+
-	struct advisory_locking *locking = get_advisory_locking(vnode);
+	while (true) {
-	team_id team = team_get_current_team_id();
+		// if this vnode has an advisory_locking structure attached,
-	sem_id waitForLock = -1;
+		// lock that one and search for any colliding file lock
+		status = create_advisory_locking(vnode);
+		if (status != B_OK)
+			return status;
+
+		locking = vnode->advisory_locking;
+		team_id team = team_get_current_team_id();
+		waitForLock = -1;
 
-	if (locking != NULL) {
 		// test for collisions
 		LockList::Iterator iterator = locking->locks.GetIterator();
 		while (iterator.HasNext()) {
 			struct advisory_lock *lock = iterator.Next();
-
+	
 			// TODO: locks from the same team might be joinable!
 			if (lock->team != team && advisory_lock_intersects(lock, flock)) {
 				// locks do overlap
@@ -1352,41 +1374,27 @@ restart:
 			}
 		}
 
-		if (waitForLock < B_OK || !wait)
+		if (waitForLock < 0)
+			break;
+
+		// We need to wait. Do that or fail now, if we've been asked not to.
+
+		if (!wait) {
 			put_advisory_locking(locking);
-	}
+			return session != -1 ? B_WOULD_BLOCK : B_PERMISSION_DENIED;
-
-	// wait for the lock if we have to, or else return immediately
-
-	if (waitForLock >= B_OK) {
-		if (!wait)
-			status = session != -1 ? B_WOULD_BLOCK : B_PERMISSION_DENIED;
-		else {
-			status = switch_sem_etc(locking->lock, waitForLock, 1,
-				B_CAN_INTERRUPT, 0);
-			if (status == B_OK) {
-				// see if we're still colliding
-				goto restart;
-			}
 		}
-	}
 
-	if (status < B_OK)
+		status = switch_sem_etc(locking->lock, waitForLock, 1,
-		return status;
+			B_CAN_INTERRUPT, 0);
-
+		if (status != B_OK && status != B_BAD_SEM_ID)
-	// install new lock
-
-	locking = get_advisory_locking(vnode);
-	if (locking == NULL) {
-		// we need to create a new locking object
-		status = create_advisory_locking(vnode);
-		if (status < B_OK)
 			return status;
 
-		locking = vnode->advisory_locking;
+		// We have been notified, but we need to re-lock the locking object. So
-			// we own the locking object, so it can't go away
+		// go another round...
 	}
 
+	// install new lock
+
 	struct advisory_lock *lock = (struct advisory_lock *)malloc(
 		sizeof(struct advisory_lock));
 	if (lock == NULL) {