[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v1 03/10] libxl: store a copy of vanilla domain configuration when creating domain
Wei Liu writes ("Re: [PATCH v1 03/10] libxl: store a copy of vanilla domain configuration when creating domain"): > On Thu, Jul 24, 2014 at 07:52:46PM +0100, Ian Jackson wrote: > > This race actually exists in libxl_userdata_store so it is my fault. > > Sorry! > > No worries. I can fix this. Thanks. > > I think this should be fixed as follows: > > > > * domain destruction should take a lock across deleting the > > userdata and destroying the domain in Xen > > > > * libxl_userdata_store should take take the lock, > > check domain exists, store userdata, unlock lock. > > > > This would be a separate lock to the one you introduce, I think. > > Your lock covers a lot of xenstore processing. But perhaps we can > > reuse some of the fcntl and lockfile massaging stuff. > > > > Why do we need a second lock? The one I introduced in previous patch is > used to serialise access to user data. Isn't that just what we want in > this case? I think you could use the same lock. I suggested otherwise because the xenstore manipulations might hold the lock for a bit longer than ideal but today that doesn't seem so important. (Sorry if I seem rather vague. I have a head cold at the moment and am quite confused.) > > > + /* At this point we've got domid and UUID, store configuration */ > > > + ret = libxl__set_domain_configuration(gc, domid, &d_config_saved); > > > > This libxl__set_domain_configuration is the only thing which creates > > the libxl-json userdata ? If so I think there is no > > -EPARSE. :-( You can forget about that comment. I was writing about the race I mentioned further up, and forgot to come back and delete this after I had added the other text to my mail. > > > + /* update network interface information */ ... > > Does creating the config early, and then updating it, not mean that > > there will be a window during domain creation when configuration > > exists but lacks these updates ? > > > > I think that might be a (theoretically application-visible) bug. > > Yes, there's such windows. > > Ian C suggested I add lock during creation, which I think should fix > this problem. I don't think that's a particularly good idea. Such a lock would presumably cover most of the domain creation work (hotplug scripts etc) which might take a long time. During that time various operations would block unreasonably. Also, I think you mustn't use an fcntl lock across ao operation callback chains. fcntl locks do not exclude other threads in the same process. > > If they do you can replace this whole block with a one-line > > if (rc) goto out; > > > > > + callback(gc, &d_config_saved, src); > > > > Callback should probably have the ability to fail. > > I guess you mean I should write > if (callback(XXX)) > > This of course can be done. I was thinking more rc = callback(...); if (rc) goto out :-). (Unless you are sure that none of these callbacks will ever want to fail...) Thanks, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |