Supporting patches for netns/netdev

Supporting patches for netns/netdev

[Dan Smith]

These two patches make a couple of small (yet potentially controversial)
changes to the cleanup of the objhash and the restore_obj() function.
The way network devices have to get torn down (in the case of a failed
restore) and the way I need to create veth pairs are why I need this
sort of functionality.

Each could be done a number of different ways, but I'm hoping these
quick changes are acceptable.

[PATCH 1/2] Make restore_obj() tolerate a preexisting object in the hash

[Dan Smith]

... as long as the pointer is the same as that returned from the restore
function.  Also move the compulsory ref_drop() so that it only gets
done if we created the new object.

The existing object tolerance is important for netdev restore because it
means that I can refer to a peer by its objref instead of needing the
(previously-rejected) veth_peer() function.  If this is not acceptable,
then I'll need to keep a separate list of pairs.

Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/objhash.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
index 0b06b06..6051c8d 100644
--- a/checkpoint/objhash.c
+++ b/checkpoint/objhash.c
@@ -1059,16 +1059,19 @@ int restore_obj(struct ckpt_ctx *ctx, struct ckpt_hdr_objref *h)
 	if (IS_ERR(ptr))
 		return PTR_ERR(ptr);
 
-	if (obj_find_by_objref(ctx, h->objref))
+	obj = obj_find_by_objref(ctx, h->objref);
+	if (obj && (obj->ptr != ptr))
 		obj = ERR_PTR(-EINVAL);
-	else
+	else {
 		obj = obj_new(ctx, ptr, h->objref, h->objtype);
-	/*
-	 * Drop an extra reference to the object returned by ops->restore:
-	 * On success, this clears the extra reference taken by obj_new(),
-	 * and on failure, this cleans up the object itself.
-	 */
-	ops->ref_drop(ptr, 0);
+		/*
+		 * Drop an extra reference to the object returned by
+		 * ops->restore: On success, this clears the extra
+		 * reference taken by obj_new(), and on failure, this
+		 * cleans up the object itself.
+		 */
+		ops->ref_drop(ptr, 0);
+	}
 	if (IS_ERR(obj)) {
 		ops->ref_drop(ptr, 1);
 		return PTR_ERR(obj);
-- 
1.6.2.5

Re: [PATCH 1/2] Make restore_obj() tolerate a preexisting object in the hash

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> ... as long as the pointer is the same as that returned from the restore
> function.  Also move the compulsory ref_drop() so that it only gets
> done if we created the new object.
> 
> The existing object tolerance is important for netdev restore because it
> means that I can refer to a peer by its objref instead of needing the
> (previously-rejected) veth_peer() function.  If this is not acceptable,
> then I'll need to keep a separate list of pairs.
> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  checkpoint/objhash.c |   19 +++++++++++--------
>  1 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
> index 0b06b06..6051c8d 100644
> --- a/checkpoint/objhash.c
> +++ b/checkpoint/objhash.c
> @@ -1059,16 +1059,19 @@ int restore_obj(struct ckpt_ctx *ctx, struct ckpt_hdr_objref *h)
>  	if (IS_ERR(ptr))
>  		return PTR_ERR(ptr);
> 
> -	if (obj_find_by_objref(ctx, h->objref))
> +	obj = obj_find_by_objref(ctx, h->objref);
> +	if (obj && (obj->ptr != ptr))
>  		obj = ERR_PTR(-EINVAL);
> -	else
> +	else {
>  		obj = obj_new(ctx, ptr, h->objref, h->objtype);

Sorry, I *must* not be thinking straight, but...

don't you want to do the obj_new only if obj==NULL here?  So

if (obj) {
	if (obj->ptr != ptr)
		return -EINVAL;
	else
		return obj->objref;
}
obj = obj_new(...)
...

?

> -	/*
> -	 * Drop an extra reference to the object returned by ops->restore:
> -	 * On success, this clears the extra reference taken by obj_new(),
> -	 * and on failure, this cleans up the object itself.
> -	 */
> -	ops->ref_drop(ptr, 0);
> +		/*
> +		 * Drop an extra reference to the object returned by
> +		 * ops->restore: On success, this clears the extra
> +		 * reference taken by obj_new(), and on failure, this
> +		 * cleans up the object itself.
> +		 */
> +		ops->ref_drop(ptr, 0);
> +	}
>  	if (IS_ERR(obj)) {
>  		ops->ref_drop(ptr, 1);
>  		return PTR_ERR(obj);
> -- 
> 1.6.2.5
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [PATCH 1/2] Make restore_obj() tolerate a preexisting object in the hash

[Dan Smith]

SH> Sorry, I *must* not be thinking straight, but...
SH> don't you want to do the obj_new only if obj==NULL here?  So

Er, um, yes :)

-- 
Dan Smith
IBM Linux Technology Center
email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org

[PATCH 2/2] Move ckpt_objhash_free() to before we destroy the deferqueues

[Dan Smith]

This makes the deferqueue cleanup function run after all the objects
have been purged from the hash and thus gives us an opportunity to cleanup
things that *must* happen after the last reference is dropped (such as
unregister_netdev()).

Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/sys.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/checkpoint/sys.c b/checkpoint/sys.c
index f04b8b0..f739932 100644
--- a/checkpoint/sys.c
+++ b/checkpoint/sys.c
@@ -223,6 +223,8 @@ static void ckpt_ctx_free(struct ckpt_ctx *ctx)
 	if (ctx->kflags & CKPT_CTX_RESTART)
 		restore_debug_free(ctx);
 
+	ckpt_obj_hash_free(ctx);
+
 	if (ctx->deferqueue)
 		deferqueue_destroy(ctx->deferqueue);
 
@@ -234,7 +236,6 @@ static void ckpt_ctx_free(struct ckpt_ctx *ctx)
 	if (ctx->logfile)
 		fput(ctx->logfile);
 
-	ckpt_obj_hash_free(ctx);
 	path_put(&ctx->root_fs_path);
 	ckpt_pgarr_free(ctx);
 
-- 
1.6.2.5

Re: [PATCH 2/2] Move ckpt_objhash_free() to before we destroy the deferqueues

[Serge E. Hallyn]

Well you can't quite do that bc fs/eventpoll.c:ep_items_restore()
will use ckpt_obj_fetch().

Would it be safe to make the order:

	files_deferqueue()

	ckpt_obj_hash_free()

	deferqueue()

?

Though that's just waiting to bite us in the ankles whenever
we put something in general deferqueue which must ckpt_obj_fetch()...

-serge

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> This makes the deferqueue cleanup function run after all the objects
> have been purged from the hash and thus gives us an opportunity to cleanup
> things that *must* happen after the last reference is dropped (such as
> unregister_netdev()).
> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  checkpoint/sys.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/checkpoint/sys.c b/checkpoint/sys.c
> index f04b8b0..f739932 100644
> --- a/checkpoint/sys.c
> +++ b/checkpoint/sys.c
> @@ -223,6 +223,8 @@ static void ckpt_ctx_free(struct ckpt_ctx *ctx)
>  	if (ctx->kflags & CKPT_CTX_RESTART)
>  		restore_debug_free(ctx);
> 
> +	ckpt_obj_hash_free(ctx);
> +
>  	if (ctx->deferqueue)
>  		deferqueue_destroy(ctx->deferqueue);
> 
> @@ -234,7 +236,6 @@ static void ckpt_ctx_free(struct ckpt_ctx *ctx)
>  	if (ctx->logfile)
>  		fput(ctx->logfile);
> 
> -	ckpt_obj_hash_free(ctx);
>  	path_put(&ctx->root_fs_path);
>  	ckpt_pgarr_free(ctx);
> 
> -- 
> 1.6.2.5
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [PATCH 2/2] Move ckpt_objhash_free() to before we destroy the deferqueues

[Dan Smith]

SH> Well you can't quite do that bc fs/eventpoll.c:ep_items_restore()
SH> will use ckpt_obj_fetch().

That runs in the deferqueue_run() function, not the
deferqueue_destroy() bit, which is what I'm actually changing the
order of here.  Right?

-- 
Dan Smith
IBM Linux Technology Center
email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org

Re: [PATCH 2/2] Move ckpt_objhash_free() to before we destroy the deferqueues

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> SH> Well you can't quite do that bc fs/eventpoll.c:ep_items_restore()
> SH> will use ckpt_obj_fetch().
> 
> That runs in the deferqueue_run() function, not the
> deferqueue_destroy() bit, which is what I'm actually changing the
> order of here.  Right?

Oh.  Uh, yeah :)

Can't see any problem with that then.

We'll just need to remember (bc I can't think where to sanely document
it) that deferqueue->destructor method may not use ckpt_obj_fetch().

-serge

Supporting patches for netns/netdev (v2)

[Dan Smith]

These two patches make a couple of small (yet potentially controversial)
changes to the cleanup of the objhash and the restore_obj() function.
The way network devices have to get torn down (in the case of a failed
restore) and the way I need to create veth pairs are why I need this
sort of functionality.

Each could be done a number of different ways, but I'm hoping these
quick changes are acceptable.

Version 2, with changes to patch #1 only.

[PATCH 2/2] Move ckpt_objhash_free() to before we destroy the deferqueues

[Dan Smith]

This makes the deferqueue cleanup function run after all the objects
have been purged from the hash and thus gives us an opportunity to cleanup
things that *must* happen after the last reference is dropped (such as
unregister_netdev()).

Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/sys.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/checkpoint/sys.c b/checkpoint/sys.c
index f04b8b0..f739932 100644
--- a/checkpoint/sys.c
+++ b/checkpoint/sys.c
@@ -223,6 +223,8 @@ static void ckpt_ctx_free(struct ckpt_ctx *ctx)
 	if (ctx->kflags & CKPT_CTX_RESTART)
 		restore_debug_free(ctx);
 
+	ckpt_obj_hash_free(ctx);
+
 	if (ctx->deferqueue)
 		deferqueue_destroy(ctx->deferqueue);
 
@@ -234,7 +236,6 @@ static void ckpt_ctx_free(struct ckpt_ctx *ctx)
 	if (ctx->logfile)
 		fput(ctx->logfile);
 
-	ckpt_obj_hash_free(ctx);
 	path_put(&ctx->root_fs_path);
 	ckpt_pgarr_free(ctx);
 
-- 
1.6.2.5