[CRIU] [PATCH 3/5] deduplication: look up for old pages in previous snapshot

[Andrew Vagin]

On Fri, Oct 11, 2013 at 11:57:33PM +0400, Tikhomirov Pavel wrote:
> old snapshot from "parent" symlink, and pids from pagemap-PID.img files
> 
> Signed-off-by: Tikhomirov Pavel <snorcht at gmail.com>
> ---
>  cr-dedup.c          |  130 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  include/page-read.h |    1 +
>  page-read.c         |   22 +++++++--
>  3 files changed, 149 insertions(+), 4 deletions(-)
>  mode change 100644 => 100755 cr-dedup.c

Why do we need executable bit for these files?

>  mode change 100644 => 100755 include/page-read.h
>  mode change 100644 => 100755 page-read.c
> 
> diff --git a/cr-dedup.c b/cr-dedup.c
> old mode 100644
> new mode 100755
> index 4f959dd..a51d90d
> --- a/cr-dedup.c
> +++ b/cr-dedup.c
> @@ -1,9 +1,137 @@
>  #include <unistd.h>
>  
>  #include "crtools.h"
> +#include "page-read.h"
> +#include "restorer.h"
>  #include "cr-dedup.h"
>  
> -int cr_dedup(void)
> +int _cr_dedup(DIR * dirp, int pid);
> +
> +int cr_dedup(void) 
>  {
> +	int ret;
> +	int pid;
> +	DIR * dirp;
> +	struct dirent *ent;
> +	
> +	dirp = opendir("parent");

Where do you close this dirp?

> +	if (dirp == NULL) {
> +		ret = -1;
> +		return ret;

Should we print the error here?

		return -1;
> +	}
> +
> +	while (1) {
> +		ent = readdir(dirp);
> +		if (ent == NULL)
> +			break;

Do we need to check errors here?

If  the  end  of  the directory stream is reached, NULL is returned and
errno is not changed.  If an error occurs, NULL is returned  and  errno
is set appropriately.

> +
> +		ret = sscanf(ent->d_name, "pagemap-%d.img", &pid);
> +		if (ret == 1) {
> +			pr_info("pid=%d\n", pid);
> +			ret = _cr_dedup(dirp, pid);
> +			if (ret < 0)
> +				return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +int _cr_dedup(DIR * dirp, int pid)
> +{
> +	int ret, predump_fd = -1;
> +	struct page_read pr, pr2;
> +	unsigned long off;
> +	struct iovec iov, iov2;	
> +	
> +	ret = open_page_read(pid, &pr);
> +	if (ret)
> +		return -1;
> +	
> +	predump_fd = dirfd(dirp);
> +	if (predump_fd == -1) {
> +		ret = -1;
> +		goto exit;
> +	}
> +
> +	ret = open_page_rw_dedup(predump_fd, pid, &pr2);
> +	if (ret)
> +		goto exit;
> +
> +	ret = pr.get_pagemap(&pr, &iov);
> +	if (ret <= 0)
> +		goto exit;
> +
> +	ret = pr2.get_pagemap(&pr2, &iov2);
> +	if (ret <= 0)
> +		goto exit;
> +
> +	off = 0;

Where is off used?

> +	while (1) {
> +		unsigned long i_off;
> +		long i_len;
> +
> +		i_off = (unsigned long)max(iov.iov_base, iov2.iov_base) - (unsigned long)iov2.iov_base;
> +		i_len = (long)(min(iov.iov_base + iov.iov_len, iov2.iov_base + iov2.iov_len) - max(iov.iov_base, iov2.iov_base));

Pls, split this line, it's too long.

		i_len = min(iov.iov_base + iov.iov_len, iov2.iov_base + iov2.iov_len) -
			max(iov.iov_base, iov2.iov_base));

> +		if (i_len <= 0) {

I don't understand why do we need to handle this case separatly.

> +			pr_info("No pagemaps intersection\n");
> +
> +			if (iov.iov_base < iov2.iov_base) {
> +				ret = pr.get_pagemap(&pr, &iov);
> +				if (ret <= 0)
> +					break;
> +			} else {
> +				if (!pr2.pe->in_parent)
> +					off += pr2.pe->nr_pages;
> +				ret = pr2.get_pagemap(&pr2, &iov2);
> +				if (ret <= 0)
> +					break;
> +			}
> +			continue;
> +		}
> +
> +		if (iov.iov_base + iov.iov_len == iov2.iov_base + iov2.iov_len) {

Can this block be merged into the next two blocks:
	ret = 0;

	if (iov.iov_base + iov.iov_len <= iov2.iov_base + iov2.iov_len)
		ret |= pr.get_pagemap(&pr, &iov);
	if ((iov.iov_base + iov.iov_len >= iov2.iov_base + iov2.iov_len))
		ret |= pr2.get_pagemap(&pr2, &iov2);

	if (ret)
		break;

> +			ret = pr.get_pagemap(&pr, &iov);
> +                        if (ret <= 0)
	^^^^ spaces instead of tabs
> +                                break;
> +
> +			if (!pr2.pe->in_parent)
> +                                off += pr2.pe->nr_pages;
> +                        ret = pr2.get_pagemap(&pr2, &iov2);
> +                        if (ret <= 0)
> +                                break;
> +			continue;
> +		}
> +
> +		if (iov.iov_base + iov.iov_len < iov2.iov_base + iov2.iov_len) {
> +			ret = pr.get_pagemap(&pr, &iov);
> +			if (ret <= 0)
> +				break;
> +		} else {
> +			if (!pr2.pe->in_parent)
> +				off += pr2.pe->nr_pages;
> +			ret = pr2.get_pagemap(&pr2, &iov2);
> +			if (ret <= 0)
> +				break;
> +		}
> +	}
> +exit:
> +	pr_info("Dedup test END\n");
> +
> +	pr.close(&pr);
> +	pr2.close(&pr2);
> +
> +	if (dirp) {	
> +		ret = closedir(dirp);
> +		if (ret == -1)
> +			return ret;
> +	}
> +
> +	if (predump_fd >= 0) {
> +		ret = close(predump_fd);
> +		if (ret == -1)
> +			return ret;
> +	}
> +
>  	return 0;