Supporting patches for netns/netdev (v2)

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 1/2] Make restore_obj() tolerate a preexisting object in the hash (v2)

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

Changes in v2:
 - Check that the type of the object already in the hash matches that
   of the objref header we're reading.
 - Add a comment about why and how we might get into this sort of
   situation.

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

diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
index 0b06b06..52827a4 100644
--- a/checkpoint/objhash.c
+++ b/checkpoint/objhash.c
@@ -1059,16 +1059,27 @@ 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->ops->obj_type != h->objtype))) {
+		/* Normally, we expect an object to not already exist
+		 * in the hash.  However, for some special scenarios
+		 * where we're restoring sets of objects that must be
+		 * co-allocated (such, as veth netdev pairs) we need
+		 * to tolerate this case if the second restore returns
+		 * the correct type and pointer, as specified in the
+		 * existing object.  Otherwise, we fail.
+		 */
 		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

[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: Supporting patches for netns/netdev (v2)

[Oren Laadan]

Both look good.

Oren.

Dan Smith wrote:
> 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.
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers
> 

Re: Supporting patches for netns/netdev (v2)

[Dan Smith]

OL> Both look good.

Okay, but per Serge's suggestion shall we change the restore_obj() to
the one included below? :)

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

commit d0c71c159decd47c4a6f9778d02dc521b74ff414
Author: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Date:   Tue Feb 9 14:04:23 2010 -0800

    Make restore_obj() tolerate a preexisting object in the hash (v2)
    
    ... 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.
    
    Changes in v2:
     - Check that the type of the object already in the hash matches that
       of the objref header we're reading.
     - Add a comment about why and how we might get into this sort of
       situation.
    
    Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
index 0b06b06..4ca7799 100644
--- a/checkpoint/objhash.c
+++ b/checkpoint/objhash.c
@@ -1059,16 +1059,29 @@ int restore_obj(struct ckpt_ctx *ctx, struct ckpt_hdr_ob
        if (IS_ERR(ptr))
                return PTR_ERR(ptr);
 
-       if (obj_find_by_objref(ctx, h->objref))
-               obj = ERR_PTR(-EINVAL);
-       else
+       obj = obj_find_by_objref(ctx, h->objref);
+       if (!obj) {
                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);
+       } else if ((obj->ptr != ptr) || (obj->ops->obj_type != h->objtype)) {
+               /* Normally, we expect an object to not already exist
+                * in the hash.  However, for some special scenarios
+                * where we're restoring sets of objects that must be
+                * co-allocated (such, as veth netdev pairs) we need
+                * to tolerate this case if the second restore returns
+                * the correct type and pointer, as specified in the
+                * existing object.  If either of those don't match,
+                * we fail.
+                */
+               obj = ERR_PTR(-EINVAL);
+       }
+
        if (IS_ERR(obj)) {
                ops->ref_drop(ptr, 1);
                return PTR_ERR(obj);

Re: Supporting patches for netns/netdev (v2)

[Oren Laadan]

I did wonder why I it looked awfully familiar !

Dan Smith wrote:
> OL> Both look good.
> 
> Okay, but per Serge's suggestion shall we change the restore_obj() to
> the one included below? :)
> 

Re: Supporting patches for netns/netdev (v2)

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> OL> Both look good.
> 
> Okay, but per Serge's suggestion shall we change the restore_obj() to
> the one included below? :)
> 
> -- 
> Dan Smith
> IBM Linux Technology Center
> email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org
> 
> commit d0c71c159decd47c4a6f9778d02dc521b74ff414
> Author: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Date:   Tue Feb 9 14:04:23 2010 -0800
> 
>     Make restore_obj() tolerate a preexisting object in the hash (v2)
>     
>     ... 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.
>     
>     Changes in v2:
>      - Check that the type of the object already in the hash matches that
>        of the objref header we're reading.
>      - Add a comment about why and how we might get into this sort of
>        situation.
>     
>     Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

/me thinks he'd better send a patch defining CKPT_OBJ_LASTREF to help out
those poor ->obj_ref(x, 0) who so badly want to be more informative...

thanks,
-serge

> 
> diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
> index 0b06b06..4ca7799 100644
> --- a/checkpoint/objhash.c
> +++ b/checkpoint/objhash.c
> @@ -1059,16 +1059,29 @@ int restore_obj(struct ckpt_ctx *ctx, struct ckpt_hdr_ob
>         if (IS_ERR(ptr))
>                 return PTR_ERR(ptr);
> 
> -       if (obj_find_by_objref(ctx, h->objref))
> -               obj = ERR_PTR(-EINVAL);
> -       else
> +       obj = obj_find_by_objref(ctx, h->objref);
> +       if (!obj) {
>                 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);
> +       } else if ((obj->ptr != ptr) || (obj->ops->obj_type != h->objtype)) {
> +               /* Normally, we expect an object to not already exist
> +                * in the hash.  However, for some special scenarios
> +                * where we're restoring sets of objects that must be
> +                * co-allocated (such, as veth netdev pairs) we need
> +                * to tolerate this case if the second restore returns
> +                * the correct type and pointer, as specified in the
> +                * existing object.  If either of those don't match,
> +                * we fail.
> +                */
> +               obj = ERR_PTR(-EINVAL);
> +       }
> +
>         if (IS_ERR(obj)) {
>                 ops->ref_drop(ptr, 1);
>                 return PTR_ERR(obj);
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers