[4/5] files: Cleanup ghost subdirectories

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

Details

Message ID 20190226091335.21922-5-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.
In case if some of ghost directories has transient entries
which we had to creat while restoring they cause inability
to remove toplevel directory due to ENOTEMPTY error raised.

To address this we do the following:

 - when ghost entries are created remember which
   dentries were created in @remap->subdirs

 - when we're cleaning them up we remove these
   transient entries up to @remap->subdirs

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 criu/files-reg.c         | 51 ++++++++++++++++++++++++++++++++++++----
 criu/include/files-reg.h |  2 ++
 2 files changed, 49 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/files-reg.c b/criu/files-reg.c
index a36b60142760..f304efa8c770 100644
--- a/criu/files-reg.c
+++ b/criu/files-reg.c
@@ -355,6 +355,7 @@  static int ghost_apply_metadata(const char *path, GhostFileEntry *gfe)
 
 static int create_ghost(struct ghost_file *gf, GhostFileEntry *gfe, struct cr_img *img)
 {
+	const char *subdir = NULL;
 	struct mount_info *mi;
 	char path[PATH_MAX];
 	int ret, root_len;
@@ -391,7 +392,8 @@  static int create_ghost(struct ghost_file *gf, GhostFileEntry *gfe, struct cr_im
 		if ((ret = mknod(path, gfe->mode, gfe->rdev)) < 0)
 			msg = "Can't create node for ghost dev";
 	} else if (S_ISDIR(gfe->mode)) {
-		if ((ret = mkdirpat(AT_FDCWD, path, gfe->mode)) < 0)
+		ret = mkdirpat_precise(AT_FDCWD, path, gfe->mode, &subdir);
+		if (ret < 0)
 			msg = "Can't make ghost dir";
 	} else {
 		if ((ret = mkreg_ghost(path, gfe, img)) < 0)
@@ -415,6 +417,14 @@  static int create_ghost(struct ghost_file *gf, GhostFileEntry *gfe, struct cr_im
 	strcpy(gf->remap.rpath, path + root_len);
 	pr_debug("Remap rpath is %s\n", gf->remap.rpath);
 
+	/*
+	 * Save nested dirs for better cleanup.
+	 */
+	if (subdir && subdir != path) {
+		gf->remap.subdir  = gf->remap.rpath;
+		gf->remap.subdir += subdir - path - root_len;
+	}
+
 	ret = -1;
 	if (ghost_apply_metadata(path, gfe))
 		goto err;
@@ -758,6 +768,9 @@  static int order_remap_dirs(void)
 			pr_debug("remap: ghost ord %3s %s\n",
 				 ri->rfi->remap->is_dir ? "dir" : "fil",
 				 ri->rfi->path);
+			if (ri->rfi->remap->subdir)
+				pr_debug("remap:\tnew subdir %s\n",
+					 ri->rfi->remap->subdir);
 		}
 	}
 
@@ -783,6 +796,31 @@  int prepare_remaps(void)
 	return ret ? : order_remap_dirs();
 }
 
+static int cleanup_subdir(int dirfd, struct file_remap *remap)
+{
+	char *pos;
+	int ret;
+
+	if (!remap->subdir)
+		return 0;
+
+	/*
+	 * Note that we're modifying @remap->rpath but
+	 * caller is dropping this variable a bit later
+	 * anyway!
+	 */
+	for (pos = strrchr(remap->subdir, '/'); pos;
+	     pos = strrchr(pos, '/')) {
+		*pos = '\0';
+		pr_debug("\tUnlink remap subdir %s\n", remap->rpath);
+		ret = unlinkat(dirfd, remap->rpath, AT_REMOVEDIR);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int clean_ghost_dir(int rmntns_root, struct remap_info *ri)
 {
 	struct file_remap *remap = ri->rfi->remap;
@@ -791,10 +829,15 @@  static int clean_ghost_dir(int rmntns_root, struct remap_info *ri)
 	 * When deleting ghost directories here is two issues:
 	 * - names might duplicate, so we may receive ENOENT
 	 *   and should not treat it as an error
+	 * - some ghost directories might have transient entries
+	 *   which we have to make during former path restoration
 	 */
-	if (ret)
-		return errno == ENOENT ? 0 : -errno;
-	return 0;
+	if (ret) {
+		if (errno == ENOENT)
+			ret = 0;
+	} else
+		ret = cleanup_subdir(rmntns_root, remap);
+	return ret;
 }
 
 static int clean_one_remap(struct remap_info *ri)
diff --git a/criu/include/files-reg.h b/criu/include/files-reg.h
index ec142db8c882..9b1339353878 100644
--- a/criu/include/files-reg.h
+++ b/criu/include/files-reg.h
@@ -10,6 +10,7 @@  struct cr_imgset;
 struct fd_parms;
 
 struct file_remap {
+	char *subdir;
 	char *rpath;
 	bool is_dir;
 	int  rmnt_id;
@@ -20,6 +21,7 @@  struct file_remap {
 static inline void file_remap_init(struct file_remap *remap)
 {
 	remap->rpath	= NULL;
+	remap->subdir	= NULL;
 	remap->is_dir	= false;
 	remap->uid	= -1;
 	remap->gid	= -1;

Comments

Andrei Vagin Feb. 27, 2019, 6:49 a.m.
On Tue, Feb 26, 2019 at 12:13:34PM +0300, Cyrill Gorcunov wrote:
> In case if some of ghost directories has transient entries
> which we had to creat while restoring they cause inability
> to remove toplevel directory due to ENOTEMPTY error raised.
> 
> To address this we do the following:
> 
>  - when ghost entries are created remember which
>    dentries were created in @remap->subdirs
> 
>  - when we're cleaning them up we remove these
>    transient entries up to @remap->subdirs
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  criu/files-reg.c         | 51 ++++++++++++++++++++++++++++++++++++----
>  criu/include/files-reg.h |  2 ++
>  2 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/criu/files-reg.c b/criu/files-reg.c
> index a36b60142760..f304efa8c770 100644
> --- a/criu/files-reg.c
> +++ b/criu/files-reg.c
> @@ -355,6 +355,7 @@ static int ghost_apply_metadata(const char *path, GhostFileEntry *gfe)
>  
>  static int create_ghost(struct ghost_file *gf, GhostFileEntry *gfe, struct cr_img *img)
>  {
> +	const char *subdir = NULL;
>  	struct mount_info *mi;
>  	char path[PATH_MAX];
>  	int ret, root_len;
> @@ -391,7 +392,8 @@ static int create_ghost(struct ghost_file *gf, GhostFileEntry *gfe, struct cr_im
>  		if ((ret = mknod(path, gfe->mode, gfe->rdev)) < 0)
>  			msg = "Can't create node for ghost dev";
>  	} else if (S_ISDIR(gfe->mode)) {
> -		if ((ret = mkdirpat(AT_FDCWD, path, gfe->mode)) < 0)
> +		ret = mkdirpat_precise(AT_FDCWD, path, gfe->mode, &subdir);
> +		if (ret < 0)
>  			msg = "Can't make ghost dir";
>  	} else {
>  		if ((ret = mkreg_ghost(path, gfe, img)) < 0)
> @@ -415,6 +417,14 @@ static int create_ghost(struct ghost_file *gf, GhostFileEntry *gfe, struct cr_im
>  	strcpy(gf->remap.rpath, path + root_len);
>  	pr_debug("Remap rpath is %s\n", gf->remap.rpath);
>  
> +	/*
> +	 * Save nested dirs for better cleanup.
> +	 */
> +	if (subdir && subdir != path) {
> +		gf->remap.subdir  = gf->remap.rpath;
> +		gf->remap.subdir += subdir - path - root_len;
> +	}
> +
>  	ret = -1;
>  	if (ghost_apply_metadata(path, gfe))
>  		goto err;
> @@ -758,6 +768,9 @@ static int order_remap_dirs(void)
>  			pr_debug("remap: ghost ord %3s %s\n",
>  				 ri->rfi->remap->is_dir ? "dir" : "fil",
>  				 ri->rfi->path);
> +			if (ri->rfi->remap->subdir)
> +				pr_debug("remap:\tnew subdir %s\n",
> +					 ri->rfi->remap->subdir);
>  		}
>  	}
>  
> @@ -783,6 +796,31 @@ int prepare_remaps(void)
>  	return ret ? : order_remap_dirs();
>  }
>  
> +static int cleanup_subdir(int dirfd, struct file_remap *remap)
> +{
> +	char *pos;
> +	int ret;
> +
> +	if (!remap->subdir)
> +		return 0;
> +
> +	/*
> +	 * Note that we're modifying @remap->rpath but
> +	 * caller is dropping this variable a bit later
> +	 * anyway!
> +	 */
> +	for (pos = strrchr(remap->subdir, '/'); pos;
> +	     pos = strrchr(pos, '/')) {
> +		*pos = '\0';
> +		pr_debug("\tUnlink remap subdir %s\n", remap->rpath);
> +		ret = unlinkat(dirfd, remap->rpath, AT_REMOVEDIR);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int clean_ghost_dir(int rmntns_root, struct remap_info *ri)
>  {
>  	struct file_remap *remap = ri->rfi->remap;
> @@ -791,10 +829,15 @@ static int clean_ghost_dir(int rmntns_root, struct remap_info *ri)
>  	 * When deleting ghost directories here is two issues:
>  	 * - names might duplicate, so we may receive ENOENT
>  	 *   and should not treat it as an error
> +	 * - some ghost directories might have transient entries
> +	 *   which we have to make during former path restoration
>  	 */
> -	if (ret)
> -		return errno == ENOENT ? 0 : -errno;
> -	return 0;
> +	if (ret) {
> +		if (errno == ENOENT)
> +			ret = 0;
> +	} else
> +		ret = cleanup_subdir(rmntns_root, remap);

should we call cleanup_subdir() if errno == ENOENT?
> +	return ret;
>  }
>  
>  static int clean_one_remap(struct remap_info *ri)
> diff --git a/criu/include/files-reg.h b/criu/include/files-reg.h
> index ec142db8c882..9b1339353878 100644
> --- a/criu/include/files-reg.h
> +++ b/criu/include/files-reg.h
> @@ -10,6 +10,7 @@ struct cr_imgset;
>  struct fd_parms;
>  
>  struct file_remap {
> +	char *subdir;
>  	char *rpath;
>  	bool is_dir;
>  	int  rmnt_id;
> @@ -20,6 +21,7 @@ struct file_remap {
>  static inline void file_remap_init(struct file_remap *remap)
>  {
>  	remap->rpath	= NULL;
> +	remap->subdir	= NULL;
>  	remap->is_dir	= false;
>  	remap->uid	= -1;
>  	remap->gid	= -1;
> -- 
> 2.20.1
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
Cyrill Gorcunov Feb. 27, 2019, 6:59 a.m.
On Tue, Feb 26, 2019 at 10:49:18PM -0800, Andrei Vagin wrote:
...
> > -	return 0;
> > +	if (ret) {
> > +		if (errno == ENOENT)
> > +			ret = 0;
> > +	} else
> > +		ret = cleanup_subdir(rmntns_root, remap);
> 
> should we call cleanup_subdir() if errno == ENOENT?
Nope :) If errno == enoent it means there is notthing to clean :)
But I think I got what is confusing you. We should call
subdirs cleanup only on enotempty. Will update, thanks!
Cyrill Gorcunov Feb. 27, 2019, 8:34 a.m.
On Wed, Feb 27, 2019 at 09:59:32AM +0300, Cyrill Gorcunov wrote:
> On Tue, Feb 26, 2019 at 10:49:18PM -0800, Andrei Vagin wrote:
> ...
> > > -	return 0;
> > > +	if (ret) {
> > > +		if (errno == ENOENT)
> > > +			ret = 0;
> > > +	} else
> > > +		ret = cleanup_subdir(rmntns_root, remap);
> > 
> > should we call cleanup_subdir() if errno == ENOENT?
> Nope :) If errno == enoent it means there is notthing to clean :)
> But I think I got what is confusing you. We should call
> subdirs cleanup only on enotempty. Will update, thanks!

Well, not really. We need to clean up all transient entries
once we cleaned up the lower dentry. I simply put more
detailed comment here.

	Cyrill