[CRIU] [PATCH] pagemap: fix reading pages from socket for --remote case

[Pavel Emelyanov]

On 04/24/2017 04:52 PM, Mike Rapoport wrote:
> On Mon, Apr 24, 2017 at 03:10:04PM +0300, Pavel Emelyanov wrote:
>> On 04/21/2017 06:25 PM, Mike Rapoport wrote:
>>> When --remote option is specified, read_local_page tries to pread from a
>>> socket, and fails with "Illegal seek" error.
>>> Restore single pread call for regular image files case and use read syscall
>>> instead of pread for the --remote case.
>>>
>>> Signed-off-by: Mike Rapoport <rppt at linux.vnet.ibm.com>
>>> ---
>>>  criu/pagemap.c | 18 +++++++++++++-----
>>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/criu/pagemap.c b/criu/pagemap.c
>>> index 6c741b4..7d25c6f 100644
>>> --- a/criu/pagemap.c
>>> +++ b/criu/pagemap.c
>>> @@ -265,15 +265,23 @@ static int read_local_page(struct page_read *pr, unsigned long vaddr,
>>>  		return -1;
>>>  
>>>  	pr_debug("\tpr%d-%u Read page from self %lx/%"PRIx64"\n", pr->pid, pr->id, pr->cvaddr, pr->pi_off);
>>> -	while (1) {
>>> +	if (!opts.remote) {
>>
>> Maybe it's better to introduce separate ->maybe_read_page callback for opts.remote case?
>> This would let us localize the cache/proxy pages reading code in one place.
> 
> Well, we could. E.g. something like the patch below (untested). Note that
> we already have maybe_read_page_remote for the remote lazy restore case,
> and, ideally, the cache/proxy and lazy cases should use the same code to
> read pages.

Yup. I like this patch :)
Rodrigo, what do you think?

> ----------------
> diff --git a/criu/pagemap.c b/criu/pagemap.c
> index 6c741b4..353c469 100644
> --- a/criu/pagemap.c
> +++ b/criu/pagemap.c
> @@ -255,7 +255,6 @@ static int read_local_page(struct page_read *pr, unsigned long vaddr,
>  {
>  	int fd = img_raw_fd(pr->pi);
>  	int ret;
> -	size_t curr = 0;
>  
>  	/*
>  	 * Flush any pending async requests if any not to break the
> @@ -265,15 +264,10 @@ static int read_local_page(struct page_read *pr, unsigned long vaddr,
>  		return -1;
>  
>  	pr_debug("\tpr%d-%u Read page from self %lx/%"PRIx64"\n", pr->pid, pr->id, pr->cvaddr, pr->pi_off);
> -	while (1) {
> -		ret = pread(fd, buf + curr, len - curr, pr->pi_off + curr);
> -		if (ret < 1) {
> -			pr_perror("Can't read mapping page %d", ret);
> -			return -1;
> -		}
> -		curr += ret;
> -		if (curr == len)
> -			break;
> +	ret = pread(fd, buf, len, pr->pi_off);
> +	if (ret != len) {
> +		pr_perror("Can't read mapping page %d", ret);
> +		return -1;
>  	}
>  
>  	if (opts.auto_dedup) {
> @@ -390,6 +384,40 @@ static int maybe_read_page_local(struct page_read *pr, unsigned long vaddr,
>  	return ret;
>  }
>  
> +static int maybe_read_page_img_cache(struct page_read *pr, unsigned long vaddr,
> +				     int nr, void *buf, unsigned flags)
> +{
> +	unsigned long len = nr * PAGE_SIZE;
> +	int fd = img_raw_fd(pr->pi);
> +	int ret;
> +	size_t curr = 0;
> +
> +	pr_debug("\tpr%d-%u Read page from self %lx/%"PRIx64"\n", pr->pid, pr->id, pr->cvaddr, pr->pi_off);
> +	while (1) {
> +		ret = read(fd, buf + curr, len - curr);
> +		if (ret < 0) {
> +			pr_perror("Can't read mapping page %d", ret);
> +			return -1;
> +		}
> +		curr += ret;
> +		if (curr == len)
> +			break;
> +	}
> +
> +	if (opts.auto_dedup) {
> +		ret = punch_hole(pr, pr->pi_off, len, false);
> +		if (ret == -1)
> +			return -1;
> +	}
> +
> +	if (ret == 0 && pr->io_complete)
> +		ret = pr->io_complete(pr, vaddr, nr);
> +
> +	pr->pi_off += len;
> +
> +	return ret;
> +}
> +
>  static int read_page_complete(int pid, unsigned long vaddr, int nr_pages, void *priv)
>  {
>  	int ret = 0;
> @@ -803,7 +831,9 @@ int open_page_read_at(int dfd, int pid, struct page_read *pr, int pr_flags)
>  	pr->id = ids++;
>  	pr->pid = pid;
>  
> -	if (remote)
> +	if (opts.remote)
> +		pr->maybe_read_page = maybe_read_page_img_cache;
> +	else if (remote)
>  		pr->maybe_read_page = maybe_read_page_remote;
>  	else
>  		pr->maybe_read_page = maybe_read_page_local;
> ----------------
> 
> 
>> -- Pavel
>>
>>>  		ret = pread(fd, buf + curr, len - curr, pr->pi_off + curr);
>>> -		if (ret < 1) {
>>> +		if (ret != len) {
>>>  			pr_perror("Can't read mapping page %d", ret);
>>>  			return -1;
>>>  		}
>>> -		curr += ret;
>>> -		if (curr == len)
>>> -			break;
>>> +	} else {
>>> +		while (1) {
>>> +			ret = read(fd, buf + curr, len - curr);
>>> +			if (ret < 0) {
>>> +				pr_perror("Can't read mapping page %d", ret);
>>> +				return -1;
>>> +			}
>>> +			curr += ret;
>>> +			if (curr == len)
>>> +				break;
>>> +		}
>>>  	}
>>>  
>>>  	if (opts.auto_dedup) {
>>>
>>
> 
> .
>