From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Liu Subject: Re: [PATCH v3 09/15] libxl: synchronise configuration when we hotplug a device Date: Tue, 9 Sep 2014 14:37:49 +0100 Message-ID: <20140909133749.GD24977@zion.uk.xensource.com> References: <1409870601-7538-1-git-send-email-wei.liu2@citrix.com> <1409870601-7538-10-git-send-email-wei.liu2@citrix.com> <1410261112.8217.126.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1410261112.8217.126.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: ian.jackson@eu.citrix.com, Wei Liu , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Tue, Sep 09, 2014 at 12:11:52PM +0100, Ian Campbell wrote: [...] > > + * All device hotplug routines should comply to following pattern: > > + * lock json config (json_lock) > > + * read json config > > + * update in-memory json config with new entry, rewrite > > + * if there's stale entry > > Does "rewrite" here mean back to disk, or just the in-memory copy? I > think it is important for the protocol that it is the in-memory one > only. > > I think you can just say "replacing any stale entry". It means replacing the entry in in-memory copy. I will rephrase this line. > > > + * for loop -- xs transaction > > + * open xs transaction > > + * check device existence, abort if it exists > > + * write in-memory json config to disk > > + * commit xs transaction > > + * end for loop > > + * unlock json config > > + * > > + * Device removal routines are not touched. > > + * > > + * Here is the proof that we always maintain that invariant and we > > + * don't leak files during interaction of hotplug thread and other > > + * threads / processes. > > + * > > + * # Safe against parallel add > > + * > > + * When another thread / process tries to add same device, it's > > + * blocked by json_lock. The loser of two threads will bail at > > + * existence check, so that we don't overwrite anything. > > + * > > + * # Safe against domain destruction > > + * > > + * When another thread / process tries to destroy domain, it's blocked > > + * by json_lock. If domain destruction thread is loser, it deletes > > + * every userdata file after it requires the lock. If hotplug thread > > + * is loser, it bails at acquiring lock, no device is added. Either > > + * way, no file is leaked. > > I don't follow this one. > > For the destructor loses case I think all you need to say is that the > json lock prevents the destruction process from removing the userdata > while the add is ongoing, the reference to deleting userdata after it > acquires the lock is just confusing. > > In the "hotplug thread loses" case I think you should explain why it > bails (the existence check I suppose). > How about this: If the thread / process trying to destroy domain loses the rase, it's blocked by json_lock. If the hotplug thread is loser, it bails at acquiring lock because lock acquisition function checks existence of the domain. Re typos, I will fix them in next round. Wei.