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) {
         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);
-- 
2.26.2

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);

On 14.07.20 20:37, Andrey Shinkevich wrote:
> 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?

Hm.  Good question.

I think the answer is it’s OK.

For DATA and FILTERED, it makes absolute sense to just use their
alignments.  For COW, maybe not so much?  But if there’s a COW child,
there has to be a DATA child as well (in practice).  So we’ll always
consider its alignment, too.

(And hypothetically speaking, if there was a COW child but no DATA
child, then the only alignment we need to observe is in fact the one of
the COW child.)

Max