Re: [Qemu-devel] [PATCH V4 1/3] block: Add bdrv_are_busy()

[Kevin Wolf <kwolf@redhat.com>]

Am 24.07.2012 16:37, schrieb Benoît Canet:
> Le Tuesday 24 Jul 2012 à 15:29:11 (+0200), Kevin Wolf a écrit :
>> Am 24.07.2012 14:55, schrieb Luiz Capitulino:
>>> On Tue, 24 Jul 2012 12:10:39 +0200
>>> Benoît Canet <benoit.canet@irqsave.net> wrote:
>>>
>>>> Le Monday 23 Jul 2012 à 14:15:01 (-0300), Luiz Capitulino a écrit :
>>>>> On Mon, 23 Jul 2012 16:22:58 +0200
>>>>> benoit.canet@gmail.com wrote:
>>>>>
>>>>>> From: Benoît Canet <benoit@irqsave.net>
>>>>>>
>>>>>> bdrv_are_busy will be used to check if any of the bs are in use
>>>>>> or if one of them have a running block job.
>>>>>>
>>>>>> The first user will be qmp_migrate().
>>>>>>
>>>>>> Signed-off-by: Benoit Canet <benoit@irqsave.net>
>>>>>> ---
>>>>>>  block.c |   13 +++++++++++++
>>>>>>  block.h |    2 ++
>>>>>>  2 files changed, 15 insertions(+)
>>>>>>
>>>>>> diff --git a/block.c b/block.c
>>>>>> index ce7eb8f..bc8f160 100644
>>>>>> --- a/block.c
>>>>>> +++ b/block.c
>>>>>> @@ -4027,6 +4027,19 @@ out:
>>>>>>      return ret;
>>>>>>  }
>>>>>>  
>>>>>> +int bdrv_are_busy(void)
>>>>>> +{
>>>>>> +    BlockDriverState *bs;
>>>>>> +
>>>>>> +    QTAILQ_FOREACH(bs, &bdrv_states, list) {
>>>>>> +        if (bs->job || bdrv_in_use(bs)) {
>>>>>> +            return -EBUSY;
>>>>>> +        }
>>>>>> +    }
>>>>>
>>>>> IMO, this should return true/false. The name is a bit misleading too, as it
>>>>> gives the impression that are existing bdrvs are busy. I'd call it
>>>>> bdrv_any_busy() or bdrv_any_in_use().
>>>>
>>>> Hello Anthony,
>>>>
>>>> Stefanha is in favor of returning -EBUSY and Luiz Capitulino would prefer
>>>> the function to return a boolean.
>>>> Could you decide which option is the best ?
>>>
>>> Stefan's opnion certainly has precedence over mine on block layer stuff,
>>> this was just an IMO.
>>>
>>> Stefan, did you consider returning a boolean?
>>
>> I'm with you in this point, Luiz (as well as with the rename to
>> bdrv_is_any_busy). And actually I think Benoît may have misunderstood
>> and Stefan is as well. What he said is:
>>
>>> I think bdrv_have_block_jobs() is too specific and would use
>>> bdrv_in_use(bs) here to give basically an EBUSY-type error.
>>
>> I don't think this was about bool vs. -errno, but more about checking
>> only block jobs vs. all kinds of things that can have a block device in use.
>>
>> Anyway, I believe we came to the conclusion that even the intention of
>> the series is wrong, as in many cases migrating while an image is being
>> streamed is perfectly fine. So the details don't really matter any more.
>>
> 
> Just to be sure.
> 
> In case of a migration with shared storage the migration stops the streaming
> when the switch between vm is done.
> So starting a streaming after the begining of a migration is also right.
> Is that correct ?

Yes, starting streaming itself shouldn't be a problem. Usually streaming
is combined with doing a snapshot first, though, and that could become a
problem if the destination didn't already know the snapshot when it was
started. I believe it's already blocked today.

Kevin