[Qemu-devel] [PATCH V4 0/3] Block migration if any of the block device is busy

[Qemu-devel] [PATCH V4 0/3] Block migration if any of the block device is busy

[benoit.canet]

From: Benoît Canet <benoit@irqsave.net>

This patchset is designed to avoid starting a live migration while any of
the block device is busy.

Tested with the following sequence:

QEMU 1.1.50 monitor - type 'help' for more information
(qemu) block_stream virtio0 1k
(qemu) migrate tcp:localhost:4444
migrate: Migration is blocked by streaming
(qemu)  block_job_cancel virtio0
(qemu)  migrate tcp:localhost:4444
migrate: Connection can not be completed immediately
(qemu) 
=> migration then succeed

in v2:
stefanha: Rename bdrv_have_block_jobs() to bdrv_are_busy() and make it return -EBUSY.
paolo: remove spurious bdrv_close()

in v3
pm215: rewrite confusing error message

in v4:
stefanha: make the error message more general

Benoît Canet (3):
  block: Add bdrv_are_busy()
  qerror: Add error telling that block dev usage prevents migration
  migration: block migration when streaming block jobs are running.

 block.c     |   13 +++++++++++++
 block.h     |    2 ++
 migration.c |    5 +++++
 qerror.c    |    4 ++++
 qerror.h    |    3 +++
 5 files changed, 27 insertions(+)

-- 
1.7.9.5

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

[benoit.canet]

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;
+        }
+    }
+
+    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,
-- 
1.7.9.5

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

[Luiz Capitulino]

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,

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

[Benoît Canet]

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,
> 

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

[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?

> 
> 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,
> > 
> 

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

[Kevin Wolf]

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

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

[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 ?

Benoît

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

[Kevin Wolf]

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

[Qemu-devel] [PATCH V4 2/3] qerror: Add error telling that block dev usage prevents migration

[benoit.canet]

From: Benoît Canet <benoit@irqsave.net>

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+)

diff --git a/qerror.c b/qerror.c
index 92c4eff..d2e76ca 100644
--- a/qerror.c
+++ b/qerror.c
@@ -283,6 +283,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Could not set password",
     },
     {
+        .error_fmt = QERR_BLOCK_DEV_IN_USE_MIGRATION_PREVENTED,
+        .desc      = "Block device in use migration prevented",
+    },
+    {
         .error_fmt = QERR_TOO_MANY_FILES,
         .desc      = "Too many open files",
     },
diff --git a/qerror.h b/qerror.h
index b4c8758..2ef40b5 100644
--- a/qerror.h
+++ b/qerror.h
@@ -233,6 +233,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_SET_PASSWD_FAILED \
     "{ 'class': 'SetPasswdFailed', 'data': {} }"
 
+#define QERR_BLOCK_DEV_IN_USE_MIGRATION_PREVENTED \
+    "{ 'class': 'BlockDevInUseMigrationPrevented', 'data': {} }"
+
 #define QERR_TOO_MANY_FILES \
     "{ 'class': 'TooManyFiles', 'data': {} }"
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH V4 2/3] qerror: Add error telling that block dev usage prevents migration

[Luiz Capitulino]

On Mon, 23 Jul 2012 16:22:59 +0200
benoit.canet@gmail.com wrote:

> From: Benoît Canet <benoit@irqsave.net>
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  qerror.c |    4 ++++
>  qerror.h |    3 +++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/qerror.c b/qerror.c
> index 92c4eff..d2e76ca 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -283,6 +283,10 @@ static const QErrorStringTable qerror_table[] = {
>          .desc      = "Could not set password",
>      },
>      {
> +        .error_fmt = QERR_BLOCK_DEV_IN_USE_MIGRATION_PREVENTED,
> +        .desc      = "Block device in use migration prevented",
> +    },

Let's avoid an specific error. We have two options:

 1. Use QERR_MIGRATION_NOT_SUPPORTED

 2. Add QERR_MIGRATION_BLOCKED

> +    {
>          .error_fmt = QERR_TOO_MANY_FILES,
>          .desc      = "Too many open files",
>      },
> diff --git a/qerror.h b/qerror.h
> index b4c8758..2ef40b5 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -233,6 +233,9 @@ QError *qobject_to_qerror(const QObject *obj);
>  #define QERR_SET_PASSWD_FAILED \
>      "{ 'class': 'SetPasswdFailed', 'data': {} }"
>  
> +#define QERR_BLOCK_DEV_IN_USE_MIGRATION_PREVENTED \
> +    "{ 'class': 'BlockDevInUseMigrationPrevented', 'data': {} }"
> +
>  #define QERR_TOO_MANY_FILES \
>      "{ 'class': 'TooManyFiles', 'data': {} }"
>  

[Qemu-devel] [PATCH V4 3/3] migration: block migration when any of the block device is busy

[benoit.canet]

From: Benoît Canet <benoit@irqsave.net>

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 migration.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/migration.c b/migration.c
index 8db1b43..3d68996 100644
--- a/migration.c
+++ b/migration.c
@@ -425,6 +425,11 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
         return;
     }
 
+    if (bdrv_are_busy()) {
+        error_set(errp, QERR_BLOCK_DEV_IN_USE_MIGRATION_PREVENTED);
+        return;
+    }
+
     s = migrate_init(&params);
 
     if (strstart(uri, "tcp:", &p)) {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH V4 0/3] Block migration if any of the block device is busy

[Stefan Hajnoczi]

On Mon, Jul 23, 2012 at 3:22 PM,  <benoit.canet@gmail.com> wrote:
> From: Benoît Canet <benoit@irqsave.net>
>
> This patchset is designed to avoid starting a live migration while any of
> the block device is busy.
>
> Tested with the following sequence:
>
> QEMU 1.1.50 monitor - type 'help' for more information
> (qemu) block_stream virtio0 1k
> (qemu) migrate tcp:localhost:4444
> migrate: Migration is blocked by streaming
> (qemu)  block_job_cancel virtio0
> (qemu)  migrate tcp:localhost:4444
> migrate: Connection can not be completed immediately
> (qemu)
> => migration then succeed
>
> in v2:
> stefanha: Rename bdrv_have_block_jobs() to bdrv_are_busy() and make it return -EBUSY.
> paolo: remove spurious bdrv_close()
>
> in v3
> pm215: rewrite confusing error message
>
> in v4:
> stefanha: make the error message more general
>
> Benoît Canet (3):
>   block: Add bdrv_are_busy()
>   qerror: Add error telling that block dev usage prevents migration
>   migration: block migration when streaming block jobs are running.
>
>  block.c     |   13 +++++++++++++
>  block.h     |    2 ++
>  migration.c |    5 +++++
>  qerror.c    |    4 ++++
>  qerror.h    |    3 +++
>  5 files changed, 27 insertions(+)
>
> --
> 1.7.9.5

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH V4 0/3] Block migration if any of the block device is busy

[Kevin Wolf]

Am 23.07.2012 16:22, schrieb benoit.canet@gmail.com:
> From: Benoît Canet <benoit@irqsave.net>
> 
> This patchset is designed to avoid starting a live migration while any of
> the block device is busy.
> 
> Tested with the following sequence:
> 
> QEMU 1.1.50 monitor - type 'help' for more information
> (qemu) block_stream virtio0 1k
> (qemu) migrate tcp:localhost:4444
> migrate: Migration is blocked by streaming
> (qemu)  block_job_cancel virtio0
> (qemu)  migrate tcp:localhost:4444
> migrate: Connection can not be completed immediately
> (qemu) 
> => migration then succeed

Maybe I'm missing the obvious, but why? Migration will stop the
streaming if it isn't restarted explicitly on the destination, but I
think that's expected.

Also, there are migration blockers. Wouldn't it be better to use them
instead of adding more special-case code to migration.c?

Kevin

Re: [Qemu-devel] [PATCH V4 0/3] Block migration if any of the block device is busy

[Stefan Hajnoczi]

On Tue, Jul 24, 2012 at 10:52 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 23.07.2012 16:22, schrieb benoit.canet@gmail.com:
>> From: Benoît Canet <benoit@irqsave.net>
>>
>> This patchset is designed to avoid starting a live migration while any of
>> the block device is busy.
>>
>> Tested with the following sequence:
>>
>> QEMU 1.1.50 monitor - type 'help' for more information
>> (qemu) block_stream virtio0 1k
>> (qemu) migrate tcp:localhost:4444
>> migrate: Migration is blocked by streaming
>> (qemu)  block_job_cancel virtio0
>> (qemu)  migrate tcp:localhost:4444
>> migrate: Connection can not be completed immediately
>> (qemu)
>> => migration then succeed
>
> Maybe I'm missing the obvious, but why? Migration will stop the
> streaming if it isn't restarted explicitly on the destination, but I
> think that's expected.

Hmm...maybe this is a policy decision.  I figure if you are running
image streaming and try to migrate, chances are you're migration will
break on the destination host because you were trying to do pre-copy
storage migration and never finished.

Stefan

Re: [Qemu-devel] [PATCH V4 0/3] Block migration if any of the block device is busy

[Kevin Wolf]

Am 24.07.2012 12:04, schrieb Stefan Hajnoczi:
> On Tue, Jul 24, 2012 at 10:52 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 23.07.2012 16:22, schrieb benoit.canet@gmail.com:
>>> From: Benoît Canet <benoit@irqsave.net>
>>>
>>> This patchset is designed to avoid starting a live migration while any of
>>> the block device is busy.
>>>
>>> Tested with the following sequence:
>>>
>>> QEMU 1.1.50 monitor - type 'help' for more information
>>> (qemu) block_stream virtio0 1k
>>> (qemu) migrate tcp:localhost:4444
>>> migrate: Migration is blocked by streaming
>>> (qemu)  block_job_cancel virtio0
>>> (qemu)  migrate tcp:localhost:4444
>>> migrate: Connection can not be completed immediately
>>> (qemu)
>>> => migration then succeed
>>
>> Maybe I'm missing the obvious, but why? Migration will stop the
>> streaming if it isn't restarted explicitly on the destination, but I
>> think that's expected.
> 
> Hmm...maybe this is a policy decision.  I figure if you are running
> image streaming and try to migrate, chances are you're migration will
> break on the destination host because you were trying to do pre-copy
> storage migration and never finished.

Sure, if you're migrating without shared storage, you always need to
know what you're doing.

But you could be doing a post-copy storage migration and migrate to the
next host before it has completed. I think this is a completely
legitimate action that shouldn't be blocked.

Kevin

Re: [Qemu-devel] [PATCH V4 0/3] Block migration if any of the block device is busy

[Paolo Bonzini]

> > Hmm...maybe this is a policy decision.  I figure if you are running
> > image streaming and try to migrate, chances are you're migration
> > will break on the destination host because you were trying to do
> > pre-copy storage migration and never finished.

Streaming is post-copy, and can be interrupted every time you want.

Mirroring is pre-copy.  That's another story, but even then the way
to "emulate" migrate -b/-i (and make it much more efficient, as you can
see from the last few patches I just posted) is exactly to run mirroring
in parallel to RAM migration.  This is relatively difficult to do by hand
(though I'd like to improve it in 1.3 if only to get rid of block-migration.c)
and requires giving the user a considerable amount of rope, but it is doable.

So, thinking about it a bit more, this patch looks like a good idea in principle,
but actually it is counterproductive.

Paolo