[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.
> 
> 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.

You've formed it a way too vague.
I slightly able to see it under the fog of letters.

Come on, what do you suggest, *exactly*?
ITOW, renaming rst-malloc to cr-malloc?

> 
>> +	if (!page_ids) {
>> +		pr_err("Failed to shmalloc page_ids\n");
>> +		return -1;
>> +	}
>> +	atomic_set(page_ids, 1);
>> +
>> +	return 0;
>> +}
>>   
>>   void up_page_ids_base(void)
>>   {
>> @@ -517,8 +534,8 @@ void up_page_ids_base(void)
>>   	 * higher IDs.
>>   	 */
>>   
>> -	BUG_ON(page_ids != 1);
>> -	page_ids += 0x10000;
>> +	BUG_ON(atomic_read(page_ids) != 1);
>> +	atomic_add(0x10000, page_ids);
>>   }
>>   
>>   struct cr_img *open_pages_image_at(int dfd, unsigned long flags, struct cr_img *pmi, u32 *id)
>> @@ -531,7 +548,7 @@ struct cr_img *open_pages_image_at(int dfd, unsigned long flags, struct cr_img *
>>   		pagemap_head__free_unpacked(h, NULL);
>>   	} else {
>>   		PagemapHead h = PAGEMAP_HEAD__INIT;
>> -		*id = h.pages_id = page_ids++;
>> +		*id = h.pages_id = atomic_inc_return(page_ids);
>>   		if (pb_write_one(pmi, &h, PB_PAGEMAP_HEAD) < 0)
>>   			return NULL;
>>   	}
>> diff --git a/criu/include/image.h b/criu/include/image.h
>> index d9c4bdbc8da4..60e44297a858 100644
>> --- a/criu/include/image.h
>> +++ b/criu/include/image.h
>> @@ -170,4 +170,6 @@ extern int read_img_str(struct cr_img *, char **pstr, int size);
>>   
>>   extern void close_image(struct cr_img *);
>>   
>> +extern int images_init(void);
>> +
>>   #endif /* __CR_IMAGE_H__ */
>> -- 
>> 2.13.1
>>
>> _______________________________________________
>> CRIU mailing list
>> CRIU at openvz.org
>> https://lists.openvz.org/mailman/listinfo/criu


-- 
              Dmitry