<div dir="ltr">Hi,<br><div class="gmail_extra"><br><div class="gmail_quote">2017-04-24 18:00 GMT+01:00 Pavel Emelyanov <span dir="ltr"><<a href="mailto:xemul@virtuozzo.com" target="_blank">xemul@virtuozzo.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 04/24/2017 04:52 PM, Mike Rapoport wrote:<br>
> On Mon, Apr 24, 2017 at 03:10:04PM +0300, Pavel Emelyanov wrote:<br>
<span class="">>> On 04/21/2017 06:25 PM, Mike Rapoport wrote:<br>
>>> When --remote option is specified, read_local_page tries to pread from a<br>
>>> socket, and fails with "Illegal seek" error.<br>
>>> Restore single pread call for regular image files case and use read syscall<br>
>>> instead of pread for the --remote case.<br>
>>><br>
>>> Signed-off-by: Mike Rapoport <<a href="mailto:rppt@linux.vnet.ibm.com">rppt@linux.vnet.ibm.com</a>><br>
</span><span class="">>>> ---<br>
>>> criu/pagemap.c | 18 +++++++++++++-----<br>
>>> 1 file changed, 13 insertions(+), 5 deletions(-)<br>
>>><br>
>>> diff --git a/criu/pagemap.c b/criu/pagemap.c<br>
>>> index 6c741b4..7d25c6f 100644<br>
>>> --- a/criu/pagemap.c<br>
>>> +++ b/criu/pagemap.c<br>
>>> @@ -265,15 +265,23 @@ static int read_local_page(struct page_read *pr, unsigned long vaddr,<br>
>>> return -1;<br>
>>><br>
>>> pr_debug("\tpr%d-%u Read page from self %lx/%"PRIx64"\n", pr->pid, pr->id, pr->cvaddr, pr->pi_off);<br>
>>> - while (1) {<br>
>>> + if (!opts.remote) {<br>
>><br>
</span>>> Maybe it's better to introduce separate ->maybe_read_page callback for opts.remote case?<br>
>> This would let us localize the cache/proxy pages reading code in one place.<br>
><br>
> Well, we could. E.g. something like the patch below (untested). Note that<br>
> we already have maybe_read_page_remote for the remote lazy restore case,<br>
> and, ideally, the cache/proxy and lazy cases should use the same code to<br>
> read pages.<br>
<br>
Yup. I like this patch :)<br>
Rodrigo, what do you think?<br></blockquote><div><br></div><div><br></div><div>I agree.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> ----------------<br>
> diff --git a/criu/pagemap.c b/criu/pagemap.c<br>
> index 6c741b4..353c469 100644<br>
> --- a/criu/pagemap.c<br>
> +++ b/criu/pagemap.c<br>
> @@ -255,7 +255,6 @@ static int read_local_page(struct page_read *pr, unsigned long vaddr,<br>
> {<br>
> int fd = img_raw_fd(pr->pi);<br>
> int ret;<br>
> - size_t curr = 0;<br>
><br>
> /*<br>
> * Flush any pending async requests if any not to break the<br>
> @@ -265,15 +264,10 @@ static int read_local_page(struct page_read *pr, unsigned long vaddr,<br>
<span class="">> return -1;<br>
><br>
> pr_debug("\tpr%d-%u Read page from self %lx/%"PRIx64"\n", pr->pid, pr->id, pr->cvaddr, pr->pi_off);<br>
> - while (1) {<br>
</span>> - ret = pread(fd, buf + curr, len - curr, pr->pi_off + curr);<br>
> - if (ret < 1) {<br>
> - pr_perror("Can't read mapping page %d", ret);<br>
> - return -1;<br>
> - }<br>
<span class="">> - curr += ret;<br>
> - if (curr == len)<br>
> - break;<br>
</span>> + ret = pread(fd, buf, len, pr->pi_off);<br>
<span class="">> + if (ret != len) {<br>
</span><span class="">> + pr_perror("Can't read mapping page %d", ret);<br>
> + return -1;<br>
> }<br>
><br>
</span>> if (opts.auto_dedup) {<br>
> @@ -390,6 +384,40 @@ static int maybe_read_page_local(struct page_read *pr, unsigned long vaddr,<br>
> return ret;<br>
> }<br>
><br>
> +static int maybe_read_page_img_cache(<wbr>struct page_read *pr, unsigned long vaddr,<br>
> + int nr, void *buf, unsigned flags)<br>
> +{<br>
> + unsigned long len = nr * PAGE_SIZE;<br>
> + int fd = img_raw_fd(pr->pi);<br>
> + int ret;<br>
> + size_t curr = 0;<br>
> +<br>
> + pr_debug("\tpr%d-%u Read page from self %lx/%"PRIx64"\n", pr->pid, pr->id, pr->cvaddr, pr->pi_off);<br>
<span class="">> + while (1) {<br>
> + ret = read(fd, buf + curr, len - curr);<br>
> + if (ret < 0) {<br>
> + pr_perror("Can't read mapping page %d", ret);<br>
> + return -1;<br>
> + }<br>
> + curr += ret;<br>
> + if (curr == len)<br>
> + break;<br>
> + }<br>
</span>> +<br>
> + if (opts.auto_dedup) {<br>
> + ret = punch_hole(pr, pr->pi_off, len, false);<br>
> + if (ret == -1)<br>
<span class="">> + return -1;<br>
> + }<br>
> +<br>
</span>> + if (ret == 0 && pr->io_complete)<br>
> + ret = pr->io_complete(pr, vaddr, nr);<br>
> +<br>
> + pr->pi_off += len;<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> static int read_page_complete(int pid, unsigned long vaddr, int nr_pages, void *priv)<br>
> {<br>
> int ret = 0;<br>
> @@ -803,7 +831,9 @@ int open_page_read_at(int dfd, int pid, struct page_read *pr, int pr_flags)<br>
> pr->id = ids++;<br>
> pr->pid = pid;<br>
><br>
> - if (remote)<br>
> + if (opts.remote)<br>
> + pr->maybe_read_page = maybe_read_page_img_cache;<br>
> + else if (remote)<br>
> pr->maybe_read_page = maybe_read_page_remote;<br>
> else<br>
> pr->maybe_read_page = maybe_read_page_local;<br>
> ----------------<br>
><br>
><br>
>> -- Pavel<br>
<span class="">>><br>
>>> ret = pread(fd, buf + curr, len - curr, pr->pi_off + curr);<br>
>>> - if (ret < 1) {<br>
>>> + if (ret != len) {<br>
>>> pr_perror("Can't read mapping page %d", ret);<br>
>>> return -1;<br>
>>> }<br>
>>> - curr += ret;<br>
>>> - if (curr == len)<br>
>>> - break;<br>
>>> + } else {<br>
>>> + while (1) {<br>
>>> + ret = read(fd, buf + curr, len - curr);<br>
>>> + if (ret < 0) {<br>
>>> + pr_perror("Can't read mapping page %d", ret);<br>
>>> + return -1;<br>
>>> + }<br>
>>> + curr += ret;<br>
>>> + if (curr == len)<br>
>>> + break;<br>
>>> + }<br>
>>> }<br>
>>><br>
>>> if (opts.auto_dedup) {<br>
>>><br>
>><br>
><br>
</span>> .<br>
><br>
<br>
</blockquote></div><br></div></div>