diff mbox series

[6/9] libxl_disk: Cut libxl_cdrom_insert into steps ..

Message ID 20190409164542.30274-7-anthony.perard@citrix.com (mailing list archive)
State Superseded
Headers show
Series libxl: New slow lock + fix libxl_cdrom_insert with QEMU depriv | expand

Commit Message

Anthony PERARD April 9, 2019, 4:45 p.m. UTC 
.. and use a new "slow" lock.

This patch cut libxl_cdrom_insert into different step/function but there
are still called synchronously. A later patch will call them
asynchronously when QMP is involved.

The json_lock has been replaced by the qmp_lock for protection against
concurrent changes to the cdrom. The json_lock is now only used when
reading/modifying the domain userdata.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_disk.c | 172 ++++++++++++++++++++++++++++++---------
 1 file changed, 135 insertions(+), 37 deletions(-)

Comments

Ian Jackson June 4, 2019, 5:29 p.m. UTC | #1 
Anthony PERARD writes ("[PATCH 6/9] libxl_disk: Cut libxl_cdrom_insert into steps .."):
> This patch cut libxl_cdrom_insert into different step/function but there
> are still called synchronously. A later patch will call them
> asynchronously when QMP is involved.
> 
> The json_lock has been replaced by the qmp_lock for protection against
> concurrent changes to the cdrom. The json_lock is now only used when
> reading/modifying the domain userdata.


Your documentation for the qmp lock, taken roughly from my idea, is
that the qmp lock covers modifying things via qmp.  But I think here
you use it for updates via xenstore ?

I think this is OK because what matters is that any one thing is
always covered by the same lock - and here the cdrom is is a "thing".
But I think this means the commentary is wrong ?


> -    /* Note: CTX lock is already held at this point so lock hierarchy
> -     * is maintained.
> -     */
> -    lock = libxl__lock_domain_userdata(gc, domid);
> -    if (!lock) {
> +    cis->qmp_lock = libxl__lock_domain_qmp(gc, domid);

I think this is in fact precisely backwards.  The lock hierarchy is
*violated* here.  The qmp lock is supposed to be outside the ctx lock.

There will have to be an entrypoint for taking the qmp lock which
takes a gc and which makes a callback later.


AFAICT apart from these two aspects the rest of this code
reorganisation is good.

Thanks,
Ian.
Anthony PERARD June 7, 2019, 5:22 p.m. UTC | #2 
On Tue, Jun 04, 2019 at 06:29:14PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH 6/9] libxl_disk: Cut libxl_cdrom_insert into steps .."):
> > This patch cut libxl_cdrom_insert into different step/function but there
> > are still called synchronously. A later patch will call them
> > asynchronously when QMP is involved.
> > 
> > The json_lock has been replaced by the qmp_lock for protection against
> > concurrent changes to the cdrom. The json_lock is now only used when
> > reading/modifying the domain userdata.
> 
> 
> Your documentation for the qmp lock, taken roughly from my idea, is
> that the qmp lock covers modifying things via qmp.  But I think here
> you use it for updates via xenstore ?

That xenstore code update is kind of weird, libxl writes it but nothing
really reads it.

For the eject part of the xenstore update, it probably doesn't matter if
the json_lock is held or not. But for the insert part of the xenstore
update, libxl writes the domain config while in the middle of the xenstore
transaction. So in the second case, the json_lock needs to be held.

> I think this is OK because what matters is that any one thing is
> always covered by the same lock - and here the cdrom is is a "thing".
> But I think this means the commentary is wrong ?

I can try to rewrite the commentary to make it less wrong. Or change the
code so the comment applies to it. (I'll go with rewriting the patch
description for now.)
diff mbox series

Patch

diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
index f7a1b75ae1..14dfc67971 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -661,24 +661,38 @@  int libxl_device_disk_getinfo(libxl_ctx *ctx, uint32_t domid,
     return rc;
 }
 
+typedef struct {
+    libxl__ao *ao;
+    libxl_domid domid;
+    libxl_device_disk *disk;
+    libxl_device_disk disk_saved;
+    libxl__domain_qmp_lock *qmp_lock;
+    int dm_ver;
+} libxl__cdrom_insert_state;
+
+static void cdrom_insert_ejected(libxl__egc *egc,
+                                 libxl__cdrom_insert_state *cis);
+static void cdrom_insert_inserted(libxl__egc *egc,
+                                  libxl__cdrom_insert_state *cis);
+static void cdrom_insert_done(libxl__egc *egc,
+                              libxl__cdrom_insert_state *cis,
+                              int rc);
+
 int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
                        const libxl_asyncop_how *ao_how)
 {
     AO_CREATE(ctx, domid, ao_how);
     int num = 0, i;
-    libxl_device_disk *disks = NULL, disk_saved;
-    libxl_domain_config d_config;
-    int rc, dm_ver;
-    libxl__device device;
-    const char *be_path, *libxl_path;
-    char * tmp;
-    libxl__domain_userdata_lock *lock = NULL;
-    xs_transaction_t t = XBT_NULL;
-    flexarray_t *insert = NULL, *empty = NULL;
+    libxl_device_disk *disks = NULL;
+    int rc;
+    libxl__cdrom_insert_state *cis;
 
-    libxl_domain_config_init(&d_config);
-    libxl_device_disk_init(&disk_saved);
-    libxl_device_disk_copy(ctx, &disk_saved, disk);
+    GCNEW(cis);
+    cis->ao = ao;
+    cis->domid = domid;
+    cis->disk = disk;
+    libxl_device_disk_init(&cis->disk_saved);
+    libxl_device_disk_copy(ctx, &cis->disk_saved, disk);
 
     libxl_domain_type type = libxl__domain_type(gc, domid);
     if (type == LIBXL_DOMAIN_TYPE_INVALID) {
@@ -697,8 +711,8 @@  int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         goto out;
     }
 
-    dm_ver = libxl__device_model_version_running(gc, domid);
-    if (dm_ver == -1) {
+    cis->dm_ver = libxl__device_model_version_running(gc, domid);
+    if (cis->dm_ver == -1) {
         LOGD(ERROR, domid, "Cannot determine device model version");
         rc = ERROR_FAIL;
         goto out;
@@ -727,17 +741,8 @@  int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         disk->format = LIBXL_DISK_FORMAT_EMPTY;
     }
 
-    rc = libxl__device_from_disk(gc, domid, disk, &device);
-    if (rc) goto out;
-
-    be_path = libxl__device_backend_path(gc, &device);
-    libxl_path = libxl__device_libxl_path(gc, &device);
-
-    /* Note: CTX lock is already held at this point so lock hierarchy
-     * is maintained.
-     */
-    lock = libxl__lock_domain_userdata(gc, domid);
-    if (!lock) {
+    cis->qmp_lock = libxl__lock_domain_qmp(gc, domid);
+    if (!cis->qmp_lock) {
         rc = ERROR_LOCK_FAIL;
         goto out;
     }
@@ -746,7 +751,7 @@  int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
      * by inserting empty media. JSON is not updated.
      */
 
-    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+    if (cis->dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
         libxl_device_disk disk_empty;
 
         libxl_device_disk_init(&disk_empty);
@@ -761,6 +766,48 @@  int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         if (rc) goto out;
     }
 
+    rc = 0;
+
+out:
+    libxl__device_list_free(&libxl__disk_devtype, disks, num);
+    if (rc) {
+        cdrom_insert_done(egc, cis, rc); /* must be last */
+    } else {
+        cdrom_insert_ejected(egc, cis); /* must be last */
+    }
+    return AO_INPROGRESS;
+}
+
+static void cdrom_insert_ejected(libxl__egc *egc,
+                                 libxl__cdrom_insert_state *cis)
+{
+    EGC_GC;
+    int rc;
+    libxl__domain_userdata_lock *data_lock = NULL;
+    libxl__device device;
+    const char *be_path, *libxl_path;
+    flexarray_t *empty = NULL;
+    xs_transaction_t t = XBT_NULL;
+    char *tmp;
+    libxl_domain_config d_config;
+
+    /* convenience aliases */
+    libxl_domid domid = cis->domid;
+    libxl_device_disk *disk = cis->disk;
+
+    libxl_domain_config_init(&d_config);
+
+    rc = libxl__device_from_disk(gc, domid, disk, &device);
+    if (rc) goto out;
+    be_path = libxl__device_backend_path(gc, &device);
+    libxl_path = libxl__device_libxl_path(gc, &device);
+
+    data_lock = libxl__lock_domain_userdata(gc, domid);
+    if (!data_lock) {
+        rc = ERROR_LOCK_FAIL;
+        goto out;
+    }
+
     empty = flexarray_make(gc, 4, 1);
     flexarray_append_pair(empty, "type",
                           libxl__device_disk_string_of_backend(disk->backend));
@@ -799,16 +846,66 @@  int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     rc = libxl__get_domain_configuration(gc, domid, &d_config);
     if (rc) goto out;
 
-    device_add_domain_config(gc, &d_config, &libxl__disk_devtype, &disk_saved);
+    device_add_domain_config(gc, &d_config, &libxl__disk_devtype,
+                             &cis->disk_saved);
 
     rc = libxl__dm_check_start(gc, &d_config, domid);
     if (rc) goto out;
 
-    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+    if (cis->dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
         rc = libxl__qmp_insert_cdrom(gc, domid, disk);
         if (rc) goto out;
     }
 
+    rc = 0;
+
+out:
+    libxl__xs_transaction_abort(gc, &t);
+    libxl_domain_config_dispose(&d_config);
+    if (data_lock) libxl__unlock_domain_userdata(data_lock);
+    if (rc) {
+        cdrom_insert_done(egc, cis, rc); /* must be last */
+    } else {
+        cdrom_insert_inserted(egc, cis); /* must be last */
+    }
+}
+
+static void cdrom_insert_inserted(libxl__egc *egc,
+                                  libxl__cdrom_insert_state *cis)
+{
+    EGC_GC;
+    int rc;
+    libxl__domain_userdata_lock *data_lock = NULL;
+    libxl_domain_config d_config;
+    flexarray_t *insert = NULL;
+    xs_transaction_t t = XBT_NULL;
+    libxl__device device;
+    const char *be_path, *libxl_path;
+    char * tmp;
+
+    /* convenience aliases */
+    libxl_domid domid = cis->domid;
+    libxl_device_disk *disk = cis->disk;
+
+    libxl_domain_config_init(&d_config);
+
+    rc = libxl__device_from_disk(gc, domid, disk, &device);
+    if (rc) goto out;
+    be_path = libxl__device_backend_path(gc, &device);
+    libxl_path = libxl__device_libxl_path(gc, &device);
+
+    data_lock = libxl__lock_domain_userdata(gc, domid);
+    if (!data_lock) {
+        rc = ERROR_LOCK_FAIL;
+        goto out;
+    }
+
+    rc = libxl__get_domain_configuration(gc, domid, &d_config);
+    if (rc) goto out;
+
+    device_add_domain_config(gc, &d_config, &libxl__disk_devtype,
+                             &cis->disk_saved);
+
     insert = flexarray_make(gc, 4, 1);
     flexarray_append_pair(insert, "type",
                       libxl__device_disk_string_of_backend(disk->backend));
@@ -849,21 +946,22 @@  int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         if (rc < 0) goto out;
     }
 
-    /* success, no actual async */
-    libxl__ao_complete(egc, ao, 0);
-
     rc = 0;
 
 out:
     libxl__xs_transaction_abort(gc, &t);
-    libxl__device_list_free(&libxl__disk_devtype, disks, num);
-    libxl_device_disk_dispose(&disk_saved);
     libxl_domain_config_dispose(&d_config);
+    if (data_lock) libxl__unlock_domain_userdata(data_lock);
+    cdrom_insert_done(egc, cis, rc); /* must be last */
+}
 
-    if (lock) libxl__unlock_domain_userdata(lock);
-
-    if (rc) return AO_CREATE_FAIL(rc);
-    return AO_INPROGRESS;
+static void cdrom_insert_done(libxl__egc *egc,
+                              libxl__cdrom_insert_state *cis,
+                              int rc)
+{
+    if (cis->qmp_lock) libxl__unlock_domain_qmp(cis->qmp_lock);
+    libxl_device_disk_dispose(&cis->disk_saved);
+    libxl__ao_complete(egc, cis->ao, rc);
 }
 
 /* libxl__alloc_vdev only works on the local domain, that is the domain