debian/patches/pve/0041-PVE-Backup-avoid-use-QemuRecMutex-inside-coroutines.patch

   1 From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
   2 From: Dietmar Maurer <dietmar@proxmox.com>
   3 Date: Fri, 17 Apr 2020 08:57:47 +0200
   4 Subject: [PATCH] PVE Backup: avoid use QemuRecMutex inside coroutines
   5
   6 ---
   7  pve-backup.c | 59 +++++++++++++++++++++++++++++++++-------------------
   8  1 file changed, 38 insertions(+), 21 deletions(-)
   9
  10 diff --git a/pve-backup.c b/pve-backup.c
  11 index 169f0c68d0..dddf430399 100644
  12 --- a/pve-backup.c
  13 +++ b/pve-backup.c
  14 @@ -11,6 +11,23 @@
  15
  16  /* PVE backup state and related function */
  17
  18 +/*
  19 + * Note: A resume from a qemu_coroutine_yield can happen in a different thread,
  20 + * so you may not use normal mutexes within coroutines:
  21 + *
  22 + * ---bad-example---
  23 + * qemu_rec_mutex_lock(lock)
  24 + * ...
  25 + * qemu_coroutine_yield() // wait for something
  26 + * // we are now inside a different thread
  27 + * qemu_rec_mutex_unlock(lock) // Crash - wrong thread!!
  28 + * ---end-bad-example--
  29 + *
  30 + * ==> Always use CoMutext inside coroutines.
  31 + * ==> Never acquire/release AioContext withing coroutines (because that use QemuRecMutex)
  32 + *
  33 + */
  34 +
  35  static struct PVEBackupState {
  36      struct {
  37          // Everithing accessed from qmp_backup_query command is protected using lock
  38 @@ -30,12 +47,14 @@ static struct PVEBackupState {
  39      ProxmoxBackupHandle *pbs;
  40      GList *di_list;
  41      QemuRecMutex backup_mutex;
  42 +    CoMutex dump_callback_mutex;
  43  } backup_state;
  44
  45  static void pvebackup_init(void)
  46  {
  47      qemu_rec_mutex_init(&backup_state.stat.lock);
  48      qemu_rec_mutex_init(&backup_state.backup_mutex);
  49 +    qemu_co_mutex_init(&backup_state.dump_callback_mutex);
  50  }
  51
  52  // initialize PVEBackupState at startup
  53 @@ -114,16 +133,16 @@ pvebackup_co_dump_pbs_cb(
  54      Error *local_err = NULL;
  55      int pbs_res = -1;
  56
  57 -    qemu_rec_mutex_lock(&backup_state.backup_mutex);
  58 +    qemu_co_mutex_lock(&backup_state.dump_callback_mutex);
  59
  60      // avoid deadlock if job is cancelled
  61      if (pvebackup_error_or_canceled()) {
  62 -        qemu_rec_mutex_unlock(&backup_state.backup_mutex);
  63 +        qemu_co_mutex_unlock(&backup_state.dump_callback_mutex);
  64          return -1;
  65      }
  66
  67      pbs_res = proxmox_backup_co_write_data(backup_state.pbs, di->dev_id, buf, start, size, &local_err);
  68 -    qemu_rec_mutex_unlock(&backup_state.backup_mutex);
  69 +    qemu_co_mutex_unlock(&backup_state.dump_callback_mutex);
  70
  71      if (pbs_res < 0) {
  72          pvebackup_propagate_error(local_err);
  73 @@ -149,7 +168,6 @@ pvebackup_co_dump_vma_cb(
  74      const unsigned char *buf = pbuf;
  75      PVEBackupDevInfo *di = opaque;
  76
  77 -
  78      int ret = -1;
  79
  80      assert(backup_state.vmaw);
  81 @@ -167,16 +185,16 @@ pvebackup_co_dump_vma_cb(
  82      }
  83
  84      while (remaining > 0) {
  85 -        qemu_rec_mutex_lock(&backup_state.backup_mutex);
  86 +        qemu_co_mutex_lock(&backup_state.dump_callback_mutex);
  87          // avoid deadlock if job is cancelled
  88          if (pvebackup_error_or_canceled()) {
  89 -            qemu_rec_mutex_unlock(&backup_state.backup_mutex);
  90 +            qemu_co_mutex_unlock(&backup_state.dump_callback_mutex);
  91              return -1;
  92          }
  93
  94          size_t zero_bytes = 0;
  95          ret = vma_writer_write(backup_state.vmaw, di->dev_id, cluster_num, buf, &zero_bytes);
  96 -        qemu_rec_mutex_unlock(&backup_state.backup_mutex);
  97 +        qemu_co_mutex_unlock(&backup_state.dump_callback_mutex);
  98
  99          ++cluster_num;
 100          if (buf) {
 101 @@ -203,12 +221,11 @@ pvebackup_co_dump_vma_cb(
 102      return size;
 103  }
 104
 105 +// assumes the caller holds backup_mutex
 106  static void coroutine_fn pvebackup_co_cleanup(void *unused)
 107  {
 108      assert(qemu_in_coroutine());
 109
 110 -    qemu_rec_mutex_lock(&backup_state.backup_mutex);
 111 -
 112      qemu_rec_mutex_lock(&backup_state.stat.lock);
 113      backup_state.stat.end_time = time(NULL);
 114      qemu_rec_mutex_unlock(&backup_state.stat.lock);
 115 @@ -239,9 +256,9 @@ static void coroutine_fn pvebackup_co_cleanup(void *unused)
 116
 117      g_list_free(backup_state.di_list);
 118      backup_state.di_list = NULL;
 119 -    qemu_rec_mutex_unlock(&backup_state.backup_mutex);
 120  }
 121
 122 +// assumes the caller holds backup_mutex
 123  static void coroutine_fn pvebackup_complete_stream(void *opaque)
 124  {
 125      PVEBackupDevInfo *di = opaque;
 126 @@ -295,6 +312,8 @@ static void pvebackup_complete_cb(void *opaque, int ret)
 127
 128  static void pvebackup_cancel(void)
 129  {
 130 +    assert(!qemu_in_coroutine());
 131 +
 132      Error *cancel_err = NULL;
 133      error_setg(&cancel_err, "backup canceled");
 134      pvebackup_propagate_error(cancel_err);
 135 @@ -348,6 +367,7 @@ void qmp_backup_cancel(Error **errp)
 136      pvebackup_cancel();
 137  }
 138
 139 +// assumes the caller holds backup_mutex
 140  static int coroutine_fn pvebackup_co_add_config(
 141      const char *file,
 142      const char *name,
 143 @@ -431,9 +451,9 @@ static void pvebackup_run_next_job(void)
 144          }
 145      }
 146
 147 -    qemu_rec_mutex_unlock(&backup_state.backup_mutex);
 148 -
 149      block_on_coroutine_fn(pvebackup_co_cleanup, NULL); // no more jobs, run cleanup
 150 +
 151 +    qemu_rec_mutex_unlock(&backup_state.backup_mutex);
 152  }
 153
 154  static bool create_backup_jobs(void) {
 155 @@ -520,6 +540,7 @@ typedef struct QmpBackupTask {
 156      UuidInfo *result;
 157  } QmpBackupTask;
 158
 159 +// assumes the caller holds backup_mutex
 160  static void coroutine_fn pvebackup_co_prepare(void *opaque)
 161  {
 162      assert(qemu_in_coroutine());
 163 @@ -543,11 +564,8 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
 164      const char *config_name = "qemu-server.conf";
 165      const char *firewall_name = "qemu-server.fw";
 166
 167 -    qemu_rec_mutex_lock(&backup_state.backup_mutex);
 168 -
 169      if (backup_state.di_list) {
 170 -        qemu_rec_mutex_unlock(&backup_state.backup_mutex);
 171 -        error_set(task->errp, ERROR_CLASS_GENERIC_ERROR,
 172 +         error_set(task->errp, ERROR_CLASS_GENERIC_ERROR,
 173                    "previous backup not finished");
 174          return;
 175      }
 176 @@ -792,8 +810,6 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
 177
 178      backup_state.di_list = di_list;
 179
 180 -    qemu_rec_mutex_unlock(&backup_state.backup_mutex);
 181 -
 182      uuid_info = g_malloc0(sizeof(*uuid_info));
 183      uuid_info->UUID = uuid_str;
 184
 185 @@ -836,8 +852,6 @@ err:
 186          rmdir(backup_dir);
 187      }
 188
 189 -    qemu_rec_mutex_unlock(&backup_state.backup_mutex);
 190 -
 191      task->result = NULL;
 192      return;
 193  }
 194 @@ -881,13 +895,16 @@ UuidInfo *qmp_backup(
 195          .errp = errp,
 196      };
 197
 198 +    qemu_rec_mutex_lock(&backup_state.backup_mutex);
 199 +
 200      block_on_coroutine_fn(pvebackup_co_prepare, &task);
 201
 202      if (*errp == NULL) {
 203 -        qemu_rec_mutex_lock(&backup_state.backup_mutex);
 204          create_backup_jobs();
 205          qemu_rec_mutex_unlock(&backup_state.backup_mutex);
 206          pvebackup_run_next_job();
 207 +    } else {
 208 +        qemu_rec_mutex_unlock(&backup_state.backup_mutex);
 209      }
 210
 211      return task.result;