diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 04aa4a098..d96560c9d 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -250,7 +250,6 @@ static MXS_ROUTER *createInstance(SERVICE *service, char **options) return NULL; } router->service = service; - spinlock_init(&router->lock); /* * Until we know otherwise assume we have some available slaves. @@ -331,7 +330,6 @@ static MXS_ROUTER_SESSION *newSession(MXS_ROUTER *router_inst, MXS_SESSION *sess client_rses->client_dcb = session->client_dcb; client_rses->have_tmp_tables = false; client_rses->forced_node = NULL; - spinlock_init(&client_rses->rses_lock); memcpy(&client_rses->rses_config, &router->rwsplit_config, sizeof(client_rses->rses_config)); int router_nservers = router->service->n_dbref; @@ -409,7 +407,7 @@ static void closeSession(MXS_ROUTER *instance, MXS_ROUTER_SESSION *router_sessio ROUTER_CLIENT_SES *router_cli_ses = (ROUTER_CLIENT_SES *)router_session; CHK_CLIENT_RSES(router_cli_ses); - if (!router_cli_ses->rses_closed && rses_begin_locked_router_action(router_cli_ses)) + if (!router_cli_ses->rses_closed) { /** * Mark router session as closed. @c rses_closed is checked at the start @@ -465,8 +463,6 @@ static void closeSession(MXS_ROUTER *instance, MXS_ROUTER_SESSION *router_sessio } } } - - rses_end_locked_router_action(router_cli_ses); } } @@ -670,31 +666,22 @@ static void clientReply(MXS_ROUTER *instance, GWBUF *writebuf, DCB *backend_dcb) { - DCB *client_dcb; - ROUTER_INSTANCE *router_inst; - ROUTER_CLIENT_SES *router_cli_ses; - sescmd_cursor_t *scur = NULL; - backend_ref_t *bref; + ROUTER_CLIENT_SES *router_cli_ses = (ROUTER_CLIENT_SES *)router_session; + ROUTER_INSTANCE *router_inst = (ROUTER_INSTANCE *)instance; + DCB *client_dcb = backend_dcb->session->client_dcb; - router_cli_ses = (ROUTER_CLIENT_SES *)router_session; - router_inst = (ROUTER_INSTANCE *)instance; CHK_CLIENT_RSES(router_cli_ses); /** * Lock router client session for secure read of router session members. * Note that this could be done without lock by using version # */ - if (!rses_begin_locked_router_action(router_cli_ses)) + if (router_cli_ses->rses_closed) { gwbuf_free(writebuf); - goto lock_failed; + return; } - /** Holding lock ensures that router session remains open */ - ss_dassert(backend_dcb->session != NULL); - client_dcb = backend_dcb->session->client_dcb; - /** Unlock */ - rses_end_locked_router_action(router_cli_ses); /** * 1. Check if backend received reply to sescmd. * 2. Check sescmd's state whether OK_PACKET has been @@ -704,32 +691,10 @@ static void clientReply(MXS_ROUTER *instance, * 3. If reply for this sescmd is sent, lock property cursor * and */ - if (client_dcb == NULL) - { - gwbuf_free(writebuf); - /** Log that client was closed before reply */ - goto lock_failed; - } - /** Lock router session */ - if (!rses_begin_locked_router_action(router_cli_ses)) - { - /** Log to debug that router was closed */ - goto lock_failed; - } - bref = get_bref_from_dcb(router_cli_ses, backend_dcb); - -#if !defined(FOR_BUG548_FIX_ONLY) - /** This makes the issue becoming visible in poll.c */ - if (bref == NULL) - { - /** Unlock router session */ - rses_end_locked_router_action(router_cli_ses); - goto lock_failed; - } -#endif + backend_ref_t *bref = get_bref_from_dcb(router_cli_ses, backend_dcb); CHK_BACKEND_REF(bref); - scur = &bref->bref_sescmd_cur; + sescmd_cursor_t *scur = &bref->bref_sescmd_cur; /** Statement was successfully executed, free the stored statement */ session_clear_stmt(backend_dcb->session); @@ -790,15 +755,7 @@ static void clientReply(MXS_ROUTER *instance, /** Write reply to client DCB */ MXS_SESSION_ROUTE_REPLY(backend_dcb->session, writebuf); } - /** Unlock router session */ - rses_end_locked_router_action(router_cli_ses); - /** Lock router session */ - if (!rses_begin_locked_router_action(router_cli_ses)) - { - /** Log to debug that router was closed */ - goto lock_failed; - } /** There is one pending session command to be executed. */ if (sescmd_cursor_is_active(scur)) { @@ -849,11 +806,6 @@ static void clientReply(MXS_ROUTER *instance, gwbuf_free(bref->bref_pending_cmd); bref->bref_pending_cmd = NULL; } - /** Unlock router session */ - rses_end_locked_router_action(router_cli_ses); - -lock_failed: - return; } @@ -878,64 +830,6 @@ static uint64_t getCapabilities(MXS_ROUTER* instance) * functions are not intended for use outside the read write split router. */ -/** - * @brief Acquires lock to router client session if it is not closed. - * - * Parameters: - * @param rses - in, use - * - * - * @return true if router session was not closed. If return value is true - * it means that router is locked, and must be unlocked later. False, if - * router was closed before lock was acquired. - * - */ -bool rses_begin_locked_router_action(ROUTER_CLIENT_SES *rses) -{ - bool succp = false; - - if (rses == NULL) - { - return false; - } - - CHK_CLIENT_RSES(rses); - - if (rses->rses_closed) - { - - goto return_succp; - } - spinlock_acquire(&rses->rses_lock); - if (rses->rses_closed) - { - spinlock_release(&rses->rses_lock); - goto return_succp; - } - succp = true; - -return_succp: - return succp; -} - -/** to be inline'd */ - -/** - * @brief Releases router client session lock. - * - * Parameters: - * @param rses - - * - * - * @return void - * - */ -void rses_end_locked_router_action(ROUTER_CLIENT_SES *rses) -{ - CHK_CLIENT_RSES(rses); - spinlock_release(&rses->rses_lock); -} - /* * @brief Clear one or more bits in the backend reference state * @@ -1158,6 +1052,10 @@ backend_ref_t *get_bref_from_dcb(ROUTER_CLIENT_SES *rses, DCB *dcb) { bref = NULL; } + + /** We should always have a valid backend reference */ + ss_dassert(bref); + return bref; } @@ -1366,7 +1264,7 @@ static void handleError(MXS_ROUTER *instance, CHK_DCB(problem_dcb); - if (!rses_begin_locked_router_action(rses)) + if (rses->rses_closed) { /** Session is already closed */ problem_dcb->dcb_errhandle_called = true; @@ -1383,7 +1281,6 @@ static void handleError(MXS_ROUTER *instance, * be safe with the code as it stands on 9 Sept 2015 - MNB */ *succp = true; - rses_end_locked_router_action(rses); return; } else @@ -1521,8 +1418,6 @@ static void handleError(MXS_ROUTER *instance, break; } } - - rses_end_locked_router_action(rses); } /** @@ -1642,7 +1537,6 @@ static bool handle_error_new_connection(ROUTER_INSTANCE *inst, bool succp; myrses = *rses; - ss_dassert(SPINLOCK_IS_LOCKED(&myrses->rses_lock)); ses = backend_dcb->session; CHK_SESSION(ses); diff --git a/server/modules/routing/readwritesplit/readwritesplit.h b/server/modules/routing/readwritesplit/readwritesplit.h index 679984d93..d687d0e4b 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.h +++ b/server/modules/routing/readwritesplit/readwritesplit.h @@ -307,7 +307,6 @@ struct router_client_session #if defined(SS_DEBUG) skygw_chk_t rses_chk_top; #endif - SPINLOCK rses_lock; /*< protects rses_deleted */ bool rses_closed; /*< true when closeSession is called */ rses_property_t* rses_properties[RSES_PROP_TYPE_COUNT]; /*< Properties listed by their type */ backend_ref_t* rses_master_ref; @@ -349,7 +348,6 @@ typedef struct typedef struct router_instance { SERVICE* service; /*< Pointer to service */ - SPINLOCK lock; /*< Lock for the instance data */ rwsplit_config_t rwsplit_config; /*< expanded config info from SERVICE */ int rwsplit_version; /*< version number for router's config */ ROUTER_STATS stats; /*< Statistics for this router */ diff --git a/server/modules/routing/readwritesplit/rwsplit_internal.h b/server/modules/routing/readwritesplit/rwsplit_internal.h index 0fdcaeb86..78f415218 100644 --- a/server/modules/routing/readwritesplit/rwsplit_internal.h +++ b/server/modules/routing/readwritesplit/rwsplit_internal.h @@ -56,7 +56,6 @@ bool handle_target_is_all(route_target_t route_target, GWBUF *querybuf, int packet_type, qc_query_type_t qtype); int determine_packet_type(GWBUF *querybuf, bool *non_empty_packet); void log_transaction_status(ROUTER_CLIENT_SES *rses, GWBUF *querybuf, qc_query_type_t qtype); -void session_lock_failure_handling(GWBUF *querybuf, int packet_type, qc_query_type_t qtype); bool is_packet_a_one_way_message(int packet_type); sescmd_cursor_t *backend_ref_get_sescmd_cursor(backend_ref_t *bref); bool is_packet_a_query(int packet_type); @@ -65,8 +64,6 @@ bool send_readonly_error(DCB *dcb); /* * The following are implemented in readwritesplit.c */ -bool rses_begin_locked_router_action(ROUTER_CLIENT_SES *rses); -void rses_end_locked_router_action(ROUTER_CLIENT_SES *rses); void bref_clear_state(backend_ref_t *bref, bref_state_t state); void bref_set_state(backend_ref_t *bref, bref_state_t state); int router_handle_state_switch(DCB *dcb, DCB_REASON reason, void *data); diff --git a/server/modules/routing/readwritesplit/rwsplit_mysql.c b/server/modules/routing/readwritesplit/rwsplit_mysql.c index 6c4c8d56d..ab767e250 100644 --- a/server/modules/routing/readwritesplit/rwsplit_mysql.c +++ b/server/modules/routing/readwritesplit/rwsplit_mysql.c @@ -287,32 +287,6 @@ handle_target_is_all(route_target_t route_target, return result; } -/* This is MySQL specific */ -/** - * @brief Write an error message to the log indicating failure - * - * Used when an attempt to lock the router session fails. - * - * @param querybuf Query buffer containing packet - * @param packet_type Integer (enum) indicating type of packet - * @param qtype Query type - */ -void -session_lock_failure_handling(GWBUF *querybuf, int packet_type, qc_query_type_t qtype) -{ - if (packet_type != MYSQL_COM_QUIT) - { - /* NOTE: modutil_get_query is MySQL specific */ - char *query_str = modutil_get_query(querybuf); - - MXS_ERROR("Can't route %s:%s:\"%s\" to " - "backend server. Router is closed.", - STRPACKETTYPE(packet_type), STRQTYPE(qtype), - (query_str == NULL ? "(empty)" : query_str)); - MXS_FREE(query_str); - } -} - /* * Probably MySQL specific because of modutil function */ diff --git a/server/modules/routing/readwritesplit/rwsplit_route_stmt.c b/server/modules/routing/readwritesplit/rwsplit_route_stmt.c index ae3f8a24c..183709d4f 100644 --- a/server/modules/routing/readwritesplit/rwsplit_route_stmt.c +++ b/server/modules/routing/readwritesplit/rwsplit_route_stmt.c @@ -70,16 +70,10 @@ bool route_single_stmt(ROUTER_INSTANCE *inst, ROUTER_CLIENT_SES *rses, /* packet_type is a problem as it is MySQL specific */ packet_type = determine_packet_type(querybuf, &non_empty_packet); qtype = determine_query_type(querybuf, packet_type, non_empty_packet); + if (non_empty_packet) { - /** This might not be absolutely necessary as some parts of the code - * can only be executed by one thread at a time. */ - if (!rses_begin_locked_router_action(rses)) - { - return false; - } handle_multi_temp_and_load(rses, querybuf, packet_type, (int *)&qtype); - rses_end_locked_router_action(rses); if (MXS_LOG_PRIORITY_IS_ENABLED(LOG_INFO)) { @@ -116,7 +110,7 @@ bool route_single_stmt(ROUTER_INSTANCE *inst, ROUTER_CLIENT_SES *rses, { succp = handle_target_is_all(route_target, inst, rses, querybuf, packet_type, qtype); } - else if (rses_begin_locked_router_action(rses)) + else { /* Now we have a lock on the router session */ bool store_stmt = false; @@ -152,12 +146,6 @@ bool route_single_stmt(ROUTER_INSTANCE *inst, ROUTER_CLIENT_SES *rses, ss_dassert(!store_stmt || TARGET_IS_SLAVE(route_target)); handle_got_target(inst, rses, querybuf, target_dcb, store_stmt); } - rses_end_locked_router_action(rses); - } - else - { - session_lock_failure_handling(querybuf, packet_type, qtype); - succp = false; } return succp; @@ -214,12 +202,6 @@ bool route_session_write(ROUTER_CLIENT_SES *router_cli_ses, { int rc; - /** Lock router session */ - if (!rses_begin_locked_router_action(router_cli_ses)) - { - goto return_succp; - } - for (i = 0; i < router_cli_ses->rses_nbackends; i++) { DCB *dcb = backend_ref[i].bref_dcb; @@ -244,15 +226,9 @@ bool route_session_write(ROUTER_CLIENT_SES *router_cli_ses, } } } - rses_end_locked_router_action(router_cli_ses); gwbuf_free(querybuf); goto return_succp; } - /** Lock router session */ - if (!rses_begin_locked_router_action(router_cli_ses)) - { - goto return_succp; - } if (router_cli_ses->rses_nbackends <= 0) { @@ -318,7 +294,6 @@ bool route_session_write(ROUTER_CLIENT_SES *router_cli_ses, if ((prop = rses_property_init(RSES_PROP_TYPE_SESCMD)) == NULL) { MXS_ERROR("Router session property initialization failed"); - rses_end_locked_router_action(router_cli_ses); return false; } @@ -328,7 +303,6 @@ bool route_session_write(ROUTER_CLIENT_SES *router_cli_ses, if (rses_property_add(router_cli_ses, prop) != 0) { MXS_ERROR("Session property addition failed."); - rses_end_locked_router_action(router_cli_ses); return false; } @@ -387,9 +361,6 @@ bool route_session_write(ROUTER_CLIENT_SES *router_cli_ses, atomic_add(&router_cli_ses->rses_nsescmd, 1); - /** Unlock router session */ - rses_end_locked_router_action(router_cli_ses); - return_succp: /** * Routing must succeed to all backends that are used. @@ -1364,7 +1335,6 @@ int rses_property_add(ROUTER_CLIENT_SES *rses, rses_property_t *prop) CHK_CLIENT_RSES(rses); CHK_RSES_PROP(prop); - ss_dassert(SPINLOCK_IS_LOCKED(&rses->rses_lock)); prop->rses_prop_rsession = rses; p = rses->rses_properties[prop->rses_prop_type]; diff --git a/server/modules/routing/readwritesplit/rwsplit_session_cmd.c b/server/modules/routing/readwritesplit/rwsplit_session_cmd.c index 4100dc1b8..72b474073 100644 --- a/server/modules/routing/readwritesplit/rwsplit_session_cmd.c +++ b/server/modules/routing/readwritesplit/rwsplit_session_cmd.c @@ -60,8 +60,6 @@ mysql_sescmd_t *rses_property_get_sescmd(rses_property_t *prop) } CHK_RSES_PROP(prop); - ss_dassert(prop->rses_prop_rsession == NULL || - SPINLOCK_IS_LOCKED(&prop->rses_prop_rsession->rses_lock)); sescmd = &prop->rses_prop_data.sescmd; @@ -134,14 +132,9 @@ GWBUF *sescmd_cursor_process_replies(GWBUF *replybuf, backend_ref_t *bref, bool *reconnect) { - mysql_sescmd_t *scmd; - sescmd_cursor_t *scur; - ROUTER_CLIENT_SES *ses; - - scur = &bref->bref_sescmd_cur; - ss_dassert(SPINLOCK_IS_LOCKED(&(scur->scmd_cur_rses->rses_lock))); - scmd = sescmd_cursor_get_command(scur); - ses = (*scur->scmd_cur_ptr_property)->rses_prop_rsession; + sescmd_cursor_t *scur = &bref->bref_sescmd_cur; + mysql_sescmd_t *scmd = sescmd_cursor_get_command(scur); + ROUTER_CLIENT_SES *ses = (*scur->scmd_cur_ptr_property)->rses_prop_rsession; CHK_GWBUF(replybuf); /** @@ -273,7 +266,6 @@ mysql_sescmd_t *sescmd_cursor_get_command(sescmd_cursor_t *scur) { mysql_sescmd_t *scmd; - ss_dassert(SPINLOCK_IS_LOCKED(&(scur->scmd_cur_rses->rses_lock))); scur->scmd_cur_cmd = rses_property_get_sescmd(*scur->scmd_cur_ptr_property); CHK_MYSQL_SESCMD(scur->scmd_cur_cmd); @@ -293,7 +285,6 @@ bool sescmd_cursor_is_active(sescmd_cursor_t *sescmd_cursor) MXS_ERROR("[%s] Error: NULL parameter.", __FUNCTION__); return false; } - ss_dassert(SPINLOCK_IS_LOCKED(&sescmd_cursor->scmd_cur_rses->rses_lock)); succp = sescmd_cursor->scmd_cur_active; return succp; @@ -303,7 +294,6 @@ bool sescmd_cursor_is_active(sescmd_cursor_t *sescmd_cursor) void sescmd_cursor_set_active(sescmd_cursor_t *sescmd_cursor, bool value) { - ss_dassert(SPINLOCK_IS_LOCKED(&sescmd_cursor->scmd_cur_rses->rses_lock)); /** avoid calling unnecessarily */ ss_dassert(sescmd_cursor->scmd_cur_active != value); sescmd_cursor->scmd_cur_active = value; @@ -420,8 +410,6 @@ static bool sescmd_cursor_next(sescmd_cursor_t *scur) ss_dassert(scur != NULL); ss_dassert(*(scur->scmd_cur_ptr_property) != NULL); - ss_dassert(SPINLOCK_IS_LOCKED( - &(*(scur->scmd_cur_ptr_property))->rses_prop_rsession->rses_lock)); /** Illegal situation */ if (scur == NULL || *scur->scmd_cur_ptr_property == NULL ||