blockdev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drive_backup_prepare() does bdrv_drained_begin() in hope that
bdrv_drained_end() will be called in drive_backup_clean(). Still we
need to set state->bs for this to work. That's done too late: a lot of
failure paths in drive_backup_prepare() miss setting state->bs. Fix
that.
Fixes: 2288ccfac96281c316db942d10e3f921c1373064
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/399
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
blockdev.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index f08192deda..094c085962 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1714,6 +1714,7 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
+ state->bs = bs;
/* Paired with .clean() */
bdrv_drained_begin(bs);
@@ -1813,8 +1814,6 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
}
}
- state->bs = bs;
-
state->job = do_backup_common(qapi_DriveBackup_base(backup),
bs, target_bs, aio_context,
common->block_job_txn, errp);
--
2.29.2
[try add gitlab issue email to cc, will see how it work :)] 08.06.2021 20:18, Vladimir Sementsov-Ogievskiy wrote: > drive_backup_prepare() does bdrv_drained_begin() in hope that > bdrv_drained_end() will be called in drive_backup_clean(). Still we > need to set state->bs for this to work. That's done too late: a lot of > failure paths in drive_backup_prepare() miss setting state->bs. Fix > that. > > Fixes: 2288ccfac96281c316db942d10e3f921c1373064 > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/399 Reported-by: Sergey Zhuravlev > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > blockdev.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index f08192deda..094c085962 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1714,6 +1714,7 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) > aio_context = bdrv_get_aio_context(bs); > aio_context_acquire(aio_context); > > + state->bs = bs; > /* Paired with .clean() */ > bdrv_drained_begin(bs); > > @@ -1813,8 +1814,6 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) > } > } > > - state->bs = bs; > - > state->job = do_backup_common(qapi_DriveBackup_base(backup), > bs, target_bs, aio_context, > common->block_job_txn, errp); > -- Best regards, Vladimir
08.06.2021 20:24, Vladimir Sementsov-Ogievskiy wrote: > [try add gitlab issue email to cc, will see how it work :)] That was bad idea. The message becomes a new comment at issue page. Formatting is broken, looks bad. I've removed that comment > > 08.06.2021 20:18, Vladimir Sementsov-Ogievskiy wrote: >> drive_backup_prepare() does bdrv_drained_begin() in hope that >> bdrv_drained_end() will be called in drive_backup_clean(). Still we >> need to set state->bs for this to work. That's done too late: a lot of >> failure paths in drive_backup_prepare() miss setting state->bs. Fix >> that. >> >> Fixes: 2288ccfac96281c316db942d10e3f921c1373064 >> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/399 > > Reported-by: Sergey Zhuravlev > >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> blockdev.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/blockdev.c b/blockdev.c >> index f08192deda..094c085962 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -1714,6 +1714,7 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) >> aio_context = bdrv_get_aio_context(bs); >> aio_context_acquire(aio_context); >> + state->bs = bs; >> /* Paired with .clean() */ >> bdrv_drained_begin(bs); >> @@ -1813,8 +1814,6 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) >> } >> } >> - state->bs = bs; >> - >> state->job = do_backup_common(qapi_DriveBackup_base(backup), >> bs, target_bs, aio_context, >> common->block_job_txn, errp); >> > > -- Best regards, Vladimir
On Tue, Jun 08, 2021 at 08:18:52PM +0300, Vladimir Sementsov-Ogievskiy wrote: > drive_backup_prepare() does bdrv_drained_begin() in hope that > bdrv_drained_end() will be called in drive_backup_clean(). Still we > need to set state->bs for this to work. That's done too late: a lot of > failure paths in drive_backup_prepare() miss setting state->bs. Fix > that. > > Fixes: 2288ccfac96281c316db942d10e3f921c1373064 > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/399 > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > blockdev.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> > > diff --git a/blockdev.c b/blockdev.c > index f08192deda..094c085962 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1714,6 +1714,7 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) > aio_context = bdrv_get_aio_context(bs); > aio_context_acquire(aio_context); > > + state->bs = bs; > /* Paired with .clean() */ > bdrv_drained_begin(bs); Commit 2288ccfac9 had these two lines in the opposite order, but that doesn't matter, the important part is that we do indeed need to set state->bs regardless of any later failure detections, to get .clean to do the matching drained_end. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Forgotten thing :( Kevin, could you please queue it in your block branch? For me not to bother Peter with one-patch pull request. 08.06.2021 20:18, Vladimir Sementsov-Ogievskiy wrote: > drive_backup_prepare() does bdrv_drained_begin() in hope that > bdrv_drained_end() will be called in drive_backup_clean(). Still we > need to set state->bs for this to work. That's done too late: a lot of > failure paths in drive_backup_prepare() miss setting state->bs. Fix > that. > > Fixes: 2288ccfac96281c316db942d10e3f921c1373064 > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/399 > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > blockdev.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index f08192deda..094c085962 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1714,6 +1714,7 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) > aio_context = bdrv_get_aio_context(bs); > aio_context_acquire(aio_context); > > + state->bs = bs; > /* Paired with .clean() */ > bdrv_drained_begin(bs); > > @@ -1813,8 +1814,6 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) > } > } > > - state->bs = bs; > - > state->job = do_backup_common(qapi_DriveBackup_base(backup), > bs, target_bs, aio_context, > common->block_job_txn, errp); > -- Best regards, Vladimir
Am 07.07.2021 um 15:35 hat Vladimir Sementsov-Ogievskiy geschrieben: > Forgotten thing :( > > Kevin, could you please queue it in your block branch? For me not to > bother Peter with one-patch pull request. No problem, I've queued it now. Kevin
© 2016 - 2024 Red Hat, Inc.