<div dir="ltr">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.<div><br></div><div>I also got rid of --aufs command line option but kept opts.aufs which is automatically set in the .parse callback. Since opts.aufs is now used in only one place, it'll be easier to get rid of it in a future patch.</div>
<div><br></div><div>I rebased my code to the head and made sure all comments correctly reflect the current state. Please review the attached patch file and let me know what you think.</div><div><br></div><div>Cheers!</div>
<div><br></div><div>--Saied</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Aug 20, 2014 at 11:00 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="HOEnZb"><div class="h5">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>
</div></div>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>
</blockquote></div><br></div>