[lxc-devel] [PATCH 1/1] api locking: take process lock around open/close
Dwight Engen
dwight.engen at oracle.com
Thu May 30 14:05:31 UTC 2013
On Wed, 29 May 2013 22:21:22 -0500
Serge Hallyn <serge.hallyn at ubuntu.com> wrote:
> And update the comment explaining the locking.
>
> Also take memlock in want_daemonize.
Hi Serge, could you explain a bit what the locking is protecting about
open/close? It looks like you are locking around just the open or
just the close, and not the whole open to close region so I'm a bit
confused on what scenario the locks are protecting against.
> Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>
> ---
> src/lxc/lxccontainer.c | 153
> +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 115
> insertions(+), 38 deletions(-)
>
> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> index 9bc1caf..145ed7c 100644
> --- a/src/lxc/lxccontainer.c
> +++ b/src/lxc/lxccontainer.c
> @@ -131,7 +131,12 @@ void remove_partial(struct lxc_container *c, int
> fd) char *path = alloca(len);
> int ret;
>
> + if (process_lock()) {
> + ERROR("Failed getting process lock");
> + return;
> + }
> close(fd);
> + process_unlock();
> ret = snprintf(path, len, "%s/%s/partial", c->config_path,
> c->name); if (ret < 0 || ret >= len) {
> ERROR("Error writing partial pathname");
> @@ -145,11 +150,11 @@ void remove_partial(struct lxc_container *c,
> int fd) }
>
> /* LOCKING
> - * 1. c->privlock protects the struct lxc_container from multiple
> threads.
> - * 2. c->slock protects the on-disk container data
> + * 1. container_mem_lock(c) protects the struct lxc_container from
> multiple threads.
> + * 2. container_disk_lock(c) protects the on-disk container data -
> in particular the
> + * container configuration file.
> + * The container_disk_lock also takes the container_mem_lock.
> * 3. thread_mutex protects process data (ex: fd table) from
> multiple threads
> - * slock is an flock, which does not exclude threads. Therefore
> slock should
> - * always be wrapped inside privlock.
> * NOTHING mutexes two independent programs with their own struct
> * lxc_container for the same c->name, between API calls. For
> instance,
> * c->config_read(); c->start(); Between those calls, data on disk
> @@ -160,7 +165,7 @@ void remove_partial(struct lxc_container *c, int
> fd)
> * due to hung callers. So I prefer to keep the locks only within
> our own
> * functions, not across functions.
> *
> - * If you're going to fork while holding a lxccontainer, increment
> + * If you're going to clone while holding a lxccontainer, increment
> * c->numthreads (under privlock) before forking. When deleting,
> * decrement numthreads under privlock, then if it hits 0 you can
> delete.
> * Do not ever use a lxccontainer whose numthreads you did not bump.
> @@ -358,11 +363,14 @@ static pid_t lxcapi_init_pid(struct
> lxc_container *c)
> static bool load_config_locked(struct lxc_container *c, const char
> *fname) {
> + bool ret = false;
> if (!c->lxc_conf)
> c->lxc_conf = lxc_conf_init();
> + process_lock();
From here on you start not checking if process_lock() failed. Just
curious as to why we need to check for failure above but not in the
rest of these.
FWIW I'd much prefer not having to check in all calls if we can
convince ourselves its not needed. The only reasons I can see that
process_lock()'s pthread_mutex_lock() would fail would be EBUSY, EINVAL,
EDEADLK all of which would be because of errors in lxc itself so maybe
we could make process_lock() abort() or exit() if pthread_mutex_lock()
returns an error?
> if (c->lxc_conf && !lxc_config_read(fname, c->lxc_conf))
> - return true;
> - return false;
> + ret = true;
> + process_unlock();
> + return ret;
> }
>
> static bool lxcapi_load_config(struct lxc_container *c, const char
> *alt_file) @@ -406,7 +414,12 @@ static void
> lxcapi_want_daemonize(struct lxc_container *c) {
> if (!c)
> return;
> + if (!container_mem_lock(c)) {
> + ERROR("Error getting mem lock");
> + return;
> + }
> c->daemonize = 1;
> + container_mem_unlock(c);
> }
>
> static bool lxcapi_wait(struct lxc_container *c, const char *state,
> int timeout) @@ -512,6 +525,7 @@ static bool lxcapi_start(struct
> lxc_container *c, int useinit, char * const argv process_unlock();
> return ret;
> }
> + // In a forked task, no longer threaded
> process_unlock();
> /* second fork to be reparented by init */
> pid = fork();
> @@ -1023,7 +1037,9 @@ char** lxcapi_get_ips(struct lxc_container *c,
> char* interface, char* family, in goto out;
>
> /* Save reference to old netns */
> + process_lock();
> old_netns = open("/proc/self/ns/net", O_RDONLY);
> + process_unlock();
> if (old_netns < 0) {
> SYSERROR("failed to open /proc/self/ns/net");
> goto out;
> @@ -1034,7 +1050,9 @@ char** lxcapi_get_ips(struct lxc_container *c,
> char* interface, char* family, in if (ret < 0 || ret >= MAXPATHLEN)
> goto out;
>
> + process_lock();
> new_netns = open(new_netns_path, O_RDONLY);
> + process_unlock();
> if (new_netns < 0) {
> SYSERROR("failed to open %s", new_netns_path);
> goto out;
> @@ -1097,10 +1115,12 @@ out:
> /* Switch back to original netns */
> if (old_netns >= 0 && setns(old_netns, CLONE_NEWNET))
> SYSERROR("failed to setns");
> + process_lock();
> if (new_netns >= 0)
> close(new_netns);
> if (old_netns >= 0)
> close(old_netns);
> + process_unlock();
>
> /* Append NULL to the array */
> if (count) {
> @@ -1194,11 +1214,15 @@ static bool lxcapi_save_config(struct
> lxc_container *c, const char *alt_file) if (lret)
> return false;
>
> + process_lock();
> fout = fopen(alt_file, "w");
> + process_unlock();
> if (!fout)
> goto out;
> write_config(fout, c->lxc_conf);
> + process_lock();
> fclose(fout);
> + process_unlock();
> ret = true;
>
> out:
> @@ -1214,16 +1238,13 @@ static bool lxcapi_destroy(struct
> lxc_container *c) {
> struct bdev *r = NULL;
> bool ret = false;
> + int v;
>
> if (!c || !lxcapi_is_defined(c))
> return false;
>
> - if (lxclock(c->privlock, 0))
> + if (container_disk_lock(c))
> return false;
> - if (lxclock(c->slock, 0)) {
> - lxcunlock(c->privlock);
> - return false;
> - }
>
> if (!is_stopped(c)) {
> // we should queue some sort of error - in
> c->error_string? @@ -1231,10 +1252,16 @@ static bool
> lxcapi_destroy(struct lxc_container *c) goto out;
> }
>
> - if (c->lxc_conf->rootfs.path && c->lxc_conf->rootfs.mount)
> + if (c->lxc_conf->rootfs.path && c->lxc_conf->rootfs.mount) {
> + process_lock();
> r = bdev_init(c->lxc_conf->rootfs.path,
> c->lxc_conf->rootfs.mount, NULL);
> + process_unlock();
> + }
> if (r) {
> - if (r->ops->destroy(r) < 0) {
> + process_lock();
> + v = r->ops->destroy(r);
> + process_unlock();
> + if (v < 0) {
> ERROR("Error destroying rootfs for %s",
> c->name); goto out;
> }
> @@ -1243,15 +1270,17 @@ static bool lxcapi_destroy(struct
> lxc_container *c) const char *p1 = lxcapi_get_config_path(c);
> char *path = alloca(strlen(p1) + strlen(c->name) + 2);
> sprintf(path, "%s/%s", p1, c->name);
> - if (lxc_rmdir_onedev(path) < 0) {
> + process_lock();
> + v = lxc_rmdir_onedev(path);
> + process_unlock();
> + if (v < 0) {
> ERROR("Error destroying container directory for %s",
> c->name); goto out;
> }
> ret = true;
>
> out:
> - lxcunlock(c->privlock);
> - lxcunlock(c->slock);
> + container_disk_unlock(c);
> return ret;
> }
>
> @@ -1374,23 +1403,17 @@ err:
> static bool lxcapi_set_cgroup_item(struct lxc_container *c, const
> char *subsys, const char *value) {
> int ret;
> - bool b = false;
>
> if (!c)
> return false;
>
> - if (container_mem_lock(c))
> - return false;
> -
> if (is_stopped(c))
> - goto err;
> + return false;
>
> + process_lock();
> ret = lxc_cgroup_set(c->name, subsys, value, c->config_path);
> - if (!ret)
> - b = true;
> -err:
> - container_mem_unlock(c);
> - return b;
> + process_unlock();
> + return ret == 0;
> }
>
> static int lxcapi_get_cgroup_item(struct lxc_container *c, const
> char *subsys, char *retv, int inlen) @@ -1400,32 +1423,41 @@ static
> int lxcapi_get_cgroup_item(struct lxc_container *c, const char
> *subsys, c if (!c || !c->lxc_conf) return -1;
>
> - if (container_mem_lock(c))
> - return -1;
> -
> if (is_stopped(c))
> - goto out;
> + return -1;
>
> + process_lock();
> ret = lxc_cgroup_get(c->name, subsys, retv, inlen,
> c->config_path);
> + process_unlock();
>
> -out:
> - container_mem_unlock(c);
> return ret;
> }
>
> const char *lxc_get_default_config_path(void)
> {
> - return default_lxc_path();
> + const char *ret;
> + process_lock();
> + ret = default_lxc_path();
> + process_unlock();
> + return ret;
> }
>
> const char *lxc_get_default_lvm_vg(void)
> {
> - return default_lvm_vg();
> + const char *ret;
> + process_lock();
> + ret = default_lvm_vg();
> + process_unlock();
> + return ret;
> }
>
> const char *lxc_get_default_zfs_root(void)
> {
> - return default_zfs_root();
> + const char *ret;
> + process_lock();
> + ret = default_zfs_root();
> + process_unlock();
> + return ret;
> }
>
> const char *lxc_get_version(void)
> @@ -1450,12 +1482,16 @@ static int copy_file(char *old, char *new)
> return -1;
> }
>
> + process_lock();
> in = open(old, O_RDONLY);
> + process_unlock();
> if (in < 0) {
> SYSERROR("opening original file %s", old);
> return -1;
> }
> + process_lock();
> out = open(new, O_CREAT | O_EXCL | O_WRONLY, 0644);
> + process_unlock();
> if (out < 0) {
> SYSERROR("opening new file %s", new);
> close(in);
> @@ -1476,8 +1512,10 @@ static int copy_file(char *old, char *new)
> goto err;
> }
> }
> + process_lock();
> close(in);
> close(out);
> + process_unlock();
>
> // we set mode, but not owner/group
> ret = chmod(new, sbuf.st_mode);
> @@ -1489,8 +1527,10 @@ static int copy_file(char *old, char *new)
> return 0;
>
> err:
> + process_lock();
> close(in);
> close(out);
> + process_unlock();
> return -1;
> }
>
> @@ -1547,48 +1587,67 @@ static int update_name_and_paths(const char
> *path, struct lxc_container *oldc, const char *p0, *p1, *p2, *end;
> const char *oldpath = oldc->get_config_path(oldc);
> const char *oldname = oldc->name;
> + int ret;
>
> + process_lock();
> f = fopen(path, "r");
> + process_unlock();
> if (!f) {
> SYSERROR("opening old config");
> return -1;
> }
> if (fseek(f, 0, SEEK_END) < 0) {
> SYSERROR("seeking to end of old config");
> + process_lock();
> fclose(f);
> + process_unlock();
> return -1;
> }
> flen = ftell(f);
> if (flen < 0) {
> + process_lock();
> fclose(f);
> + process_unlock();
> SYSERROR("telling size of old config");
> return -1;
> }
> if (fseek(f, 0, SEEK_SET) < 0) {
> + process_lock();
> fclose(f);
> + process_unlock();
> SYSERROR("rewinding old config");
> return -1;
> }
> contents = malloc(flen+1);
> if (!contents) {
> SYSERROR("out of memory");
> + process_lock();
> fclose(f);
> + process_unlock();
> return -1;
> }
> if (fread(contents, 1, flen, f) != flen) {
> free(contents);
> + process_lock();
> fclose(f);
> + process_unlock();
> SYSERROR("reading old config");
> return -1;
> }
> contents[flen] = '\0';
> - if (fclose(f) < 0) {
> +
> + process_lock();
> + ret = fclose(f);
> + process_unlock();
> + if (ret < 0) {
> free(contents);
> SYSERROR("closing old config");
> return -1;
> }
>
> + process_lock();
> f = fopen(path, "w");
> + process_unlock();
> if (!f) {
> SYSERROR("reopening config");
> free(contents);
> @@ -1605,11 +1664,15 @@ static int update_name_and_paths(const char
> *path, struct lxc_container *oldc, if (fwrite(p0, 1, (end-p0), f) !=
> (end-p0)) { SYSERROR("writing new config");
> free(contents);
> + process_lock();
> fclose(f);
> + process_unlock();
> return -1;
> }
> free(contents);
> + process_lock();
> fclose(f);
> + process_unlock();
> // success
> return 0;
> } else {
> @@ -1618,7 +1681,9 @@ static int update_name_and_paths(const char
> *path, struct lxc_container *oldc, if (fwrite(p0, 1, (p-p0), f) !=
> (p-p0)) { SYSERROR("writing new config");
> free(contents);
> + process_lock();
> fclose(f);
> + process_unlock();
> return -1;
> }
> p0 = p;
> @@ -1626,7 +1691,9 @@ static int update_name_and_paths(const char
> *path, struct lxc_container *oldc, if (fwrite(new, 1, strlen(new),
> f) != strlen(new)) { SYSERROR("writing new name or path in new
> config"); free(contents);
> + process_lock();
> fclose(f);
> + process_unlock();
> return -1;
> }
> p0 += (p == p2) ? strlen(oldname) :
> strlen(oldpath); @@ -1671,13 +1738,19 @@ static int copyhooks(struct
> lxc_container *oldc, struct lxc_container *c)
> static void new_hwaddr(char *hwaddr)
> {
> - FILE *f = fopen("/dev/urandom", "r");
> + FILE *f;
> +
> + process_lock();
> + f = fopen("/dev/urandom", "r");
> + process_unlock();
> if (f) {
> unsigned int seed;
> int ret = fread(&seed, sizeof(seed), 1, f);
> if (ret != 1)
> seed = time(NULL);
> + process_lock();
> fclose(f);
> + process_unlock();
> srand(seed);
> } else
> srand(time(NULL));
> @@ -1891,13 +1964,17 @@ struct lxc_container *lxcapi_clone(struct
> lxc_container *c, const char *newname, }
>
> // copy the configuration, tweak it as needed,
> + process_lock();
> fout = fopen(newpath, "w");
> + process_unlock();
> if (!fout) {
> SYSERROR("open %s", newpath);
> goto out;
> }
> write_config(fout, c->lxc_conf);
> + process_lock();
> fclose(fout);
> + process_unlock();
>
> if (update_name_and_paths(newpath, c, n, l) < 0) {
> ERROR("Error updating name in cloned config");
More information about the lxc-devel
mailing list