Re: [PATCH 3/7] block: protect parallel jobs from overlapping

[Vladimir Sementsov-Ogievskiy]

20.04.2020 21:36, Andrey Shinkevich wrote:
> When it comes to the check for the blocked operations, the node may be
> a filter linked to blk.

"blk" commonly refers to BlockBackend, which is unrelated here. You mean just a 
filter.

> In that case, do not miss to set blocked
> operations for the underlying node.
>
> Signed-off-by: Andrey Shinkevich <address@hidden>
> ---
>   blockjob.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/blockjob.c b/blockjob.c
> index 73d9f1b..2898929 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -189,7 +189,14 @@ void block_job_remove_all_bdrv(BlockJob *job)
>       GSList *l;
>       for (l = job->nodes; l; l = l->next) {
>           BdrvChild *c = l->data;
> -        bdrv_op_unblock_all(c->bs, job->blocker);
> +        BlockDriverState *bs = c->bs;
> +        bdrv_op_unblock_all(bs, job->blocker);
> +        if (bs->drv && bs->drv->is_filter) {
> +            bs = bdrv_filtered_bs(bs);
> +            if (bs) {
> +                bdrv_op_unblock_all(bs, job->blocker);
> +            }
> +        }
>           bdrv_root_unref_child(c);
>       }
>       g_slist_free(job->nodes);
> @@ -230,6 +237,12 @@ int block_job_add_bdrv(BlockJob *job, const char *name, 
> BlockDriverState *bs,
>   
>       job->nodes = g_slist_prepend(job->nodes, c);
>       bdrv_op_block_all(bs, job->blocker);
> +    if (bs->drv && bs->drv->is_filter) {
> +        bs = bdrv_filtered_bs(bs);
> +        if (bs) {
> +            bdrv_op_block_all(bs, job->blocker);

This will lead to setting op blocker twice, if there are filters inside backing 
chain. Is it safe?

Still, I don't think it's correct thing. Job should add all it's nodes by hand. 
If it add some
filter node, but don't add it's filtered node, it is definitely doing something 
wrong (see my
answer to 1/7).


> +        }
> +    }
>   
>       return 0;
>   }


--
Best regards,
Vladimir