[PATCH] blockdev: fix drive-backup transaction endless drained section

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH] blockdev: fix drive-backup transaction endless drained section
@ 2021-06-08 17:18 Vladimir Sementsov-Ogievskiy
  2021-06-08 17:24 ` Vladimir Sementsov-Ogievskiy
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-06-08 17:18 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, armbru, mreitz, kwolf, jsnow, eblake, vsementsov

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



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] blockdev: fix drive-backup transaction endless drained section
  2021-06-08 17:18 [PATCH] blockdev: fix drive-backup transaction endless drained section Vladimir Sementsov-Ogievskiy
@ 2021-06-08 17:24 ` Vladimir Sementsov-Ogievskiy
  2021-06-08 17:30   ` Vladimir Sementsov-Ogievskiy
  2021-06-08 19:10 ` Eric Blake
  2021-07-07 13:35 ` Vladimir Sementsov-Ogievskiy
  2 siblings, 1 reply; 6+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-06-08 17:24 UTC (permalink / raw)
  To: qemu-block
  Cc: qemu-devel, armbru, mreitz, kwolf, jsnow, eblake,
	incoming+qemu-project-qemu-11167699-5rxbjn6o3eu8n4pzsbiyd55wo-issue-399

[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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] blockdev: fix drive-backup transaction endless drained section
  2021-06-08 17:24 ` Vladimir Sementsov-Ogievskiy
@ 2021-06-08 17:30   ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 6+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-06-08 17:30 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, armbru, mreitz, kwolf, jsnow, eblake

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] blockdev: fix drive-backup transaction endless drained section
  2021-06-08 17:18 [PATCH] blockdev: fix drive-backup transaction endless drained section Vladimir Sementsov-Ogievskiy
  2021-06-08 17:24 ` Vladimir Sementsov-Ogievskiy
@ 2021-06-08 19:10 ` Eric Blake
  2021-07-07 13:35 ` Vladimir Sementsov-Ogievskiy
  2 siblings, 0 replies; 6+ messages in thread
From: Eric Blake @ 2021-06-08 19:10 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy
  Cc: kwolf, qemu-block, armbru, qemu-devel, mreitz, jsnow

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] blockdev: fix drive-backup transaction endless drained section
  2021-06-08 17:18 [PATCH] blockdev: fix drive-backup transaction endless drained section Vladimir Sementsov-Ogievskiy
  2021-06-08 17:24 ` Vladimir Sementsov-Ogievskiy
  2021-06-08 19:10 ` Eric Blake
@ 2021-07-07 13:35 ` Vladimir Sementsov-Ogievskiy
  2021-07-08 12:26   ` Kevin Wolf
  2 siblings, 1 reply; 6+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-07-07 13:35 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, armbru, mreitz, kwolf, jsnow, eblake

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] blockdev: fix drive-backup transaction endless drained section
  2021-07-07 13:35 ` Vladimir Sementsov-Ogievskiy
@ 2021-07-08 12:26   ` Kevin Wolf
  0 siblings, 0 replies; 6+ messages in thread
From: Kevin Wolf @ 2021-07-08 12:26 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy
  Cc: qemu-block, jsnow, qemu-devel, mreitz, eblake, armbru

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-07-08 12:29 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-08 17:18 [PATCH] blockdev: fix drive-backup transaction endless drained section Vladimir Sementsov-Ogievskiy
2021-06-08 17:24 ` Vladimir Sementsov-Ogievskiy
2021-06-08 17:30   ` Vladimir Sementsov-Ogievskiy
2021-06-08 19:10 ` Eric Blake
2021-07-07 13:35 ` Vladimir Sementsov-Ogievskiy
2021-07-08 12:26   ` Kevin Wolf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.