Thanks Neel for spotting this. Will fix befoe pushing.
/AndersBj -----Original Message----- From: Neelakanta Reddy [mailto:[email protected]] Sent: den 24 september 2014 14:38 To: Anders Björnerstedt Cc: [email protected] Subject: Re: [PATCH 1 of 1] IMM: Failure to send completed to PBE defaulted to ccb-recovery [#1127] Hi AndersBj, Reviewed and tested the patch. Ack when pushed with below inline comments: /Neel On Wednesday 24 September 2014 04:47 PM, Anders Bjornerstedt wrote: > osaf/services/saf/immsv/immnd/immnd_evt.c | 63 > ++++++++++++++++++++---------- > 1 files changed, 42 insertions(+), 21 deletions(-) > > > The fix is mainly in immnd_evt_proc_ccb_apply, but also some cleanup > in immnd_evt_proc_ccb_compl_rsp where part of the same problem was > addressed by the earlier ticket: #1096. > > The general case is that the ccb has just gone critical in ImmModel > (at all IMMNDs) and one IMMND is just about to send the completed > callback to the PBE. The completed callback can be sent to PBE either > in 'proc_ccb_apply' when there are no regular OIs involved with the > ccb; or in 'proc_ccb_compl_rsp' if there are regular OIs and the reply > from all of them has been received. > > Solution: > > The impossible case of the PBE-OI being known to ImmModel yet the > client_node not existing is handled by osafassert. > > The rare but possible case of the client_node existing but being stale > is handled by skipping over the send-attempt (which would fail) and > letting ccb-recovery sort things out. > > The rare but possible case of the client_node being ok but the send > over MDS resulting in an error return from MDS is also handled by > letting ccb-recovery sort it out. Note that the handling an error code > from MDS for this case as the send having been processed is safer, > since the message could possibly have reached the PBE despite that MDS > reported error. > > The client node stale case could in principle have been handled by > aborting the CCB, but the ImmModel has entered the critical state for > this ccb at all nodes and so such a solution would require a new message type: > revert-critical-and-abort-ccb being broadcast. Just broadcasting the > currenttly supported ccb-abort would not work because ccbs in critical > will (correctly) discard such a request. > > diff --git a/osaf/services/saf/immsv/immnd/immnd_evt.c > b/osaf/services/saf/immsv/immnd/immnd_evt.c > --- a/osaf/services/saf/immsv/immnd/immnd_evt.c > +++ b/osaf/services/saf/immsv/immnd/immnd_evt.c > @@ -3733,19 +3733,21 @@ static void immnd_evt_proc_ccb_compl_rsp > SaImmOiHandleT implHandle = > m_IMMSV_PACK_HANDLE(pbeConn, pbeNodeId); > /*Fetch client node for OI ! */ > immnd_client_node_get(cb, implHandle, > &oi_cl_node); > - if (oi_cl_node == NULL || oi_cl_node->mIsStale) > { > + osafassert(cl_node); osafassert(oi_cl_node) > + if (oi_cl_node->mIsStale) { > LOG_WA("PBE went down"); > - /* ###TODO need to ABORT ccb in > CRITICAL. or set CRITICAL below*/ > /* This is a bad case. The immnds have > just delegated the decision > - to commit or abort this ccb to the > PBE, yet it has just crashed. > - I should mark the ccb being in limbo > until a new PBE has attached, > - then resend the completed upcall, > which should generate an aborted > - reply to all IMMNDs. > - err = SA_AIS_ERR_FAILED_OPERATION; > - But actually we know here that we > have not sent the completed call to > - the pbe. This means it should be > easy to abort! > + to commit or abort this ccb to the > PBE, yet it has just detached. > + We know here that we have not sent > the completed call to the pbe, > + so in principle the abort should be > simple. But the Ccb has just > + entered the critical state in > ImmModel, so we would need to undo > + that critical state. Problem is it > would have to be undone at all > + IMMNDs which is complicated. Instead > of optimizing this error case > + with complex logic and messaging, we > unify it with the case of > + having sent completed to the PBE, > not getting any response and > + let ccb-recovery sort it out. > */ > - goto done; > + goto skip_send; > } > > memset(&send_evt, '\0', sizeof(IMMSV_EVT)); @@ > -3759,13 +3761,17 > @@ static void immnd_evt_proc_ccb_compl_rsp > TRACE_2("MAKING PBE-IMPLEMENTER CCB COMPLETED > upcall"); > if(immnd_mds_msg_send(cb, NCSMDS_SVC_ID_IMMA_OI, > oi_cl_node->agent_mds_dest, > &send_evt) != NCSCC_RC_SUCCESS) { > - LOG_ER("CCB COMPLETED UPCALL SEND TO > PBE FAILED"); > + LOG_WA("CCB COMPLETED UPCALL SEND TO > PBE FAILED"); > + /* If the send to PBE fails in Mds then > we *dont* know for sure > + that the message did not reach the > PBE. Therefore act as if > + send succceeded and let ccb-recovery > sort it out. > + */ > } else { > TRACE_5("IMMND UPCALL TO PBE for ccb > %u, SEND SUCCEEDED", > evt->info.ccbUpcallRsp.ccbId); > } > - > - } > + } > + skip_send: > reqConn = 0; /* Ensure we dont reply to OM client yet. > */ > } > } else { > @@ -7358,7 +7364,7 @@ static void immnd_evt_proc_ccb_apply(IMM > delayedReply = SA_TRUE; > if(pbeNodeId) { > /* There is be a PBE. */ > - osafassert(err == SA_AIS_OK); /* I not OK then > we should not be waiting. */ > + osafassert(err == SA_AIS_OK); /* If not OK then > we should not be > +waiting. */ > TRACE_5("Wait for PBE commit decision for ccb > %u", evt->info.ccbId); > if(pbeConn) { > TRACE_5("PBE is LOCAL - send completed > upcall for %u", > evt->info.ccbId); @@ -7369,11 +7375,21 @@ static void > immnd_evt_proc_ccb_apply(IMM > SaImmOiHandleT implHandle = > m_IMMSV_PACK_HANDLE(pbeConn, pbeNodeId); > /*Fetch client node for OI ! */ > immnd_client_node_get(cb, implHandle, > &oi_cl_node); > - if (oi_cl_node == NULL || > oi_cl_node->mIsStale) { > - LOG_WA("PBE went down"); > - /* TODO need to ABORT ccb in > CRITICAL. or set CRITICAL below*/ > - err = > SA_AIS_ERR_FAILED_OPERATION; > - abort(); > + osafassert(cl_node); osafassert(oi_cl_node) > + if (oi_cl_node->mIsStale) { > + LOG_WA("PBE-OI has detached"); > + /* This is a bad case. The > immnds have just delegated the decision > + to commit or abort this ccb > to the PBE, yet it has just detached. > + We know here that we have > not sent the completed call to the pbe, > + so in principle the abort > should be simple. But the Ccb has just > + entered the critical state > in ImmModel, so we would need to undo > + that critical state. Problem > is it would have to be undone at all > + IMMNDs which is complicated. > Instead of optimizing this error case > + with complex logic and > messaging, we unify it with the case of > + having sent completed to the > PBE, not getting any response and > + let ccb-recovery sort it out. > + */ > + goto skip_send; > } > > memset(&send_evt, '\0', > sizeof(IMMSV_EVT)); @@ -7387,8 +7403,11 > @@ static void immnd_evt_proc_ccb_apply(IMM > TRACE_2("MAKING PBE-IMPLEMENTER CCB > COMPLETED upcall"); > if(immnd_mds_msg_send(cb, > NCSMDS_SVC_ID_IMMA_OI, > oi_cl_node->agent_mds_dest, > &send_evt) != NCSCC_RC_SUCCESS) { > - LOG_ER("CCB COMPLETED UPCALL > SEND TO PBE FAILED"); > - abort(); > + LOG_WA("CCB COMPLETED UPCALL > SEND TO PBE FAILED"); > + /* If the send to PBE fails in > Mds then we *dont* know for sure > + that the message did not > reach the PBE. Therefore act as if > + send succceeded and let > ccb-recovery sort it out. > + */ > } else { > TRACE_5("IMMND UPCALL TO PBE > for ccb %u, SEND SUCCEEDED", > evt->info.ccbId); > @@ -7400,6 +7419,8 @@ static void immnd_evt_proc_ccb_apply(IMM > TRACE("delayedReply:%u", delayedReply); > } > } > + > + skip_send: > /* err != SA_AIS_OK or no implementers => immediate reply. */ > if (arrSize) { > free(implConnArr); ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
