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

[Pavel Emelyanov]

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.

> 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);
>>>   	}
>>>   
>>>
>>
>