[CRIU] [PATCH 3/5] files: Rework clean_one_remap to order ghost dirs removal

[Pavel Tikhomirov]

On 2/22/19 7:02 PM, Cyrill Gorcunov wrote:
> Ghost files may lay inside ghost directories so we should
> order their cleanup to eliminate situation where we remove
> toplevel directory which still has not yet cleaned up entries
> 
>   | (00.002446) Unlink remap static/unlink_dir.test/0.cr.1.ghost
>   | (00.002461) Unlink remap static/unlink_dir.test/1.cr.2.ghost
>   | (00.002467) Unlink remap static/unlink_dir.test/gd1/gd2
>   | (00.002477) Error (criu/files-reg.c:794): Couldn't unlink remap / static/unlink_dir.test/gd1/gd2: Directory not empty
>   | (00.002480) Unlink remap static/unlink_dir.test/gd1/gd2/2.cr.4.ghost
>   | (00.002490) Unlink remap static/unlink_dir.test/gd1/gd2/3.cr.5.ghost
> 
> Thus upon dentries collection lets order them so they would be
> cleaned up in reverse order. Same time if two directories are
> pointing to same entry we should not fail if ENOENT returned.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov at gmail.com>
> ---
>   criu/files-reg.c | 115 ++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 108 insertions(+), 7 deletions(-)
> 
> diff --git a/criu/files-reg.c b/criu/files-reg.c
> index 9d5e1d743117..0b3f6493f718 100644
> --- a/criu/files-reg.c
> +++ b/criu/files-reg.c
> @@ -43,6 +43,7 @@
>   
>   #include "files-reg.h"
>   #include "plugin.h"
> +#include "criu-log.h"
>   
>   int setfsuid(uid_t fsuid);
>   int setfsgid(gid_t fsuid);
> @@ -681,6 +682,89 @@ static int prepare_one_remap(struct remap_info *ri)
>   	return ret;
>   }
>   
> +static int remap_info_cmp(const void *_a, const void *_b)
> +{
> +	struct remap_info *a = ((struct remap_info **)_a)[0];
> +	struct remap_info *b = ((struct remap_info **)_b)[0];
> +	int32_t mnt_id_a = a->rfi->rfe->mnt_id;
> +	int32_t mnt_id_b = b->rfi->rfe->mnt_id;
> +
> +	/*
> +	 * If entries are laying on same mount, which is
> +	 * a common case, we can safely use paths comparision.
> +	 */
> +	if (mnt_id_a == mnt_id_b)
> +		return -strcmp(a->rfi->path, b->rfi->path);
> +
> +	/*
> +	 * Otherwise simply order by mnt_id order, we just
> +	 * can't do anything beter (it is early stage where
> +	 * mounts are not yet processed).

What exact problem is with different mounts, can you show an example 
paths? (AFAICS Mount trees are already built at these point)

> +	 */
> +	return mnt_id_a > mnt_id_b ? 1 : -1;
> +}
> +
> +/*
> + * Ghost directories may carry ghost files but file descriptors
> + * are unordered in compare with this ghost paths, thus on cleanup
> + * we might try to remove the directory itself without waiting
> + * all files (and subdirectories) are cleaned up first.
> + *
> + * What we do here is we're move all ghost dirs into own list,
> + * sort them (to address subdirectories order) and move back
> + * to the end of the remap list.
> + */
> +static int order_remap_dirs(void)
> +{
> +	struct remap_info *ri, *tmp;
> +	struct remap_info **p, **t;
> +	size_t nr_remaps = 0, i;
> +	LIST_HEAD(ghost_dirs);
> +
> +	list_for_each_entry_safe(ri, tmp, &remaps, list) {
> +		if (ri->rpe->remap_type != REMAP_TYPE__GHOST)
> +			continue;
> +		if (!ri->rfi->remap->is_dir)
> +			continue;
> +		list_move_tail(&ri->list, &ghost_dirs);
> +		nr_remaps++;
> +	}
> +
> +	if (list_empty(&ghost_dirs))
> +		return 0;
> +
> +	p = t = xmalloc(sizeof(*p) * nr_remaps);
> +	if (!p) {
> +		list_splice_tail_init(&ghost_dirs, &remaps);
> +		return -ENOMEM;
> +	}
> +
> +	list_for_each_entry_safe(ri, tmp, &ghost_dirs, list) {
> +		list_del_init(&ri->list);
> +		p[0] = ri, p++;
> +	}
> +
> +	qsort(t, nr_remaps, sizeof(t[0]), remap_info_cmp);
> +
> +	for (i = 0; i < nr_remaps; i++) {
> +		list_add_tail(&t[i]->list, &remaps);
> +		pr_debug("remap: ghost mov dir %s\n", t[i]->rfi->path);
> +	}
> +
> +	if (!pr_quelled(LOG_DEBUG)) {
> +		list_for_each_entry_safe(ri, tmp, &remaps, list) {
> +			if (ri->rpe->remap_type != REMAP_TYPE__GHOST)
> +				continue;
> +			pr_debug("remap: ghost ord %3s %s\n",
> +				 ri->rfi->remap->is_dir ? "dir" : "fil",
> +				 ri->rfi->path);
> +		}
> +	}
> +
> +	xfree(t);
> +	return 0;
> +}
> +
>   int prepare_remaps(void)
>   {
>   	struct remap_info *ri;
> @@ -696,7 +780,21 @@ int prepare_remaps(void)
>   			break;
>   	}
>   
> -	return ret;
> +	return ret ? : order_remap_dirs();
> +}
> +
> +static int clean_ghost_dir(int rmntns_root, struct remap_info *ri)
> +{
> +	struct file_remap *remap = ri->rfi->remap;
> +	int ret = unlinkat(rmntns_root, remap->rpath, AT_REMOVEDIR);
> +	/*
> +	 * When deleting ghost directories here is two issues:
> +	 * - names might duplicate, so we may receive ENOENT
> +	 *   and should not treat it as an error
> +	 */
> +	if (ret)
> +		return errno == ENOENT ? 0 : -errno;
> +	return 0;
>   }
>   
>   static int clean_one_remap(struct remap_info *ri)
> @@ -733,15 +831,17 @@ static int clean_one_remap(struct remap_info *ri)
>   
>   	pr_info("Unlink remap %s\n", remap->rpath);
>   
> -	ret = unlinkat(rmntns_root, remap->rpath, remap->is_dir ? AT_REMOVEDIR : 0);
> -	if (ret < 0) {
> -		close(rmntns_root);
> -		pr_perror("Couldn't unlink remap %s %s", path, remap->rpath);
> +	if (remap->is_dir)
> +		ret = clean_ghost_dir(rmntns_root, ri);
> +	else
> +		ret = unlinkat(rmntns_root, remap->rpath, 0);
> +
> +	if (ret) {
> +		pr_perror("Couldn't unlink remap %d %s", rmntns_root, remap->rpath);
>   		return -1;
>   	}
> -	close(rmntns_root);

Don't you leak rmntns_root fd here?

> -	remap->rpath[0] = 0;
>   
> +	remap->rpath[0] = 0;
>   	return 0;
>   }
>   
> @@ -767,6 +867,7 @@ static struct collect_image_info remap_cinfo = {
>   	.pb_type = PB_REMAP_FPATH,
>   	.priv_size = sizeof(struct remap_info),
>   	.collect = collect_one_remap,
> +	.flags = COLLECT_SHARED,
>   };
>   
>   /* Tiny files don't need to generate chunks in ghost image. */
> 

-- 
Best regards, Tikhomirov Pavel
Software Developer, Virtuozzo.