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

[Pavel Emelyanov]

On 04/23/2017 04:29 AM, Rodrigo Bruno wrote:
> Hi,
> 
> the patch looks good. It avoids using 'pread' which is not supported for remote images.
> 
> However, the patch that I sent for the mailing list introducing remote images used 'read' instead of 'pread'. 

Yes, sorry about that, it was me wrongly merging async reads with remotes :(

I have a question regarding this place, while we're at it. Here's the code from
maybe_read_page_local():

        if ((flags & (PR_ASYNC|PR_ASAP)) == PR_ASYNC && !opts.remote)
                ret = enqueue_async_page(pr, vaddr, len, buf);
        else {
                ret = read_local_page(pr, vaddr, len, buf);

Why don't we enqueue the read request for opts.remote case?

> If more code locations were reverted to 'pread', it might be necessary to protect them as well.
> 
> Is there any reason for using 'pread' instead of 'read'? It seems to me that the code always reads files forwards. 
> Additionally, I also made some experiments (including zdtm tests) back in February using 'read' instead of 'pread' and it worked.
> 
> Best,
> rodrigo
> 
> 
> 2017-04-22 4:42 GMT+01:00 Andrei Vagin <avagin at virtuozzo.com <mailto:avagin at virtuozzo.com>>:
> 
>     Rodrigo, could you review this patch?
> 
>     On Fri, Apr 21, 2017 at 06:25:54PM +0300, 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 <mailto: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) {
>     >               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) {
>     > --
>     > 1.9.1
>     >
> 
>