<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Aug 13, 2014 at 1:13 AM, Pavel Emelyanov <span dir="ltr"><<a href="mailto:xemul@parallels.com" target="_blank">xemul@parallels.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">On 08/13/2014 04:29 AM, Saied Kazemi wrote:<br>
<br>
Hi, Saied.<br>
<div><br>
> Here is a new patch. Â I rebased to the head (commit ded04267f8) and cleaned up the code a<br>
> bit more. Â Please use the attached patch instead of the one I sent yesterday.<br>
><br>
> Note that I had to delete 3 lines from cgroup.c for properties that I don't have on my<br>
> Ubuntu 14.04. Â Also, please note that you have to add --manage-cgroups to criu command<br>
> line for both dump and restore.<br>
<br>
</div>Thank you for looking into this. I'm not familiar with AUFS at all, thus I<br>
only have comments about the way AUFS parsing/fixing code is integrated into<br>
the rest of the criu code.<br></blockquote><div><br></div><div>I am not familiar with AUFS internals either. Â I just looked at how it's set up and what information it passes through /proc.</div><div><br></div><div>Â </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Please, see my comments inline.<br>
<br>
> @@ -197,6 +199,19 @@ int fill_fdlink(int lfd, const struct fd_parms *p, struct fd_link *link)<br>
> Â Â Â Â Â Â Â return -1;<br>
> Â Â Â }<br>
><br>
> + Â Â /*<br>
> + Â Â Â * For AUFS support, we need to replace absolute<br>
> + Â Â Â * branch pathnames with relative pathnames from root.<br>
> + Â Â Â */<br>
> + Â Â if (opts.aufs) {<br>
<br>
At this point we have the statfs() information. Can we rely on the<br>
fs_type being aufs magic to check that the file is aufs one?<br></blockquote><div><br></div><div>No, because depending on how we end up in fill_fdlink(), p->fstype may be NULL. Â Besides, even when set, its fstyp isn't aufs. Â It'd be something like ext4 because the branch is in ext4.</div>
<div><br></div><div>Â </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
> + Â Â Â Â Â Â int n = sizeof link->name - 1;<br>
> + Â Â Â Â Â Â n = fixup_aufs_path(&link->name[1], n, true);<br>
> + Â Â Â Â Â Â if (n < 0)<br>
> + Â Â Â Â Â Â Â Â Â Â return -1;<br>
> + Â Â Â Â Â Â if (n > 0)<br>
> + Â Â Â Â Â Â Â Â Â Â len = n;<br>
> + Â Â }<br>
> +<br>
> Â Â Â link->len = len + 1;<br>
> Â Â Â return 0;<br>
<br>
> @@ -450,12 +453,24 @@ int parse_smaps(pid_t pid, struct vm_area_list *vma_area_list, bool use_map_file<br>
> Â Â Â Â Â Â Â Â Â Â Â vma_area->st = prev->st;<br>
> Â Â Â Â Â Â Â } else if (vma_area->vm_file_fd >= 0) {<br>
> Â Â Â Â Â Â Â Â Â Â Â struct stat *st_buf;<br>
> + Â Â Â Â Â Â Â Â Â Â char *f;<br>
><br>
> Â Â Â Â Â Â Â Â Â Â Â st_buf = vma_area->st = xmalloc(sizeof(*st_buf));<br>
> Â Â Â Â Â Â Â Â Â Â Â if (!st_buf)<br>
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â goto err;<br>
><br>
> - Â Â Â Â Â Â Â Â Â Â if (fstat(vma_area->vm_file_fd, st_buf) < 0) {<br>
> + Â Â Â Â Â Â Â Â Â Â /*<br>
> + Â Â Â Â Â Â Â Â Â Â Â * For AUFS support, we cannot fstat() file a descriptor that<br>
> + Â Â Â Â Â Â Â Â Â Â Â * is a symbolic link to a branch. Â Instead, we obtain the<br>
> + Â Â Â Â Â Â Â Â Â Â Â * pathname of the file from the root and use stat().<br>
> + Â Â Â Â Â Â Â Â Â Â Â */<br>
> + Â Â Â Â Â Â Â Â Â Â if (opts.aufs && (f = fixup_aufs_fd_path(vma_area->vm_file_fd))) {<br>
<br>
Would this work if the vm_file_fd sits in another mount namespace? Or<br>
the path reported by fixup_aufs_fd_path() is always in the criu's one?<br></blockquote><div><br></div><div>I think it should work, although I don't know how to force it in a different mount namespace in the tests that I am running. Â Can you whip up an example?</div>
<div>Â </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
And another question -- do we need the fixed stat buffer that early?<br></blockquote><div><br></div><div>I am doing this exactly at the same time that CRIU is doing it (i.e., the fstat() is original code).  I'm just using stat() instead of fstat(), so effectively it's the same logic/time as before. </div>
<div>Â </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
The vma fd is dumped in dump_filemap() and in that place we call<br>
get_fd_mntid() for missing mount ID. Can we fixup the device there too?<br></blockquote><div><br></div><div>The way I handle it is like this: Â If the link file descriptor points to an AUFS branch name, we replace it with a pathname from root. Â Here is the call sequence from dump_filemap():</div>
<div><br></div><div>dump_filemap()</div><div>Â Â dump_one_reg_file(lfd)<br></div><div>Â Â Â fill_fdlink(lfd)</div><div>Â Â Â Â Â read_fd_link(lfd)</div><div>Â Â Â Â Â Â readlink(lfd) Â Â Â Â Â Â Â // returns path in branch</div>
<div>Â Â Â Â Â fixup_aufs_brnach() Â Â // replaces path in branch with path from root</div><div>Â Â Â check_path_remap()</div><div><br></div><div>In other words, when we see a pathname in a branch, we replace it with a pathname from root as if we never saw the branch pathname. Â When using a link file descriptor in fstat(), because the kernel returns the stat info of the pathname in branch, we use stat() with a pathname from the root instead of fstat(). Â If we didn't do this, we'd get different device/inode values and CRIU fails with an error message like "Unaccessible path opened 33:23, need 2049:53764".</div>
<div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (stat(f, st_buf) < 0) {<br>
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pr_perror("Failed fstat on %d's map %lu (%s)",<br>
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pid, start, f);<br>
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â goto err;<br>
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â }<br>
> + Â Â Â Â Â Â Â Â Â Â } else if (fstat(vma_area->vm_file_fd, st_buf) < 0) {<br>
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pr_perror("Failed fstat on %d's map %lu", pid, start);<br>
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â goto err;<br>
> Â Â Â Â Â Â Â Â Â Â Â }<br>
<br>
> @@ -921,6 +942,16 @@ static int parse_mountinfo_ent(char *str, struct mount_info *new)<br>
> Â Â Â if (ret != 3)<br>
> Â Â Â Â Â Â Â return -1;<br>
><br>
> + Â Â /* see comments in sysfs_parse.c */<br>
> + Â Â if (opts.aufs && !strcmp(new->mountpoint, "./")) {<br>
> + Â Â Â Â Â Â if (strcmp(fstype, "aufs")) {<br>
<br>
Can we reuse the fstype->parse() callback for this? This one gets called<br>
in parse_mountinfo() in a loop after parse_mountinfo_ent().<br></blockquote><div><br></div><div>Yes, we can and I changed the code accordingly (added aufs to fstypes[]). Â Please use the new patch.</div><div><br></div><div>
 <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> + Â Â Â Â Â Â Â Â Â Â pr_err("Expected fstype aufs got %s\n", fstype);<br>
> + Â Â Â Â Â Â Â Â Â Â return -1;<br>
> + Â Â Â Â Â Â }<br>
> + Â Â Â Â Â Â if (fixup_src_opt(&new->source, &opt) < 0)<br>
> + Â Â Â Â Â Â Â Â Â Â return -1;<br>
> + Â Â }<br>
> +<br>
> Â Â Â ret = -1;<br>
> Â Â Â new->fstype = find_fstype_by_name(fstype);<br>
><br>
<br>
> +/*<br>
> + * Copy the line in process's mountinfo file that corresponds<br>
> + * to the mountpoint specified by the mntpoint argument. Â Return<br>
> + * the number of characters parsed in the line, or -1 on error.<br>
> + */<br>
> +int get_mountinfo_by_mountpoint(pid_t pid, char *mntpoint, char *line, int linelen)<br>
<br>
I do not fully understand why this mountpoint->something-else parser<br>
is required. Can we put all the code parsing aufs-specific stuff into<br>
the fstype->parse() callback I mentioned above? When criu starts<br>
parsing mount namespaces (it does this before doing anything else)<br>
this info will be parsed and stored into mount_info->private. Later,<br>
when we need to e.g. fixup file paths we can get file->mnt_id, get<br>
mount_info by it, then check the fs being aufs and fix the path<br>
according to the mount_info->private data we have. Would this work?<br></blockquote><div><br></div><div>I am now using the parse callback that you suggested but we need the "reference" values *before* parse_mountinfo() is called. Â Otherwise, when we call the parse callback, we won't know what values to replace with.<br>
</div><div><br></div><div>That said, I noticed that with my latest rebase there is apparently no longer a need to specify --aufs-ref. Â This is great as we can get rid of the code fixing up the mountinfo root entry (note that we'd still need to replace / with the --aufs-root).</div>
<div><br></div><div>Since I am not familiar with the internals of CRIU as to how it builds its bind mount list, could you please review this part of the code? Â As mentioned before, it used to be that without --aufs-ref, mounts_equal() would return false, in my test cases, for /etc/hosts, /etc/hostname, and /etc/resolv.conf because their dev, fstype, source, and options were different. Â As a result, these files would not be added to the mnt_bind list in collect_shared() and restore would fail with the error message "A few mount points can't be mounted." Â I am puzzled that restore is now successful without --aufs-ref. Â I need to look into this deeper myself but would really appreciate your thoughts and feedback.</div>
<div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> +{<br>
> + Â Â char buf[PATH_MAX];<br>
> + Â Â int n, ret;<br>
> + Â Â FILE *f;<br>
<br>
> +<br>
> + Â Â snprintf(buf, sizeof buf, "/proc/%d/mountinfo", pid);<br>
> + Â Â f = fopen(buf, "r");<br>
> + Â Â if (!f) {<br>
> + Â Â Â Â Â Â pr_perror("Cannot fopen %s", buf);<br>
> + Â Â Â Â Â Â return -1;<br>
> + Â Â }<br>
> +<br>
> + Â Â do {<br>
> + Â Â Â Â Â Â n = linelen - 2;<br>
> + Â Â Â Â Â Â line[n] = '\0'; Â Â Â Â /* detect long input */<br>
> + Â Â Â Â Â Â if (fgets(line, linelen, f) == NULL) {<br>
> + Â Â Â Â Â Â Â Â Â Â ret = 0;<br>
> + Â Â Â Â Â Â Â Â Â Â break;<br>
> + Â Â Â Â Â Â }<br>
> + Â Â Â Â Â Â if (line[n] && line[n] != '\n') {<br>
> + Â Â Â Â Â Â Â Â Â Â pr_err("Line in mountinfo too long\n");<br>
> + Â Â Â Â Â Â Â Â Â Â ret = -1;<br>
> + Â Â Â Â Â Â Â Â Â Â goto out;<br>
> + Â Â Â Â Â Â }<br>
> +<br>
> + Â Â Â Â Â Â ret = sscanf(line, "%*i %*i %*u:%*u %*s %s %*s - %n", buf, &n);<br>
<br>
Wow. Doesn't gcc prints a warning about "too few arguments for format"?<br></blockquote><div><br></div><div>No, because * means discard what's scanned.</div><div><br></div><div>Â </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> + Â Â Â Â Â Â if (ret != 1) {<br>
> + Â Â Â Â Â Â Â Â Â Â pr_err("Cannot parse mountpoint (%s)\n", line);<br>
> + Â Â Â Â Â Â Â Â Â Â ret = -1;<br>
> + Â Â Â Â Â Â Â Â Â Â goto out;<br>
> + Â Â Â Â Â Â }<br>
> + Â Â } while (strcmp(buf, mntpoint));<br>
> +<br>
> + Â Â if (!ret) {<br>
> + Â Â Â Â Â Â pr_err("Did not find %s in mountinfo\n", mntpoint);<br>
> + Â Â Â Â Â Â ret = -1;<br>
> + Â Â } else<br>
> + Â Â Â Â Â Â ret = n;<br>
> +<br>
> +out:<br>
> + Â Â fclose(f);<br>
> + Â Â return ret;<br>
> +}<br>
<br>
Thanks,<br>
Pavel<br>
<br>
</blockquote></div></div><div><br></div>Cheers!<div><br><div>--Saied</div><div><br></div></div></div>