[PATCH] Do not skip records with nonblocking connections (take 2)

[PATCH] Do not skip records with nonblocking connections (take 2)

[Steve Dickson]

With non-blocking connections, do not skip records when receiving
the streams since entire value messages can be ignored which
in cause the entire stream to become out of sync.

For example, two mounts simultaneously send two unmaps
commands. The first one is read, then the second thrown
away due to skipping the record. Skipping this record
will cause XDR error later in processing of the stream.

Signed-off-by: Steve Dickson <steved@redhat.com>
---
 src/svc_vc.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/src/svc_vc.c b/src/svc_vc.c
index aaaf2d7..87406f1 100644
--- a/src/svc_vc.c
+++ b/src/svc_vc.c
@@ -610,7 +610,11 @@ svc_vc_recv(xprt, msg)
 	}
 
 	xdrs->x_op = XDR_DECODE;
-	(void)xdrrec_skiprecord(xdrs);
+	/*
+	 * No need skip records with nonblocking connections
+	 */
+	if (cd->nonblock == FALSE)
+		(void)xdrrec_skiprecord(xdrs);
 	if (xdr_callmsg(xdrs, msg)) {
 		cd->x_id = msg->rm_xid;
 		return (TRUE);
-- 
1.7.4.4

Re: [PATCH] Do not skip records with nonblocking connections (take 2)

[Chuck Lever]

On Jun 18, 2011, at 10:01 AM, Steve Dickson wrote:

> With non-blocking connections, do not skip records when receiving
> the streams since entire value messages can be ignored which
> in cause the entire stream to become out of sync.
> 
> For example, two mounts simultaneously send two unmaps
> commands. The first one is read, then the second thrown
> away due to skipping the record. Skipping this record
> will cause XDR error later in processing of the stream.

I notice that xdrrec_skiprecord() also does a __xdrrec_getrec() on nonblocking connections.  In the current code, then, __xdrrec_getrec() is likely invoked twice in svc_vc_recv() on non-blocking connections.  After this patch, it is only invoked once.

Invoking __xdrrec_getrec() just once in both the blocking and non-blocking cases feels more correct to me, but I don't have a deep understanding of this code.  So this patch seems like the right thing to do.

> Signed-off-by: Steve Dickson <steved@redhat.com>
> ---
> src/svc_vc.c |    6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/src/svc_vc.c b/src/svc_vc.c
> index aaaf2d7..87406f1 100644
> --- a/src/svc_vc.c
> +++ b/src/svc_vc.c
> @@ -610,7 +610,11 @@ svc_vc_recv(xprt, msg)
> 	}
> 
> 	xdrs->x_op = XDR_DECODE;
> -	(void)xdrrec_skiprecord(xdrs);
> +	/*
> +	 * No need skip records with nonblocking connections
> +	 */
> +	if (cd->nonblock == FALSE)
> +		(void)xdrrec_skiprecord(xdrs);

cd->nonblock is already checked just before this code block.  As a minor optimization, I'd suggest reordering slightly so cd->nonblock is checked only once:

	xdrs->x_op = XDR_DECODE;
	if (cd->nonblock) {
		if (!__xdrrec_getrec(xdrs, &cd->strm_stat, TRUE))
			return FALSE;
	} else 
		(void)xdrrec_skiprecord(xdrs);

As long as __xdrrec_getrec() doesn't care about the value of xdrs->x_op, this should be OK to do.

> 	if (xdr_callmsg(xdrs, msg)) {
> 		cd->x_id = msg->rm_xid;
> 		return (TRUE);

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [Libtirpc-devel] [PATCH] Do not skip records with nonblocking connections (take 2)

[Steve Dickson]

Committed...

steved.

On 06/18/2011 10:01 AM, Steve Dickson wrote:
> With non-blocking connections, do not skip records when receiving
> the streams since entire value messages can be ignored which
> in cause the entire stream to become out of sync.
> 
> For example, two mounts simultaneously send two unmaps
> commands. The first one is read, then the second thrown
> away due to skipping the record. Skipping this record
> will cause XDR error later in processing of the stream.
> 
> Signed-off-by: Steve Dickson <steved@redhat.com>
> ---
>  src/svc_vc.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/src/svc_vc.c b/src/svc_vc.c
> index aaaf2d7..87406f1 100644
> --- a/src/svc_vc.c
> +++ b/src/svc_vc.c
> @@ -610,7 +610,11 @@ svc_vc_recv(xprt, msg)
>  	}
>  
>  	xdrs->x_op = XDR_DECODE;
> -	(void)xdrrec_skiprecord(xdrs);
> +	/*
> +	 * No need skip records with nonblocking connections
> +	 */
> +	if (cd->nonblock == FALSE)
> +		(void)xdrrec_skiprecord(xdrs);
>  	if (xdr_callmsg(xdrs, msg)) {
>  		cd->x_id = msg->rm_xid;
>  		return (TRUE);