<div dir="ltr">Hi,<br><div class="gmail_extra"><br><div class="gmail_quote">2017-04-24 12:55 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"><span class="">On 04/23/2017 04:29 AM, Rodrigo Bruno wrote:<br>
> Hi,<br>
><br>
> the patch looks good. It avoids using 'pread' which is not supported for remote images.<br>
><br>
> However, the patch that I sent for the mailing list introducing remote images used 'read' instead of 'pread'.<br>
<br>
</span>Yes, sorry about that, it was me wrongly merging async reads with remotes :(<br>
<br>
I have a question regarding this place, while we're at it. Here's the code from<br>
maybe_read_page_local():<br>
<br>
if ((flags & (PR_ASYNC|PR_ASAP)) == PR_ASYNC && !opts.remote)<br>
ret = enqueue_async_page(pr, vaddr, len, buf);<br>
else {<br>
ret = read_local_page(pr, vaddr, len, buf);<br>
<br>
Why don't we enqueue the read request for opts.remote case?<br></blockquote><div><br></div><div>To be honest, I don't remember why I avoided async pages... I looked through the code and I don't see any</div><div>particular reason for it not to work with remote images.</div><div><br></div><div>Anyway, just to clarify, what is the motivation behind async pages? Are we avoiding any page read that might be unnecessary</div><div>in the future? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> If more code locations were reverted to 'pread', it might be necessary to protect them as well.<br>
><br>
> Is there any reason for using 'pread' instead of 'read'? It seems to me that the code always reads files forwards.<br>
> Additionally, I also made some experiments (including zdtm tests) back in February using 'read' instead of 'pread' and it worked.<br>
><br>
> Best,<br>
> rodrigo<br>
><br>
><br>
</span>> 2017-04-22 4:42 GMT+01:00 Andrei Vagin <<a href="mailto:avagin@virtuozzo.com">avagin@virtuozzo.com</a> <mailto:<a href="mailto:avagin@virtuozzo.com">avagin@virtuozzo.com</a>>><wbr>:<br>
<span class="">><br>
> Rodrigo, could you review this patch?<br>
><br>
> On Fri, Apr 21, 2017 at 06:25:54PM +0300, 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>
</span>> > Signed-off-by: Mike Rapoport <<a href="mailto:rppt@linux.vnet.ibm.com">rppt@linux.vnet.ibm.com</a> <mailto:<a href="mailto:rppt@linux.vnet.ibm.com">rppt@linux.vnet.ibm.<wbr>com</a>>><br>
<div class="HOEnZb"><div class="h5">> > ---<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>
> > 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>
> > 1.9.1<br>
> ><br>
><br>
><br>
<br>
</div></div></blockquote></div><br></div></div>