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

[Rodrigo Bruno]

Hi,

2017-04-24 12:55 GMT+01:00 Pavel Emelyanov <xemul at virtuozzo.com>:

> 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?
>

To be honest, I don't remember why I avoided async pages... I looked
through the code and I don't see any
particular reason for it not to work with remote images.

Anyway, just to clarify, what is the motivation behind async pages? Are we
avoiding any page read that might be unnecessary
in the future?


>
> > 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
> >     >
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/criu/attachments/20170425/b2c8bb2a/attachment.html>