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

Submitted by Cyrill Gorcunov on Feb. 26, 2019, 9:13 a.m.

Details

Message ID 20190226091335.21922-4-gorcunov@gmail.com
State New
Series "Fix order problem in ghost files cleanup"
Headers show

Commit Message

Cyrill Gorcunov Feb. 26, 2019, 9:13 a.m.
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@gmail.com>
---
 criu/files-reg.c | 113 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 108 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/files-reg.c b/criu/files-reg.c
index 9d5e1d743117..a36b60142760 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 for
+	 * simplicity case, in future we might need to
+	 * make more complex full path comparision.
+	 */
+	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,19 @@  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) {
+	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);
 		close(rmntns_root);
-		pr_perror("Couldn't unlink remap %s %s", path, remap->rpath);
 		return -1;
 	}
+
 	close(rmntns_root);
 	remap->rpath[0] = 0;
-
 	return 0;
 }
 
@@ -767,6 +869,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. */

Comments

Pavel Tikhomirov Feb. 26, 2019, 1:09 p.m.
>   static int clean_one_remap(struct remap_info *ri)
> @@ -733,15 +831,19 @@ 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) {
> +	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);
>   		close(rmntns_root);
> -		pr_perror("Couldn't unlink remap %s %s", path, remap->rpath);

Missed these previously, why you change path to rmntns_root? Please see 
commit caaedad3f ("files-reg: make error message in clean_one_remap 
contain mntns path")

>   		return -1;
>   	}
> +
>   	close(rmntns_root);
>   	remap->rpath[0] = 0;
> -
>   	return 0;
>   }
>   
> @@ -767,6 +869,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. */
>
Cyrill Gorcunov Feb. 26, 2019, 1:16 p.m.
On Tue, Feb 26, 2019 at 01:09:15PM +0000, Pavel Tikhomirov wrote:
> > +	if (ret) {
> > +		pr_perror("Couldn't unlink remap %d %s", rmntns_root, remap->rpath);
> >   		close(rmntns_root);
> > -		pr_perror("Couldn't unlink remap %s %s", path, remap->rpath);
> 
> Missed these previously, why you change path to rmntns_root? Please see 
> commit caaedad3f ("files-reg: make error message in clean_one_remap 
> contain mntns path")

Because I start working with this set on criu-stable when we had
no such commit, and cherry-picked it here. Good point, I'll send
a fix on top.