[PATCH] DRAFT! write lock priority

[PATCH] DRAFT! write lock priority

[Petr Rockai]

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>

[PATCH] DRAFT! write lock priority

[Alasdair G Kergon]

On Wed, Aug 12, 2009 at 09:40:22AM +0200, Peter Rockai wrote:
> 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.
 
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).
  
> +		if (!stat(file, &buf1) && !fstat(*fd, &buf2) &&
> +		    is_same_inode(buf1, buf2))
> +			return 1;
> +	} while (!nonblock);
> +	return 0;

That 'return 0' looks like a bug fix - previously I think it did the equivalent
of 'return 1' which seems wrong.

Please revalidate the first patch after my changes and commit it if it seems
OK (minus my FIXME line).

Alasdair

-------------- next part --------------
--- file_locking_orig.c	2009-08-12 19:47:46.000000000 +0100
+++ file_locking.c	2009-08-12 21:14:10.000000000 +0100
@@ -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,
-------------- next part --------------
--- file_locking.c	2009-08-12 21:14:10.000000000 +0100
+++ file_locking_new_edited.c	2009-08-12 21:14:05.000000000 +0100
@@ -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;
 

[PATCH] DRAFT! write lock priority

[Alasdair G Kergon]

> +#define DEFAULT_WRITE_LOCK_PRIORITY 1
  
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.

> +static int _write_priority;

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

[PATCH] DRAFT! write lock priority

[Alasdair G Kergon]

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

Alasdair

-------------- next part --------------
--- file_locking_cleanup.c	2009-08-12 21:14:10.000000000 +0100
+++ file_locking_new_edited.c	2009-08-12 23:43:51.000000000 +0100
@@ -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;
 

[PATCH] DRAFT! write lock priority

[Milan Broz]

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


--- file_locking.c.old	2009-08-13 11:09:26.000000000 +0200
+++ file_locking.c	2009-08-13 11:54:37.000000000 +0200
@@ -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);

[PATCH] DRAFT! write lock priority

[Petr Rockai]

Hi,

Alasdair G Kergon <agk@redhat.com> writes:
> 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.
Yes, that should be documented indeed.

> 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.
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.

> @@ -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.
Ok.

>
> -	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).
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.

[PATCH] DRAFT! write lock priority

[Petr Rockai]

Hi,

Alasdair G Kergon <agk@redhat.com> writes:
> Updated version of the second patch.
> Handing back over to you for further cleanup, review & testing.
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...)

[PATCH] DRAFT! write lock priority

[Alasdair G Kergon]

On Thu, Aug 13, 2009 at 02:54:15PM +0200, Peter Rockai wrote:
> Alasdair G Kergon <agk@redhat.com> writes:
> > 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.
> 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.

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

[PATCH] DRAFT! write lock priority

[Petr Rockai]

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>

[PATCH] DRAFT! write lock priority

[Alasdair G Kergon]

On Fri, Aug 14, 2009 at 02:52:05PM +0200, Peter Rockai wrote:
> 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.
 
Milan and I discussed it yesterday and concluded we *did* actually desire the
new behaviour for those locks:-)

> Ok to apply?
 
Ack.

Alasdair