On 03/17/10 11:46 AM, Thomas Whitten wrote: ... > Antonello Cruz writes: ... >> usr/src/cmd/svc/configd/backend.c >> flight_recorder_event() >> The comments for flight_recorder_event() advises to take a gcore >> of configd to look into the events with mdb. Why not extend >> >> usr/src/cmd/mdb/common/modules/svc.configd/configd.c >> >> to display that information? >> It could be a follow up RFE if schedule is tight. > > I'd like to do that. As you suggest it has been a matter of time. I will > file an RFE. Thanks.
>> >> So flight_recorder_missed counts failed attempts to grab the >> lock, not the number of events overwritten. What kind of errors >> are you expecting from a pthread_mutex_t initialized with the >> defaults? > > I'm not expecting any errors, but it wasn't clear from my reading of the > man page that I wouldn't get any. Perhaps ENOMEM. I also think the only error you could get is ENOMEM. More below. >> >> The mutex synchronizes access to the counters only? What about >> the be_flight_event_t data itself? Doesn't it need to be >> synchronized? > > The locks protect the selection of which be_flight_event_t we are going to > write by setting item while under lock. The only potential problem is if > we completely wrap around to the selection before the update is completed. > This is unlikely, but possible if as you say below we have a large number > of threads. To fix this, I think that I would like to get rid of the wrap > around all together. It is the early boot events that are the most > interesting and hardest to capture without the flight recorder, so I don't > think that over writing them makes sense. Once the system is up and > running there are other ways to capture information. Getting rid of the wrap around will bring sense, at least in my perspective, to flight_recorder_missed. Is there any operation that would generate flight recorder events once emi and lmi are finished? wouldn't restarting manifest import generate more events? What about a sysadmin creating a repository backup? I am just trying to understand if it makes sense to keep logging these events or not. If not, you should probably make it a no op when you reach MAX_FLIGHT_RECORDER_EVENTS. > >> >> You are obviously not expecting configd to have more threads >> than MAX_FLIGHT_RECORDER_EVENTS. Are configd number of threads >> bounded somehow? > > No. The number of theads is not capped. Thanks for answering. >> >> 71: either IS_VOLATILE(be) has its logic reversed or the test should >> be ((be)->be_ppath == NULL). The comment on line 103 says >> be_ppath is /* path to persistent db */. In this case the answer >> to IS_VOLATILE(be) should be false. Maybe I am confused... >> Please clarify. > > be_ppath is used to hold the path to persistent storage, but only > temporarily. It is used to hold the path to persistent storage when we > have moved the repository to volatile storage. > > Here is how it works. be_path contains the path to the currently open > repository. When configd starts up it opens the persistent repository (in > RO mode if we're on UFS), and sets be_path to point to the persistent > repository. At this point be_ppath is NULL. > > When we receive the command to copy the repository to volatile storage, we > save the path to the persistent db in be_ppath, open the volatile storage > and save the path to volatile storage in be_path. > > When we get the command to move the repository back to permanent storage we: > - copy the data back to a temporary file on persistent storage > - rename the file to the path held in be_ppath > - set be_path back to the name of the persistent db > - open the persistent db for our use > - set be_ppath to NULL Thanks for explaining it. It makes sense now. > > Any suggestions on improving the comments in 92-96? I have two suggestions: In the block 92-96 you could say somewhere that when the BE (backend) points to a volatile storage, be_ppath points to its original persistent location. Otherwise it is NULL. Also, in line 103, you could add: when backend is volatile > >> >> 1466-1467: It was easier to understand remove_src after I read the >> old comment block. However in the new code, I think explaining >> sw_back in backend_switch() would do a favor to someone reading the >> code. > > How about this comment for backend_copy_repository()? > > /* > * This function makes a copy of the repository at src, placing the copy at > * dst. It is used to copy a repository on permanent storage to volatile > * storage or vice versa. If the source file is on volatile storage, it is > * often times desirable to delete it after the copy has been made and > * verified. To remove the source repository, set remove_src to 1. > */ It is good. Thanks. > > As you suggest, I'll also document sw_back in backend_switch(). OK. > >> >> 1758-1761: you use the variable pres only to hold the result of >> backend_copy_repository() for the following if. You could just >> compare the return value of the function instead. >> > Agreed. Will do. Thanks. > >> 2672: is FAST_REPOSITORY_DB persistent? if not, why be->be_ppath is >> assigned to it? I guess this is because of my interpretation >> of IS_VOLATILE(be) and be_ppath. > > FAST_REPOSITORY_DB is not persistent. In 2672, be_ppath is being used to > save the name of the persistent reposity from be->be_path. In 2673 be_path > is then overwritten with FAST_REPOSITORY_DB. Your explanation above clarified it. Thanks. Antonello > >> >> >> usr/src/cmd/svc/milestone/emi.xml >> OK >> >> usr/src/cmd/svc/shell/netservices.sh >> OK >> >> usr/src/lib/libscf/inc/libscf_priv.h >> OK >> >> >> On 03/ 9/10 06:30 PM, Tony Nguyen wrote: >>> An updated webrev is generated to reflect the comments we've had thus far: >>> >>> http://cr.opensolaris.org/~tonyn/emi_mar_08_2010/ >>> >>> The last webrev is still available at >>> >>> http://cr.opensolaris.org/~tonyn/EMI_webrev >>> >>> Thanks, >>> tn >>> >>>> Subject: Early Manifest Import - code review request >>>> Date: Fri, 05 Feb 2010 14:21:46 -0800 >>>> From: Tony Nguyen<Truong.Q.Nguyen at sun.com> >>>> To: Antonello Cruz<Antonello.Cruz at Sun.COM>, Liane Praza >>>> <Liane.Praza at Sun.COM>, Susan Kamm-Worrell >>>> <Susan.Kamm-Worrell at Sun.COM>, Jean McCormack<Jean.McCormack at Sun.COM>, >>>> smf-discuss<smf-discuss at opensolaris.org> >>>> >>>> >>>> >>>> We completed testing and fixed all found bugs. We'd appreciate feedback >>>> by Feb 19th as that would allow us to turnaround comments and still make >>>> the scheduled integration date. >>>> >>>> Webrev: >>>> http://cr.opensolaris.org/~tonyn/EMI_webrev/ >>>> >>>> Design Doc: >>>> http://hub.opensolaris.org/bin/download/Community+Group+smf/smf_design_docs/emidesign.html >>>> >>>> >>>> >>> >>> >> _______________________________________________ >> smf-discuss mailing list >> smf-discuss at opensolaris.org
