[pve-devel] [PATCH v2 qemu] PVE backup: backup access api: simplify bitmap logic

[Fiona Ebner <f.ebner@proxmox.com>]

Currently, only one bitmap name per target is planned to be used.
Simply use the target ID itself as the bitmap name. This allows to
simplify the logic quite a bit and there also is no need for the
backup_access_bitmaps hash table anymore.

For the return value, the bitmap names are still passed along for
convenience in the caller.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v2:
* Clarify QAPI docs a bit more: mention explicitly that each target
  controls its bitmaps. Remove superfluous mention of what bitmap
  name will be, it's still explicitly returned. Thought about not
  returning it for a bit, but opted against it.

 pve-backup.c         | 72 ++++++++++++--------------------------------
 qapi/block-core.json | 15 ++++-----
 2 files changed, 26 insertions(+), 61 deletions(-)

diff --git a/pve-backup.c b/pve-backup.c
index 18bcf29533..0ea0343b22 100644
--- a/pve-backup.c
+++ b/pve-backup.c
@@ -74,7 +74,6 @@ static struct PVEBackupState {
     CoMutex backup_mutex;
     CoMutex dump_callback_mutex;
     char *target_id;
-    GHashTable *backup_access_bitmaps; // key=target_id, value=bitmap_name
 } backup_state;
 
 static void pvebackup_init(void)
@@ -106,7 +105,7 @@ typedef struct PVEBackupDevInfo {
     PBSBitmapAction bitmap_action;
     BlockDriverState *target;
     BlockJob *job;
-    char *requested_bitmap_name; // used by external backup access during initialization
+    BackupAccessSetupBitmapMode requested_bitmap_mode;
 } PVEBackupDevInfo;
 
 static void pvebackup_propagate_error(Error *err)
@@ -1043,16 +1042,7 @@ BackupAccessInfoList *coroutine_fn qmp_backup_access_setup(
             error_propagate(errp, local_err);
             goto err;
         }
-        if (it->value->bitmap_mode == BACKUP_ACCESS_SETUP_BITMAP_MODE_NONE) {
-            di->bitmap_action = PBS_BITMAP_ACTION_NOT_USED;
-        } else {
-            di->requested_bitmap_name = g_strdup(it->value->bitmap_name);
-            if (it->value->bitmap_mode == BACKUP_ACCESS_SETUP_BITMAP_MODE_NEW) {
-                di->bitmap_action = PBS_BITMAP_ACTION_NEW;
-            } else if (it->value->bitmap_mode == BACKUP_ACCESS_SETUP_BITMAP_MODE_USE) {
-                di->bitmap_action = PBS_BITMAP_ACTION_USED;
-            }
-        }
+        di->requested_bitmap_mode = it->value->bitmap_mode;
         di_list = g_list_append(di_list, di);
     }
     bdrv_graph_co_rdunlock();
@@ -1082,10 +1072,7 @@ BackupAccessInfoList *coroutine_fn qmp_backup_access_setup(
     /* clear previous backup's bitmap_list */
     clear_backup_state_bitmap_list();
 
-    if (!backup_state.backup_access_bitmaps) {
-        backup_state.backup_access_bitmaps =
-            g_hash_table_new_full(g_str_hash, g_str_equal, free, free);
-    }
+    const char *bitmap_name = target_id;
 
     /* create bitmaps if requested */
     l = di_list;
@@ -1098,59 +1085,43 @@ BackupAccessInfoList *coroutine_fn qmp_backup_access_setup(
         PBSBitmapAction action = PBS_BITMAP_ACTION_NOT_USED;
         size_t dirty = di->size;
 
-        const char *old_bitmap_name =
-            (const char*)g_hash_table_lookup(backup_state.backup_access_bitmaps, target_id);
-
-        bool same_bitmap_name = old_bitmap_name
-            && di->requested_bitmap_name
-            && strcmp(di->requested_bitmap_name, old_bitmap_name) == 0;
-
-        /* special case: if we explicitly requested a *new* bitmap, treat an
-         * existing bitmap as having a different name */
-        if (di->bitmap_action == PBS_BITMAP_ACTION_NEW) {
-            same_bitmap_name = false;
-        }
-
-        if (old_bitmap_name && !same_bitmap_name) {
-            BdrvDirtyBitmap *old_bitmap = bdrv_find_dirty_bitmap(di->bs, old_bitmap_name);
-            if (!old_bitmap) {
-                warn_report("setup backup access: expected old bitmap '%s' not found for drive "
-                            "'%s'", old_bitmap_name, di->device_name);
-            } else {
-                g_hash_table_remove(backup_state.backup_access_bitmaps, target_id);
+        if (di->requested_bitmap_mode == BACKUP_ACCESS_SETUP_BITMAP_MODE_NONE ||
+            di->requested_bitmap_mode == BACKUP_ACCESS_SETUP_BITMAP_MODE_NEW) {
+            BdrvDirtyBitmap *old_bitmap = bdrv_find_dirty_bitmap(di->bs, bitmap_name);
+            if (old_bitmap) {
                 bdrv_release_dirty_bitmap(old_bitmap);
-                action = PBS_BITMAP_ACTION_NOT_USED_REMOVED;
+                action = PBS_BITMAP_ACTION_NOT_USED_REMOVED; // set below for new
             }
         }
 
         BdrvDirtyBitmap *bitmap = NULL;
-        if (di->requested_bitmap_name) {
-            bitmap = bdrv_find_dirty_bitmap(di->bs, di->requested_bitmap_name);
+        if (di->requested_bitmap_mode == BACKUP_ACCESS_SETUP_BITMAP_MODE_NEW ||
+            di->requested_bitmap_mode == BACKUP_ACCESS_SETUP_BITMAP_MODE_USE) {
+            bitmap = bdrv_find_dirty_bitmap(di->bs, bitmap_name);
             if (!bitmap) {
                 bitmap = bdrv_create_dirty_bitmap(di->bs, PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE,
-                                                  di->requested_bitmap_name, errp);
+                                                  bitmap_name, errp);
                 if (!bitmap) {
                     qemu_mutex_unlock(&backup_state.stat.lock);
                     goto err;
                 }
                 bdrv_set_dirty_bitmap(bitmap, 0, di->size);
-                if (same_bitmap_name) {
+                if (di->requested_bitmap_mode == BACKUP_ACCESS_SETUP_BITMAP_MODE_USE) {
                     action = PBS_BITMAP_ACTION_MISSING_RECREATED;
                 } else {
                     action = PBS_BITMAP_ACTION_NEW;
                 }
             } else {
+                if (di->requested_bitmap_mode == BACKUP_ACCESS_SETUP_BITMAP_MODE_NEW) {
+                    qemu_mutex_unlock(&backup_state.stat.lock);
+                    error_setg(errp, "internal error - removed old bitmap still present");
+                    goto err;
+                }
                 /* track clean chunks as reused */
                 dirty = MIN(bdrv_get_dirty_count(bitmap), di->size);
                 backup_state.stat.reused += di->size - dirty;
                 action = PBS_BITMAP_ACTION_USED;
             }
-
-            if (!same_bitmap_name) {
-                g_hash_table_insert(backup_state.backup_access_bitmaps,
-                                    strdup(target_id), strdup(di->requested_bitmap_name));
-            }
-
         }
 
         PBSBitmapInfo *info = g_malloc(sizeof(*info));
@@ -1207,9 +1178,9 @@ BackupAccessInfoList *coroutine_fn qmp_backup_access_setup(
         info->value->node_name = g_strdup(bdrv_get_node_name(di->fleecing.snapshot_access));
         info->value->device = g_strdup(di->device_name);
         info->value->size = di->size;
-        if (di->requested_bitmap_name) {
+        if (di->bitmap) {
             info->value->bitmap_node_name = g_strdup(bdrv_get_node_name(di->bs));
-            info->value->bitmap_name = g_strdup(di->requested_bitmap_name);
+            info->value->bitmap_name = g_strdup(bitmap_name);
             info->value->bitmap_action = di->bitmap_action;
             info->value->has_bitmap_action = true;
         }
@@ -1274,9 +1245,6 @@ void backup_access_teardown(bool success)
         g_free(di->device_name);
         di->device_name = NULL;
 
-        g_free(di->requested_bitmap_name);
-        di->requested_bitmap_name = NULL;
-
         g_free(di);
     }
     g_list_free(backup_state.di_list);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 09beb3217c..02c043f0f7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1140,18 +1140,12 @@
 #
 # @device: the block device name.
 #
-# @bitmap-name: use/create a bitmap with this name for the device. Re-using the
-#     same name allows for making incremental backups. Check the @bitmap-action
-#     in the result to see if you can actually re-use the bitmap or if it had to
-#     be newly created.
-#
 # @bitmap-mode: used to control whether the bitmap should be reused or
-#     recreated.
+#     recreated or not used. Default is not using a bitmap.
 #
 ##
 { 'struct': 'BackupAccessSourceDevice',
-  'data': { 'device': 'str', '*bitmap-name': 'str',
-            '*bitmap-mode': 'BackupAccessSetupBitmapMode' } }
+  'data': { 'device': 'str', '*bitmap-mode': 'BackupAccessSetupBitmapMode' } }
 
 ##
 # @BackupAccessSetupBitmapMode:
@@ -1175,7 +1169,10 @@
 #
 # @target-id: the unique ID of the backup target.
 #
-# @devices: list of devices for which to create the backup access.
+# @devices: list of devices for which to create the backup access.  Also
+#     controls whether to use/create a bitmap for the device.  Check the
+#     @bitmap-action in the result to see what action was actually taken for the
+#     bitmap.  Each target controls its own bitmaps.
 #
 # Returns: a list of @BackupAccessInfo, one for each device.
 #
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel