<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Aug 21, 2014 at 4:54 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On 08/21/2014 02:32 AM, Saied Kazemi wrote:<br>
> OK, I used ppage_bitmap as aufs_fpath during dump to get rid of fixup_aufs_fd_path(). So, we scan the branches only once.<br>
><br>
> I also got rid of --aufs command line option but kept opts.aufs which is automatically set in the .parse callback.<br>
> Since opts.aufs is now used in only one place, it'll be easier to get rid of it in a future patch.<br>
><br>
> I rebased my code to the head and made sure all comments correctly reflect the current state. Please review the attached<br>
> patch file and let me know what you think.<br>
<br>
</div>I have no more concerns, thanks :) I plan to make 1.3 release at the end of the<br>
next week. If this is the final version of the patch, I can merge it in 1.3<br>
<br>
Thanks,<br>
Pavel<br></blockquote><div><br></div><div>Great! 1.3 will be a feature-rich release.</div><div><br></div><div>I will let you know by the end of this week if there are any changes as I plan to test UnionFS which is similar to AUFS but not identical.</div>
<div><br></div><div>Also, I hope to find time before the end of next week to write an instruction page for <a href="http://criu.org">criu.org</a> on how to dump and restore Docker containers.</div><div><br></div><div>Cheers!</div>
<div><br></div><div>--Saied<br></div><div><br></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">> On Wed, Aug 20, 2014 at 11:00 AM, Pavel Emelyanov <<a href="mailto:xemul@parallels.com">xemul@parallels.com</a> <mailto:<a href="mailto:xemul@parallels.com">xemul@parallels.com</a>>> wrote:<br>
><br>
> On 08/20/2014 08:54 PM, Saied Kazemi wrote:<br>
><br>
> ><br>
> > > @@ -240,6 +244,31 @@ static int vma_get_mapfile(struct vma_area *vma, DIR *mfd,<br>
> > > vma->vm_socket_id = buf.st_ino;<br>
> > > } else if (errno != ENOENT)<br>
> > > return -1;<br>
> > > + } else if (opts.aufs) {<br>
> > > + /*<br>
> > > + * AUFS support to compensate for the kernel bug<br>
> > > + * exposing branch pathnames in map_files.<br>
> > > + *<br>
> > > + * If the link points inside a branch, replace it<br>
> > > + * with a pathname from the root for later use in<br>
> > > + * dump_filemap().<br>
> > > + */<br>
> > > + char p[PATH_MAX];<br>
> > > + int n;<br>
> > > +<br>
> > > + p[0] = '.';<br>
> > > + n = read_fd_link(vma->vm_file_fd, &p[1], sizeof p - 1);<br>
> > > + if (n < 0)<br>
> > > + return -1;<br>
> > > + n = fixup_aufs_path(&p[1], sizeof p - 1, true);<br>
> > > + if (n < 0)<br>
> > > + return -1;<br>
> > > + if (n > 0) {<br>
> > > + vma->aufs_rpath = xmalloc(n + 2);<br>
> > > + if (!vma->aufs_rpath)<br>
> > > + return -1;<br>
> > > + strcpy(vma->aufs_rpath, p);<br>
> > > + }<br>
> > > }<br>
> > ><br>
> > > return 0;<br>
> > > @@ -450,12 +479,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() a file 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>
> > > + if (stat(f, st_buf) < 0) {<br>
> > > + pr_perror("Failed stat 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>
> > Why do we need the 2nd call to fixup branch name? Can't we just stat<br>
> > the vma->aufs_rpath here?<br>
> ><br>
> ><br>
> > That's because vma->aufs_rpath is relative from the root of the mount namespace (e.g., ./bin/busybox) but we are not in that directory (we're in /). To avoid calling fixup we can construct the full path by prepending aufs_root as follows:<br>
> ><br>
> > - if (opts.aufs && (f = fixup_aufs_fd_path(vma_area->vm_file_fd))) {<br>
> > - if (stat(f, st_buf) < 0) {<br>
> > + if (opts.aufs && vma_area->aufs_rpath && opts.aufs_root) {<br>
> > + char path[PATH_MAX];<br>
> > + int n;<br>
> > +<br>
> > + n = snprintf(path, PATH_MAX, "%s/%s", opts.aufs_root, vma_area->aufs_rpath);<br>
> > + if (n >= PATH_MAX) {<br>
> > + pr_err("Path %s/%s too long\n", opts.aufs_root, vma_area->aufs_rpath);<br>
> > + goto err;<br>
> > + }<br>
> > + if (stat(path, st_buf) < 0) {<br>
> ><br>
> ><br>
> > If you prefer this approach, I will send you a new patch with this change and removal of fixup_aufs_fd_path().<br>
><br>
> Well yes :) I was confused by the 2nd call to fixup_aufs_path from inside<br>
> fixup_aufs_fd_path. It would call read_fd_link and walk the array of branches<br>
> again, which is not necessary, since we did it already in parse_smaps(). So<br>
> it's better, from my perspective, not to do extra work.<br>
><br>
> Just an optimization suggestion -- since we see the full path in parse_smaps(),<br>
> we can save one on vma->ppage_bitmap (it's also restore-only field) and just<br>
> re-use one here.<br>
><br>
> Thanks,<br>
> Pavel<br>
><br>
><br>
<br>
</div></div></blockquote></div><br></div></div>