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

[Rodrigo Bruno]

Hi,

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

> On 04/24/2017 04:52 PM, Mike Rapoport wrote:
> > On Mon, Apr 24, 2017 at 03:10:04PM +0300, Pavel Emelyanov wrote:
> >> On 04/21/2017 06:25 PM, 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>
> >>> ---
> >>>  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) {
> >>
> >> 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.


>
> > ----------------
> > 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;
> > +     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;
> > ----------------
> >
> >
> >> -- Pavel
> >>
> >>>             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) {
> >>>
> >>
> >
> > .
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/criu/attachments/20170425/0f1417a7/attachment-0001.html>