[1/3] backup: Remove nodes from job in .clean()

Commit Message

Max Reitz Feb. 19, 2021, 3:33 p.m. UTC

The block job holds a reference to the backup-top node (because it is
passed as the main job BDS to block_job_create()).  Therefore,
bdrv_backup_top_drop() cannot delete the backup-top node (replacing it
by its child does not affect the job parent, because that has
.stay_at_node set).  That is a problem, because all of its I/O functions
assume the BlockCopyState (s->bcs) to be valid and that it has a
filtered child; but after bdrv_backup_top_drop(), neither of those
things are true.

It does not make sense to add new parents to backup-top after
backup_clean(), so we should detach it from the job before
bdrv_backup_top_drop().  Because there is no function to do that for a
single node, just detach all of the job's nodes -- the job does not do
anything past backup_clean() anyway.

Signed-off-by: Max Reitz <[e-mail]>
---
 block/backup.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Kevin Wolf Feb. 24, 2021, 3:33 p.m. UTC | #1

Am 19.02.2021 um 16:33 hat Max Reitz geschrieben:
> The block job holds a reference to the backup-top node (because it is
> passed as the main job BDS to block_job_create()).  Therefore,
> bdrv_backup_top_drop() cannot delete the backup-top node (replacing it
> by its child does not affect the job parent, because that has
> .stay_at_node set).  That is a problem, because all of its I/O functions
> assume the BlockCopyState (s->bcs) to be valid and that it has a
> filtered child; but after bdrv_backup_top_drop(), neither of those
> things are true.

This kind of suggests that block_copy_state_free() doesn't really belong
in bdrv_backup_top_drop(), but in a .bdrv_close callback.

Doesn't make this patch less correct, of course. We still want to have
all references dropped at the end of bdrv_backup_top_drop().

> It does not make sense to add new parents to backup-top after
> backup_clean(), so we should detach it from the job before
> bdrv_backup_top_drop().  Because there is no function to do that for a
> single node, just detach all of the job's nodes -- the job does not do
> anything past backup_clean() anyway.
> 
> Signed-off-by: Max Reitz <[e-mail]>

Reviewed-by: Kevin Wolf <[e-mail]>

Patch

diff --git a/block/backup.c b/block/backup.c
index 94e6dcd72e..6cf2f974aa 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -103,6 +103,7 @@  static void backup_abort(Job *job)
 static void backup_clean(Job *job)
 {
     BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
+    block_job_remove_all_bdrv(&s->common);
     bdrv_backup_top_drop(s->backup_top);
 }