Re: [PATCH v4 4/5] blockdev: honor bdrv_try_set_aio_context() context requirements

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2616 bytes --]

On 21.11.19 14:57, Sergio Lopez wrote:
> bdrv_try_set_aio_context() requires that the old context is held, and
> the new context is not held. Fix all the occurrences where it's not
> done this way.
> 
> Suggested-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Sergio Lopez <slp@redhat.com>
> ---
>  blockdev.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 58 insertions(+), 9 deletions(-)

I wonder whether we even need to set the target’s context, because I
suppose it should be done automatically when it is attached to the
backup job with bdrv_attach_child() in bdrv_backup_top_append().  *shrug*

> diff --git a/blockdev.c b/blockdev.c
> index 152a0f7454..b0647d8d33 100644
> --- a/blockdev.c
> +++ b/blockdev.c

[...]

> @@ -1868,6 +1880,20 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
>          goto out;
>      }
>  
> +    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
> +    old_context = bdrv_get_aio_context(target_bs);
> +    aio_context_release(aio_context);
> +    aio_context_acquire(old_context);
> +
> +    ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
> +    if (ret < 0) {
> +        aio_context_release(old_context);
> +        return;
> +     }
> +
> +    aio_context_release(old_context);
> +    aio_context_acquire(aio_context);

Would it work to put the error block after these two calls so it can
just goto out again?  (Which would help if someone were to e.g.
introduce a new resource that is to be freed behind the out label.)

>      if (set_backing_hd) {
>          bdrv_set_backing_hd(target_bs, source, &local_err);
>          if (local_err) {

[...]

> @@ -4001,14 +4046,18 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
>  
>      zero_target = (sync == MIRROR_SYNC_MODE_FULL);
>  
> +    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
> +    old_context = bdrv_get_aio_context(target_bs);
>      aio_context = bdrv_get_aio_context(bs);
> -    aio_context_acquire(aio_context);
> +    aio_context_acquire(old_context);
>  
>      ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
>      if (ret < 0) {
>          goto out;
>      }
>  
> +    aio_context_acquire(aio_context);
> +

old_context is never released here.

Max

>      blockdev_mirror_common(has_job_id ? job_id : NULL, bs, target_bs,
>                             has_replaces, replaces, sync, backing_mode,
>                             zero_target, has_speed, speed,
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]