qemu-stable
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-stable] [PATCH] mirror: Drop permissions on s->target on compl


From: Paolo Bonzini
Subject: Re: [Qemu-stable] [PATCH] mirror: Drop permissions on s->target on completion
Date: Mon, 29 May 2017 14:42:38 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0


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: address@hidden
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]