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

Submitted by Cyrill Gorcunov on Feb. 26, 2019, 2:07 p.m.

Details

Message ID 20190226140746.GS7198@uranus
State New
Series "Fix order problem in ghost files cleanup"
Headers show

Commit Message

Cyrill Gorcunov Feb. 26, 2019, 2:07 p.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..55b1f2e58dff 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) {
-		close(rmntns_root);
+	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 %s %s", path, remap->rpath);
+		close(rmntns_root);
 		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. */