blockdev: fix drive-backup transaction endless drained section

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

Posted by Vladimir Sementsov-Ogievskiy 2 years, 10 months ago

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

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

Posted by Eric Blake 2 years, 10 months ago

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

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

Posted by Vladimir Sementsov-Ogievskiy 2 years, 10 months ago

[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

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

Posted by Vladimir Sementsov-Ogievskiy 2 years, 10 months ago

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

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

Posted by Vladimir Sementsov-Ogievskiy 2 years, 9 months ago

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

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

Posted by Kevin Wolf 2 years, 9 months ago

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