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

[Kirill Tkhai]

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