Hi Hung, Comments inline.
On 2016/12/28 01:01 PM, Hung Nguyen wrote: > Hi Neel, > > Please find my comments inline. > > BR, > Hung Nguyen - DEK Technologies > > -------------------------------------------------------------------------------- > From: Neelakanta [email protected] > Sent: Thursday, December 22, 2016 4:44PM > To: Zoran Milinkovic, Hung Nguyen > [email protected],[email protected] > Cc: Opensaf-devel > [email protected] > Subject: [PATCH 1 of 1] imm: return TRY_AGAIN when RepositoryInit mode is > chaged to file, during the CCB PBE disable processing [#2229] > > > osaf/services/saf/immsv/immnd/ImmModel.cc | 22 ++++++++++++++++++++-- > osaf/services/saf/immsv/immnd/ImmModel.hh | 3 ++- > osaf/services/saf/immsv/immnd/immnd_cb.h | 3 +++ > osaf/services/saf/immsv/immnd/immnd_evt.c | 24 ++++++++++++++++++++++++ > 4 files changed, 49 insertions(+), 3 deletions(-) > > > TRY_AGAIN will be returned to the ccbApply when the saImmRepositoryInit is > changed from > SA_IMM_KEEP_REPOSITORY to SA_IMM_INIT_FROM_FILE. The duration of the > TRY_AGAIN is from > the time ccb upcall is sent to PBE to receiving ACK from the PBE for the > repository change CCB. > > diff --git a/osaf/services/saf/immsv/immnd/ImmModel.cc > b/osaf/services/saf/immsv/immnd/ImmModel.cc > --- a/osaf/services/saf/immsv/immnd/ImmModel.cc > +++ b/osaf/services/saf/immsv/immnd/ImmModel.cc > @@ -962,14 +962,19 @@ immModel_ccbObjectModify(IMMND_CB *cb, > { > std::string objectName; > bool pbeFile = (cb->mPbeFile != NULL); > + bool changeRim = false; > SaAisErrorT err = ImmModel::instance(&cb->immModel)-> > ccbObjectModify(req, implConn, implNodeId, continuationId, > - pbeConn, pbeNodeId, objectName, hasLongDns, pbeFile); > + pbeConn, pbeNodeId, objectName, hasLongDns, pbeFile, &changeRim); > > if(err == SA_AIS_OK) { > osaf_extended_name_alloc(objectName.c_str(), objName); > } > > + if (err == SA_AIS_OK && changeRim){ > + cb->mPbeDisableCcbId = req->ccbId; > + TRACE("The mPbeDisableCcbId is set to ccbid:%u", cb->mPbeDisableCcbId); > + } > return err; > } > > @@ -1361,6 +1366,12 @@ immModel_ccbAbort(IMMND_CB *cb, > unsigned int ix=0; > > bool aborted = ImmModel::instance(&cb->immModel)->ccbAbort(ccbId, cv, > cl, nodeId, pbeNodeId); > + > + if ( aborted && (cb->mPbeDisableCcbId == ccbId)){ > + TRACE("PbeDisableCcbId %u has been aborted", ccbId); > + cb->mPbeDisableCcbId = 0; > + cb->mPbeDisableCritical = false; > + } > > *arrSize = (SaUint32T) cv.size(); > if(*arrSize) { > @@ -8860,7 +8871,8 @@ ImmModel::ccbObjectModify(const ImmsvOmC > unsigned int* pbeNodeIdPtr, > std::string& objectName, > bool* hasLongDns, > - bool pbeFile) > + bool pbeFile, > + bool * changeRim) > { > TRACE_ENTER(); > osafassert(hasLongDns); > @@ -9342,6 +9354,7 @@ ImmModel::ccbObjectModify(const ImmsvOmC > } > > if (modifiedRim) { > + SaImmRepositoryInitModeT oldRim = > getRepositoryInitMode(); > SaImmRepositoryInitModeT newRim = > (SaImmRepositoryInitModeT) attrValue->getValue_int(); > if((newRim != SA_IMM_INIT_FROM_FILE) && (newRim != > SA_IMM_KEEP_REPOSITORY)) { > TRACE_7("ERR_BAD_OPERATION: attr '%s' in IMM object > %s can not have value %u", > @@ -9352,6 +9365,11 @@ ImmModel::ccbObjectModify(const ImmsvOmC > err = SA_AIS_ERR_BAD_OPERATION; > break; > } > + > + if((oldRim == SA_IMM_KEEP_REPOSITORY) && (newRim == > SA_IMM_INIT_FROM_FILE)){ > + LOG_NO("Request for rim change is arrived in ccb%u", > ccbId); > + * changeRim = true; > + } > } > > > diff --git a/osaf/services/saf/immsv/immnd/ImmModel.hh > b/osaf/services/saf/immsv/immnd/ImmModel.hh > --- a/osaf/services/saf/immsv/immnd/ImmModel.hh > +++ b/osaf/services/saf/immsv/immnd/ImmModel.hh > @@ -254,7 +254,8 @@ public: > unsigned int* pbeNodeId, > std::string& objectName, > bool* hasLongDns, > - bool pbeFile); > + bool pbeFile, > + bool* changeRim); > > SaAisErrorT ccbObjectDelete( > const ImmsvOmCcbObjectDelete* req, > diff --git a/osaf/services/saf/immsv/immnd/immnd_cb.h > b/osaf/services/saf/immsv/immnd/immnd_cb.h > --- a/osaf/services/saf/immsv/immnd/immnd_cb.h > +++ b/osaf/services/saf/immsv/immnd/immnd_cb.h > @@ -129,6 +129,9 @@ typedef struct immnd_cb_tag { > uint8_t mBlockPbeEnable; //Current PBE has not completed shutdown yet. > uint8_t mPbeKills; //If != 0 then immnd has tried to kill Pbe. > uint8_t m2Pbe; //If!=0 => 2PBE, 2 => fetch PBE file info. > + SaUint32T mPbeDisableCcbId; // CcbId, operation of the Disable PBE. > + bool mPbeDisableCritical; //If true then PBE disable is sent to PBE for > ACK. > + > bool mIsOtherScUp; //If set & this is an SC then other SC is up(2pbe). > //False=> *allow* 1safe 2pbe. May err conservatively (true) > bool mForceClean; //true => Force cleanTheHouse to run once *now*. > 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 > @@ -3323,6 +3323,11 @@ static SaAisErrorT immnd_fevs_local_chec > } > /* intentional fallthrough. */ > case IMMND_EVT_A2ND_CCB_APPLY: > + if(cb->mPbeDisableCritical){ > + LOG_WA("ERR_TRY_AGAIN: Disable of PBE has been > initiated, waiting for the reply from the PBE"); > + error = SA_AIS_ERR_TRY_AGAIN; > + break; > + } > > *[Hung] If it's A2ND_CCB_VALIDATE (**intentional fallthrough), we should let > it pass. saImmOmCcbValidate() > only involves OI, it doesn't involve PBE. * This should be added at immnd_evt_proc_ccb_apply and not at immnd_fevs_local_checks. > if(immModel_pbeNotWritable(cb) || (cb->fevs_replies_pending >= > IMMSV_DEFAULT_FEVS_MAX_PENDING) > || !immnd_is_immd_up(cb)) { > /* NO_RESOURCES is here imm internal proxy for > TRY_AGAIN. > @@ -4012,6 +4017,10 @@ static void immnd_evt_proc_ccb_compl_rsp > evt->info.ccbUpcallRsp.ccbId); > } > } > + if(cb->mPbeDisableCcbId == evt->info.ccbId){ > + TRACE(" Disable of PBE is sent to PBE for ACK, > in ccb:%u", evt->info.ccbId); > + cb->mPbeDisableCritical = true; > + } > skip_send: > reqConn = 0; /* Ensure we dont reply to OM client yet. > */ > } > @@ -4063,6 +4072,9 @@ static void immnd_evt_proc_ccb_compl_rsp > SaUint32T arrSize = 0; > > if(immModel_ccbCommit(cb, evt->info.ccbUpcallRsp.ccbId, > &arrSize, &implConnArr)) { > + osafassert(cb->mPbeDisableCcbId == > evt->info.ccbUpcallRsp.ccbId); > + cb->mPbeDisableCcbId = 0; > + cb->mPbeDisableCritical = false; > SaImmRepositoryInitModeT oldRim = cb->mRim; > cb->mRim = immModel_getRepositoryInitMode(cb); > if(oldRim != cb->mRim) { > @@ -6943,6 +6955,7 @@ static void immnd_evt_ccb_abort(IMMND_CB > TRACE_2("ccb:%u is originated from this node", ccbId); > } > > + > if (arrSize) { > > TRACE_2("THERE ARE LOCAL IMPLEMENTERS in ccb:%u", ccbId); > @@ -7792,6 +7805,13 @@ static void immnd_evt_proc_ccb_apply(IMM > abort(); > } > #endif > + if(cb->mPbeDisableCritical){ > + LOG_WA("Disable of PBE has been initiated, waiting for the > reply from the PBE"); > + err = SA_AIS_ERR_TRY_AGAIN; > + goto immediate_reply; > + } > > *[Hung] If validateOnly is true, we should let it pass. > saImmOmCcbValidate() only involves OI, it doesn't involve PBE.* > Yes, this will be added. > + > + > if(cb->mPbeFile && (cb->mRim == SA_IMM_KEEP_REPOSITORY)) { > if(immModel_pbeNotWritable(cb)) { > /* NO_RESOURCES is here imm internal proxy for > TRY_AGAIN. > @@ -7912,6 +7932,10 @@ static void immnd_evt_proc_ccb_apply(IMM > evt->info.ccbId); > } > } > + if(cb->mPbeDisableCcbId == evt->info.ccbId){ > + TRACE(" Disable of PBE is sent to PBE > for ACK, in ccb:%u", evt->info.ccbId); > + cb->mPbeDisableCritical = true; > + } > } > } else { > TRACE_2("NO IMPLEMENTERS AT ALL AND NO PBE. for ccb:%u > err:%u sz:%u", evt->info.ccbId, err, arrSize); > > *[Hung] There's chance that we receive A2ND_CCB_COMPLETED_RSP (from > regular OI) for another CCB after pbe-completed-upcall is made > (cb->mPbeDisableCritical set to true). In that case we should do the > resource-abort for that ccb. If not, that ccb wil go to CRITICAL state > and the problem in the ticket will happen. * yes, this has to be added at ccbWaitForCompletedAck. will re-publish the patch. Thanks, Neel. > > > > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
