From 758e5e8448d2694a49f31dee99b793b2f21b66c4 Mon Sep 17 00:00:00 2001 From: Atin Mukherjee Date: Thu, 8 Sep 2016 11:33:59 +0530 Subject: [PATCH 87/94] socket: pollerr event shouldn't trigger socket_connnect_finish If connect fails with any other error than EINPROGRESS we cannot get the error status using getsockopt (... SO_ERROR ... ). Hence we need to remember the state of connect and take appropriate action in the event_handler for the same. As an added note, a event can come where poll_err is HUP and we have poll_in as well (i.e some status was written to the socket), so for such cases we need to finish the connect, process the data and then the poll_err as is the case in the current code. Special thanks to Kaushal M & Raghavendra G for figuring out the issue. >Signed-off-by: Shyam >Reviewed-on: http://review.gluster.org/15440 >Smoke: Gluster Build System >NetBSD-regression: NetBSD Build System >CentOS-regression: Gluster Build System >Reviewed-by: Raghavendra G >Reviewed-on: http://review.gluster.org/15533 >Smoke: Gluster Build System >NetBSD-regression: NetBSD Build System >CentOS-regression: Gluster Build System >Reviewed-by: Pranith Kumar Karampuri Change-Id: Ic45ad59ff8ab1d0a9d2cab2c924ad940b9d38528 BUG: 1377387 Signed-off-by: Atin Mukherjee Reviewed-on: https://code.engineering.redhat.com/gerrit/85608 Reviewed-by: Milind Changire Reviewed-by: Raghavendra Gowdappa --- rpc/rpc-transport/socket/src/socket.c | 42 ++++++++++++++++++++++++++++++++- rpc/rpc-transport/socket/src/socket.h | 3 ++ tests/bugs/changelog/bug-1211327.t | 8 ++---- tests/bugs/ec/bug-1236065.t | 4 +- 4 files changed, 49 insertions(+), 8 deletions(-) diff --git a/rpc/rpc-transport/socket/src/socket.c b/rpc/rpc-transport/socket/src/socket.c index deec0cb..d9383c3 100644 --- a/rpc/rpc-transport/socket/src/socket.c +++ b/rpc/rpc-transport/socket/src/socket.c @@ -2346,6 +2346,7 @@ out: return ret; } +static int socket_disconnect (rpc_transport_t *this); /* reads rpc_requests during pollin */ static int @@ -2370,7 +2371,23 @@ socket_event_handler (int fd, int idx, void *data, } pthread_mutex_unlock (&priv->lock); - ret = (priv->connected == 1) ? 0 : socket_connect_finish(this); + if (priv->connected != 1) { + if (priv->connect_failed) { + /* connect failed with some other error than + EINPROGRESS or ENOENT, so nothing more to do, fail + reading/writing anything even if poll_in or poll_out + is set */ + ret = socket_disconnect (this); + + /* Force ret to be -1, as we are officially done with + this socket */ + ret = -1; + } else { + ret = socket_connect_finish (this); + } + } else { + ret = 0; + } if (!ret && poll_out) { ret = socket_event_poll_out (this); @@ -3046,6 +3063,16 @@ socket_connect (rpc_transport_t *this, int port) gf_log (this->name, GF_LOG_WARNING, "Ignore failed connection attempt on %s, (%s) ", this->peerinfo.identifier, strerror (errno)); + + /* connect failed with some other error than EINPROGRESS + so, getsockopt (... SO_ERROR ...), will not catch any + errors and return them to us, we need to remember this + state, and take actions in socket_event_handler + appropriately */ + /* TBD: What about ENOENT, we will do getsockopt there + as well, so how is that exempt from such a problem? */ + priv->connect_failed = 1; + goto handler; } @@ -3058,9 +3085,22 @@ socket_connect (rpc_transport_t *this, int port) GF_LOG_DEBUG : GF_LOG_ERROR), "connection attempt on %s failed, (%s)", this->peerinfo.identifier, strerror (errno)); + + /* connect failed with some other error than EINPROGRESS + so, getsockopt (... SO_ERROR ...), will not catch any + errors and return them to us, we need to remember this + state, and take actions in socket_event_handler + appropriately */ + /* TBD: What about ENOENT, we will do getsockopt there + as well, so how is that exempt from such a problem? */ + priv->connect_failed = 1; + goto handler; } else { + /* reset connect_failed so that any previous attempts + state is not carried forward */ + priv->connect_failed = 0; ret = 0; } diff --git a/rpc/rpc-transport/socket/src/socket.h b/rpc/rpc-transport/socket/src/socket.h index 8395fd2..7c7005b 100644 --- a/rpc/rpc-transport/socket/src/socket.h +++ b/rpc/rpc-transport/socket/src/socket.h @@ -200,6 +200,9 @@ typedef struct { int32_t idx; /* -1 = not connected. 0 = in progress. 1 = connected */ char connected; + /* 1 = connect failed for reasons other than EINPROGRESS/ENOENT + see socket_connect for details */ + char connect_failed; char bio; char connect_finish_log; char submit_log; diff --git a/tests/bugs/changelog/bug-1211327.t b/tests/bugs/changelog/bug-1211327.t index 19d6e76..a849ec3 100644 --- a/tests/bugs/changelog/bug-1211327.t +++ b/tests/bugs/changelog/bug-1211327.t @@ -27,15 +27,13 @@ TEST $CLI volume set $V0 changelog.changelog on; sleep 1 TEST killall_gluster; -sleep 1 -EXPECT 0 online_brick_count; +EXPECT_WITHIN $CHILD_UP_TIMEOUT "0" online_brick_count TEST glusterd; TEST pidof glusterd; -##Let the brick processes starts -sleep 1; -EXPECT 1 online_brick_count; +##Let the brick processes starts +EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" online_brick_count ##On brick restart only one HTIME should be found. EXPECT 1 count_htime_files; diff --git a/tests/bugs/ec/bug-1236065.t b/tests/bugs/ec/bug-1236065.t index 9038cb9..e425f3e 100644 --- a/tests/bugs/ec/bug-1236065.t +++ b/tests/bugs/ec/bug-1236065.t @@ -48,7 +48,7 @@ TEST ec_test_make ## step 4 TEST $CLI volume start $V0 force -EXPECT '7' online_brick_count +EXPECT_WITHIN $CHILD_UP_TIMEOUT "7" online_brick_count # active heal EXPECT_WITHIN $PROCESS_UP_TIMEOUT "[0-9][0-9]*" get_shd_process_pid @@ -70,7 +70,7 @@ EXPECT '5' online_brick_count ## step 6 TEST $CLI volume start $V0 force -EXPECT '7' online_brick_count +EXPECT_WITHIN $CHILD_UP_TIMEOUT "7" online_brick_count # self-healing EXPECT_WITHIN $PROCESS_UP_TIMEOUT "[0-9][0-9]*" get_shd_process_pid -- 1.7.1