public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Fiona Ebner <f.ebner@proxmox.com>
To: pve-devel@lists.proxmox.com
Subject: [pve-devel] [PATCH qemu 02/23] PVE backup: fixup error handling for fleecing
Date: Tue, 23 Jul 2024 11:56:03 +0200	[thread overview]
Message-ID: <20240723095624.53621-3-f.ebner@proxmox.com> (raw)
In-Reply-To: <20240723095624.53621-1-f.ebner@proxmox.com>

The drained section needs to be terminated before breaking out of the
loop in the error scenarios. Otherwise, guest IO on the drive would
become stuck.

If the job is created successfully, then the job completion callback
will clean up the snapshot access block nodes. In case failure
happened before the job is created, there was no cleanup for the
snapshot access block nodes yet. Add it.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 pve-backup.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/pve-backup.c b/pve-backup.c
index 4e730aa3da..c4178758b3 100644
--- a/pve-backup.c
+++ b/pve-backup.c
@@ -357,22 +357,23 @@ static void coroutine_fn pvebackup_co_complete_stream(void *opaque)
     qemu_co_mutex_unlock(&backup_state.backup_mutex);
 }
 
+static void cleanup_snapshot_access(PVEBackupDevInfo *di)
+{
+    if (di->fleecing.snapshot_access) {
+        bdrv_unref(di->fleecing.snapshot_access);
+        di->fleecing.snapshot_access = NULL;
+    }
+    if (di->fleecing.cbw) {
+        bdrv_cbw_drop(di->fleecing.cbw);
+        di->fleecing.cbw = NULL;
+    }
+}
+
 static void pvebackup_complete_cb(void *opaque, int ret)
 {
     PVEBackupDevInfo *di = opaque;
     di->completed_ret = ret;
 
-    /*
-     * Handle block-graph specific cleanup (for fleecing) outside of the coroutine, because the work
-     * won't be done as a coroutine anyways:
-     * - For snapshot_access, allows doing bdrv_unref() directly. Doing it via bdrv_co_unref() would
-     *   just spawn a BH calling bdrv_unref().
-     * - For cbw, draining would need to spawn a BH.
-     */
-    if (di->fleecing.snapshot_access) {
-        bdrv_unref(di->fleecing.snapshot_access);
-        di->fleecing.snapshot_access = NULL;
-    }
     if (di->fleecing.cbw) {
         /*
          * With fleecing, failure for cbw does not fail the guest write, but only sets the snapshot
@@ -383,10 +384,17 @@ static void pvebackup_complete_cb(void *opaque, int ret)
         if (di->completed_ret == -EACCES && snapshot_error) {
             di->completed_ret = snapshot_error;
         }
-        bdrv_cbw_drop(di->fleecing.cbw);
-        di->fleecing.cbw = NULL;
     }
 
+    /*
+     * Handle block-graph specific cleanup (for fleecing) outside of the coroutine, because the work
+     * won't be done as a coroutine anyways:
+     * - For snapshot_access, allows doing bdrv_unref() directly. Doing it via bdrv_co_unref() would
+     *   just spawn a BH calling bdrv_unref().
+     * - For cbw, draining would need to spawn a BH.
+     */
+    cleanup_snapshot_access(di);
+
     /*
      * Needs to happen outside of coroutine, because it takes the graph write lock.
      */
@@ -587,6 +595,7 @@ static void create_backup_jobs_bh(void *opaque) {
             if (!di->fleecing.cbw) {
                 error_setg(errp, "appending cbw node for fleecing failed: %s",
                            local_err ? error_get_pretty(local_err) : "unknown error");
+                bdrv_drained_end(di->bs);
                 break;
             }
 
@@ -599,6 +608,8 @@ static void create_backup_jobs_bh(void *opaque) {
             if (!di->fleecing.snapshot_access) {
                 error_setg(errp, "setting up snapshot access for fleecing failed: %s",
                            local_err ? error_get_pretty(local_err) : "unknown error");
+                cleanup_snapshot_access(di);
+                bdrv_drained_end(di->bs);
                 break;
             }
             source_bs = di->fleecing.snapshot_access;
@@ -637,6 +648,7 @@ static void create_backup_jobs_bh(void *opaque) {
         }
 
         if (!job || local_err) {
+            cleanup_snapshot_access(di);
             error_setg(errp, "backup_job_create failed: %s",
                        local_err ? error_get_pretty(local_err) : "null");
             break;
-- 
2.39.2



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


  parent reply	other threads:[~2024-07-23  9:56 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-07-23  9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [PATCH qemu 01/23] block/reqlist: allow adding overlapping requests Fiona Ebner
2024-07-23  9:56 ` Fiona Ebner [this message]
2024-07-23  9:56 ` [pve-devel] [PATCH qemu 03/23] PVE backup: factor out setting up snapshot access for fleecing Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [PATCH qemu 04/23] PVE backup: save device name in device info structure Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [PATCH qemu 05/23] PVE backup: include device name in error when setting up snapshot access fails Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [RFC qemu 06/23] PVE backup: add target ID in backup state Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [RFC qemu 07/23] PVE backup: get device info: allow caller to specify filter for which devices use fleecing Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [RFC qemu 08/23] PVE backup: implement backup access setup and teardown API for external providers Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [RFC qemu 09/23] PVE backup: implement bitmap support for external backup access Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [RFC storage 10/23] plugin: introduce new_backup_provider() method Fiona Ebner
2024-07-25  9:48   ` Max Carrara
2024-07-25 13:11     ` Fiona Ebner
2024-07-25 13:25       ` Fiona Ebner
2024-07-25 15:32       ` Max Carrara
2024-07-26  9:52         ` Fiona Ebner
2024-07-26 12:02           ` Max Carrara
2024-07-26 12:45             ` Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [RFC storage 11/23] extract backup config: delegate to backup provider if there is one Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [POC storage 12/23] add backup provider example Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [PATCH qemu-server 13/23] move nbd_stop helper to QMPHelpers module Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [PATCH qemu-server 14/23] backup: move cleanup of fleecing images to cleanup method Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [PATCH qemu-server 15/23] backup: cleanup: check if VM is running before issuing QMP commands Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [RFC qemu-server 16/23] backup: allow adding fleecing images also for EFI and TPM Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [RFC qemu-server 17/23] backup: implement backup for external providers Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [PATCH qemu-server 18/23] restore: die early when there is no size for a device Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [RFC qemu-server 19/23] backup: implement restore for external providers Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [RFC container 20/23] backup: implement backup " Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [RFC container 21/23] backup: implement restore " Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [PATCH manager 22/23] ui: backup: also check for backup subtype to classify archive Fiona Ebner
2024-07-23  9:56 ` [pve-devel] [RFC manager 23/23] backup: implement backup for external providers Fiona Ebner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240723095624.53621-3-f.ebner@proxmox.com \
    --to=f.ebner@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal