pages: Share page_ids between ns dump-helpers

Submitted by Dmitry Safonov on June 29, 2017, 8:09 p.m.

Details

Message ID 20170629200932.17022-1-dsafonov@virtuozzo.com
State New
Series "pages: Share page_ids between ns dump-helpers"
Headers show

Commit Message

Dmitry Safonov June 29, 2017, 8:09 p.m.
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@virtuozzo.com>
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
---
 criu/crtools.c       |  3 +++
 criu/image.c         | 25 +++++++++++++++++++++----
 criu/include/image.h |  2 ++
 3 files changed, 26 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

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));
+	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__ */

Comments

Pavel Emelianov June 30, 2017, 6:40 a.m.
On 06/29/2017 11:09 PM, 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@virtuozzo.com>
> Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>

Acked-by: Pavel Emelyanov <xemul@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));
> +	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__ */
>
Kirill Tkhai June 30, 2017, 9:07 a.m.
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@virtuozzo.com>
> Signed-off-by: Dmitry Safonov <dsafonov@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.

> +	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@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
Dmitry Safonov June 30, 2017, 9:32 a.m.
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@virtuozzo.com>
>> Signed-off-by: Dmitry Safonov <dsafonov@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@openvz.org
>> https://lists.openvz.org/mailman/listinfo/criu
Kirill Tkhai June 30, 2017, 10:37 a.m.
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@virtuozzo.com>
> > > Signed-off-by: Dmitry Safonov <dsafonov@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.
 
> > 
> > > +	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@openvz.org
> > > https://lists.openvz.org/mailman/listinfo/criu
> 
> 
> -- 
>              Dmitry
Dmitry Safonov June 30, 2017, 11:46 a.m.
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@virtuozzo.com>
>>>> Signed-off-by: Dmitry Safonov <dsafonov@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@openvz.org
>>>> https://lists.openvz.org/mailman/listinfo/criu
>>
>>
>> -- 
>>               Dmitry
Dmitry Safonov June 30, 2017, 1:37 p.m.
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@virtuozzo.com>
>> Signed-off-by: Dmitry Safonov <dsafonov@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 Safonov June 30, 2017, 4:57 p.m.
On 06/29/2017 11:09 PM, 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@virtuozzo.com>
> Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>

Andrew, drop this patch please.
I'll send v2, found an issue with several pre-dumps.