[CRIU] [PATCH] namespace: mark mount namespaces as populated after the forking stage

[Pavel Emelyanov]

On 06/11/2016 09:01 PM, Andrey Vagin wrote:
> From: Andrew Vagin <avagin at virtuozzo.com>
> 
> Currently we mark a mount namespaces as populated when a target process
> (ns_pid) switches into it. But if a process inherited the right
> namespace from a parent, it doesn't call do_restore_task_mnt_ns()
> and a namespace can remain unmarked.

Why? It should be marked by the task that created one.

> af55c059fb6 ("mount: fix a race between restoring namespaces and file mappings")
> After this patch we could simplify logic around ns_populated. Currently
> it's a futex, but nodoby waits on it. We can set ns_populated when we
> are going to close namespace descriptors. To avoid additional locks, we
> can do this when all task pass the forking stage and don't start the
> next stage.

Don't we need barriers between ns->populated = true and close(ns->fd)?

(more comments inline)

> https://jira.sw.ru/browse/PSBM-48222
> 
> Signed-off-by: Andrew Vagin <avagin at virtuozzo.com>
> ---
>  criu/cr-restore.c         |  7 ++++---
>  criu/include/namespaces.h |  2 +-
>  criu/mount.c              | 14 ++++++--------
>  criu/namespaces.c         |  4 ++--
>  4 files changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/criu/cr-restore.c b/criu/cr-restore.c
> index 1e3d823..5511447 100644
> --- a/criu/cr-restore.c
> +++ b/criu/cr-restore.c
> @@ -1309,16 +1309,17 @@ static int restore_task_with_children(void *_arg)
>  
>  	restore_pgid();
>  
> -	if (restore_finish_stage(CR_STATE_FORKING) < 0)
> -		goto err;
> -
>  	if (current->parent == NULL) {
> +		futex_wait_while_gt(&task_entries->nr_in_progress, 1);

Why is it needed here?

>  		if (depopulate_roots_yard())
>  			goto err;
>  
>  		fini_restore_mntns();
>  	}
>  
> +	if (restore_finish_stage(CR_STATE_FORKING) < 0)
> +		goto err;
> +
>  	if (restore_one_task(current->pid.virt, ca->core))
>  		goto err;
>  
> diff --git a/criu/include/namespaces.h b/criu/include/namespaces.h
> index 4cd2c85..3a3f7d4 100644
> --- a/criu/include/namespaces.h
> +++ b/criu/include/namespaces.h
> @@ -93,7 +93,7 @@ struct ns_id {
>  	 * are mounted) and other tasks may do setns on it
>  	 * and proceed.
>  	 */
> -	futex_t ns_populated;
> +	bool ns_populated;
>  
>  	union {
>  		struct {
> diff --git a/criu/mount.c b/criu/mount.c
> index e891c92..7c59ac6 100644
> --- a/criu/mount.c
> +++ b/criu/mount.c
> @@ -2856,7 +2856,7 @@ static int rst_collect_local_mntns(enum ns_type typ)
>  	if (!mntinfo)
>  		return -1;
>  
> -	futex_set(&nsid->ns_populated, 1);
> +	nsid->ns_populated = true;
>  	return 0;
>  }
>  
> @@ -3112,9 +3112,6 @@ static int do_restore_task_mnt_ns(struct ns_id *nsid, struct pstree_item *curren
>  	}
>  	close(fd);
>  
> -	if (nsid->ns_pid == current->pid.virt)
> -		futex_set_and_wake(&nsid->ns_populated, 1);
> -
>  	return 0;
>  }
>  
> @@ -3161,9 +3158,10 @@ void fini_restore_mntns(void)
>  	for (nsid = ns_ids; nsid != NULL; nsid = nsid->next) {
>  		if (nsid->nd != &mnt_ns_desc)
>  			continue;
> -		close(nsid->mnt.ns_fd);
> +		close_safe(&nsid->mnt.ns_fd);

Why _safe()?

>  		if (nsid->type != NS_ROOT)
> -			close(nsid->mnt.root_fd);
> +			close_safe(&nsid->mnt.root_fd);
> +		nsid->ns_populated = true;

This assignment is new. Was it lost?

>  	}
>  }
>  
> @@ -3431,7 +3429,7 @@ ns_created:
>  			if (nsid->mnt.ns_fd < 0)
>  				goto err;
>  			/* we set ns_populated so we don't need to open root_fd */
> -			futex_set(&nsid->ns_populated, 1);
> +			nsid->ns_populated = true;
>  			continue;
>  		}
>  
> @@ -3565,7 +3563,7 @@ int mntns_get_root_fd(struct ns_id *mntns)
>  	 * root from the root task.
>  	 */
>  
> -	if (!futex_get(&mntns->ns_populated)) {
> +	if (!mntns->ns_populated) {
>  		int fd;
>  
>  		fd = open_proc(root_item->pid.virt, "fd/%d", mntns->mnt.root_fd);
> diff --git a/criu/namespaces.c b/criu/namespaces.c
> index 6c42df0..157c9bf 100644
> --- a/criu/namespaces.c
> +++ b/criu/namespaces.c
> @@ -298,7 +298,7 @@ struct ns_id *rst_new_ns_id(unsigned int id, pid_t pid,
>  	if (nsid) {
>  		nsid->type = type;
>  		nsid_add(nsid, nd, id, pid);
> -		futex_set(&nsid->ns_populated, 0);
> +		nsid->ns_populated = false;
>  	}
>  
>  	return nsid;
> @@ -417,7 +417,7 @@ static unsigned int generate_ns_id(int pid, unsigned int kid, struct ns_desc *nd
>  
>  	nsid->type = type;
>  	nsid->kid = kid;
> -	futex_set(&nsid->ns_populated, 1);
> +	nsid->ns_populated = true;
>  	nsid_add(nsid, nd, ns_next_id++, pid);
>  
>  found:
>