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

[Hanna Reitz <hreitz@redhat.com>]

On 01.04.22 18:08, Vladimir Sementsov-Ogievskiy wrote:
> 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.

in_flight sounds good to me.  That would automatically work for 
draining, right?