[CRIU] [PATCH] core: Introduce last_pid_mutex and use it to synchronize ns_last_pid assignment

[Andrei Vagin]

On Wed, Feb 22, 2017 at 03:44:50PM +0300, Kirill Tkhai wrote:
> This is simplier, than file lock, because it's not propagated
> to child processes, and it's not need to close the fd from them.
> Also we will need a comfortable synchronization for creating
> child pid namespaces, and mutex will be good for that.

We use flock to synronize using of this file with other users. For
example, two criu-s can restore two different process trees in the same
pid namespace.

So maybe we need to leave flock if processes are restore in a current
pid namespace?

> 
> Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
> ---
>  criu/cr-restore.c       |   33 +++++++++++++--------------------
>  criu/include/rst_info.h |    1 +
>  criu/pie/restorer.c     |   15 ++-------------
>  3 files changed, 16 insertions(+), 33 deletions(-)
> 
> diff --git a/criu/cr-restore.c b/criu/cr-restore.c
> index b5e33c859..65ca3e781 100644
> --- a/criu/cr-restore.c
> +++ b/criu/cr-restore.c
> @@ -928,7 +928,6 @@ static int restore_one_task(int pid, CoreEntry *core)
>  struct cr_clone_arg {
>  	struct pstree_item *item;
>  	unsigned long clone_flags;
> -	int fd;
>  
>  	CoreEntry *core;
>  };
> @@ -974,6 +973,7 @@ static inline int fork_with_pid(struct pstree_item *item)
>  	struct cr_clone_arg ca;
>  	int ret = -1;
>  	pid_t pid = vpid(item);
> +	bool locked;
>  
>  	if (item->pid->state != TASK_HELPER) {
>  		if (open_core(pid, &ca.core))
> @@ -1014,25 +1014,24 @@ static inline int fork_with_pid(struct pstree_item *item)
>  
>  	if (!(ca.clone_flags & CLONE_NEWPID)) {
>  		char buf[32];
> -		int len;
> +		int len, fd;
>  
> -		ca.fd = open_proc_rw(PROC_GEN, LAST_PID_PATH);
> -		if (ca.fd < 0)
> +		fd = open_proc_rw(PROC_GEN, LAST_PID_PATH);
> +		if (fd < 0)
>  			goto err;
>  
> -		if (flock(ca.fd, LOCK_EX)) {
> -			close(ca.fd);
> -			pr_perror("%d: Can't lock %s", pid, LAST_PID_PATH);
> -			goto err;
> -		}
> +		mutex_lock(&task_entries->last_pid_mutex);
> +		locked = true;
>  
>  		len = snprintf(buf, sizeof(buf), "%d", pid - 1);
> -		if (write(ca.fd, buf, len) != len) {
> +		len -= write(fd, buf, len);
> +		close(fd);
> +		if (len) {
>  			pr_perror("%d: Write %s to %s", pid, buf, LAST_PID_PATH);
>  			goto err_unlock;
>  		}
>  	} else {
> -		ca.fd = -1;
> +		locked = false;
>  		BUG_ON(pid != INIT_PID);
>  	}
>  
> @@ -1066,12 +1065,8 @@ static inline int fork_with_pid(struct pstree_item *item)
>  	}
>  
>  err_unlock:
> -	if (ca.fd >= 0) {
> -		if (flock(ca.fd, LOCK_UN))
> -			pr_perror("%d: Can't unlock %s", pid, LAST_PID_PATH);
> -
> -		close(ca.fd);
> -	}
> +	if (locked)
> +		mutex_unlock(&task_entries->last_pid_mutex);
>  err:
>  	if (ca.core)
>  		core_entry__free_unpacked(ca.core, NULL);
> @@ -1357,9 +1352,6 @@ static int restore_task_with_children(void *_arg)
>  				current->pid->real, vpid(current));
>  	}
>  
> -	if ( !(ca->clone_flags & CLONE_FILES))
> -		close_safe(&ca->fd);
> -
>  	if (current->pid->state != TASK_HELPER) {
>  		ret = clone_service_fd(rsti(current)->service_fd_id);
>  		if (ret)
> @@ -2079,6 +2071,7 @@ int prepare_task_entries(void)
>  	task_entries->nr_helpers = 0;
>  	futex_set(&task_entries->start, CR_STATE_RESTORE_NS);
>  	mutex_init(&task_entries->userns_sync_lock);
> +	mutex_init(&task_entries->last_pid_mutex);
>  
>  	return 0;
>  }
> diff --git a/criu/include/rst_info.h b/criu/include/rst_info.h
> index 92dfc9d93..d6e8a3c9b 100644
> --- a/criu/include/rst_info.h
> +++ b/criu/include/rst_info.h
> @@ -11,6 +11,7 @@ struct task_entries {
>  	futex_t start;
>  	atomic_t cr_err;
>  	mutex_t userns_sync_lock;
> +	mutex_t last_pid_mutex;
>  };
>  
>  struct fdt {
> diff --git a/criu/pie/restorer.c b/criu/pie/restorer.c
> index 2da95cf28..7e29cc225 100644
> --- a/criu/pie/restorer.c
> +++ b/criu/pie/restorer.c
> @@ -1425,12 +1425,7 @@ long __export_restore_task(struct task_restore_args *args)
>  			goto core_restore_end;
>  		}
>  
> -		ret = sys_flock(fd, LOCK_EX);
> -		if (ret) {
> -			pr_err("Can't lock last_pid %d\n", fd);
> -			sys_close(fd);
> -			goto core_restore_end;
> -		}
> +		mutex_lock(&task_entries_local->last_pid_mutex);
>  
>  		for (i = 0; i < args->nr_threads; i++) {
>  			char last_pid_buf[16], *s;
> @@ -1459,13 +1454,7 @@ long __export_restore_task(struct task_restore_args *args)
>  			RUN_CLONE_RESTORE_FN(ret, clone_flags, new_sp, parent_tid, thread_args, args->clone_restore_fn);
>  		}
>  
> -		ret = sys_flock(fd, LOCK_UN);
> -		if (ret) {
> -			pr_err("Can't unlock last_pid %ld\n", ret);
> -			sys_close(fd);
> -			goto core_restore_end;
> -		}
> -
> +		mutex_unlock(&task_entries_local->last_pid_mutex);
>  		sys_close(fd);
>  	}
>  
>