Re: [PATCH v7 20/47] block: Iterate over children in refresh_limits

[Andrey Shinkevich]

On 25.06.2020 18:21, Max Reitz wrote:
> Instead of looking at just bs->file and bs->backing, we should look at
> all children that could end up receiving forwarded requests.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/io.c | 32 ++++++++++++++++----------------
>   1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index c2af7711d6..37057f13e0 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -135,6 +135,8 @@ static void bdrv_merge_limits(BlockLimits *dst, const 
> BlockLimits *src)
>   void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>   {
>       BlockDriver *drv = bs->drv;
> +    BdrvChild *c;
> +    bool have_limits;
>       Error *local_err = NULL;
>   
>       memset(&bs->bl, 0, sizeof(bs->bl));
> @@ -149,14 +151,21 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error 
> **errp)
>                                   drv->bdrv_co_preadv_part) ? 1 : 512;
>   
>       /* Take some limits from the children as a default */
> -    if (bs->file) {
> -        bdrv_refresh_limits(bs->file->bs, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> -            return;
> +    have_limits = false;
> +    QLIST_FOREACH(c, &bs->children, next) {
> +        if (c->role & (BDRV_CHILD_DATA | BDRV_CHILD_FILTERED | BDRV_CHILD_COW))
> +        {
> +            bdrv_refresh_limits(c->bs, &local_err);
> +            if (local_err) {
> +                error_propagate(errp, local_err);
> +                return;
> +            }
> +            bdrv_merge_limits(&bs->bl, &c->bs->bl);
> +            have_limits = true;
>           }
> -        bdrv_merge_limits(&bs->bl, &bs->file->bs->bl);
> -    } else {
> +    }
> +
> +    if (!have_limits) {


This conditioned piece of code worked with (bs->file == NULL) only.

Now, it works only if there are neither bs->file, nor bs->backing, nor 
else filtered children.

Is it OK and doesn't break the logic for all cases?

Andrey


>           bs->bl.min_mem_alignment = 512;
>           bs->bl.opt_mem_alignment = qemu_real_host_page_size;
>   
> @@ -164,15 +173,6 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>           bs->bl.max_iov = IOV_MAX;
>       }
>   
> -    if (bs->backing) {
> -        bdrv_refresh_limits(bs->backing->bs, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> -            return;
> -        }
> -        bdrv_merge_limits(&bs->bl, &bs->backing->bs->bl);
> -    }
> -
>       /* Then let the driver override it */
>       if (drv->bdrv_refresh_limits) {
>           drv->bdrv_refresh_limits(bs, errp);