[lxc-devel] [PATCH 1/1] locking: update per Dwight's comment
Serge Hallyn
serge.hallyn at ubuntu.com
Fri May 24 21:09:59 UTC 2013
( These are pushed to github.com/hallyn/lxc branch lxclock-flock.rebase )
Create three pairs of functions:
int process_lock(void);
void process_unlock(void);
int container_mem_lock(struct lxc_container *c)
void container_mem_unlock(struct lxc_container *c)
int container_disk_lock(struct lxc_container *c);
void container_disk_unlock(struct lxc_container *c);
and use those in lxccontainer.c
process_lock() is to protect the process state among multiple threads.
container_mem_lock() is to protect a struct container among multiple
threads. container_disk_lock is to protect a container on disk.
Also remove the lock in lxcapi_init_pid() as Dwight suggested.
Fix a typo (s/container/contain) spotted by Dwight.
More locking fixes are needed, but let's first the the fundamentals
right. How close does this get us?
Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>
---
src/lxc/lxccontainer.c | 128 +++++++++++++++++--------------------------------
src/lxc/lxclock.c | 91 ++++++++++++++++++++++++-----------
src/lxc/lxclock.h | 9 +++-
3 files changed, 113 insertions(+), 115 deletions(-)
diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
index b2de0e6..adb1c7a 100644
--- a/src/lxc/lxccontainer.c
+++ b/src/lxc/lxccontainer.c
@@ -42,8 +42,6 @@
#include <arpa/inet.h>
#include <ifaddrs.h>
-extern pthread_mutex_t thread_mutex;
-
/* Define unshare() if missing from the C library */
/* this is also in attach.c and lxccontainer.c: commonize it in utils.c */
#ifndef HAVE_UNSHARE
@@ -153,7 +151,7 @@ int lxc_container_get(struct lxc_container *c)
if (c->numthreads < 1)
return 0;
- if (lxclock(c->privlock, 0))
+ if (container_mem_lock(c))
return 0;
if (c->numthreads < 1) {
// bail without trying to unlock, bc the privlock is now probably
@@ -161,7 +159,7 @@ int lxc_container_get(struct lxc_container *c)
return 0;
}
c->numthreads++;
- lxcunlock(c->privlock);
+ container_mem_unlock(c);
return 1;
}
@@ -169,14 +167,14 @@ int lxc_container_put(struct lxc_container *c)
{
if (!c)
return -1;
- if (lxclock(c->privlock, 0))
+ if (container_mem_lock(c))
return -1;
if (--c->numthreads < 1) {
- lxcunlock(c->privlock);
+ container_mem_unlock(c);
lxc_container_free(c);
return 1;
}
- lxcunlock(c->privlock);
+ container_mem_unlock(c);
return 0;
}
@@ -196,7 +194,7 @@ static bool lxcapi_is_defined(struct lxc_container *c)
if (!c)
return false;
- if (lxclock(c->privlock, 0))
+ if (container_mem_lock(c))
return false;
if (!c->configfile)
goto out;
@@ -206,7 +204,7 @@ static bool lxcapi_is_defined(struct lxc_container *c)
ret = true;
out:
- lxcunlock(c->privlock);
+ container_mem_unlock(c);
return ret;
}
@@ -217,15 +215,11 @@ static const char *lxcapi_state(struct lxc_container *c)
if (!c)
return NULL;
- if (lxclock(c->privlock, 0))
+ if (container_disk_lock(c))
return NULL;
- if (lxclock(c->slock, 0))
- goto bad;
s = lxc_getstate(c->name, c->config_path);
ret = lxc_state2str(s);
- lxcunlock(c->slock);
-bad:
- lxcunlock(c->privlock);
+ container_disk_unlock(c);
return ret;
}
@@ -255,15 +249,10 @@ static bool lxcapi_freeze(struct lxc_container *c)
if (!c)
return false;
- if (lxclock(c->privlock, 0))
- return false;
- if (lxclock(c->slock, 0)) {
- lxcunlock(c->privlock);
+ if (container_disk_lock(c))
return false;
- }
ret = lxc_freeze(c->name, c->config_path);
- lxcunlock(c->slock);
- lxcunlock(c->privlock);
+ container_disk_unlock(c);
if (ret)
return false;
return true;
@@ -275,15 +264,10 @@ static bool lxcapi_unfreeze(struct lxc_container *c)
if (!c)
return false;
- if (lxclock(c->privlock, 0))
- return false;
- if (lxclock(c->slock, 0)) {
- lxcunlock(c->privlock);
+ if (container_disk_lock(c))
return false;
- }
ret = lxc_unfreeze(c->name, c->config_path);
- lxcunlock(c->slock);
- lxcunlock(c->privlock);
+ container_disk_unlock(c);
if (ret)
return false;
return true;
@@ -295,16 +279,7 @@ static pid_t lxcapi_init_pid(struct lxc_container *c)
if (!c)
return -1;
- if (lxclock(c->privlock, 0))
- return -1;
- if (lxclock(c->slock, 0)) {
- lxcunlock(c->privlock);
- return -1;
- }
- ret = lxc_cmd_get_init_pid(c->name, c->config_path);
- lxcunlock(c->slock);
- lxcunlock(c->privlock);
- return ret;
+ return lxc_cmd_get_init_pid(c->name, c->config_path);
}
static bool load_config_locked(struct lxc_container *c, const char *fname)
@@ -328,15 +303,10 @@ static bool lxcapi_load_config(struct lxc_container *c, const char *alt_file)
fname = alt_file;
if (!fname)
return false;
- if (lxclock(c->privlock, 0))
- return false;
- if (lxclock(c->slock, 0)) {
- lxcunlock(c->privlock);
+ if (container_disk_lock(c))
return false;
- }
ret = load_config_locked(c, fname);
- lxcunlock(c->slock);
- lxcunlock(c->privlock);
+ container_disk_unlock(c);
return ret;
}
@@ -399,11 +369,11 @@ static bool lxcapi_start(struct lxc_container *c, int useinit, char * const argv
if (useinit && !argv)
return false;
- if (lxclock(c->privlock, 0))
+ if (container_mem_lock(c))
return false;
conf = c->lxc_conf;
daemonize = c->daemonize;
- lxcunlock(c->privlock);
+ container_mem_unlock(c);
if (useinit) {
ret = lxc_execute(c->name, argv, 1, conf, c->config_path);
@@ -424,23 +394,20 @@ static bool lxcapi_start(struct lxc_container *c, int useinit, char * const argv
return false;
lxc_monitord_spawn(c->config_path);
- ret = pthread_mutex_lock(&thread_mutex);
- if (ret != 0) {
- ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret));
+ if (process_lock())
return false;
- }
pid_t pid = fork();
if (pid < 0) {
lxc_container_put(c);
- pthread_mutex_unlock(&thread_mutex);
+ process_unlock();
return false;
}
if (pid != 0) {
ret = wait_on_daemonized_start(c);
- pthread_mutex_unlock(&thread_mutex);
+ process_unlock();
return ret;
}
- pthread_mutex_unlock(&thread_mutex);
+ process_unlock();
/* second fork to be reparented by init */
pid = fork();
if (pid < 0) {
@@ -628,13 +595,8 @@ static bool lxcapi_create(struct lxc_container *c, char *t, char *const argv[])
/* we're going to fork. but since we'll wait for our child, we
* don't need to lxc_container_get */
- if (lxclock(c->privlock, 0))
+ if (container_disk_lock(c))
goto out;
- if (lxclock(c->slock, 0)) {
- ERROR("failed to grab global container lock for %s\n", c->name);
- lxcunlock(c->privlock);
- goto out_unlock1;
- }
pid = fork();
if (pid < 0) {
@@ -713,9 +675,7 @@ static bool lxcapi_create(struct lxc_container *c, char *t, char *const argv[])
bret = load_config_locked(c, c->configfile);
out_unlock:
- lxcunlock(c->slock);
-out_unlock1:
- lxcunlock(c->privlock);
+ container_disk_unlock(c);
out:
if (tpath)
free(tpath);
@@ -793,11 +753,10 @@ static bool lxcapi_clear_config_item(struct lxc_container *c, const char *key)
if (!c || !c->lxc_conf)
return false;
- if (lxclock(c->privlock, 0)) {
+ if (container_mem_lock(c))
return false;
- }
ret = lxc_clear_config_item(c->lxc_conf, key);
- lxcunlock(c->privlock);
+ container_mem_unlock(c);
return ret == 0;
}
@@ -919,11 +878,10 @@ static int lxcapi_get_config_item(struct lxc_container *c, const char *key, char
if (!c || !c->lxc_conf)
return -1;
- if (lxclock(c->privlock, 0)) {
+ if (container_mem_lock(c))
return -1;
- }
ret = lxc_get_config_item(c->lxc_conf, key, retv, inlen);
- lxcunlock(c->privlock);
+ container_mem_unlock(c);
return ret;
}
@@ -938,12 +896,12 @@ static int lxcapi_get_keys(struct lxc_container *c, const char *key, char *retv,
*/
if (!c || !c->lxc_conf)
return -1;
- if (lxclock(c->privlock, 0))
+ if (container_mem_lock(c))
return -1;
int ret = -1;
if (strncmp(key, "lxc.network.", 12) == 0)
ret = lxc_list_nicconfigs(c->lxc_conf, key, retv, inlen);
- lxcunlock(c->privlock);
+ container_mem_unlock(c);
return ret;
}
@@ -968,13 +926,13 @@ static bool lxcapi_save_config(struct lxc_container *c, const char *alt_file)
FILE *fout = fopen(alt_file, "w");
if (!fout)
return false;
- if (lxclock(c->privlock, 0)) {
+ if (container_mem_lock(c)) {
fclose(fout);
return false;
}
write_config(fout, c->lxc_conf);
fclose(fout);
- lxcunlock(c->privlock);
+ container_mem_unlock(c);
return true;
}
@@ -1015,7 +973,7 @@ static bool lxcapi_set_config_item(struct lxc_container *c, const char *key, con
if (!c)
return false;
- if (lxclock(c->privlock, 0))
+ if (container_mem_lock(c))
return false;
if (!c->lxc_conf)
@@ -1030,7 +988,7 @@ static bool lxcapi_set_config_item(struct lxc_container *c, const char *key, con
b = true;
err:
- lxcunlock(c->privlock);
+ container_mem_unlock(c);
return b;
}
@@ -1091,7 +1049,7 @@ static bool lxcapi_set_config_path(struct lxc_container *c, const char *path)
if (!c)
return b;
- if (lxclock(c->privlock, 0))
+ if (container_mem_lock(c))
return b;
p = strdup(path);
@@ -1117,7 +1075,7 @@ static bool lxcapi_set_config_path(struct lxc_container *c, const char *path)
err:
if (oldpath)
free(oldpath);
- lxcunlock(c->privlock);
+ container_mem_unlock(c);
return b;
}
@@ -1130,7 +1088,7 @@ static bool lxcapi_set_cgroup_item(struct lxc_container *c, const char *subsys,
if (!c)
return false;
- if (lxclock(c->privlock, 0))
+ if (container_mem_lock(c))
return false;
if (is_stopped_nolock(c))
@@ -1140,7 +1098,7 @@ static bool lxcapi_set_cgroup_item(struct lxc_container *c, const char *subsys,
if (!ret)
b = true;
err:
- lxcunlock(c->privlock);
+ container_mem_unlock(c);
return b;
}
@@ -1151,7 +1109,7 @@ static int lxcapi_get_cgroup_item(struct lxc_container *c, const char *subsys, c
if (!c || !c->lxc_conf)
return -1;
- if (lxclock(c->privlock, 0))
+ if (container_mem_lock(c))
return -1;
if (is_stopped_nolock(c))
@@ -1160,7 +1118,7 @@ static int lxcapi_get_cgroup_item(struct lxc_container *c, const char *subsys, c
ret = lxc_cgroup_get(c->name, subsys, retv, inlen, c->config_path);
out:
- lxcunlock(c->privlock);
+ container_mem_unlock(c);
return ret;
}
@@ -1615,7 +1573,7 @@ struct lxc_container *lxcapi_clone(struct lxc_container *c, const char *newname,
if (!c || !c->is_defined(c))
return NULL;
- if (lxclock(c->privlock, 0))
+ if (container_mem_lock(c))
return NULL;
if (c->is_running(c)) {
@@ -1697,11 +1655,11 @@ struct lxc_container *lxcapi_clone(struct lxc_container *c, const char *newname,
goto out;
// TODO: update c's lxc.snapshot = count
- lxcunlock(c->privlock);
+ container_mem_unlock(c);
return c2;
out:
- lxcunlock(c->privlock);
+ container_mem_unlock(c);
if (c2) {
c2->destroy(c2);
lxc_container_put(c2);
diff --git a/src/lxc/lxclock.c b/src/lxc/lxclock.c
index bddd1e9..629ce52 100644
--- a/src/lxc/lxclock.c
+++ b/src/lxc/lxclock.c
@@ -48,7 +48,10 @@ static char *lxclock_name(const char *p, const char *n)
free(dest);
return NULL;
}
- if (mkdir_p(dest, 0755) < 0) {
+ process_lock();
+ ret = mkdir_p(dest, 0755);
+ process_unlock();
+ if (ret < 0) {
free(dest);
return NULL;
}
@@ -80,11 +83,6 @@ static sem_t *lxc_new_unnamed_sem(void)
struct lxc_lock *lxc_newlock(const char *lxcpath, const char *name)
{
struct lxc_lock *l;
- int ret = pthread_mutex_lock(&thread_mutex);
- if (ret != 0) {
- ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret));
- return NULL;
- }
l = malloc(sizeof(*l));
if (!l)
@@ -106,18 +104,12 @@ struct lxc_lock *lxc_newlock(const char *lxcpath, const char *name)
l->u.f.fd = -1;
out:
- pthread_mutex_unlock(&thread_mutex);
return l;
}
int lxclock(struct lxc_lock *l, int timeout)
{
- int saved_errno = errno;
- int ret = pthread_mutex_lock(&thread_mutex);
- if (ret != 0) {
- ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret));
- return ret;
- }
+ int ret, saved_errno = errno;
switch(l->type) {
case LXC_LOCK_ANON_SEM:
@@ -149,35 +141,31 @@ int lxclock(struct lxc_lock *l, int timeout)
ret = -2;
goto out;
}
+ process_lock();
if (l->u.f.fd == -1) {
l->u.f.fd = open(l->u.f.fname, O_RDWR|O_CREAT,
S_IWUSR | S_IRUSR);
if (l->u.f.fd == -1) {
+ process_unlock();
ERROR("Error opening %s", l->u.f.fname);
goto out;
}
}
ret = flock(l->u.f.fd, LOCK_EX);
+ process_unlock();
if (ret == -1)
saved_errno = errno;
break;
}
out:
- pthread_mutex_unlock(&thread_mutex);
errno = saved_errno;
return ret;
}
int lxcunlock(struct lxc_lock *l)
{
- int saved_errno = errno;
- int ret = pthread_mutex_lock(&thread_mutex);
-
- if (ret != 0) {
- ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret));
- return ret;
- }
+ int ret = 0, saved_errno = errno;
switch(l->type) {
case LXC_LOCK_ANON_SEM:
@@ -188,6 +176,7 @@ int lxcunlock(struct lxc_lock *l)
saved_errno = errno;
break;
case LXC_LOCK_FLOCK:
+ process_lock();
if (l->u.f.fd != -1) {
if ((ret = flock(l->u.f.fd, LOCK_UN)) < 0)
saved_errno = errno;
@@ -195,22 +184,23 @@ int lxcunlock(struct lxc_lock *l)
l->u.f.fd = -1;
} else
ret = -2;
+ process_unlock();
break;
}
- pthread_mutex_unlock(&thread_mutex);
errno = saved_errno;
return ret;
}
+/*
+ * lxc_putlock() is only called when a container_new() fails,
+ * or during container_put(), which is already guaranteed to
+ * only be done by one task.
+ * So the only exclusion we need to provide here is for regular
+ * thread safety (i.e. file descriptor table changes).
+ */
void lxc_putlock(struct lxc_lock *l)
{
- int ret = pthread_mutex_lock(&thread_mutex);
- if (ret != 0) {
- ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret));
- return;
- }
-
if (!l)
goto out;
switch(l->type) {
@@ -219,16 +209,59 @@ void lxc_putlock(struct lxc_lock *l)
sem_close(l->u.sem);
break;
case LXC_LOCK_FLOCK:
+ process_lock();
if (l->u.f.fd != -1) {
close(l->u.f.fd);
l->u.f.fd = -1;
}
+ process_unlock();
if (l->u.f.fname) {
free(l->u.f.fname);
l->u.f.fname = NULL;
}
break;
}
-out:
+}
+
+int process_lock(void)
+{
+ int ret;
+ ret = pthread_mutex_lock(&thread_mutex);
+ if (ret != 0)
+ ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret));
+ return ret;
+}
+
+void process_unlock(void)
+{
pthread_mutex_unlock(&thread_mutex);
}
+
+int container_mem_lock(struct lxc_container *c)
+{
+ return lxclock(c->privlock, 0);
+}
+
+void container_mem_unlock(struct lxc_container *c)
+{
+ lxcunlock(c->privlock);
+}
+
+int container_disk_lock(struct lxc_container *c)
+{
+ int ret;
+
+ if ((ret = lxclock(c->privlock, 0)))
+ return ret;
+ if ((ret = lxclock(c->slock, 0))) {
+ lxcunlock(c->privlock);
+ return ret;
+ }
+ return 0;
+}
+
+void container_disk_unlock(struct lxc_container *c)
+{
+ lxcunlock(c->slock);
+ lxcunlock(c->privlock);
+}
diff --git a/src/lxc/lxclock.h b/src/lxc/lxclock.h
index 61e7bbb..d03b688 100644
--- a/src/lxc/lxclock.h
+++ b/src/lxc/lxclock.h
@@ -53,7 +53,7 @@ struct lxc_lock {
* We use that to protect the containers as represented on disk.
* lxc_newlock() for the named lock only allocates the pathname in
* memory so we can quickly open+lock it at lxclock.
- * l->u.f.fname will container the malloc'ed name (which must be
+ * l->u.f.fname will contain the malloc'ed name (which must be
* freed when the container is freed), and u.f.fd = -1.
*
* return lxclock on success, NULL on failure.
@@ -81,3 +81,10 @@ extern int lxclock(struct lxc_lock *lock, int timeout);
extern int lxcunlock(struct lxc_lock *lock);
extern void lxc_putlock(struct lxc_lock *l);
+
+extern int process_lock(void);
+extern void process_unlock(void);
+extern int container_mem_lock(struct lxc_container *c)
+extern void container_mem_unlock(struct lxc_container *c)
+extern int container_disk_lock(struct lxc_container *c);
+extern void container_disk_unlock(struct lxc_container *c);
--
1.8.1.2
More information about the lxc-devel
mailing list