[CRIU] [PATCH] stats/pid-reuse: put stats to image directory instead of cwd

[Andrew Vagin]

On Mon, Mar 26, 2018 at 01:43:11PM +0300, Pavel Emelyanov wrote:
> On 03/26/2018 12:16 PM, Pavel Tikhomirov wrote:
> > On 03/26/2018 11:54 AM, Pavel Emelyanov wrote:
> >> On 03/26/2018 11:12 AM, Pavel Tikhomirov wrote:
> >>> Statistics for a special dump/pre-dump/restore action should be
> >>> collected in the images directory of these action. We need these for
> >>> pid-reuse detection as we need to read the stats of parent predump.
> >>
> >> What's wrong with checking pid-reuse form stats sitting in workdir?
> > 
> > We can read previous dump statistics from workdir before 
> > cr_*dump_finish, I agree with that, it is easy.
> > 
> > But Kirill says that "writing image outside of image directory is 
> 
> Stats file is not an image. We just use PB format for it and reuse existing
> image-writing machinery.
> 
> > architectural problem", actually I think these way too for at least two 
> > reasons:
> > 
> > 1) When stats image is in images dir we can latter analize stats for 
> > each iteration if we want it. Now doing migration in VZ7 we have only 
> > stats of last dump in the end.
> 
> The same problem's for log files (I hope nobody has moved them into images dir
> so far), isn't it? Use separate workdir for each iteration.

It isn't the same, because a name of a log file is specified in options,
but the name of stats file is hardcoded.

> 
> > 2) If for instance stats image was not created,
> 
> Why can this happen?
> 
> > we will see for which > iteration it had happened (there will be no file in directory). Now we 
> > wrongly and silently reuse stats of irrelevant older iteration, and will 
> > do wrong assumptions based on wrong data.
> > 
> >>
> >>> Now then work-dir option is set statistics image is put to the work-dir
> >>> and on each iteration these image will replace previos image.
> >>>
> >>> For backward compatibility with older versions of p.haul create a
> >>> symlink in cwd so that p.haul still has access to the latest stats
> >>> image. Yet I plan to make p.haul work with stats through images dir.
> >>>
> >>> https://jira.sw.ru/browse/PSBM-82864
> >>> Signed-off-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
> >>> ---
> >>>   criu/stats.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>   1 file changed, 60 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/criu/stats.c b/criu/stats.c
> >>> index d344ad336..2c0f8bda4 100644
> >>> --- a/criu/stats.c
> >>> +++ b/criu/stats.c
> >>> @@ -1,6 +1,7 @@
> >>>   #include <unistd.h>
> >>>   #include <fcntl.h>
> >>>   #include <sys/time.h>
> >>> +#include <sys/stat.h>
> >>>   #include "int.h"
> >>>   #include "atomic.h"
> >>>   #include "cr_options.h"
> >>> @@ -157,6 +158,53 @@ static void display_stats(int what, StatsEntry *stats)
> >>>   		return;
> >>>   }
> >>>   
> >>> +static int fd_to_path(int fd, char *buf, int size) {
> >>> +	char proc_self_fd_path[PATH_MAX];
> >>> +	int ret;
> >>> +
> >>> +	if (snprintf(proc_self_fd_path, PATH_MAX, "/proc/self/fd/%d", fd) < 0) {
> >>> +		pr_perror("Failed to format /proc/self/fd/* path");
> >>> +		return -1;
> >>> +	}
> >>> +
> >>> +	ret = readlink(proc_self_fd_path, buf, size-1);
> >>> +	if (ret < 0) {
> >>> +		pr_perror("Failed to readlink %s path", proc_self_fd_path);
> >>> +		return -1;
> >>> +	}
> >>> +	buf[ret] = '\0';
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static void symlink_stats(char *target, char *name)
> >>> +{
> >>> +	char link_path[PATH_MAX];
> >>> +	struct stat st;
> >>> +	int ret;
> >>> +
> >>> +	if (snprintf(link_path, PATH_MAX, imgset_template[CR_FD_STATS].fmt, name) < 0)
> >>> +		goto exit;
> >>> +
> >>> +	ret = lstat(link_path, &st);
> >>> +	if (ret == -1 && errno != ENOENT)
> >>> +		goto exit;
> >>> +	else if (ret == 0) {
> >>> +		/* Real image occupies these location, no symlink needed */
> >>> +		if (!S_ISLNK(st.st_mode))
> >>> +			return;
> >>> +
> >>> +		/* Unlink symlink of previous iteration */
> >>> +		if (unlink(link_path) == -1)
> >>> +			goto exit;
> >>> +	}
> >>> +
> >>> +	if(!symlink(target, link_path))
> >>> +		return;
> >>> +exit:
> >>> +	pr_warn("Error when trying to create stats symlink");
> >>> +}
> >>> +
> >>>   void write_stats(int what)
> >>>   {
> >>>   	StatsEntry stats = STATS_ENTRY__INIT;
> >>> @@ -203,9 +251,20 @@ void write_stats(int what)
> >>>   	} else
> >>>   		return;
> >>>   
> >>> -	img = open_image_at(AT_FDCWD, CR_FD_STATS, O_DUMP, name);
> >>> +	img = open_image(CR_FD_STATS, O_DUMP, name);
> >>>   	if (img) {
> >>> +		char image_path[PATH_MAX];
> >>> +
> >>>   		pb_write_one(img, &stats, PB_STATS);
> >>> +
> >>> +		/*
> >>> +		 * Backward compatibility: old p.haul whants to read stats
> >>> +		 * from working directory, but not from images directory, so
> >>> +		 * create a symlink for it.
> >>> +		 */
> >>> +		if (!fd_to_path(img->fd, image_path, PATH_MAX))
> >>> +			symlink_stats(image_path, name);
> >>> +
> >>>   		close_image(img);
> >>>   	}
> >>>   
> >>>
> >>
> > 
>