AF_UNIX fixes

AF_UNIX fixes

[Dan Smith]

This set includes a couple of AF_UNIX logic corrections that cropped up
while testing a couple of large real-world workloads.

[PATCH 1/2] Fix net buffer logic

[Dan Smith]

The previous change to the buffer save/restore logic caused us to
incorrectly attempt to restore buffers on a socket in a few instances
where flags may not be restored yet or we are in a temporarily altered
state.

So, keep the same logic, but save a flag to indicate whether buffers were
checkpointed to avoid the ambiguity at restore time.

Also, move the UNIX buffer restore point to the main function to avoid
potentially ignoring the flag on a socket we assume wouldn't have saved
buffers.

Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 include/linux/checkpoint_hdr.h |    2 ++
 net/checkpoint.c               |    4 +++-
 net/ipv4/checkpoint.c          |    2 +-
 net/unix/checkpoint.c          |    9 ++++++---
 4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/linux/checkpoint_hdr.h b/include/linux/checkpoint_hdr.h
index 9e8d518..f4f9577 100644
--- a/include/linux/checkpoint_hdr.h
+++ b/include/linux/checkpoint_hdr.h
@@ -596,6 +596,8 @@ struct ckpt_hdr_file_eventfd {
 struct ckpt_hdr_socket {
 	struct ckpt_hdr h;
 
+	__u8 has_buffers;
+
 	struct { /* struct socket */
 		__u64 flags;
 		__u8 state;
diff --git a/net/checkpoint.c b/net/checkpoint.c
index 0d90af2..de8e454 100644
--- a/net/checkpoint.c
+++ b/net/checkpoint.c
@@ -761,6 +761,8 @@ static int __do_sock_checkpoint(struct ckpt_ctx *ctx, struct sock *sk)
 	if (!h)
 		return -ENOMEM;
 
+	h->has_buffers = ckpt_sock_need_buffers(sk);
+
 	/* part I: common to all sockets */
 	ret = sock_cptrst(ctx, sk, h, CKPT_CPT);
 	if (ret < 0)
@@ -776,7 +778,7 @@ static int __do_sock_checkpoint(struct ckpt_ctx *ctx, struct sock *sk)
 		goto out;
 
 	/* part III: socket buffers */
-	if (ckpt_sock_need_buffers(sk))
+	if (h->has_buffers)
 		ret = sock_defer_write_buffers(ctx, sk);
  out:
 	ckpt_hdr_put(ctx, h);
diff --git a/net/ipv4/checkpoint.c b/net/ipv4/checkpoint.c
index eb02175..ec42e57 100644
--- a/net/ipv4/checkpoint.c
+++ b/net/ipv4/checkpoint.c
@@ -539,7 +539,7 @@ int inet_restore(struct ckpt_ctx *ctx,
 		}
 	}
 
-	if (ckpt_sock_need_buffers(sock->sk))
+	if (h->has_buffers)
 		ret = inet_defer_restore_buffers(ctx, sock->sk);
  out:
 	ckpt_hdr_put(ctx, in);
diff --git a/net/unix/checkpoint.c b/net/unix/checkpoint.c
index 1fa3d7e..47d38e2 100644
--- a/net/unix/checkpoint.c
+++ b/net/unix/checkpoint.c
@@ -443,9 +443,6 @@ static int unix_restore_connected(struct ckpt_ctx *ctx,
 		ckpt_debug("unix_defer_join: %i\n", ret);
 	}
 
-	if (ckpt_sock_need_buffers(sock->sk) && !ret)
-		ret = unix_defer_restore_buffers(ctx, un->this);
-
 	return ret;
 }
 
@@ -639,6 +636,12 @@ int unix_restore(struct ckpt_ctx *ctx, struct socket *sock,
 	else
 		ckpt_err(ctx, ret, "bad af_unix state %i\n", h->sock.state);
 
+	if (ret < 0)
+		goto out;
+
+	if (h->has_buffers)
+		ret = unix_defer_restore_buffers(ctx, un->this);
+
  out:
 	ckpt_hdr_put(ctx, un);
 	kfree(cwd);
-- 
1.7.1.1

Re: [PATCH 1/2] Fix net buffer logic

[Oren Laadan]

Dan Smith wrote:
> The previous change to the buffer save/restore logic caused us to
> incorrectly attempt to restore buffers on a socket in a few instances
> where flags may not be restored yet or we are in a temporarily altered
> state.
> 
> So, keep the same logic, but save a flag to indicate whether buffers were
> checkpointed to avoid the ambiguity at restore time.
> 
> Also, move the UNIX buffer restore point to the main function to avoid
> potentially ignoring the flag on a socket we assume wouldn't have saved
> buffers.
> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Looks good.

So now that ckpt_sock_need_buffers() is used only inside
net/checkpoint.c and only in one place, let's make it static
(inline) there, or even code in-place ?

Otherwise - Ack.

Oren.

> ---
>  include/linux/checkpoint_hdr.h |    2 ++
>  net/checkpoint.c               |    4 +++-
>  net/ipv4/checkpoint.c          |    2 +-
>  net/unix/checkpoint.c          |    9 ++++++---
>  4 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/checkpoint_hdr.h b/include/linux/checkpoint_hdr.h
> index 9e8d518..f4f9577 100644
> --- a/include/linux/checkpoint_hdr.h
> +++ b/include/linux/checkpoint_hdr.h
> @@ -596,6 +596,8 @@ struct ckpt_hdr_file_eventfd {
>  struct ckpt_hdr_socket {
>  	struct ckpt_hdr h;
>  
> +	__u8 has_buffers;
> +
>  	struct { /* struct socket */
>  		__u64 flags;
>  		__u8 state;
> diff --git a/net/checkpoint.c b/net/checkpoint.c
> index 0d90af2..de8e454 100644
> --- a/net/checkpoint.c
> +++ b/net/checkpoint.c
> @@ -761,6 +761,8 @@ static int __do_sock_checkpoint(struct ckpt_ctx *ctx, struct sock *sk)
>  	if (!h)
>  		return -ENOMEM;
>  
> +	h->has_buffers = ckpt_sock_need_buffers(sk);
> +
>  	/* part I: common to all sockets */
>  	ret = sock_cptrst(ctx, sk, h, CKPT_CPT);
>  	if (ret < 0)
> @@ -776,7 +778,7 @@ static int __do_sock_checkpoint(struct ckpt_ctx *ctx, struct sock *sk)
>  		goto out;
>  
>  	/* part III: socket buffers */
> -	if (ckpt_sock_need_buffers(sk))
> +	if (h->has_buffers)
>  		ret = sock_defer_write_buffers(ctx, sk);
>   out:
>  	ckpt_hdr_put(ctx, h);
> diff --git a/net/ipv4/checkpoint.c b/net/ipv4/checkpoint.c
> index eb02175..ec42e57 100644
> --- a/net/ipv4/checkpoint.c
> +++ b/net/ipv4/checkpoint.c
> @@ -539,7 +539,7 @@ int inet_restore(struct ckpt_ctx *ctx,
>  		}
>  	}
>  
> -	if (ckpt_sock_need_buffers(sock->sk))
> +	if (h->has_buffers)
>  		ret = inet_defer_restore_buffers(ctx, sock->sk);
>   out:
>  	ckpt_hdr_put(ctx, in);
> diff --git a/net/unix/checkpoint.c b/net/unix/checkpoint.c
> index 1fa3d7e..47d38e2 100644
> --- a/net/unix/checkpoint.c
> +++ b/net/unix/checkpoint.c
> @@ -443,9 +443,6 @@ static int unix_restore_connected(struct ckpt_ctx *ctx,
>  		ckpt_debug("unix_defer_join: %i\n", ret);
>  	}
>  
> -	if (ckpt_sock_need_buffers(sock->sk) && !ret)
> -		ret = unix_defer_restore_buffers(ctx, un->this);
> -
>  	return ret;
>  }
>  
> @@ -639,6 +636,12 @@ int unix_restore(struct ckpt_ctx *ctx, struct socket *sock,
>  	else
>  		ckpt_err(ctx, ret, "bad af_unix state %i\n", h->sock.state);
>  
> +	if (ret < 0)
> +		goto out;
> +
> +	if (h->has_buffers)
> +		ret = unix_defer_restore_buffers(ctx, un->this);
> +
>   out:
>  	ckpt_hdr_put(ctx, un);
>  	kfree(cwd);

Re: [PATCH 1/2] Fix net buffer logic

[Dan Smith]

OL> So now that ckpt_sock_need_buffers() is used only inside
OL> net/checkpoint.c and only in one place, let's make it static
OL> (inline) there, or even code in-place ?

Heh, yes, makes sense.  I will respin.

Thanks :)

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

[PATCH 2/2] Fix restoring unlinked UNIX sockets

[Dan Smith]

We already support the concept, but the logic controlling whether or not we
restore the CWD path didn't follow.  This patch adds a check of the
"unlinked" flag we've already set before saving or restoring the CWD string.

This fixes an issue with a real-world workload that I have which utilizes
AF_UNIX sockets.

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

diff --git a/net/unix/checkpoint.c b/net/unix/checkpoint.c
index 47d38e2..9522c3a 100644
--- a/net/unix/checkpoint.c
+++ b/net/unix/checkpoint.c
@@ -165,7 +165,8 @@ int unix_checkpoint(struct ckpt_ctx *ctx, struct socket *sock)
 	if (ret < 0)
 		goto out;
 
-	if (unix_need_cwd(&un->laddr, un->laddr_len))
+	if (unix_need_cwd(&un->laddr, un->laddr_len) &&
+	    (un->flags & CKPT_UNIX_LINKED))
 		ret = unix_write_cwd(ctx, sock->sk, un->laddr.sun_path);
  out:
 	ckpt_hdr_put(ctx, un);
@@ -614,10 +615,12 @@ int unix_restore(struct ckpt_ctx *ctx, struct socket *sock,
 	if (un->peer < 0)
 		goto out;
 
-	if (unix_need_cwd(&un->laddr, un->laddr_len)) {
+	if (unix_need_cwd(&un->laddr, un->laddr_len) &&
+	    (un->flags & CKPT_UNIX_LINKED)) {
 		cwd = ckpt_read_string(ctx, PATH_MAX);
 		if (IS_ERR(cwd)) {
 			ret = PTR_ERR(cwd);
+			cwd = NULL;
 			goto out;
 		}
 	}
-- 
1.7.1.1