Re: [Xen-devel] [PATCH v6 06/13] libxl: convert libxl_device_disk_add to an async op

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("Re: [PATCH v6 06/13] libxl: convert 
libxl_device_disk_add to an async op"):
> Ian Jackson wrote:
> > Roger Pau Monne writes ("[PATCH v6 06/13] libxl: convert libxl_device_disk_a
> > Is it really safe to call libxl__device_disk_add without a real
> > transaction ?  I see that the current code does it but it seems wrong
> > to me.  We end up writing all of the individual settings in the
> > backend one at a time.
> 
> Well, this is not really my code, this was part of Stefanos series about 
> local disk attach. However, I don't see how we end up writing them one 
> at a time, libxl__device_generic_add creates a transaction if none is 
> given, so all backend and frontend entries are added at the same 
> transaction. Calling libxl__device_disk_add without a transaction 
> behaves just like it did previously.

Oh, right, that's OK then.  Sorry for missing that.

> > I think this applies to all the other device types too.  So I think
> > the right answer would be to make them take a transaction parameter
> > too.  Ie, insert a patch before this one which adds a transaction
> > parameter (ignored for now and always passed 0 if you don't want to
> > actually make it work properly) to libxl__add_nic etc.  Then you don't
> > need this unpleasant macro.
> 
> Ok, I will add this patch that makes all device_*_add take a transaction 
> parameter, although it will be ignored right now.

Right.  (You could pass it through to device_generic_add if that was
easy.)

> >> +void libxl__initiate_device_connection(libxl__egc *egc, libxl__ao_device 
> >> *aodev)
> >> +{
> >> +    STATE_AO_GC(aodev->ao);
> >> +    char *be_path = libxl__device_backend_path(gc, aodev->dev);
> >> +    char *state_path = libxl__sprintf(gc, "%s/state", be_path);
> >> +    int rc = 0;
> > ...
> >> +out_fail:
> >> +    assert(rc);
> >> +    aodev->rc = rc;
> >> +    device_xsentries_remove(egc, aodev);
> >> +    return;
> >> +}
> >
> > Firstly, I'm not convinced it's really proper for
> > libxl__initiate_device_connection to call device_xsentries_remove.
> > After all device_xsentries_remove is part of the implementation of
> > libxl__initiate_device_remove.
> 
> This is part of both flows, both device connection and disconnection.

Well then it should have a proper description and a better name,
perhaps ?  As it is it looks like you're doing what amounts to "goto"
from one "function" to another - only we don't notice it like that
because it's split into multiple ao callbacks.

> > And, secondly, does device_xsentries_remove really do what you want ?
> > It has a test in it which makes it only do its work if action is
> > DISCONNECT.
> 
> Yes, that's because it's called by both flows.

If it is called in the connect flow, won't it be a no-op then ?
Perhaps I'm being excessively dense here.


> > Isn't the effect of this that if the xs transaction gets a conflict,
> > we'll rerun the hotplug scripts, etc. ?  I think I may be confused
> > here, but I don't understand how this transaction loop is supposed to
> > work and how it is supposed to relate to the interaction with other
> > domains, scripts, etc.
> 
> Yes I see your point. We should disconnect the device (execute hotplug 
> scripts) but since the xenstore entries are already gone (because the 
> transaction is not committed successfully) I don't see anyway to do it, 
> we cannot execute those scripts if the backend entries have been lost.
> 
> > Indeed it seems to me like your arrangements in libxl__device_disk_add
> > couldn't work if a non-null t was supplied.  libxl__device_disk_add
> > would do all the writes in the transaction, but not commit it, so that
> > none of them are visible to anyone, and then wait for the deivde state
> > to change, which will never happen.
> 
> I'm not so sure, is it possible to add a watch to a xenstore path that 
> is inside of a not yet committed transaction? If so this will work fine, 
> the transaction will be finished correctly, and the path will be updated 
> to the correct state (2).

AIUI you can add a watch for any path that you like - the path you ask
to watch doesn't have to exist.  But you won't (necessarily) see
events for uncommitted transactions.

> If the transaction is not committed successfully, we will get a timeout 
> from the devstate event and exit.

I think that the only way all this could work is if you firstly, in
one transaction, create enough of the backend for the hotplug scripts
to run, and then in a second transaction create the rest of the
backend plus the frontend.

> > I think this code is a symptom of a problem elsewhere.  When adding a
> > disk to a domain, it should not be necessary to explicitly ask to add
> > it to the stubdom too.  But this is not your fault, or your problem
> > right now.
> 
> So I assume we can leave this for later.

Yes.

Thanks,
Ian.

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