Re: [Qemu-devel] [PATCH] mirror: Drop permissions on s->target on completion

[Paolo Bonzini <pbonzini@redhat.com>]

On 29/05/2017 14:18, Kevin Wolf wrote:
> This fixes an assertion failure that was triggered by qemu-iotests 129
> on some CI host, while the same test case didn't seem to fail on other
> hosts.
> 
> Essentially the problem is that the blk_unref(s->target) in
> mirror_exit() doesn't necessarily mean that the BlockBackend goes away
> immediately. It is possible that the job completion was triggered nested
> in mirror_drain(), which looks like this:
> 
>     BlockBackend *target = s->target;
>     blk_ref(target);
>     blk_drain(target);
>     blk_unref(target);
> 
> In this case, the write permissions for s->target are retained until
> after blk_drain(), which makes removing mirror_top_bs fail for the
> active commit case (can't have a writable backing file in the chain
> without the filter driver).
> 
> Explicitly dropping the permissions first means that the additional
> reference doesn't hurt and the job can complete successfully even if
> called from the nested blk_drain().
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/mirror.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index e86f8f8..e778ee0 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -514,7 +514,12 @@ static void mirror_exit(BlockJob *job, void *opaque)
>  
>      /* Remove target parent that still uses BLK_PERM_WRITE/RESIZE before
>       * inserting target_bs at s->to_replace, where we might not be able to get
> -     * these permissions. */
> +     * these permissions.
> +     *
> +     * Note that blk_unref() alone doesn't necessarily drop permissions because
> +     * we might be running nested inside mirror_drain(), which takes an extra
> +     * reference, so use an explicit blk_set_perm() first. */
> +    blk_set_perm(s->target, 0, BLK_PERM_ALL, &error_abort);
>      blk_unref(s->target);
>      s->target = NULL;
>  
> 

Thanks, this looks good.

Paolo