Re: [Xen-devel] [PATCH v3 09/15] libxl: synchronise configuration when we hotplug a device

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-09-04 at 23:43 +0100, Wei Liu wrote:

> Please see comment in libxl_internal.h for correctness proof.

I'm just going to comment on that on this first pass.

> +    /* If we need to udpate JSON config */

"update"

> +    bool update_json;
>  };
>  
>  /*
> @@ -2271,6 +2273,78 @@ struct libxl__multidev {
>   *
>   * Once finished, aodev->callback will be executed.
>   */
> +/*
> + * As of Xen 4.5 we maintain various infomation, including hotplug
> + * device information, in JSON files, so that we can use this JSON
> + * file as a template to reconstruct domain configuration.
> + *
> + * In essnse there are now two views of device state, one is xenstore,

"essence"

> + * the other is JSON file. We use xenstore as primary reference.
> + *
> + * Here we maintain one invariant: every device in xenstore must have
> + * an entry in JSON file.
> + *
> + * 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".

> + *       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).

> + * # Safe against parallel removal
> + *
> + * When another thread / process tries to remove a device, it's _NOT_
> + * blocked by json_lock, but xenstore transaction can help maintain
> + * invariant. The removal threads either a) sees that device in
> + * xenstore, b) doesn't see that device in xenstore.
> + *
> + * In a), it sees that device in xenstore. At that point hotplug is
> + * already finished (both JSON and xenstore change committed). So that
> + * device can be safely removed. JSON entry is left untouched and
> + * becomes stale, but this is a valid state -- next time when a
> + * device with same identifier gets added, the stale entry gets
> + * overwritten.
> + *
> + * In b), it doesn't see that device in xenstore, but it will commence
> + * anyway. Eventually a force removal is initiated, which will forcely

forcibly.

> + * remove xenstore entry.
> + *
> + * If hotplug threads creates xenstore entry (therefore JSON entry as
> + * well) before force removal, that xenstore entry is removed. We're
> + * left with JSON stale entry but not xenstore entry, which is a valid
> + * state.
> + *
> + * If hotplug thread has not created xenstore entry when the removal
> + * is committed, we're obviously safe. Hotplug thread will add in
> + * xenstore entry afterwards. We have both JSON and xenstore entry,
> + * it's a valid state.
> + */

Despite my concerns with the description of one case I think it is good
enough that I can go look at the code now.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel