[CRIU] [PATCH 2/3] files: Rework clean_one_remap to order ghost dirs removal
Cyrill Gorcunov
gorcunov at gmail.com
Fri Feb 15 20:57:29 MSK 2019
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
Still there is another problem appeared (kudos to Pasha Tikhomirov
for findings) -- the directory may have nested directory which is
not opened by anyone, for example (d - dir, f - file) consider
path d1/d2/d3/f1, where f1 is opened, d3 is opened, d1 is opened
but d2 is not. Since they all are ghost entries we should clean
them up but unlike ghost (files for which we mangle names), the
directories are different, we have to create the whole path for them.
In result when we cleaned up d3 and f1 we try to unlink d1 but it
has d2 beneath so we fail.
At moment I fixed it with recursive cleanup call but I'm unhappy
with it. I think some proper solution might be to revork remap
engine for directories -- we should analyze the paths on image
upload and
1) fold duplicates (same ghost directory may be opened more than once)
2) remember the tail of the path when we create it (but we need to
keep it somewhere, thus path[0] = '\0' won't be a sign anymore
and we need to allocate another member in remap structure).
Or maybe some come with some better idea. Which would be very welcome.
Signed-off-by: Cyrill Gorcunov <gorcunov at gmail.com>
---
criu/files-reg.c | 152 +++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 146 insertions(+), 6 deletions(-)
diff --git a/criu/files-reg.c b/criu/files-reg.c
index 9d5e1d743117..0b271afcbb40 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,74 @@ 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];
+ return -strcmp(a->rfi->path, b->rfi->path);
+}
+
+/*
+ * 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,9 +765,72 @@ int prepare_remaps(void)
break;
}
+ return ret ? : order_remap_dirs();
+}
+
+static int remove_dir_at(unsigned int rlevel, int dirdf, const char *name)
+{
+ static const unsigned rlimit = 128;
+ struct dirent *de;
+ int ret, fd;
+ DIR *dir;
+
+ if (rlevel > rlimit) {
+ pr_err("Recursion limit %u overrun\n", rlimit);
+ errno = ENOMEM;
+ return -ENOMEM;
+ }
+
+ ret = unlinkat(dirdf, name, AT_REMOVEDIR);
+ if (ret < 0) {
+ if (errno == ENOTEMPTY) {
+ fd = openat(dirdf, name, O_RDONLY | O_DIRECTORY);
+ if (fd < 0)
+ return -errno;
+ dir = fdopendir(fd);
+ if (!dir) {
+ int _errno = errno;
+ close(fd);
+ return -_errno;
+ }
+ while ((de = readdir(dir))) {
+ if (dir_dots(de))
+ continue;
+ ret = remove_dir_at(rlevel++, fd, de->d_name);
+ if (ret)
+ break;
+ }
+ closedir(dir);
+ close(fd);
+ } else
+ ret = -errno;
+ }
+
return ret;
}
+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
+ * - names might have nested directories which were
+ * recreated during restore procedure, we remove then
+ * recursively (FIXME) but probably we should do the
+ * reverse and enhance remap engine to eliminate
+ * duplicates and track _all_ directories we've
+ * recreated.
+ */
+ if (ret < 0) {
+ if (errno == ENOTEMPTY)
+ return remove_dir_at(0, rmntns_root, remap->rpath);
+ }
+ return errno == ENOENT ? 0 : -errno;
+}
+
static int clean_one_remap(struct remap_info *ri)
{
struct file_remap *remap = ri->rfi->remap;
@@ -733,15 +865,22 @@ 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);
+ /*
+ * FIXME: We might have duplicated dentries opened
+ * and unlike ghost files we don't mangle their name
+ * so we should handle them separately.
+ */
+ 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);
- remap->rpath[0] = 0;
+ remap->rpath[0] = 0;
return 0;
}
@@ -767,6 +906,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. */
--
2.20.1
More information about the CRIU
mailing list