Re: [PATCH v2 5/7] block/block-copy: block_copy(): add timeout_ns parameter

[Vladimir Sementsov-Ogievskiy <v.sementsov-og@mail.ru>]

01.04.2022 16:16, Hanna Reitz wrote:
> On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
>> Add possibility to limit block_copy() call in time. To be used in the
>> next commit.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>> ---
>>   block/block-copy.c         | 26 +++++++++++++++++++-------
>>   block/copy-before-write.c  |  2 +-
>>   include/block/block-copy.h |  2 +-
>>   3 files changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/block-copy.c b/block/block-copy.c
>> index ec46775ea5..b47cb188dd 100644
>> --- a/block/block-copy.c
>> +++ b/block/block-copy.c
> 
> [...]
> 
>> @@ -894,12 +902,16 @@ int coroutine_fn block_copy(BlockCopyState *s, int64_t start, int64_t bytes,
>>           .max_workers = BLOCK_COPY_MAX_WORKERS,
>>       };
>> -    return block_copy_common(&call_state);
>> -}
>> +    ret = qemu_co_timeout(block_copy_async_co_entry, call_state, timeout_ns,
>> +                          g_free);
> 
> A direct path for timeout_ns == 0 might still be nice to have.
> 
>> +    if (ret < 0) {
>> +        /* Timeout. call_state will be freed by running coroutine. */
> 
> Maybe assert(ret == -ETIMEDOUT);?

OK

> 
>> +        return ret;
> 
> If I’m right in understanding how qemu_co_timeout() works, block_copy_common() will continue to run here.  Shouldn’t we at least cancel it by setting call_state->cancelled to true?

Agree

> 
> (Besides this, I think that letting block_copy_common() running in the background should be OK.  I’m not sure what the implications are if we do cancel the call here, while on-cbw-error is break-guest-write, though.  Should be fine, I guess, because block_copy_common() will still correctly keep track of what it has successfully copied and what it hasn’t?)

Hmm. I now think, that we should at least wait for such cancelled background requests before block_copy_state_free in cbw_close(). But in "[PATCH v5 00/45] Transactional block-graph modifying API" I want to detach children from CBW filter before calling .close().. So, possible solution is to wait for all cancelled requests on .bdrv_co_drain_begin().

Or alternatively, may be just increase bs->in_flight for CBW filter for each background cancelled request? And decrease when it finish. For this we should add a kind of callback to be called when timed-out coroutine entry finish.

> 
>> +    }
>> -static void coroutine_fn block_copy_async_co_entry(void *opaque)
>> -{
>> -    block_copy_common(opaque);
>> +    ret = call_state->ret;
>> +
>> +    return ret;
> 
> But here we still need to free call_state, right?
> 
>>   }
>>   BlockCopyCallState *block_copy_async(BlockCopyState *s,
> 


-- 
Best regards,
Vladimir