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

[Mike Rapoport]

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.

----------------
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) {
> > 
>