[lxc-devel] [PATCH] Use pthread_atfork() to unlock mutexes after fork()
Andrey Mazo
mazo at telum.ru
Mon Jan 13 12:40:42 UTC 2014
Hi S.Çağlar,
Great thanks for testing this!
On Tue, 31 Dec 2013 19:54:32 +0400, S.Çağlar Onur <caglar at 10ur.org> wrote:
> Hi Andrey,
>
> Just finished create/start/stop/destroy loop for 10K container using
> lxc-test-concurrent as well as creating 2000 via Go concurrently.
>
> On Mon, Dec 30, 2013 at 6:06 AM, Andrey Mazo <mazo at telum.ru> wrote:
>> Signed-off-by: Andrey Mazo <mazo at telum.ru>
>
> Tested-by: S.Çağlar Onur <caglar at 10ur.org>
>
>> ---
>> src/lxc/Makefile.am | 3 ++-
>> src/lxc/attach.c | 2 --
>> src/lxc/bdev.c | 13 -------------
>> src/lxc/lxccontainer.c | 4 ----
>> src/lxc/lxclock.c | 23 ++++++++++++++++++++++-
>> src/lxc/monitor.c | 1 -
>> 6 files changed, 24 insertions(+), 22 deletions(-)
>>
>> diff --git a/src/lxc/Makefile.am b/src/lxc/Makefile.am
>> index 74b38e2..f18d6e1 100644
>> --- a/src/lxc/Makefile.am
>> +++ b/src/lxc/Makefile.am
>> @@ -135,9 +135,10 @@ AM_CFLAGS += -DHAVE_SECCOMP
>> liblxc_so_SOURCES += seccomp.c
>> endif
>>
>> -liblxc_so_CFLAGS = -fPIC -DPIC $(AM_CFLAGS)
>> +liblxc_so_CFLAGS = -fPIC -DPIC $(AM_CFLAGS) -pthread
>>
>> liblxc_so_LDFLAGS = \
>> + -pthread \
>> -shared \
>> -Wl,-soname,liblxc.so.$(firstword $(subst ., ,$(VERSION)))
>>
>> diff --git a/src/lxc/attach.c b/src/lxc/attach.c
>> index 6749617..252e172 100644
>> --- a/src/lxc/attach.c
>> +++ b/src/lxc/attach.c
>> @@ -497,7 +497,6 @@ char *lxc_attach_getpwshell(uid_t uid)
>> NULL
>> };
>>
>> - process_unlock(); // we're no longer sharing
>> close(pipes[0]);
>>
>> /* we want to capture stdout */
>> @@ -790,7 +789,6 @@ int lxc_attach(const char* name, const char* lxcpath, lxc_attach_exec_t exec_fun
>> return -1;
>> }
>>
>> - process_unlock(); // we're no longer sharing
>> /* first subprocess begins here, we close the socket that is for the
>> * initial thread
>> */
>> diff --git a/src/lxc/bdev.c b/src/lxc/bdev.c
>> index b737eff..ec0d0ee 100644
>> --- a/src/lxc/bdev.c
>> +++ b/src/lxc/bdev.c
>> @@ -72,7 +72,6 @@ static int do_rsync(const char *src, const char *dest)
>> if (pid > 0)
>> return wait_for_pid(pid);
>>
>> - process_unlock(); // we're no longer sharing
>> l = strlen(src) + 2;
>> s = malloc(l);
>> if (!s)
>> @@ -197,7 +196,6 @@ static int do_mkfs(const char *path, const char *fstype)
>> if (pid > 0)
>> return wait_for_pid(pid);
>>
>> - process_unlock(); // we're no longer sharing
>> // If the file is not a block device, we don't want mkfs to ask
>> // us about whether to proceed.
>> close(0);
>> @@ -282,7 +280,6 @@ static int detect_fs(struct bdev *bdev, char *type, int len)
>> return ret;
>> }
>>
>> - process_unlock(); // we're no longer sharing
>> if (unshare(CLONE_NEWNS) < 0)
>> exit(1);
>>
>> @@ -574,7 +571,6 @@ static int zfs_clone(const char *opath, const char *npath, const char *oname,
>> if (!pid) {
>> char dev[MAXPATHLEN];
>>
>> - process_unlock(); // we're no longer sharing
>> ret = snprintf(dev, MAXPATHLEN, "%s/%s", zfsroot, nname);
>> if (ret < 0 || ret >= MAXPATHLEN)
>> exit(1);
>> @@ -598,7 +594,6 @@ static int zfs_clone(const char *opath, const char *npath, const char *oname,
>> if ((pid = fork()) < 0)
>> return -1;
>> if (!pid) {
>> - process_unlock(); // we're no longer sharing
>> execlp("zfs", "zfs", "destroy", path1, NULL);
>> exit(1);
>> }
>> @@ -609,7 +604,6 @@ static int zfs_clone(const char *opath, const char *npath, const char *oname,
>> if ((pid = fork()) < 0)
>> return -1;
>> if (!pid) {
>> - process_unlock(); // we're no longer sharing
>> execlp("zfs", "zfs", "snapshot", path1, NULL);
>> exit(1);
>> }
>> @@ -620,7 +614,6 @@ static int zfs_clone(const char *opath, const char *npath, const char *oname,
>> if ((pid = fork()) < 0)
>> return -1;
>> if (!pid) {
>> - process_unlock(); // we're no longer sharing
>> execlp("zfs", "zfs", "clone", option, path1, path2, NULL);
>> exit(1);
>> }
>> @@ -671,7 +664,6 @@ static int zfs_destroy(struct bdev *orig)
>> if (pid)
>> return wait_for_pid(pid);
>>
>> - process_unlock(); // we're no longer sharing
>> if (!zfs_list_entry(orig->src, output, MAXPATHLEN)) {
>> ERROR("Error: zfs entry for %s not found", orig->src);
>> return -1;
>> @@ -716,7 +708,6 @@ static int zfs_create(struct bdev *bdev, const char *dest, const char *n,
>> if (pid)
>> return wait_for_pid(pid);
>>
>> - process_unlock(); // we're no longer sharing
>> char dev[MAXPATHLEN];
>> ret = snprintf(dev, MAXPATHLEN, "%s/%s", zfsroot, n);
>> if (ret < 0 || ret >= MAXPATHLEN)
>> @@ -861,7 +852,6 @@ static int do_lvm_create(const char *path, unsigned long size, const char *thinp
>> if (pid > 0)
>> return wait_for_pid(pid);
>>
>> - process_unlock(); // we're no longer sharing
>> // lvcreate default size is in M, not bytes.
>> ret = snprintf(sz, 24, "%lu", size/1000000);
>> if (ret < 0 || ret >= 24)
>> @@ -921,7 +911,6 @@ static int lvm_snapshot(const char *orig, const char *path, unsigned long size)
>> if (pid > 0)
>> return wait_for_pid(pid);
>>
>> - process_unlock(); // we're no longer sharing
>> // lvcreate default size is in M, not bytes.
>> ret = snprintf(sz, 24, "%lu", size/1000000);
>> if (ret < 0 || ret >= 24)
>> @@ -1055,7 +1044,6 @@ static int lvm_destroy(struct bdev *orig)
>> if ((pid = fork()) < 0)
>> return -1;
>> if (!pid) {
>> - process_unlock(); // we're no longer sharing
>> execlp("lvremove", "lvremove", "-f", orig->src, NULL);
>> exit(1);
>> }
>> @@ -2091,7 +2079,6 @@ struct bdev *bdev_copy(const char *src, const char *oldname, const char *cname,
>> return new;
>> }
>>
>> - process_unlock(); // we're no longer sharing
>> if (unshare(CLONE_NEWNS) < 0) {
>> SYSERROR("unshare CLONE_NEWNS");
>> exit(1);
>> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
>> index e1d004f..cf71f28 100644
>> --- a/src/lxc/lxccontainer.c
>> +++ b/src/lxc/lxccontainer.c
>> @@ -599,7 +599,6 @@ static bool lxcapi_start(struct lxc_container *c, int useinit, char * const argv
>> if (pid != 0)
>> return wait_on_daemonized_start(c, pid);
>>
>> - process_unlock(); // we're no longer sharing
>> /* second fork to be reparented by init */
>> pid = fork();
>> if (pid < 0) {
>> @@ -837,7 +836,6 @@ static bool create_run_template(struct lxc_container *c, char *tpath, bool quiet
>> char **newargv;
>> struct lxc_conf *conf = c->lxc_conf;
>>
>> - process_unlock(); // we're no longer sharing
>> if (quiet) {
>> close(0);
>> close(1);
>> @@ -1234,7 +1232,6 @@ static bool lxcapi_create(struct lxc_container *c, const char *t,
>> if (pid == 0) { // child
>> struct bdev *bdev = NULL;
>>
>> - process_unlock(); // we're no longer sharing
>> if (!(bdev = do_bdev_create(c, bdevtype, specs))) {
>> ERROR("Error creating backing store type %s for %s",
>> bdevtype ? bdevtype : "(none)", c->name);
>> @@ -2326,7 +2323,6 @@ static int clone_update_rootfs(struct lxc_container *c0,
>> if (pid > 0)
>> return wait_for_pid(pid);
>>
>> - process_unlock(); // we're no longer sharing
>> bdev = bdev_init(c->lxc_conf->rootfs.path, c->lxc_conf->rootfs.mount, NULL);
>> if (!bdev)
>> exit(1);
>> diff --git a/src/lxc/lxclock.c b/src/lxc/lxclock.c
>> index 28691b0..36c4bd3 100644
>> --- a/src/lxc/lxclock.c
>> +++ b/src/lxc/lxclock.c
>> @@ -87,7 +87,7 @@ void unlock_mutex(pthread_mutex_t *l)
>> int ret;
>>
>> if ((ret = pthread_mutex_unlock(l)) != 0) {
>> - fprintf(stderr, "pthread_mutex_lock returned:%d %s", ret, strerror(ret));
>> + fprintf(stderr, "pthread_mutex_unlock returned:%d %s", ret, strerror(ret));
>> dump_stacktrace();
>> exit(1);
>> }
>> @@ -315,6 +315,21 @@ void process_unlock(void)
>> unlock_mutex(&thread_mutex);
>> }
>>
>> +/* One thread can do fork() while another one is holding a mutex.
>> + * There is only one thread in child just after the fork(), so noone will ever release that mutex.
>> + * We setup a "child" fork handler to unlock the mutex just after the fork().
>> + * For several mutex types, unlocking an unlocked mutex can lead to undefined behavior.
>> + * One way to deal with it is to setup "prepare" fork handler
>> + * to lock the mutex before fork() and both "parent" and "child" fork handlers
>> + * to unlock the mutex.
>> + * This forbids doing fork() while explicitly holding the lock.
>> + */
>> +__attribute__((constructor))
>> +static void process_lock_setup_atfork(void)
>> +{
>> + pthread_atfork(process_lock, process_unlock, process_unlock);
>> +}
>> +
>> /* Protects static const values inside the lxc_global_config_value funtion */
>> void static_lock(void)
>> {
>> @@ -326,6 +341,12 @@ void static_unlock(void)
>> unlock_mutex(&static_mutex);
>> }
>>
>> +__attribute__((constructor))
>> +static void static_lock_setup_atfork(void)
>> +{
>> + pthread_atfork(static_lock, static_unlock, static_unlock);
>> +}
>> +
>> int container_mem_lock(struct lxc_container *c)
>> {
>> return lxclock(c->privlock, 0);
>> diff --git a/src/lxc/monitor.c b/src/lxc/monitor.c
>> index 7e0a713..0b6a065 100644
>> --- a/src/lxc/monitor.c
>> +++ b/src/lxc/monitor.c
>> @@ -300,7 +300,6 @@ int lxc_monitord_spawn(const char *lxcpath)
>> return 0;
>> }
>>
>> - process_unlock(); // we're no longer sharing
>> if (pipe(pipefd) < 0) {
>> SYSERROR("failed to create pipe");
>> exit(EXIT_FAILURE);
>> --
>> 1.8.4.5
>>
>> _______________________________________________
>> lxc-devel mailing list
>> lxc-devel at lists.linuxcontainers.org
>> http://lists.linuxcontainers.org/listinfo/lxc-devel
--
Andrey Mazo.
More information about the lxc-devel
mailing list