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

[Max Reitz]

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

signature.asc
Description: OpenPGP digital signature