Hi, Anders and I have talked about the patch and had more-less same points as Neelakanta mentioned in the review.
The patch has an impact on the performance, and it will affect every CCB (two iterations), specially loading data. The change could be done only for NO_DANGLING objects, changing addNewNoDanglingRefs, as Neelakanta commented in his review. Best regards, Zoran -----Original Message----- From: Neelakanta Reddy [mailto:[email protected]] Sent: Monday, June 01, 2015 2:33 PM To: Hung Nguyen D; Anders Björnerstedt Cc: [email protected] Subject: Re: [devel] [PATCH 1 of 1] imm: Ensure IMM_MODIFY mutations are always committed first [#1377] Hi Hung, Reviewed and tested the patch. the patch is working good. But the alternate solution may be passing ccbId, through commitModify -->addNewNoDanglingRefs In addNewNoDanglingRefs, modify the if condition (if CCB create is committed and CREATE_LOCK is removed, then it will be present in ccbvector of same CCB) CcbInfo* ccb =(*(std::find_if(sCcbVector.begin(), sCcbVector.end(), CcbIdIs(ccbId)))); ObjectMutationMap::iterator omit; omit=omit.find(omi); if((omi->second->mObjFlags & IMM_CREATE_LOCK)|| omit!=ccb->mMutations.end()) sReverseRefsNoDanglingMMap.insert(std::pair<ObjectInfo *, ObjectInfo *>(omi->second, obj)); wait for AndersBj comments on alternate solution. /Neel. On Monday 01 June 2015 12:41 PM, Hung Nguyen wrote: > osaf/services/saf/immsv/immnd/ImmModel.cc | 65 > +++++++++++++++++------------- > 1 files changed, 36 insertions(+), 29 deletions(-) > > > When an IMM_CREATE mutation is committed, the IMM_CREATE_LOCK flag is also > cleared in commitCreate(). > If the CCB has IMM_MODIFY mutations that add references to object of > committed IMM_CREATE mutations, it will fail to add NO_DANGLING references to > sReverseRefsNoDanglingMMap in addNewNoDanglingRefs() due to cleared > IMM_CREATE_LOCK flag. > mMutations map is sorted by object DN so the order of mutations to be > committed depends on the object DNs. > > This patch ensures that IMM_MODIFY mutations of the CCB are always committed > first. > The mutation list will be looped through twice. > The first loop commits all IMM_MODIFY mutations, the second loop commits the > rest and free the allocated memory. > > 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 > @@ -5470,35 +5470,42 @@ ImmModel::ccbCommit(SaUint32T ccbId, Con > ccb->mWaitStartTime = 0; > > //Do the actual commit! > - ObjectMutationMap::iterator omit; > - for(omit=ccb->mMutations.begin(); omit!=ccb->mMutations.end(); ++omit){ > - ccbNotEmpty=true; > - ObjectMutation* omut = omit->second; > - osafassert(!omut->mWaitForImplAck); > - switch(omut->mOpType){ > - case IMM_CREATE: > - if(ccbId != 1) { > - TRACE_5("COMMITING CREATE of %s", omit->first.c_str()); > - } > - osafassert(omut->mAfterImage); > - commitCreate(omut->mAfterImage); > - omut->mAfterImage=NULL; > - break; > - case IMM_MODIFY: > - osafassert(omut->mAfterImage); > - pbeModeChange = commitModify(omit->first, omut->mAfterImage) > || pbeModeChange; > - omut->mAfterImage=NULL; > - break; > - case IMM_DELETE: > - osafassert(omut->mAfterImage==NULL); > - commitDelete(omit->first); > - break; > - default: > - abort(); > - }//switch > - delete omut; > - }//for > - > + /* Ensure that modifications are committed first [#1377] */ > + for (int doIt = 0; doIt < 2; ++doIt) { > + ObjectMutationMap::iterator omit; > + for(omit=ccb->mMutations.begin(); omit!=ccb->mMutations.end(); > ++omit){ > + ObjectMutation* omut = omit->second; > + if ((!doIt && omut->mOpType != IMM_MODIFY) || (doIt && > omut->mOpType == IMM_MODIFY)) { > + if (doIt) delete omut; //only delete in the second loop > + continue; > + } > + ccbNotEmpty=true; > + osafassert(!omut->mWaitForImplAck); > + switch(omut->mOpType){ > + case IMM_CREATE: > + if(ccbId != 1) { > + TRACE_5("COMMITING CREATE of %s", > omit->first.c_str()); > + } > + osafassert(omut->mAfterImage); > + commitCreate(omut->mAfterImage); > + omut->mAfterImage=NULL; > + break; > + case IMM_MODIFY: > + osafassert(omut->mAfterImage); > + pbeModeChange = commitModify(omit->first, > omut->mAfterImage) || pbeModeChange; > + omut->mAfterImage=NULL; > + break; > + case IMM_DELETE: > + osafassert(omut->mAfterImage==NULL); > + commitDelete(omit->first); > + break; > + default: > + abort(); > + }//switch > + if (doIt) delete omut; //only delete in the second loop > + }//for omit > + }//for doIt > + > ccb->mMutations.clear(); > > if(ccbIdLongDnGuard == ccbId) {ccbIdLongDnGuard= 0;} > > ------------------------------------------------------------------------------ > _______________________________________________ > Opensaf-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/opensaf-devel ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
