Re: [Xen-devel] [PATCH v3 10/15] libxl: make libxl_cd_insert "eject" + "insert"

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Sep 09, 2014 at 12:30:33PM +0100, Ian Campbell wrote:
[...]
> > +    lock = libxl__lock_domain_userdata(gc, domid);
> > +    if (!lock) {
> > +        rc = ERROR_LOCK_FAIL;
> > +        goto out;
> > +    }
> > +    rc = libxl__get_domain_configuration(gc, domid, &d_config);
> > +    if (rc) goto out;
> > +    DEVICE_ADD(disk, disks, domid, disk, COMPARE_DISK, &d_config);
> > +    rc = libxl__set_domain_configuration(gc, domid, &d_config);
> > +    if (rc) goto out;
> > +    libxl__unlock_domain_userdata(lock);
> > +
> > +    rc = libxl__cdrom_insert(ctx, domid, disk, ao_how);
> > +
> > +out:
> > +    libxl_device_disk_dispose(&empty);
> > +    libxl_domain_config_dispose(&d_config);
> 
> This code doesn't seem to adhere to the protocol laid down in the
> previous patch and I suspect it therefore isn't safe against parallel
> eject/insert calls.
> 
>       INSERTION A             INSERTION B
> 
>       takes lock
>       updates json
>       releases lock
> 
>                               takes lock
>                               updates json
>                               releases lock
> 
>                               adds to XS
> 
>       adds to XS
> 
> Now the json == B and the xenstore == A.
> 
> I think you need to push the json update down into libxl__cdrom_insert
> and do the say dance you do elsewhere, with an XS transaction +
> existence check inside the userdata locked region.
> 

After discussing with Ian J IRL I come up with an approach which I will
detail below.

There should not be an existence check because we're not adding a new
device, we're just updating some nodes.  We would still need to stick
with the approach, to make "cdrom insert" "eject + insert".

Here are some background information:

Back in June Ian J and I had a discussion on how to deal with removable
disk. He said it was impossible to convert xenstore backend information
back to libxl structure as there's information lost during transaction
from libxl structure to xenstore entry. One being that backend_domname
and backend_domid. User can specify either backend_domname or
backend_domid in diskspec (either specifying both is a bug or we need to
make one take precedence over the other). I think the same theory
applies to other devices as well. Even if we can convert evething back
now, it doesn't necessary mean we can in the future. This means we
basically need to rely on JSON entries to be the final version we want
to use to reconstruct a domain.

For a removable device, currently user can just "swap" media in drive
from one to another (of course there can be other changes behind the
scene that might affect other nodes in xenstore). Plus the fact that we
update JSON first then commit xenstore changes, if xenstore commit fails
the media in JSON is not the same one in xenstore, and the one in
xenstore is the domain actually sees. This becomes a corner case.
We cannot use JSON entry as template anymore.

To fix this corner case, we need to introduce an intermediate empty
state. Transition from original state to empty state should not follow
the usual protocol (JSON then xenstore). Instead we only update xenstore
but not JSON, then in the following update which writes in the disk user
supplied we follow protocol.

    lock json
    write empty to xenstore
    // usual protocol follows
    for () {
      update JSON
      write disk to xenstore
    }
    unlock json

So we ensure that if the second xenstore write ever fails we still have
a known state in xenstore (which is empty state). Without the empty
state, we can end up with a situation that JSON and xenstore disagree
with each other and we don't know which one is correct.

In later retrieval, read "disk" from xenstore:

    if (disk is removable && disk is empty)
        disk is empty
    else
        use JSON version

Does this make sense?

Wei.

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