[CRIU] Re: [PATCH cr 07/16] restore: map private vma-s before forking children

[Andrey Vagin]

On Wed, Oct 31, 2012 at 02:16:01PM +0400, Andrey Vagin wrote:
> On Tue, Oct 30, 2012 at 10:56:13PM +0400, Pavel Emelyanov wrote:
> > >>
> > >>> +			break;
> > >>> +		}
> > >>> +
> > >>> +	}
> > >>> +
> > >>> +	*pvma = list_entry(p->list.prev, struct vma_area, list);
> > >>
> > >> prev? Why prev? We should move forward the parent list. No?
> > > 
> > > list_for_each_entry_continue() starts from a next element, so we need to
> > > save prev here.
> > 
> > Indeed. Then the initialization of the pvma with _first_ element is wrong,
> > you should start with the list head instead.
> 
> Yes, thanks.
> > 
> > >>
> > >>> +
> > >>> +		list_del(&p->list);
> > >>> +		xfree(p);
> > >>
> > >> Why do we remove parent vma from its list?
> > > 
> > > Then all unused parent's vma-s will be unmapped.
> > 
> > What do you mean by "unused"?
> 
> This vma-s are present in parent and they are not present in a child.
> 
> > 
> > >>
> > >>> +	}
> > >>> +
> > >>> +	if (vma_entry_is(&vma->vma, VMA_FILE_PRIVATE))
> > >>> +		close(vma->vma.fd);
> > >>
> > >> Plz, explain this close.
> > > 
> > > This fd was opened upper and a vma was mapped and this fd isn't needed more.
> > 
> > Upper? Where is it?
> 
> static int map_private_vma(pid_t pid, struct vma_area *vma, void
> *tgt_addr,
>                         struct vma_area **pvma, struct list_head
> *pvma_list)
> {       
>         int ret;
>         void *addr;
>         unsigned long nr_pages;
>         struct vma_area *p = *pvma;
>                 
>         if (vma_entry_is(&vma->vma, VMA_FILE_PRIVATE)) {
>                 ret = get_filemap_fd(pid, &vma->vma);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Eh, It'a a part of "[PATCH cr 10/16] restore: use a new scheme for
restoring of file private mappings". Sorry.
> > 
> > >>
> > >>> +
> > >>> +		addr = (void *) vma_premmaped_start(&vma->vma);
> > >>> +		len = vma_area_len(vma);
> > >>> +
> > >>> +		if (!addr)
> > >>> +			continue;
> > >>> +
> > >>> +		if (munmap(addr, len)) {
> > >>
> > >> What is this munmap for?
> > > 
> > > Why do we need this vma-s, they were inherited from a parent.
> > > And one more thing is that all this vma-s will be inherited by
> > > children of this process and so on...
> > 
> > So this is unmap of all parent's maps, right? They were mapped with
> > one big chunk, why not unmapping them the same way?
> Yes, we can.