Re: [Qemu-devel] [PATCH v6 25/42] mirror: Deal with filters

[Vladimir Sementsov-Ogievskiy]

09.08.2019 19:13, Max Reitz wrote:
> This includes some permission limiting (for example, we only need to
> take the RESIZE permission for active commits where the base is smaller
> than the top).
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>   block/mirror.c | 117 ++++++++++++++++++++++++++++++++++++++-----------
>   blockdev.c     |  47 +++++++++++++++++---
>   2 files changed, 131 insertions(+), 33 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 54bafdf176..6ddbfb9708 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c


[..]

> @@ -1693,15 +1734,39 @@ static BlockJob *mirror_start_job(
>       /* In commit_active_start() all intermediate nodes disappear, so
>        * any jobs in them must be blocked */
>       if (target_is_backing) {
> -        BlockDriverState *iter;
> -        for (iter = backing_bs(bs); iter != target; iter = backing_bs(iter)) 
> {
> -            /* XXX BLK_PERM_WRITE needs to be allowed so we don't block
> -             * ourselves at s->base (if writes are blocked for a node, they 
> are
> -             * also blocked for its backing file). The other options would 
> be a
> -             * second filter driver above s->base (== target). */
> +        BlockDriverState *iter, *filtered_target;
> +        uint64_t iter_shared_perms;
> +
> +        /*
> +         * The topmost node with
> +         * bdrv_skip_rw_filters(filtered_target) == 
> bdrv_skip_rw_filters(target)
> +         */
> +        filtered_target = bdrv_filtered_cow_bs(bdrv_find_overlay(bs, 
> target));
> +
> +        assert(bdrv_skip_rw_filters(filtered_target) ==
> +               bdrv_skip_rw_filters(target));
> +
> +        /*
> +         * XXX BLK_PERM_WRITE needs to be allowed so we don't block
> +         * ourselves at s->base (if writes are blocked for a node, they are
> +         * also blocked for its backing file). The other options would be a
> +         * second filter driver above s->base (== target).
> +         */
> +        iter_shared_perms = BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE;
> +
> +        for (iter = bdrv_filtered_bs(bs); iter != target;
> +             iter = bdrv_filtered_bs(iter))
> +        {
> +            if (iter == filtered_target) {
> +                /*
> +                 * From here on, all nodes are filters on the base.
> +                 * This allows us to share BLK_PERM_CONSISTENT_READ.
> +                 */
> +                iter_shared_perms |= BLK_PERM_CONSISTENT_READ;


Hmm, I don't understand, why read from upper nodes is not shared?

> +            }
> +
>               ret = block_job_add_bdrv(&s->common, "intermediate node", iter, 
> 0,
> -                                     BLK_PERM_WRITE_UNCHANGED | 
> BLK_PERM_WRITE,
> -                                     errp);
> +                                     iter_shared_perms, errp);
>               if (ret < 0) {
>                   goto fail;
>               }

[..]

-- 
Best regards,
Vladimir