rpcsvc: do a 'iobuf_ref()' on buffer while synctaskizing actor

Starting rpc actors using synctask causes rpcsvc to return before the actor
actually finishes its actions. This would cause the buffer to be unreffed by
socket and be possibly reused, before the actor used it, leading to failures
in the actor.
This patch makes rpcsvc take a ref on the buffer when synctaskizing the actor
and store it in the request structure, and then later unref it when the request
structure is destroyed. This makes sure that the buffer is not reused before the
actor has finished.

Change-Id: I8f81e1fef8f3719db220471d2d8ffb8916958956
BUG: 884452
Signed-off-by: Kaushal M <kaushal@redhat.com>
Reviewed-on: http://review.gluster.org/4277
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Anand Avati <avati@redhat.com>

3 files changed, 70 insertions, 13 deletions

diff --git a/rpc/rpc-lib/src/rpcsvc.c b/rpc/rpc-lib/src/rpcsvc.c
index ad1e4f47888..eef1f05044c 100644
--- a/rpc/rpc-lib/src/rpcsvc.c
+++ b/rpc/rpc-lib/src/rpcsvc.c
@@ -276,6 +276,9 @@ rpcsvc_request_destroy (rpcsvc_request_t *req)
                 iobref_unref (req->iobref);
         }
 
+        if (req->hdr_iobuf)
+                iobuf_unref (req->hdr_iobuf);
+
         rpc_transport_unref (req->trans);
 
         mem_put (req);
@@ -434,19 +437,22 @@ err:
 
 
 int
-rpcsvc_synctask_cbk (int ret, call_frame_t *frame, void *opaque)
+rpcsvc_check_and_reply_error (int ret, call_frame_t *frame, void *opaque)
 {
 	rpcsvc_request_t  *req = NULL;
 
 	req = opaque;
 
-        if (ret == RPCSVC_ACTOR_ERROR)
-                rpcsvc_error_reply (req);
+        if (ret == RPCSVC_ACTOR_ERROR) {
+                ret = rpcsvc_error_reply (req);
+                if (ret)
+                        gf_log ("rpcsvc", GF_LOG_WARNING,
+                                "failed to queue error reply");
+        }
 
 	return 0;
 }
 
-
 int
 rpcsvc_handle_rpc_call (rpcsvc_t *svc, rpc_transport_t *trans,
                         rpc_transport_pollin_t *msg)
@@ -527,22 +533,23 @@ rpcsvc_handle_rpc_call (rpcsvc_t *svc, rpc_transport_t *trans,
 			goto err_reply;
 		}
 
-		if (req->synctask)
+		if (req->synctask) {
+                        if (msg->hdr_iobuf)
+                                req->hdr_iobuf = iobuf_ref (msg->hdr_iobuf);
+
 			ret = synctask_new (THIS->ctx->env,
 					    (synctask_fn_t) actor_fn,
-					    rpcsvc_synctask_cbk, NULL, req);
-		else
+					    rpcsvc_check_and_reply_error, NULL,
+                                            req);
+                } else {
 			ret = actor_fn (req);
+                        req->hdr_iobuf = NULL;
+                }
         }
 
 err_reply:
-        if (ret == RPCSVC_ACTOR_ERROR) {
-                ret = rpcsvc_error_reply (req);
-        }
-
-        if (ret)
-                gf_log ("rpcsvc", GF_LOG_WARNING, "failed to queue error reply");
 
+        ret = rpcsvc_check_and_reply_error (ret, NULL, req);
         /* No need to propagate error beyond this function since the reply
          * has now been queued. */
         ret = 0;
diff --git a/rpc/rpc-lib/src/rpcsvc.h b/rpc/rpc-lib/src/rpcsvc.h
index 5a0ddc9da9d..39ae383f4a2 100644
--- a/rpc/rpc-lib/src/rpcsvc.h
+++ b/rpc/rpc-lib/src/rpcsvc.h
@@ -234,6 +234,9 @@ struct rpcsvc_request {
 
         /* Container for transport to store request-specific item */
         void                    *trans_private;
+
+        /* we need to ref the 'iobuf' in case of 'synctasking' it */
+        struct iobuf            *hdr_iobuf;
 };
 
 #define rpcsvc_request_program(req) ((rpcsvc_program_t *)((req)->prog))
diff --git a/tests/bugs/bug-884452.t b/tests/bugs/bug-884452.t
new file mode 100644
index 00000000000..420b4bc8847
--- /dev/null
+++ b/tests/bugs/bug-884452.t
@@ -0,0 +1,47 @@
+#!/bin/bash
+
+. $(dirname $0)/../include.rc
+
+cleanup;
+
+TEST glusterd
+TEST pidof glusterd
+
+TEST $CLI volume create $V0 $H0:$B0/$V0
+TEST $CLI volume start $V0
+
+TEST glusterfs -s $H0 --volfile-id $V0 $M0
+TEST touch $M0/{1..10000}
+
+function ls-loop
+{
+  while true; do
+    ls -lR $M0 1>/dev/null 2>&1
+  done;
+}
+ls-loop &
+LS_LOOP=$!
+
+function vol-status-loop
+{
+  for i in {1..1000}; do
+    $CLI volume status $V0 clients >/dev/null 2>&1
+    if [ $? -ne 0 ]; then
+      return 1
+    fi
+  done;
+
+  return 0
+}
+
+TEST vol-status-loop
+
+kill $LS_LOOP >/dev/null 2>&1
+sleep 2
+
+TEST umount $M0
+
+cleanup;
+
+
+