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

[Mike Rapoprt]

On April 25, 2017 11:47:59 PM GMT+03:00, Andrei Vagin <avagin at virtuozzo.com> wrote:
>On Tue, Apr 25, 2017 at 01:38:01PM +0300, Mike Rapoport wrote:
>> On Tue, Apr 25, 2017 at 08:56:19AM +0100, Rodrigo Bruno wrote:
>> > Hi,
>> > 
>> > 2017-04-24 18:00 GMT+01:00 Pavel Emelyanov <xemul at virtuozzo.com>:
>> > 
>> > > >>
>> > > >> 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?
>> > >
>> > 
>> > I agree.
>> 
>> All right, below is the more formal version of the patch. I'd suggest
>to
>> start with it to make --remote able to restore the memory. The next
>step
>> related to --remote and the page-read would be to see if it's
>possible to
>> use ASYNC with sockets, what should be refactored to enable it and
>how we
>> reduce code and functionality duplication.
>> 
>>  
>> From 645c2f8e5afe090ee362d3225b97e2b5f1bc95cb Mon Sep 17 00:00:00
>2001
>> From: Mike Rapoport <rppt at linux.vnet.ibm.com>
>> Date: Fri, 21 Apr 2017 17:29:10 +0300
>> Subject: [CRIU][PATCH v3] pagemap: fix reading pages from socket for
>--remote case
>> 
>> 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 introduce
>> maybe_read_page_img_cache version of maybe_read_page method.
>> 
>> Generally-approved-by: Rodrigo Bruno <rbruno at gsd.inesc-id.pt>
>> Signed-off-by: Mike Rapoport <rppt at linux.vnet.ibm.com>
>> ---
>> v3: use different ->maybe_read_page method for --remote case
>> v2: simplify call to pread in non-remote case, it does not need
>'curr'
>> 
>>  criu/pagemap.c | 52
>+++++++++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 41 insertions(+), 11 deletions(-)
>> 
>> 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;
>
>Why do you remove this loop?

This loop is introduced by the commit 86b9170ff3cd "page-read: Support reading by chunks" to allow read data from image-cache through socket.
pread and socket don't play well together, so read_local_page is used only with local images and data from the socket is read in maybe_read_page_image_cache.

>> +	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;
>> -- 
>> 1.9.1
>> 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.