DRAFT! write lock priority

Hi,

I am attaching a draft version of the write lock priority patch. Other than running the testsuite, it is completely untested. I am running away for about an hour now, I'll read it through again when I'm back and see what can be done about testing, too.

Yours,
Petr.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: locking.diff
Type: text/x-diff
Size: 5414 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090812/e0800c63/attachment.bin>

I have split the changes to file_locking.c into two patches to aid reviewability and attached them (out of tree context, sorry).

The first does the refactoring (reviewed).

The second adds the new functionality (not reviewed yet, but now only 60-odd lines).

Changes I made include:
Using _do_flock() and _undo_flock(). (Underscore plus existing library fn
name is not a good idea.)
Dropping the implicit cast from uint32_t to int for nonblock. Adding some stacks (missing from existing code).

@@ -43,13 +43,26 @@ static sig_t _oldhandler;
 static sigset_t _fullsigset, _intsigset;
 static volatile sig_atomic_t _handler_installed;
 
+static void _undo_flock(const char *file, int fd)
+{
+	struct stat buf1, buf2;
+
+	if (!flock(fd, LOCK_NB | LOCK_EX) &&
+	    !stat(file, &buf1) &&
+	    !fstat(fd, &buf2) &&
+	    is_same_inode(buf1, buf2))
+		if (unlink(file))
+			log_sys_error("unlink", file);
+
+	if (close(fd) < 0)
+		log_sys_error("close", file);
+}
+
 static int _release_lock(const char *file, int unlock)
 {
 	struct lock_list *ll;
 	struct dm_list *llh, *llt;
 
-	struct stat buf1, buf2;
-
 	dm_list_iterate_safe(llh, llt, &_lock_list) {
 		ll = dm_list_item(llh, struct lock_list);
 

@@ -61,15 +74,7 @@ static int _release_lock(const char *fil
 					log_sys_error("flock", ll->res);
 			}
 
-			if (!flock(ll->lf, LOCK_NB | LOCK_EX) &&
-			    !stat(ll->res, &buf1) &&
-			    !fstat(ll->lf, &buf2) &&
-			    is_same_inode(buf1, buf2))
-				if (unlink(ll->res))
-					log_sys_error("unlink", ll->res);
-
-			if (close(ll->lf) < 0)
-				log_sys_error("close", ll->res);
+			_undo_flock(ll->res, ll->lf);
 
 			dm_free(ll->res);
 			dm_free(llh);

@@ -124,14 +129,53 @@ static void _install_ctrl_c_handler()
 	siginterrupt(SIGINT, 1);
 }
 
-static int _lock_file(const char *file, uint32_t flags)
+static int _do_flock(const char *file, int *fd, int operation, uint32_t nonblock)
 {
-	int operation;
 	int r = 1;
 	int old_errno;
+	struct stat buf1, buf2;
+
+	do {
+		if ((*fd > -1) && close(*fd))
+			log_sys_error("close", file);
+
+		if ((*fd = open(file, O_CREAT | O_APPEND | O_RDWR, 0777)) < 0) {
+			log_sys_error("open", file);
+			return 0;
+		}
+
+		if (nonblock)
+			operation |= LOCK_NB;
+		else
+			_install_ctrl_c_handler();
+
+		r = flock(*fd, operation);
+		old_errno = errno;
+		if (!nonblock)
+			_remove_ctrl_c_handler();
+
+		if (r) {
+			errno = old_errno;
+			log_sys_error("flock", file);
+			close(*fd);
+			return 0;
+		}
+
+		if (!stat(file, &buf1) && !fstat(*fd, &buf2) &&
+		    is_same_inode(buf1, buf2))
+			return 1;
+	} while (!nonblock);
+
+	return_0; // AGK FIXME Note this bug fix in WHATS_NEW.
+}
+
+static int _lock_file(const char *file, uint32_t flags)
+{
+	int operation;
+	uint32_t nonblock = flags & LCK_NONBLOCK;
+	int r;
 
 	struct lock_list *ll;
-	struct stat buf1, buf2;
 	char state;
 
 	switch (flags & LCK_TYPE_MASK) {

@@ -151,56 +195,28 @@ static int _lock_file(const char *file, 
 	}
 
 	if (!(ll = dm_malloc(sizeof(struct lock_list))))
-		return 0;
+		return_0;
 
 	if (!(ll->res = dm_strdup(file))) {
 		dm_free(ll);
-		return 0;
+		return_0;
 	}
 
 	ll->lf = -1;
 
 	log_very_verbose("Locking %s %c%c", ll->res, state,
-			 flags & LCK_NONBLOCK ? ' ' : 'B');
-	do {
-		if ((ll->lf > -1) && close(ll->lf))
-			log_sys_error("close", file);
-
-		if ((ll->lf = open(file, O_CREAT | O_APPEND | O_RDWR, 0777))
-		    < 0) {
-			log_sys_error("open", file);
-			goto err;
-		}
+			 nonblock ? ' ' : 'B');
 
-		if ((flags & LCK_NONBLOCK))
-			operation |= LOCK_NB;
-		else
-			_install_ctrl_c_handler();
-
-		r = flock(ll->lf, operation);
-		old_errno = errno;
-		if (!(flags & LCK_NONBLOCK))
-			_remove_ctrl_c_handler();
-
-		if (r) {
-			errno = old_errno;
-			log_sys_error("flock", ll->res);
-			close(ll->lf);
-			goto err;
-		}
-
-		if (!stat(ll->res, &buf1) && !fstat(ll->lf, &buf2) &&
-		    is_same_inode(buf1, buf2))
-			break;
-	} while (!(flags & LCK_NONBLOCK));
-
-	dm_list_add(&_lock_list, &ll->list);
-	return 1;
+	r = _do_flock(file, &ll->lf, operation, nonblock);
+	if (r)
+		dm_list_add(&_lock_list, &ll->list);
+	else {
+		dm_free(ll->res);
+		dm_free(ll);
+		stack;
+	}
 
-      err:
-	dm_free(ll->res);
-	dm_free(ll);
-	return 0;
+	return r;
 }
 
 static int _file_lock_resource(struct cmd_context *cmd, const char *resource,

@@ -38,6 +38,7 @@ struct lock_list {
 
 static struct dm_list _lock_list;
 static char _lock_dir[NAME_LEN];
+static int _write_priority;
 
 static sig_t _oldhandler;
 static sigset_t _fullsigset, _intsigset;

@@ -172,11 +173,19 @@ static int _do_flock(const char *file, i
 static int _lock_file(const char *file, uint32_t flags)
 {
 	int operation;
+	int fd_aux = -1;
 	uint32_t nonblock = flags & LCK_NONBLOCK;
 	int r;
 
 	struct lock_list *ll;
 	char state;
+	char *file_aux = alloca(strlen(file) + 5);
+
+	strcpy(file_aux, file);
+	/* There is always a slash before the basename part of the filename. */
+	*(strrchr(file_aux, '/') + 1) = 0;
+	strcat(file_aux, "aux_");
+	strcat(file_aux, strrchr(file, '/') + 1);
 
 	switch (flags & LCK_TYPE_MASK) {
 	case LCK_READ:

@@ -207,7 +216,22 @@ static int _lock_file(const char *file, 
 	log_very_verbose("Locking %s %c%c", ll->res, state,
 			 nonblock ? ' ' : 'B');
 
-	r = _do_flock(file, &ll->lf, operation, nonblock);
+	if (!_write_priority) {
+		r = _do_flock(file, &ll->lf, operation, nonblock);
+	else {
+		if (flags & LCK_WRITE) {
+			if ((r = _do_flock(file_aux, &fd_aux, LOCK_EX, nonblock))) {
+				r = _do_flock(file, &ll->lf, operation, nonblock);
+				_unflock(file_aux, fd_aux);
+			}
+		} else {
+			if ((r = _do_flock(file_aux, &fd_aux, LOCK_EX, nonblock))) {
+				_unflock(file_aux, fd_aux);
+				r = _do_flock(file, &ll->lf, operation, nonblock);
+			}
+		}
+	}
+
 	if (r)
 		dm_list_add(&_lock_list, &ll->list);
 	else {

@@ -299,6 +323,9 @@ int init_file_locking(struct locking_typ
 						DEFAULT_LOCK_DIR),
 		sizeof(_lock_dir));
 
+	_write_priority = find_config_tree_bool(cmd, "global/write_lock_priority",
+						DEFAULT_WRITE_LOCK_PRIORITY);
+
 	if (!dm_create_dir(_lock_dir))
 		return 0;
 

Naming should follow a consistent pattern throughout - the #define here, the config option name, internal variables etc.

I'm still not sure what best to call it, without it becoming rather too long. I reckon 'prioritisation' works better than 'priority' though with this proposal i.e. global/write_lock_prioritisation.
That it only (currently) affects "local file-based locking (type 1)" can be covered by the comments in example.conf.

The names should all match, once one has been selected.

+ char *file_aux = alloca(strlen(file) + 5);
Use sizeof instead of 5.

+ strcpy(file_aux, file);
+ /* There is always a slash before the basename part of the filename. */
+ *(strrchr(file_aux, '/') + 1) = 0;
+ strcat(file_aux, "aux_");
Use #define (tie to the sizeof above).

+ strcat(file_aux, strrchr(file, '/') + 1);

Add a comment to show what the result of this code is. "aux_" is a decent idea for the prefix: any other ideas? Something based on 'waiting' or 'queue'?
Best might be a suffix on the existing name, with a separator character that cannot appear in a VG name.
V_vg1:queue
Then a sorted 'ls' will list them adacently.

@@ -207,7 +216,22 @@ static int _lock_file(const char *file,
log_very_verbose("Locking %s %c%c", ll->res, state,
nonblock ? ' ' : 'B');

We may be missing some further log messages here: I want every flock operation logged. Maybe the messages should move to _do_flock / _undo_flock, or we should just have additional log_debug ones at this level for the new calls.

- r = _do_flock(file, &ll->lf, operation, nonblock);
+ if (!_write_priority) {
+ r = _do_flock(file, &ll->lf, operation, nonblock);
+ else {
+ if (flags & LCK_WRITE) {
+ if ((r = _do_flock(file_aux, &fd_aux, LOCK_EX, nonblock))) {

Ah - something got missed here: In the design, all these fd_aux locking operations block if the lock is not available, so remove 'nonblock' from those 2 lines. (I don't see that causing any deadlocks, and it removes avoidable failure modes.) Non-blocking locks in LVM are only used to avoid deadlocks - waiting is always acceptable (but will permit the ctrl-c option).

+ r = _do_flock(file, &ll->lf, operation, nonblock);
+ _undo_flock(file_aux, fd_aux);
+ }
+ } else {
+ if ((r = _do_flock(file_aux, &fd_aux, LOCK_EX, nonblock))) {
+ _undo_flock(file_aux, fd_aux);
+ r = _do_flock(file, &ll->lf, operation, nonblock);
+ }
+ }
+ }

Alasdair

Updated version of the second patch.
Handing back over to you for further cleanup, review & testing.

Alasdair

-------------- next part --------------

@@ -38,6 +38,7 @@ struct lock_list {
 
 static struct dm_list _lock_list;
 static char _lock_dir[NAME_LEN];
+static int _prioritise_write_locks;
 
 static sig_t _oldhandler;
 static sigset_t _fullsigset, _intsigset;

@@ -172,11 +173,19 @@ static int _do_flock(const char *file, i
 static int _lock_file(const char *file, uint32_t flags)
 {
 	int operation;
+	int fd_aux = -1;
 	uint32_t nonblock = flags & LCK_NONBLOCK;
 	int r;
 
 	struct lock_list *ll;
 	char state;
+	char *file_aux = alloca(strlen(file) + 5);
+
+	strcpy(file_aux, file);
+	/* There is always a slash before the basename part of the filename. */
+	*(strrchr(file_aux, '/') + 1) = 0;
+	strcat(file_aux, "aux_");
+	strcat(file_aux, strrchr(file, '/') + 1);
 
 	switch (flags & LCK_TYPE_MASK) {
 	case LCK_READ:

@@ -207,7 +216,22 @@ static int _lock_file(const char *file, 
 	log_very_verbose("Locking %s %c%c", ll->res, state,
 			 nonblock ? ' ' : 'B');
 
-	r = _do_flock(file, &ll->lf, operation, nonblock);
+	if (!_prioritise_write_locks)
+		r = _do_flock(file, &ll->lf, operation, nonblock);
+	else {
+		if (flags & LCK_WRITE) {
+			if ((r = _do_flock(file_aux, &fd_aux, LOCK_EX, 0))) {
+				r = _do_flock(file, &ll->lf, operation, nonblock);
+				_undo_flock(file_aux, fd_aux);
+			}
+		} else {
+			if ((r = _do_flock(file_aux, &fd_aux, LOCK_EX, 0))) {
+				_undo_flock(file_aux, fd_aux);
+				r = _do_flock(file, &ll->lf, operation, nonblock);
+			}
+		}
+	}
+
 	if (r)
 		dm_list_add(&_lock_list, &ll->list);
 	else {

@@ -299,6 +323,10 @@ int init_file_locking(struct locking_typ
 						DEFAULT_LOCK_DIR),
 		sizeof(_lock_dir));
 
+	_prioritise_write_locks =
+	    find_config_tree_bool(cmd, "global/prioritise_write_locks",
+				  DEFAULT_PRIORITISE_WRITE_LOCKS);
+
 	if (!dm_create_dir(_lock_dir))
 		return 0;
 

And I do not see reason why do aux lock name exercise when write priority is not used, see patch (on top of Alasdair's change).
Milan

@@ -170,15 +170,9 @@ static int _do_flock(const char *file, i
 	return_0; // AGK FIXME Note this bug fix in WHATS_NEW.
 }
 
-static int _lock_file(const char *file, uint32_t flags)
+static int _do_write_priority_flock(const char *file, int *fd, int operation, uint32_t nonblock)
 {
-	int operation;
-	int fd_aux = -1;
-	uint32_t nonblock = flags & LCK_NONBLOCK;
-	int r;
-
-	struct lock_list *ll;
-	char state;
+	int r, fd_aux = -1;
 	char *file_aux = alloca(strlen(file) + 5);
 
 	strcpy(file_aux, file);

@@ -187,6 +181,28 @@ static int _lock_file(const char *file, 
 	strcat(file_aux, "aux_");
 	strcat(file_aux, strrchr(file, '/') + 1);
 
+	if ((r = _do_flock(file_aux, &fd_aux, LOCK_EX, 0))) {
+		if (operation == LOCK_EX) {
+			r = _do_flock(file, fd, operation, nonblock);
+			_undo_flock(file_aux, fd_aux);
+		} else {
+			_undo_flock(file_aux, fd_aux);
+			r = _do_flock(file, fd, operation, nonblock);
+		}
+	}
+
+	return r;
+}
+
+static int _lock_file(const char *file, uint32_t flags)
+{
+	int operation;
+	uint32_t nonblock = flags & LCK_NONBLOCK;
+	int r;
+
+	struct lock_list *ll;
+	char state;
+
 	switch (flags & LCK_TYPE_MASK) {
 	case LCK_READ:
 		operation = LOCK_SH;

@@ -216,21 +232,10 @@ static int _lock_file(const char *file, 
 	log_very_verbose("Locking %s %c%c", ll->res, state,
 			 nonblock ? ' ' : 'B');
 
-	if (!_prioritise_write_locks)
+	if (_prioritise_write_locks)
+		r = _do_write_priority_flock(file, &ll->lf, operation, nonblock);
+	else 
 		r = _do_flock(file, &ll->lf, operation, nonblock);
-	else {
-		if (flags & LCK_WRITE) {
-			if ((r = _do_flock(file_aux, &fd_aux, LOCK_EX, 0))) {
-				r = _do_flock(file, &ll->lf, operation, nonblock);
-				_undo_flock(file_aux, fd_aux);
-			}
-		} else {
-			if ((r = _do_flock(file_aux, &fd_aux, LOCK_EX, 0))) {
-				_undo_flock(file_aux, fd_aux);
-				r = _do_flock(file, &ll->lf, operation, nonblock);
-			}
-		}
-	}
 
 	if (r)
 		dm_list_add(&_lock_list, &ll->list);

Hi,

Another iteration of second patch attached. This should mostly address the raised concerns, and also adds documentation to example.conf. It still uses the aux_ prefix, see also my other mail.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: locking2.diff
Type: text/x-diff
Size: 4349 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090813/7775056a/attachment.bin>
-------------- next part --------------

Yours,
Petr.

PS: Sorry for the file_aux vs LOCK_EX screwup on my part, good you noticed and fixed that part. (At least we know why we are reviewing patches...)

Ok, this is the version of the patch with a suffix instead of prefix. (I just usually trade robustness for fanciness -- relying on special characters strikes me as unnecessarily brittle, but I concede.)

I also fixed the docs typo Dave pointed out yesterday. I didn't make any further changes to the patch.

As for orphan/global locks, even if we don't strictly *need* the new behaviour, it wouldn't hurt -- and it buys us consistency to have all file locks go this route.

Ok to apply?

Yours,
Petr.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: locking3.diff
Type: text/x-diff
Size: 4125 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090814/ead709be/attachment.bin>

Milan and I discussed it yesterday and concluded we *did* actually desire the new behaviour for those locks:-)

Ack.

Alasdair

Hi,

Yes, that should be documented indeed.

I have thought about that at first (it was actually easier to implement), but I decided against because it feels fragile. So unless you think it's worth it, I'd stick with the prefix solution, which is I think quite foolproof.

Ok.

Ok, that makes sense. I didn't realize right away we only want nonblocking for deadlock avoidance.

I'll check the refactor patch in, and amend the actual-fix one and send it in a while.

Yours,
Petr.

What is it that feels 'fragile'?

When debugging a problem, listing all the locks with 'ls' (or lsof/grep/sort) and finding them paired up nicely is a definite advantage.

The namespace for VGs and LVs is well-defined. (By contrast, the PV namespace is poorly-defined and still causes problems.)

Alasdair