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

[Dmitry Safonov]

On 06/30/2017 01:37 PM, Kirill Tkhai wrote:
> On Fri, Jun 30, 2017 at 12:32, Dmitry Safonov wrote:
>> 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 you want to use restore-only function in dump code,
> you should make a generic engine, which is clearly may
> be used either on dump and on restore.
> 
> Yeah, I think you should rename the file and the variables
> in it, and you should separate restore-only functions and
> variables from generic entities definitions in it. This
> will help a future developer to change the code and do
> not brake something of dump case.

Fair enough.
I suppose it can be made on the top, and fits Pavel's enhancements bugs
for novices in CRIU. I'll fill a bug in github.
If no-one will do it in some time - I'll have it in TODO list.

>   
>>>
>>>> +	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


-- 
              Dmitry