diff mbox

[V4,1/3] block: Add bdrv_are_busy()

Message ID 1343053380-12133-2-git-send-email-benoit@irqsave.net
State New
Headers show

Commit Message

Benoit Canet July 23, 2012, 2:22 p.m. UTC 
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(+)

Comments

Luiz Capitulino July 23, 2012, 5:15 p.m. UTC | #1 
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().

> +
> +    return 0;
> +}
> +
>  void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
>                         int64_t speed, BlockDriverCompletionFunc *cb,
>                         void *opaque, Error **errp)
> diff --git a/block.h b/block.h
> index c89590d..0a3de2f 100644
> --- a/block.h
> +++ b/block.h
> @@ -337,6 +337,8 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
>  void bdrv_set_in_use(BlockDriverState *bs, int in_use);
>  int bdrv_in_use(BlockDriverState *bs);
>  
> +int bdrv_are_busy(void);
> +
>  enum BlockAcctType {
>      BDRV_ACCT_READ,
>      BDRV_ACCT_WRITE,
Benoît Canet July 24, 2012, 10:10 a.m. UTC | #2 
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 ?

Regards

Benoît

> 
> > +
> > +    return 0;
> > +}
> > +
> >  void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
> >                         int64_t speed, BlockDriverCompletionFunc *cb,
> >                         void *opaque, Error **errp)
> > diff --git a/block.h b/block.h
> > index c89590d..0a3de2f 100644
> > --- a/block.h
> > +++ b/block.h
> > @@ -337,6 +337,8 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
> >  void bdrv_set_in_use(BlockDriverState *bs, int in_use);
> >  int bdrv_in_use(BlockDriverState *bs);
> >  
> > +int bdrv_are_busy(void);
> > +
> >  enum BlockAcctType {
> >      BDRV_ACCT_READ,
> >      BDRV_ACCT_WRITE,
>
Luiz Capitulino July 24, 2012, 12:55 p.m. UTC | #3 
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?

> 
> Regards
> 
> Benoît
> 
> > 
> > > +
> > > +    return 0;
> > > +}
> > > +
> > >  void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
> > >                         int64_t speed, BlockDriverCompletionFunc *cb,
> > >                         void *opaque, Error **errp)
> > > diff --git a/block.h b/block.h
> > > index c89590d..0a3de2f 100644
> > > --- a/block.h
> > > +++ b/block.h
> > > @@ -337,6 +337,8 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
> > >  void bdrv_set_in_use(BlockDriverState *bs, int in_use);
> > >  int bdrv_in_use(BlockDriverState *bs);
> > >  
> > > +int bdrv_are_busy(void);
> > > +
> > >  enum BlockAcctType {
> > >      BDRV_ACCT_READ,
> > >      BDRV_ACCT_WRITE,
> > 
>
Kevin Wolf July 24, 2012, 1:29 p.m. UTC | #4 
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.

Kevin
Benoît Canet July 24, 2012, 2:37 p.m. UTC | #5 
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 ?

Benoît
Kevin Wolf July 24, 2012, 2:42 p.m. UTC | #6 
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
diff mbox

Patch

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;
+        }
+    }
+
+    return 0;
+}
+
 void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
                        int64_t speed, BlockDriverCompletionFunc *cb,
                        void *opaque, Error **errp)
diff --git a/block.h b/block.h
index c89590d..0a3de2f 100644
--- a/block.h
+++ b/block.h
@@ -337,6 +337,8 @@  void bdrv_disable_copy_on_read(BlockDriverState *bs);
 void bdrv_set_in_use(BlockDriverState *bs, int in_use);
 int bdrv_in_use(BlockDriverState *bs);
 
+int bdrv_are_busy(void);
+
 enum BlockAcctType {
     BDRV_ACCT_READ,
     BDRV_ACCT_WRITE,