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

[Kirill Tkhai]

On 17.03.2017 00:26, Andrei Vagin wrote:
> 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?

Yeah, sounds reasonable
 
>>
>> 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);
>>  	}
>>  
>>