[CRIU] [PATCH] mem: fix typo in determining an address of parent vma

[Filipe Brandenburger]

Hi Andrew,

Yes, you're right, with the three patches I got cow01 to succeed now.
For some reason when I was testing this yesterday I was running into
data mismatch problems, but it was probably due to other changes I
introduced while trying to debug the issue.

Thanks for the fixes!

Cheers,
Filipe


On Tue, May 13, 2014 at 11:16 AM, Andrew Vagin <avagin at parallels.com> wrote:
> On Tue, May 13, 2014 at 09:54:58AM -0700, Filipe Brandenburger wrote:
>> It turns out that when you apply this patch then it will actually COW
>> too much...
>>
>> For instance, the sep_tcs test case of cow01 fails for me with this patch in.
>
> Do you see two other patches? On my host cow01 passes with these patches.
>
>>
>> The reason is that the vmas on parent and child have same start and
>> end but are not really COWed from one another.
>>
>> The problem is that the zero pages on the child not returned by
>> pr.get_pagemap() are not handled by restore_priv_vma_content() so they
>> end up with the contents of the parent instead of zeroed out contents.
>> (There seems to be a similar problem with the file private mmap in
>> file_tcs as well but I didn't go far enough in troubleshooting that
>> one yet.)
>
> Do you look at the code below the "/* Remove pages, which were not shared
> with a child */" comment?
>
>>
>> I'd suggest holding on this patch until those other issues are
>> addressed, otherwise you'll go from having not COWed pages with the
>> right memory contents to having pages with the wrong contents in them
>> which is worse.
>>
>> I think the patches to fix traversal of COW vmas belong before this
>> one on the order of patches.
>>
>> Cheers,
>> Filipe
>>
>>
>> On Tue, May 13, 2014 at 4:39 AM, Andrey Vagin <avagin at openvz.org> wrote:
>> > Look at this hunk from 7659c995f58f:
>> > -    paddr = decode_pointer(vma_premmaped_start(&p->vma));
>> > +    paddr = decode_pointer(vma->premmaped_addr);
>> >
>> > Obviously we want to use p->premmaped_addr instead of
>> > vma->premmaped_addr.
>> >
>> > Fixes: 7659c995f58f ("vm: don't overwrite vma->shmid for private mappings")
>> > Reported-by: Filipe Brandenburger <filbranden at google.com>
>> > Signed-off-by: Andrey Vagin <avagin at openvz.org>
>> > ---
>> >  cr-restore.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/cr-restore.c b/cr-restore.c
>> > index d79da59..897c3fc 100644
>> > --- a/cr-restore.c
>> > +++ b/cr-restore.c
>> > @@ -260,7 +260,7 @@ static int map_private_vma(pid_t pid, struct vma_area *vma, void **tgt_addr,
>> >
>> >                 pr_info("COW 0x%016"PRIx64"-0x%016"PRIx64" 0x%016"PRIx64" vma\n",
>> >                         vma->e->start, vma->e->end, vma->e->pgoff);
>> > -               paddr = decode_pointer(vma->premmaped_addr);
>> > +               paddr = decode_pointer(p->premmaped_addr);
>> >         }
>> >
>> >         *pvma = p;
>> > --
>> > 1.8.5.3
>> >