Hi Vu, Ack from Me. I think this is an important fix to be made available for RC2. Mathi.
> -----Original Message----- > From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au] > Sent: Friday, April 22, 2016 7:37 AM > To: Lennart Lund; Mathivanan Naickan Palanivelu > Cc: opensaf-devel@lists.sourceforge.net > Subject: RE: [PATCH 1 of 1] log: miss mutex protection for common resource > in log agent [#1705] > > Hi Mathi, > > Do you have any comment on the updated patch - > lgsv_missMutexProtection_1705_r3.patch? > > Regards, Vu. > > >-----Original Message----- > >From: Lennart Lund [mailto:lennart.l...@ericsson.com] > >Sent: Thursday, April 21, 2016 2:37 PM > >To: Vu Minh Nguyen; mathi.naic...@oracle.com > >Cc: opensaf-devel@lists.sourceforge.net > >Subject: RE: [PATCH 1 of 1] log: miss mutex protection for common > >resource in log agent [#1705] > > > >Hi Vu, > > > >Ack with a minor comment > > > >- The _e suffix for typedef is not used in OpenSAF should be _t e.g. > >lgs_state_e > >-> lgs_state_t > > > >Thanks > >Lennart > > > >> -----Original Message----- > >> From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au] > >> Sent: den 20 april 2016 12:16 > >> To: Lennart Lund; mathi.naic...@oracle.com > >> Cc: opensaf-devel@lists.sourceforge.net > >> Subject: RE: [PATCH 1 of 1] log: miss mutex protection for common > >> resource in log agent [#1705] > >> > >> Hi Lennart, > >> > >> Thanks. I have updated some code in your patch to fix > >> problems/enhancement: > >> > >> 1) Recursively lock mutex `cb_lock` in recovery & client thread > >> eg: lock in `lga_recover_one_client()` -> lock in > >> `recover_one_stream()`/`initialize_one_client()`. > >> > >> 2) Race condition b/w client thread & mds thread. > >> e.g: > >> - `lga_recover_one_client()` vs `lga_no_server_state_set()` > >> - Free client list vs `lga_no_server_state_set()` > >> > >> 3) Make the recovery thread joinable & adding pthread_exit(). > >> > >> 4) Using osaf function osaf_mutex_lock_ordie/osaf_mutex_unlock_ordie > >> for lock/unlock mutex instead of using pthread_mutex_lock/unlock. > >> > >> 5) Introduce set/check functions with thread safe for lga_state/lgs_state. > >> > >> Please have a look and give your comments. Thanks. > >> > >> Note: > >> I ran test several times. > >> > >> Regards, Vu. > >> > >> >-----Original Message----- > >> >From: Lennart Lund [mailto:lennart.l...@ericsson.com] > >> >Sent: Tuesday, April 19, 2016 6:13 PM > >> >To: Vu Minh Nguyen; mathi.naic...@oracle.com > >> >Cc: opensaf-devel@lists.sourceforge.net > >> >Subject: RE: [PATCH 1 of 1] log: miss mutex protection for common > >> >resource > >> in > >> >log agent [#1705] > >> > > >> >Hi Vu > >> > > >> >I am not surprised that you have found problems in my patch. It was > >> created in > >> >half an hour and not very much tested. > >> >It is mostly an idea of a strategy to handle the problems. I have > >> >refined it a > >> bit > >> >hopefully solving the problems you have found so far. > >> >Attached is the refined version. > >> >The last word on how to fix this is however yours. > >> > > >> >See my comments inline [Lennart] > >> > > >> >Thanks > >> >Lennart > >> > > >> >> -----Original Message----- > >> >> From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au] > >> >> Sent: den 19 april 2016 09:56 > >> >> To: Lennart Lund; mathi.naic...@oracle.com > >> >> Cc: opensaf-devel@lists.sourceforge.net > >> >> Subject: RE: [PATCH 1 of 1] log: miss mutex protection for common > >> resource > >> >> in log agent [#1705] > >> >> > >> >> Hi Lennart, > >> >> > >> >> I tried to run the test on 2 nodes cluster (SC-1 & PL-3): sending > >> >> a record > >> using > >> >> `saflogger` in loop on PL-3, then reboot SC-1 frequently. > >> >> And I got following error message: > >> >> > >> >> # > >> >> *** Error in `/usr/local/bin/saflogger': munmap_chunk(): invalid > pointer: > >> >> 0x00000000408d5cf8 *** > >> >> Aborted (core dumped) > >> >> # > >> >> > >> >> I read code and see following points: > >> >> > >> >> 1) Static data `is_locked` and ` recovery2_lock(void)/ > >> >> recovery2_unlock()` > >> is > >> >> not thread safe. > >> >> Use case: Calling LOG API with different log handles in multi threads. > >> >> > >> >[Lennart] You are right. The problem is the is_locked variable. > >> >If the is_locked variable is a stack variable in the calling > >> >function and given > >to > >> >recovery2_lock() and recovery2_unlock() as an in parameter this > >> >should be thread safe. > >> > > >> > > >> >> 2) Conflict using resource `client_list` b/w recovery thread and > >> >> MDS > >> thread. > >> >> Use case: recovery2 thread is executing (after timeout), headless > occurs. > >> >> Then, mds thread will clear all flags while the recovery2 thread > >> >> is > >> recovering > >> >> clients. > >> >> > >> >[Lennart] Yes, the thread must be terminated before handling the > >> >flags (or > >> any > >> >other client data). Can be solved by joining the thread after > >> >ordering it to terminate. > >> > > >> >> 3) Conflict using resource `client_list` b/w client thread and MDS > thread. > >> >> Use case: client thread is recovering client, calling > >> `lga_recover_one_client`, > >> >> then headless occurs. > >> >> > >> >[Lennart] Yes, lga_recover_one_client() must be made thread safe > >> regarding > >> >the client data. Here the lga_cb lock should be used. > >> > > >> >> I think we can use only one mutex `cb_lock` to synchronize > >> >> accessing to common resource `lga_cb` among threads > (client/recovery/mds). > >> >> And to simplify the code, take the lgs_state out of the lga_cb > >> >> same as lga_state, using set/get on `private` mutexes? > >> >> > >> >> > >> >[Lennart] The main thread problem here is to make handling of the > >> >global lga_cb structure including the client "database" thread safe > >> >but this is not > >> the > >> >only problem. Also API handling client requests and the recovery > >> >thread > >> must > >> >be synchronized and to use the lga_cb lock for this as well as for > >> >protecting > >> the > >> >cb data makes things complicated and error prone. > >> >Also the API must be responsive also while recovery is ongoing. > >> >The lgs_state has nothing to do with resilience and has been there > >> >from > >> before > >> >resilience was introduced (I think it had another name). > >> > > >> >> Regards, Vu. > >> >> > >> >> >-----Original Message----- > >> >> >From: Lennart Lund [mailto:lennart.l...@ericsson.com] > >> >> >Sent: Friday, April 15, 2016 4:01 PM > >> >> >To: Vu Minh Nguyen; mathi.naic...@oracle.com > >> >> >Cc: opensaf-devel@lists.sourceforge.net; Lennart Lund > >> >> >Subject: RE: [PATCH 1 of 1] log: miss mutex protection for common > >> resource > >> >> in > >> >> >log agent [#1705] > >> >> > > >> >> >Hi Vu, > >> >> > > >> >> >When reviewing this I found several problems. Instead of trying > >> >> >to > >> >> comment > >> >> >everything I have instead created a patch of my own. > >> >> > > >> >> >The main problem fixed by this patch is the handling of the > >> >> >critical > >> section > >> >> >where both the recovery2 thread and the client thread could > >> >> >handle the > >> >> same > >> >> >global client data in a conflicting way. > >> >> >The problem was that the recovery 2 thread could start while an > >> >> >API was > >> in > >> >> a > >> >> >section handling client data. > >> >> >This solution allows the APIs to return without waiting for the > >> >> >recovery 2 thread to finish. > >> >> > > >> >> >In the agent there are 3 mutexes: > >> >> >1. The cb_lock (legacy) > >> >> >This mutex is intended for protecting write/read of the global > >> >> >variables > >> in > >> >> the > >> >> >lga_cb structure. > >> >> >Shall be locked only when changing or reading data in the > >> >> >variables not > >> for > >> >> >locking other critical sections. > >> >> > > >> >> >2. The lga_lock (legacy) > >> >> >Local to lga_util.c > >> >> >Used to protect critical sections when handling some critical > >> >> >sections in > >> lga > >> >> >startup and shut down > >> >> > > >> >> >3. The lga_state_lock > >> >> >Local in lga_state.c > >> >> >Used for protecting the lga state. The lga_state variable is > >> >> >removed from > >> >> the > >> >> >lga_cb structure and is used locally in lga_state.c The lga_state > >> >> >is handled in the mds thread and in the recovery 2 thread. > >> >> > > >> >> >3. The lga_recov2_lock (new) > >> >> >Used to protect critical sections so that client APIs and the > >> >> >recovery 2 > >> >thread > >> >> >does not try to handle > >> >> >client data in a conflicting way. > >> >> > > >> >> >4. The lga_recov_mutex > >> >> >Is removed and replaced by the lga_recov2_lock > >> >> > > >> >> >NOTE1: > >> >> >The lga_state and the lgs_state that was broken out of the lga_cb > >> structure > >> >> >(but still were global) in your patch is not realted. > >> >> >The lgs_state is legacy but had another name before resilience > >> functionality > >> >> >was introduced. I have not moved lgs_state out of the lgs_cb > structure. > >> >> >The lga_state is removed from the lga_cb structure and is handled > >> entirely > >> >> via > >> >> >lga_state functions > >> >> > > >> >> >NOTE2: > >> >> >I have run logtest with and without resilience activated and have > >> >> >run > >> both > >> >> >legacy and resilience test cases in UML cluster. > >> >> >I have not tested with helgrind. > >> >> > > >> >> >Please check my patch carefully and try to find out if there are > >> >> >any > >> >> problems. > >> >> >Also please run resilience tests with valgrind/helgrind > >> >> > > >> >> >Regards > >> >> >Lennart > >> >> > > >> >> >> -----Original Message----- > >> >> >> From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au] > >> >> >> Sent: den 8 april 2016 12:04 > >> >> >> To: Lennart Lund; mathi.naic...@oracle.com > >> >> >> Cc: opensaf-devel@lists.sourceforge.net > >> >> >> Subject: RE: [PATCH 1 of 1] log: miss mutex protection for > >> >> >> common > >> >> resource > >> >> >> in log agent [#1705] > >> >> >> > >> >> >> Hi all, > >> >> >> > >> >> >> Have you had time to look at this? Thanks. > >> >> >> > >> >> >> Regards, Vu. > >> >> >> > >> >> >> > >> >> >> >-----Original Message----- > >> >> >> >From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au] > >> >> >> >Sent: Wednesday, April 06, 2016 12:44 PM > >> >> >> >To: 'Lennart Lund' > >> >> >> >Cc: 'mathi.naic...@oracle.com'; 'opensaf- > >> de...@lists.sourceforge.net' > >> >> >> >Subject: RE: [PATCH 1 of 1] log: miss mutex protection for > >> >> >> >common > >> >> resource > >> >> >> in > >> >> >> >log agent [#1705] > >> >> >> > > >> >> >> >Hi Lennart, > >> >> >> > > >> >> >> >Thanks for your comments. > >> >> >> > > >> >> >> >I think it is good idea to pick the lgs_state & lga_state out > >> >> >> >of lga_cb, and use the mutex cb_lock to protect changes in lga > control block. > >> >> >> > > >> >> >> >I reflect these things in attached file. > >> >> >> > > >> >> >> >And to make the code a bit clean, I have defined set/get > >> >> >> >functions for > >> >> >> lgs_state > >> >> >> >& lga_state. > >> >> >> >Also, I declare the mutex cb_lock as recursive one to avoid > >> >> >> >possible > >> >> >> recursively > >> >> >> >lock in one thread. > >> >> >> > > >> >> >> >Could you have a look and give your comments? > >> >> >> > > >> >> >> >Regards, Vu. > >> >> >> > > >> >> >> >>-----Original Message----- > >> >> >> >>From: Lennart Lund [mailto:lennart.l...@ericsson.com] > >> >> >> >>Sent: Tuesday, April 05, 2016 6:59 PM > >> >> >> >>To: Vu Minh Nguyen > >> >> >> >>Cc: mathi.naic...@oracle.com; opensaf- > >> de...@lists.sourceforge.net; > >> >> >> Lennart > >> >> >> >>Lund > >> >> >> >>Subject: RE: [PATCH 1 of 1] log: miss mutex protection for > >> >> >> >>common > >> >> >> resource in > >> >> >> >>log agent [#1705] > >> >> >> >> > >> >> >> >>Hi Vu > >> >> >> >> > >> >> >> >>See my comments inline [Lennart] > >> >> >> >> > >> >> >> >>Note. My comments does not give the complete solution. For > >> >> >> >>that I > >> >> need > >> >> >> to > >> >> >> >>use more time but may give you some ideas. > >> >> >> >> > >> >> >> >>/Lennart > >> >> >> >> > >> >> >> >>> -----Original Message----- > >> >> >> >>> From: Vu Minh Nguyen > [mailto:vu.m.ngu...@dektech.com.au] > >> >> >> >>> Sent: den 5 april 2016 10:48 > >> >> >> >>> To: Lennart Lund > >> >> >> >>> Cc: mathi.naic...@oracle.com; opensaf- > >> de...@lists.sourceforge.net > >> >> >> >>> Subject: RE: [PATCH 1 of 1] log: miss mutex protection for > >> >> >> >>> common > >> >> >> resource > >> >> >> >>> in log agent [#1705] > >> >> >> >>> > >> >> >> >>> Hi Lennart, > >> >> >> >>> > >> >> >> >>> Thanks for your proposal. > >> >> >> >>> > >> >> >> >>> Still, there is race condition b/w mds thread and > >> >> >> >>> recover/client > >> >> thread, I > >> >> >> >>> think. > >> >> >> >>> 1) Headless while recover thread 2 is running > >> >> >> >>> 2) Headless while the client thread is accessing/modifying > >> >> >> >>> to client > >> >> list. > >> >> >> >>> > >> >> >> >>[Lennart] In the mds thread the lga_no_server_state_set() is > >> >> >> >>called > >> if > >> >> >> server > >> >> >> >>down is detected. If recovery state is LGA_RECOVERY2 the > >> >> >> >>thread is > >> >> >> stopped > >> >> >> >>and recovery state is set to state LGA_NO_SERVER. This means > >> >> >> >>that > >> we > >> >> >> start > >> >> >> >all > >> >> >> >>over again with recovery state LGA_RECOVERY1 when the server > >> >> comes > >> >> >> back > >> >> >> >>again. > >> >> >> >>Within the thread the cb lock is taken while the client list > >> >> >> >>is deleted > >if > >> >> >> >needed. > >> >> >> >>With a new mutex for lga_state this mutex should also be used > >> >> >> >>in all > >> >> other > >> >> >> >>places where lga_state is handled not the cb_lock. This means > >> >> >> >>that > >> the > >> >> >> name > >> >> >> >>"recovery2" mutex is not so good instead there should be a > >> lga_state > >> >> >> mutex. > >> >> >> >>The lga_state variable may also be moved outside the lga_cb > >> structure. > >> >> >> >> > >> >> >> >>> I would like to propose other one, using only one mutex > >> >> >> >>> cb_lock > >> >> >> @lga_cb > >> >> >> >>> to protect common `lga_cb` resource, including `state`, > >> >> >> >>> `client` list, > >> >> etc. in > >> >> >> >>> three threads. > >> >> >> >>> > >> >> >> >>[Lennart] The cb_lock is used to protect variables in the cb > >> >> >> >>structure > >> >> but > >> >> >> the > >> >> >> >>lga_state mutex prevent recovery or API operations from being > >> done > >> >> at > >> >> >> the > >> >> >> >>same time. > >> >> >> >> > >> >> >> >>> 1) Client thread > >> >> >> >>> - Lock mutex before getting the state > >> >> >> >>> - Unlock right before the API operation is finished > >> >> >> >>> > >> >> >> >>[Lennart] Yes but the lga_state mutex > >> >> >> >> > >> >> >> >>> 2) Recovery2 thread > >> >> >> >>> - Lock mutex after timeout > >> >> >> >>> - Unlock before ending the thread > >> >> >> >>> > >> >> >> >>[Lennart] No this will cause the API to be locked as long as > >> >> >> >>the > >> recovery > >> >> 2 > >> >> >> >>thread is running and this is not wanted. The API shall work > >> >> >> >>also > >> while > >> >> the > >> >> >> >>recovery thread is running and most of the APIs shall return > >> >> >> >>with SA_AIS_TRY_AGAIN during recovery 2 state. > >> >> >> >> > >> >> >> >>> 3) MDS thread > >> >> >> >>> - Lock/unlock before/after changing state In case of > >> >> >> >>> headless: > >> >> >> >>> - Lock/unlock mutex before modifying `client_list` in > >> >> >> >>> lga_no_server_state_set() > >> >> >> >>> > >> >> >> >>[Lennart] lga_state is not changed directly in the mds thread > >> >> >> >>but > >> >> indirectly > >> >> >> >>when the lga_serv_recov1state_set() and > >> lga_no_server_state_set() > >> >> >> functions > >> >> >> >>are called. I these functions the recovery thread and state > >> >> >> >>change is > >> >> >> handled > >> >> >> >>and the mutex used shall be changed to the lga_state mutex. > >> >> >> >> > >> >> >> >>The client data should be protected by the cb_lock mutex in > >> >> >> >>all > >> places > >> >> >> which > >> >> >> >is > >> >> >> >>not the case. There is no protection in the mds trhead in the > >> >> >> >>lga_lgs_msg_proc() where client data is used via the > >> >> >> >>ga_find_hdl_rec_by_regid() function. This function should > >> >> >> >>lock the > >> >> lga_cb > >> >> >> lock > >> >> >> >>while handling the lga_cb client data. > >> >> >> >>Also there are two functions in lga_state.c that are handling > >> >> >> >>client > >> data > >> >> >> >locking > >> >> >> >>a lga_recov_mutex. This does not protect client data handled > >> >> >> >>in the > >> >> mds > >> >> >> >>thread. > >> >> >> >> > >> >> >> >>> What is your opinion? Do you think there is impact to log > >> >> >> >>> client if > >> they > >> >> >call > >> >> >> >>> LOG API in multi threads? > >> >> >> >>> > >> >> >> >>[Lennart] The mutex handling should be cleaned up. > >> >> >> >> - Recovery state should be protected by its own mutex so > >> >> >> >>that APIs > >> >> can > >> >> >> work > >> >> >> >>in respective state without locking. > >> >> >> >> - Client data should be protected in all threads by the lga_cb > lock. > >> >> >> >> - The lga_recov_mutex can probably be removed > >> >> >> >> > >> >> >> >>> Regards, Vu. > >> >> >> >>> > >> >> >> >>> > >> >> >> >>> >-----Original Message----- > >> >> >> >>> >From: Lennart Lund [mailto:lennart.l...@ericsson.com] > >> >> >> >>> >Sent: Monday, April 04, 2016 3:51 PM > >> >> >> >>> >To: Vu Minh Nguyen > >> >> >> >>> >Cc: mathi.naic...@oracle.com; opensaf- > >> >> de...@lists.sourceforge.net; > >> >> >> >>> Lennart > >> >> >> >>> >Lund > >> >> >> >>> >Subject: RE: [PATCH 1 of 1] log: miss mutex protection for > >> common > >> >> >> >resource > >> >> >> >>> in > >> >> >> >>> >log agent [#1705] > >> >> >> >>> > > >> >> >> >>> >Hi Vu, > >> >> >> >>> > > >> >> >> >>> >A simpler solution than described below, maybe: > >> >> >> >>> > > >> >> >> >>> >Still use a separate "recovery2" mutex but a "normal one". > >> >> >> >>> > - In the API, lock the mutex just before checking if we > >> >> >> >>> >are in > >> >> recovery > >> >> >> state > >> >> >> >>> 2 > >> >> >> >>> >(before "if (lga_state == LGA_RECOVERY2) {") > >> >> >> >>> > - In the API, do not unlock until the API operation is > >> >> >> >>> >finished > >> >> >> >>> > - In the recovery thread lock the mutex just before > >> >> >> >>> >changing the > >> >> >> recovery > >> >> >> >>> >state to recovery state 2 (replace lock of lga_cb.cb_lock > >> >> >> >>> >with > >> >> recovery2 > >> >> >> >>> mutex) > >> >> >> >>> >(protect the lga_state variable) > >> >> >> >>> >- In the recovery thread the mutex shall be unlocked just > >> >> >> >>> >after > >> >> >> lga_state is > >> >> >> >>> set > >> >> >> >>> >to recovery state 2. > >> >> >> >>> > > >> >> >> >>> >This means that the recovery thread cannot set recovery > >> >> >> >>> >state2 > >> or > >> >> >> execute > >> >> >> >>> >until an eventual ongoing API operation is done. > >> >> >> >>> >If the thread starts and set recovery state just before > >> >> >> >>> >the API > >> checks > >> >> >> >>> recovery > >> >> >> >>> >state the API will correctly see that we are in recovery > >> >> >> >>> >state 2 and > >> >> >> handle > >> >> >> >>> thing > >> >> >> >>> >accordingly. > >> >> >> >>> > > >> >> >> >>> >Regards > >> >> >> >>> >Lennart > >> >> >> >>> > > >> >> >> >>> > > >> >> >> >>> >> -----Original Message----- > >> >> >> >>> >> From: Lennart Lund > >> >> >> >>> >> Sent: den 1 april 2016 16:51 > >> >> >> >>> >> To: Vu Minh Nguyen > >> >> >> >>> >> Cc: mathi.naic...@oracle.com; opensaf- > >> >> >> de...@lists.sourceforge.net; > >> >> >> >>> >> Lennart Lund > >> >> >> >>> >> Subject: RE: [PATCH 1 of 1] log: miss mutex protection > >> >> >> >>> >> for > >> common > >> >> >> >>> resource > >> >> >> >>> >> in log agent [#1705] > >> >> >> >>> >> > >> >> >> >>> >> Hi Vu, > >> >> >> >>> >> > >> >> >> >>> >> You are right, good finding, this may happen since the > >> recovery2 > >> >> >> thread > >> >> >> >is > >> >> >> >>> >> started and is waiting for a timeout. If this timeout > >> >> >> >>> >> happen at > >> the > >> >> >> "wrong > >> >> >> >>> >> time" bad things may happen. > >> >> >> >>> >> However the actual problem should be solved. > >> >> >> >>> >> > >> >> >> >>> >> This is how it could be done (maybe you can find a > >> >> >> >>> >> better > >> way?): > >> >> >> >>> >> When timeout the recovery thread shall set recovery > >> >> >> >>> >> state 2 as > >> is > >> >> but > >> >> >> >shall > >> >> >> >>> >> not start any recovery if there is any ongoing client > >> >> >> >>> >> activity. > >> >> >> >>> >> This could be solved by using a "recovery2" mutex (not > >> >> >> >>> >> the > >> >> existing > >> >> >> >>> >> lga_cb.cb_lock). > >> >> >> >>> >> - The recovery thread lock this mutex after setting > >> >> >> >>> >> recovery > >> state > >> >> 2. > >> >> >> >>> >> - During recovery state 1 all APIs check if the mutex > >> >> >> >>> >> is taken > >> and if > >> >> so > >> >> >> >>> return > >> >> >> >>> >> TRY AGAIN (this means that we are in recovery state 2 > >> >> >> >>> >> but the > >> >> state > >> >> >> >>> change > >> >> >> >>> >> may have happened after the check in the API). Note: > "trylock" > >> >> >> cannot be > >> >> >> >>> >> used for this purpose since a sequence trylock -> lock > >> >> >> >>> >> is not > >> >> atomic. > >> >> >> >>> Maybe a > >> >> >> >>> >> PTHREAD_MUTEX_ERRORCHECK type of mutex can be > used. > >> >> >> >>> >> - If the mutex is not taken it shall be locked by the > >> >> >> >>> >> API function > >> >> and > >> >> >> be > >> >> >> >>> >> unlocked just before the API function returns preventing > >> >> >> >>> >> the > >> >> >> recovery > >> >> >> >>> >> thread from starting recovery during an ongoing API > operation. > >> >> >> >>> >> The next time an API is called recovery state 2 is set > >> >> >> >>> >> and the API > >> >> >> function > >> >> >> >>> will > >> >> >> >>> >> operate accordingly. > >> >> >> >>> >> > >> >> >> >>> >> Thanks > >> >> >> >>> >> Lennart > >> >> >> >>> >> > >> >> >> >>> >> > -----Original Message----- > >> >> >> >>> >> > From: Vu Minh Nguyen > >> [mailto:vu.m.ngu...@dektech.com.au] > >> >> >> >>> >> > Sent: den 1 april 2016 16:02 > >> >> >> >>> >> > To: Lennart Lund > >> >> >> >>> >> > Cc: mathi.naic...@oracle.com; opensaf- > >> >> >> de...@lists.sourceforge.net > >> >> >> >>> >> > Subject: Re: [PATCH 1 of 1] log: miss mutex protection > >> >> >> >>> >> > for > >> >> common > >> >> >> >>> >> resource > >> >> >> >>> >> > in log agent [#1705] > >> >> >> >>> >> > > >> >> >> >>> >> > Hi Lennart, > >> >> >> >>> >> > > >> >> >> >>> >> > Is it possible for following case happens? > >> >> >> >>> >> > > >> >> >> >>> >> > Client thread comes to point send_Finalize_msg() > >> >> >> >>> >> > @lga_api, > >> >> then > >> >> >> LOG > >> >> >> >>> >> > service up, recovery thread starts and comes to the > >> >> >> >>> >> > point right after "while (pclient != NULL)" in > >> >> >> >>> >> > recovery2_thread @lga_state.c Then, the context is > >> >> >> >>> >> > switching to the client thread and it > >> deletes > >> >> the > >> >> >> >>> >> > resource while recovery2_thread is running? > >> >> >> >>> >> > > >> >> >> >>> >> > Regards, Vu. > >> >> >> >>> >> > > >> >> >> >>> >> > Quoting Lennart Lund <lennart.l...@ericsson.com>: > >> >> >> >>> >> > > >> >> >> >>> >> > > Hi Vu, > >> >> >> >>> >> > > > >> >> >> >>> >> > > /* Recover clients one at a time */ > >> >> >> >>> >> > > rc = recover_one_client(p_client); > >> >> >> >>> >> > > + if (p_client == NULL) goto done; > >> >> >> >>> >> > > + > >> >> >> >>> >> > > [Lennart] Why is this needed? This check is done in > >> >> >> >>> >> > > "while (p_client != NULL) { " > >> >> >> >>> >> > > and p_client cannot become NULL until "p_client = > >> >> >> >>> >> > > p_client- > >> >> >> >next; " > >> >> >> >>> >> > > is done as > >> >> >> >>> >> > > the last line in the while loop. > >> >> >> >>> >> > > > >> >> >> >>> >> > > [Vu] It is my intention. I am afraid if the context > >> >> >> >>> >> > > is switched > >> to > >> >> >> >>> >> > > client thread > >> >> >> >>> >> > > right after the recovery thread enters the while loop. > >> >> >> >>> >> > > If it is the case, p_client will be NULL after done > >> >> >> recover_one_client(). > >> >> >> >>> >> > > > >> >> >> >>> >> > > [Lennart] I don't really understand your > >> >> >> >>> >> > > explanation. What > >> is > >> >> >> >>> >> > > happening in the client thread when we are in > >> >> >> >>> >> > > recovery > >> state 2 > >> >> >> that > >> >> >> >>> >> > > can set p_client == NULL? The client thread should > >> >> >> >>> >> > > not be > >> able > >> >> to > >> >> >> do > >> >> >> >>> >> > > anything with the data involved when recovering > >> >> >> >>> >> > > client in > >> the > >> >> >> >>> >> > > recovery thread? > >> >> >> >>> >> > > > >> >> >> >>> >> > > Regards > >> >> >> >>> >> > > Lennart > >> >> >> >>> >> > > > >> >> >> >>> >> > >> -----Original Message----- > >> >> >> >>> >> > >> From: Vu Minh Nguyen > >> >> [mailto:vu.m.ngu...@dektech.com.au] > >> >> >> >>> >> > >> Sent: den 1 april 2016 14:14 > >> >> >> >>> >> > >> To: Lennart Lund > >> >> >> >>> >> > >> Cc: mathi.naic...@oracle.com; opensaf- > >> >> >> de...@lists.sourceforge.net > >> >> >> >>> >> > >> Subject: Re: [PATCH 1 of 1] log: miss mutex > >> >> >> >>> >> > >> protection for > >> >> >> common > >> >> >> >>> >> > resource > >> >> >> >>> >> > >> in log agent [#1705] > >> >> >> >>> >> > >> > >> >> >> >>> >> > >> Hi Lennart, > >> >> >> >>> >> > >> > >> >> >> >>> >> > >> Please see my comment inline. > >> >> >> >>> >> > >> > >> >> >> >>> >> > >> Regards, Vu > >> >> >> >>> >> > >> > >> >> >> >>> >> > >> Quoting Lennart Lund <lennart.l...@ericsson.com>: > >> >> >> >>> >> > >> > >> >> >> >>> >> > >> > Hi Vu > >> >> >> >>> >> > >> > > >> >> >> >>> >> > >> > Ack with comments > >> >> >> >>> >> > >> > > >> >> >> >>> >> > >> > See my comments inline [Lennart]: > >> >> >> >>> >> > >> > > >> >> >> >>> >> > >> > Thanks > >> >> >> >>> >> > >> > Lennart > >> >> >> >>> >> > >> > > >> >> >> >>> >> > >> >> -----Original Message----- > >> >> >> >>> >> > >> >> From: Vu Minh Nguyen > >> >> >> [mailto:vu.m.ngu...@dektech.com.au] > >> >> >> >>> >> > >> >> Sent: den 23 mars 2016 08:16 > >> >> >> >>> >> > >> >> To: mathi.naic...@oracle.com; Lennart Lund > >> >> >> >>> >> > >> >> Cc: opensaf-devel@lists.sourceforge.net > >> >> >> >>> >> > >> >> Subject: [PATCH 1 of 1] log: miss mutex > >> >> >> >>> >> > >> >> protection for > >> >> >> common > >> >> >> >>> >> > resource > >> >> >> >>> >> > >> in > >> >> >> >>> >> > >> >> log agent [#1705] > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> osaf/libs/agents/saf/lga/lga_state.c | 41 > >> >> >> >>> >> > >> >> +++++++++++++++++++++++++++++++---- > >> >> >> >>> >> > >> >> osaf/libs/agents/saf/lga/lga_state.h | 1 + > >> >> >> >>> >> > >> >> osaf/libs/agents/saf/lga/lga_util.c | 30 > >> +++++++++++---- > >> >> ---- > >> >> >> ----- > >> >> >> >>> - > >> >> >> >>> >> > >> >> 3 files changed, 50 insertions(+), 22 > >> >> >> >>> >> > >> >> deletions(-) > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> There was an race condition between client > >> >> >> >>> >> > >> >> thread and > >> >> >> recovery > >> >> >> >>> >> > thread > >> >> >> >>> >> > >> >> accessing to common resource `client_list`. > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> Add mutex protection. > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> diff --git > >> >> >> >>> >> > >> >> a/osaf/libs/agents/saf/lga/lga_state.c > >> >> >> >>> >> > >> >> b/osaf/libs/agents/saf/lga/lga_state.c > >> >> >> >>> >> > >> >> --- a/osaf/libs/agents/saf/lga/lga_state.c > >> >> >> >>> >> > >> >> +++ b/osaf/libs/agents/saf/lga/lga_state.c > >> >> >> >>> >> > >> >> @@ -355,8 +355,11 @@ static void > >> >> *recovery2_thread(void > >> >> >> >>> *dumm > >> >> >> >>> >> > >> >> /* We have been > >> >> >> signaled to exit > >> >> >> >>> >> > >> >> */ > >> >> >> >>> >> > >> >> goto done; > >> >> >> >>> >> > >> >> } > >> >> >> >>> >> > >> >> + > >> >> >> >>> >> > >> >> /* Recover clients one at a time */ > >> >> >> >>> >> > >> >> rc = recover_one_client(p_client); > >> >> >> >>> >> > >> >> + if (p_client == NULL) goto done; > >> >> >> >>> >> > >> >> + > >> >> >> >>> >> > >> > [Lennart] Why is this needed? This check is done > >> >> >> >>> >> > >> > in > >> "while > >> >> >> >(p_client > >> >> >> >>> >> > >> > != NULL) { " and p_client cannot become NULL > >> >> >> >>> >> > >> > until > >> >> "p_client = > >> >> >> >>> >> > >> > p_client->next; " is done as the last line in the > >> >> >> >>> >> > >> > while > >> loop. > >> >> >> >>> >> > >> > > >> >> >> >>> >> > >> [Vu] It is my intention. I am afraid if the context > >> >> >> >>> >> > >> is > >> switched to > >> >> >> >>> >> > >> client thread > >> >> >> >>> >> > >> right after the recovery thread enters the while loop. > >> >> >> >>> >> > >> If it is the case, p_client will be NULL after done > >> >> >> >>> recover_one_client(). > >> >> >> >>> >> > >> > >> >> >> >>> >> > >> >> TRACE("\t Client %d is recovered", > >> >> >> p_client- > >> >> >> >>> >> > >> >> >lgs_client_id); > >> >> >> >>> >> > >> >> if (rc == -1) { > >> >> >> >>> >> > >> >> TRACE("%s > >> >> >> recover_one_client > >> >> >> >>> >> > >> >> Fail Deleting cllient (id %d)", @@ -593,6 > >> >> >> >>> >> > >> >> +596,15 @@ int > >> >> >> recover_one_client(lga_client_hdl_re > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> TRACE_ENTER(); > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> + if (p_client == NULL) { > >> >> >> >>> >> > >> >> + /** > >> >> >> >>> >> > >> >> + * Probably the client_list has > >> >> >> been deleted by > >> >> >> >>> >> > >> >> client thread > >> >> >> >>> >> > >> >> + * in context calling > >> >> >> saLogFinalize(). So, give up > >> >> >> >>> >> > >> >> recovering. > >> >> >> >>> >> > >> >> + */ > >> >> >> >>> >> > >> >> + rc = -1; > >> >> >> >>> >> > >> >> + goto done; > >> >> >> >>> >> > >> >> + } > >> >> >> >>> >> > >> >> + > >> >> >> >>> >> > >> >> /* This function may be called both in the > >> >> >> recovery thread and > >> >> >> >>> >> > >> >> in the > >> >> >> >>> >> > >> >> * client thread. A possible scenario is that > >> >> >> >>> >> > >> >> the > >> >> >> recovery 2 > >> >> >> >>> >> > >> >> thread > >> >> >> >>> >> > >> >> * times out and calls this function in > >> >> >> >>> >> > >> >> recovery > >> >> >> mode 2 while > >> >> >> >>> >> > >> >> there > >> >> >> >>> >> > >> >> @@ -658,13 +670,32 @@ uint32_t > delete_one_client( > >> >> >> >>> >> > >> >> lga_client_hdl_rec_t *rm_node > >> >> >> >>> >> > >> >> ) > >> >> >> >>> >> > >> >> { > >> >> >> >>> >> > >> >> - TRACE_ENTER2(); > >> >> >> >>> >> > >> >> - uint32_t ncs_rc; > >> >> >> >>> >> > >> >> + return lga_hdl_rec_del(list_head, rm_node); > } > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> +/** > >> >> >> >>> >> > >> >> + * Free the memory allocated for one client > >> >> >> >>> >> > >> >> +handle > >> with > >> >> >> mutext > >> >> >> >>> >> > >> protection. > >> >> >> >>> >> > >> >> + * > >> >> >> >>> >> > >> >> + * @param p_client_hdl > >> >> >> >>> >> > >> >> + * @return void */ void > >> >> >> >>> >> > >> >> +free_client_hdl(lga_client_hdl_rec_t > >> >> **p_client_hdl) > >> >> >> >>> >> > >> >> +{ > >> >> >> >>> >> > >> >> + lga_client_hdl_rec_t *client_hdl; > >> >> >> >>> >> > >> >> + > >> >> >> >>> >> > >> >> + /** > >> >> >> >>> >> > >> >> + * To prevent race condition b/w client thread > >> >> >> and recovery > >> >> >> >>> >> > >> >> thread > >> >> >> >>> >> > >> >> + * accessing to common resource `client_list`. > >> >> >> [#1705] > >> >> >> >>> >> > >> >> + */ > >> >> >> >>> >> > >> >> osaf_mutex_lock_ordie(&lga_recov_mutex); > >> >> >> >>> >> > >> >> - ncs_rc = lga_hdl_rec_del(list_head, > rm_node); > >> >> >> >>> >> > >> >> + > >> >> >> >>> >> > >> >> + client_hdl = *p_client_hdl; > >> >> >> >>> >> > >> >> + if (client_hdl == NULL) goto done; > >> >> >> >>> >> > >> >> + > >> >> >> >>> >> > >> >> + free(client_hdl); > >> >> >> >>> >> > >> >> + client_hdl = NULL; > >> >> >> >>> >> > >> >> + > >> >> >> >>> >> > >> >> +done: > >> >> >> >>> >> > >> >> > osaf_mutex_unlock_ordie(&lga_recov_mutex); > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> - TRACE_LEAVE(); > >> >> >> >>> >> > >> >> - return ncs_rc; > >> >> >> >>> >> > >> >> } > >> >> >> >>> >> > >> >> diff --git > >> >> >> >>> >> > >> >> a/osaf/libs/agents/saf/lga/lga_state.h > >> >> >> >>> >> > >> >> b/osaf/libs/agents/saf/lga/lga_state.h > >> >> >> >>> >> > >> >> --- a/osaf/libs/agents/saf/lga/lga_state.h > >> >> >> >>> >> > >> >> +++ b/osaf/libs/agents/saf/lga/lga_state.h > >> >> >> >>> >> > >> >> @@ -32,6 +32,7 @@ uint32_t delete_one_client( > >> >> >> >>> >> > >> >> lga_client_hdl_rec_t **list_head, > >> >> >> >>> >> > >> >> lga_client_hdl_rec_t *rm_node > >> >> >> >>> >> > >> >> ); > >> >> >> >>> >> > >> >> +void free_client_hdl(lga_client_hdl_rec_t > >> >> **p_client_hdl); > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> #ifdef __cplusplus > >> >> >> >>> >> > >> >> } > >> >> >> >>> >> > >> >> diff --git a/osaf/libs/agents/saf/lga/lga_util.c > >> >> >> >>> >> > >> >> b/osaf/libs/agents/saf/lga/lga_util.c > >> >> >> >>> >> > >> >> --- a/osaf/libs/agents/saf/lga/lga_util.c > >> >> >> >>> >> > >> >> +++ b/osaf/libs/agents/saf/lga/lga_util.c > >> >> >> >>> >> > >> >> @@ -19,6 +19,7 @@ #include <syslog.h> #include > >> >> >> >>> >> > >> >> "lga.h" > >> >> >> >>> >> > >> >> #include "osaf_poll.h" > >> >> >> >>> >> > >> >> +#include "lga_state.h" > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> /* Variables used during startup/shutdown only > >> >> >> >>> >> > >> >> */ static pthread_mutex_t lga_lock = > >> >> >> >>> PTHREAD_MUTEX_INITIALIZER; > >> >> >> >>> >> > >> >> @@ -449,11 +450,9 @@ void > >> >> >> lga_hdl_list_del(lga_client_hdl_rec > >> >> >> >>> >> > >> >> /** clean up the channel records for this lga- > >> >> >> client > >> >> >> >>> >> > >> >> **/ > >> >> >> >>> >> > >> >> > >> >> >> lga_log_stream_hdl_rec_list_del(&client_hdl- > >> >> >> >>> >> > >> >> >stream_list); > >> >> >> >>> >> > >> >> - /** remove the association with hdl-mngr > >> >> >> >>> >> > >> >> - **/ > >> >> >> >>> >> > >> >> - free(client_hdl); > >> >> >> >>> >> > >> >> - client_hdl = 0; > >> >> >> >>> >> > >> >> + free_client_hdl(&client_hdl); > >> >> >> >>> >> > >> >> } > >> >> >> >>> >> > >> >> + > >> >> >> >>> >> > >> >> TRACE_LEAVE(); } > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> @@ -536,38 +535,35 @@ uint32_t > >> >> >> >>> lga_hdl_rec_del(lga_client_hdl_ > >> >> >> >>> >> > >> >> if (list_iter == rm_node) { > >> >> >> >>> >> > >> >> *list_head = rm_node->next; > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> - /** detach & release the IPC > >> >> >> >>> >> > >> >> - **/ > >> >> >> >>> >> > >> >> + /* detach & release the IPC */ > >> >> >> >>> >> > >> >> m_NCS_IPC_DETACH(&rm_node- > >> >> >> >mbx, > >> >> >> >>> >> > >> >> lga_clear_mbx, NULL); > >> >> >> >>> >> > >> >> m_NCS_IPC_RELEASE(&rm_node- > >> >> >> >mbx, NULL); > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> ncshm_give_hdl(rm_node- > >> >> >> >local_hdl); > >> >> >> >>> >> > >> >> > >> >> >> ncshm_destroy_hdl(NCS_SERVICE_ID_LGA, > >> >> >> >>> >> > >> >> rm_node->local_hdl); > >> >> >> >>> >> > >> >> - /** Free the channel records off this hdl > >> >> >> >>> >> > >> >> - **/ > >> >> >> >>> >> > >> >> + > >> >> >> >>> >> > >> >> + /* Free the channel records off > >> >> >> this hdl */ > >> >> >> >>> >> > >> >> > >> >> >> lga_log_stream_hdl_rec_list_del(&rm_node- > >> >> >> >>> >> > >> >> >stream_list); > >> >> >> >>> >> > >> >> + free_client_hdl(&rm_node); > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> - /** free the hdl rec > >> >> >> >>> >> > >> >> - **/ > >> >> >> >>> >> > >> >> - free(rm_node); > >> >> >> >>> >> > >> >> rc = NCSCC_RC_SUCCESS; > >> >> >> >>> >> > >> >> goto out; > >> >> >> >>> >> > >> >> - } else { /* find the rec */ > >> >> >> >>> >> > >> >> + } else { > >> >> >> >>> >> > >> >> + /* find the rec */ > >> >> >> >>> >> > >> >> while (NULL != list_iter) { > >> >> >> >>> >> > >> >> if (list_iter->next == > >> >> >> rm_node) { > >> >> >> >>> >> > >> >> > >> >> >> list_iter->next = > >> >> >> >>> >> > >> >> rm_node->next; > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> - /** detach & release the IPC */ > >> >> >> >>> >> > >> >> + /* > >> >> >> detach & release > >> >> >> >>> >> > >> >> the IPC */ > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> m_NCS_IPC_DETACH(&rm_node->mbx, > >> >> >> lga_clear_mbx, > >> >> >> >>> >> > >> NULL); > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> m_NCS_IPC_RELEASE(&rm_node->mbx, > NULL); > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> ncshm_give_hdl(rm_node->local_hdl); > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> ncshm_destroy_hdl(NCS_SERVICE_ID_LGA, > >> >> >> rm_node- > >> >> >> >>> >> > >> >> >local_hdl); > >> >> >> >>> >> > >> >> - /** Free the channel records off > >> >> >> this lga_hdl */ > >> >> >> >>> >> > >> >> + > >> >> >> >>> >> > >> >> + /* > >> >> >> Free the channel > >> >> >> >>> >> > >> >> records off this lga_hdl */ > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> lga_log_stream_hdl_rec_list_del(&rm_node- > >> >> >> >stream_list); > >> >> >> >>> >> > >> >> - > >> >> >> >>> >> > >> >> - /** free the hdl rec */ > >> >> >> >>> >> > >> >> - > >> >> >> free(rm_node); > >> >> >> >>> >> > >> >> + > >> >> >> >>> >> > >> >> free_client_hdl(&rm_node); > >> >> >> >>> >> > >> >> > >> >> >> >>> >> > >> >> rc = > >> >> >> >>> >> > >> >> NCSCC_RC_SUCCESS; > >> >> >> >>> >> > >> >> goto > >> >> >> out; > >> >> >> >>> >> > >> > >> >> >> >>> >> > > >> >> >> >>> > >> >> >> > >> >> > > ------------------------------------------------------------------------------ Find and fix application performance issues faster with Applications Manager Applications Manager provides deep performance insights into multiple tiers of your business applications. It resolves application problems quickly and reduces your MTTR. Get your free trial! https://ad.doubleclick.net/ddm/clk/302982198;130105516;z _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
