[CRIU] [PATCH] pages: Share page_ids between ns dump-helpers

[Dmitry Safonov]

On 06/30/2017 12:07 PM, Kirill Tkhai wrote:
> On Thu, Jun 29, 2017 at 23:09, Dmitry Safonov wrote:
>> After the commit 9d2e1dfebedf ("ipc: Keep shmem segments contents into
>> pagemap/page images"), SysV shmem pages are stored in the pages-<n>.img
>> files.  Where <n> is page_ids number.
>>
>> The problem here is that we dump namespaces with a helper after fork().
>> This results in non-shared between child(s) and parent-criu counter
>> with the result of overwriting pages-*.img files later by helpers/criu.
>> This resulted in restore with corrupted shmem segments or
>> in the following failures on restore:
>>> (04.501019)      1: Error (criu/pagemap.c:265): Can't read mapping page 0: No such file or directory
>>> (04.637516)      1: Error (criu/pagemap.c:265): Can't read mapping page 0: No such file or directory
>>
>> Allocate page_ids with shmalloc() and make it atomic.
>> As we need it in pre-dump, dump, page-server and rpc,
>> init it early in the main().
>>
>> Cc: Pavel Emelyanov <xemul at virtuozzo.com>
>> Signed-off-by: Dmitry Safonov <dsafonov at virtuozzo.com>
>> ---
>>   criu/crtools.c       |  3 +++
>>   criu/image.c         | 25 +++++++++++++++++++++----
>>   criu/include/image.h |  2 ++
>>   3 files changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/criu/crtools.c b/criu/crtools.c
>> index b0f7b946cc56..a3dbbcb10668 100644
>> --- a/criu/crtools.c
>> +++ b/criu/crtools.c
>> @@ -325,6 +325,9 @@ int main(int argc, char *argv[], char *envp[])
>>   	if (fault_injection_init())
>>   		return 1;
>>   
>> +	if (images_init())
>> +		return 1;
>> +
>>   	cr_pb_init();
>>   	setproctitle_init(argc, argv, envp);
>>   
>> diff --git a/criu/image.c b/criu/image.c
>> index 62e8e7109e65..59b9dad5458d 100644
>> --- a/criu/image.c
>> +++ b/criu/image.c
>> @@ -17,6 +17,7 @@
>>   #include "images/inventory.pb-c.h"
>>   #include "images/pagemap.pb-c.h"
>>   #include "img-remote.h"
>> +#include "rst-malloc.h"
> 
> ^^^
> 
>>   
>>   bool ns_per_id = false;
>>   bool img_common_magic = true;
>> @@ -505,7 +506,23 @@ void close_image_dir(void)
>>   	close_service_fd(IMG_FD_OFF);
>>   }
>>   
>> -static unsigned long page_ids = 1;
>> +/*
>> + * As we dump SysV shmem IPC into pages-<page_ids>.img,
>> + * the keys should be shared in each dump ns-helpers and in criu.
>> + */
>> +static atomic_t *page_ids;
>> +
>> +int images_init(void)
>> +{
>> +	page_ids = shmalloc(sizeof(*page_ids));
> 
> I think it's not good to use shmalloc() "as is", because
> currently it's aimed for restore code only. All variables,
> function and file names are called after restore, and now
> you are the first, who begins to use it for dumping.

BTW, it's not correct.
i.e, log already uses shmalloc() at pre-dump, dump, restore, pageserver.
Look at first_err.

> 
> If one day someone reworks shmalloc() code to fit some
> restore goals more, it will be an unpleasent surprise,
> when he founds it's used for dump too.
> 
> You should to do something with shmalloc() and the file,
> where it's described, to avoid such easter eggs.


-- 
              Dmitry