Dan, Thanks for your comments. Responses are inline below.
Steve Dan Price wrote: > On Thu 06 Dec 2007 at 02:05PM, Steve Peng wrote: > >> Please code review the following changes which address the service >> import performance issue: >> >> 6351623 Initial manifest-import is slow >> >> It is under http://cr.opensolaris.org/~stevep/6351623 >> >> The fix is to copy the repository to the fast tmpfs, import the >> manifests and once the import is completed copy the repository >> back to /etc/svc. During the verification cycle on both sparc >> and amd64 systems, I see ~85 percent performance improvement. >> >> Any comment/suggestion is greatly appreciated. >> >> Thanks >> > > Hurray! This is going to make a huge difference for users of zones, > who hit the slow repo import problem fairly often. As such, I took > a look at the code. Mostly, I have nits. See below. > > -dp > > > usr/src/cmd/svc/configd/backend.c > > --- > 1131 + /* > 1132 + * Coping > 1133 + */ > > I think you mean "Copying"? It might be nice to say a little more. > Yes and agree that it needs to say more than just a word. > --- > > 1121 + configd_critical("Backend copy faild: mkstemp > %s\n", tmppath); > > Do you mean "failed"? "faild" appears three or four times. > Updated. > --- > > 1155 + /* > 1156 + * Rename files. Note the following rename ops are done > 1157 + * "atomically": > > "atomically" does not need to be quoted. > Done. > --- > 1155 + /* > 1156 + * Rename files. Note the following rename ops are done > 1157 + * "atomically": > 1158 + * - unlink target > 1159 + * - link source to target > 1160 + * - unlink source > 1161 + * > 1162 + * Extra works needed if we switch back from tmpfs > 1163 + */ > > "works" --> work > Done. > Also, this is a bit confusing in the wording... is the comment really in > sync with the code? Do you mean to say that the whole operation proceeds > atomically? Or that each of the individual operations is atomic? Or > something > else? > Actually it has nothing to do with the code so I think I should take them out. Initially I put that in to remind myself of the fact that the rename operation will not get into the situation likes: - both the source and target exist when the target does not exist from the beginning. or - the target content is updated partially I go ahead remove the comment to avoid confusion. > It seems like this code could leak files if parts of it fail-- for example, > if the code at 1180 succeeds (tmppath renames to "trans_db"), but the code > at 1188 fails, won't "trans_db" be leaked? > > Or, if 1120 (mkstemp) succeeds, but 1125 (open) fails, won't we leak > the temp file? > --- > Ah! yes those should be unlinked. > backend_switch() > > 1250 + * If copy fails for any reason and we are switching > 1251 + * back then we should go ahead fall back to the > 1252 + * original repository since we don't want to stay > 1253 + * with tmpfs for too long > 1254 + */ > > Run-on sentence (also, please end with a period). I don't really know > what you mean by "stay with tmpfs for too long"? > This is when we are ready to copy the repository back to /etc/svc from the tmpfs area. If the copy operation fails then we like to switch back the permanent repository under /etc/svc as quickly as possible to minimize the window of losing customizations in the event of the system crash. > --- > backend_switch() > > Are there some missing calls to sqllite_close in this routine? I'm not > really familiar with the sqllite API. > Doesn't look like. > --- > backend_switch_recovery() > > I don't totally understand what is going on here. The comments should perhaps > explain the "unlink" that happens at the end of the routine. It seems to me > that if things go wrong (like you can't open the trans db) the code doesn't > say > anything about that, and winds up deleting any evidence we might use for > debugging... > So those transient files should not exist if - the switch operation runs to completion w/o any problem, or - the operation fails and are unlinked Those transient files may exist if the system crashes or svc.configd crashes and restarts during the switch operation. Since they can contain most recent useful information, we like to try to recover it. If those transient files fail the integrity check then we like to unlink them and if they pass the check then the unlink at the end has no effect. Dose it make sense? > Also this routine also seems a bit more complex than is needed... > Couldn't this be recoded as follows? > > db = NULL; > ... > > if (stat(fast_db, &s_buf) == 0) > db = fast_db; > > if (stat(trans_db, &s_buf) == 0) > db = trans_db; > > if (db == NULL) > return; > > be_db = sqlite_open(db, 0600, &errp); > ... > Sounds good. Updated. > --- > > 2158 + * If system crashes while the beakend switch is performed > > nit: backend > > 2160 + * which may contain most recent data so attemp recovery > > nit: attempt > > nit: please put a period at the end of sentences. > > Consider rewriting: > > If the system crashed during a backend switch, there might > be a leftover transient database containing useful information > which can be used for recovery. > Updated. > > Also: > > 2162 + (void) backend_switch_recovery(); > > The function is declared void, so the cast is not needed. > Updated. > > --- > > I'm curious about how you have stress tested this? A zone in a reboot > loop could be useful here, I'd think. > Or 'svcadm _smf_repository_switch fast/perm' loop as mentioned in Stephen email. > -dp > >
