Re: [PATCH v2 3/6] migration: Remove 'blk/-b' option from migrate commands
On Mon, Apr 29, 2024 at 03:35:02PM -0300, Fabiano Rosas wrote:
> Peter Xu writes:
>
> > On Mon, Apr 29, 2024 at 02:18:57PM -0300, Fabiano Rosas wrote:
> >> Peter Xu writes:
> >>
> >> > On Fri, Apr 26, 2024 at 10:14:05AM -0300, Fabiano Rosas wrote:
> >> >> @@ -2003,21 +1997,7 @@ static bool migrate_prepare(MigrationState *s,
> >> >> bool blk, bool resume,
> >> >> }
> >> >> }
> >> >>
> >> >> -if (blk) {
[1]
> >> >> -if (migrate_colo()) {
> >> >> -error_setg(errp, "No disk migration is required in COLO
> >> >> mode");
> >> >> -return false;
> >> >> -}
> >> >> -if (migrate_block()) {
> >> >> -error_setg(errp, "Command options are incompatible with "
> >> >> - "current migration capabilities");
> >> >> -return false;
> >> >> -}
> >> >> -if (!migrate_cap_set(MIGRATION_CAPABILITY_BLOCK, true, errp)) {
> >> >> -return false;
> >> >> -}
> >> >> -s->must_remove_block_options = true;
> >> >> -}
> >> >> +s->must_remove_block_options = true;
> >> >
> >> > Can we drop this var too? Perhaps with block_cleanup_parameters()?
> >>
> >> Yes, Markus mentioned it in v1 already. Take a look there. There's
> >> several other declarations I missed. v3 is coming soon.
> >
> > Right, noticed that it's removed actually in the next patch.
> >
> > But iiuc it can already been removed in this patch. If we want to remove
> > it in the next, logically we should set must_remove_block_options=false
> > here, though.. So maybe easier to just drop it here.
>
> Ah I see what you mean. I thought you're just asking for the removal
> overall.
>
> But block_cleanup_parameters sets the block capability to false and the
> whole block migration only goes away in the next patch. I think we need
> to keep this as true to preserve behavior. In theory, after this patch
> people could still use the block migration just fine by setting the cap.
I'm reading this patch the same as "blk==false" always above [1]. In that
case, only if we keep must_remove_block_options=false could we maintain the
behavior? Otherwise at the end of migration (or cancellations) QEMU can
wrongly clear MIGRATION_CAPABILITY_BLOCK bit if the user set it manually?
(But hey, we're discussing whoever applies this patch only without the
rest.. definitely not a huge deal :)
--
Peter Xu
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 3/6] migration: Remove 'blk/-b' option from migrate commands
Peter Xu writes:
> On Mon, Apr 29, 2024 at 02:18:57PM -0300, Fabiano Rosas wrote:
>> Peter Xu writes:
>>
>> > On Fri, Apr 26, 2024 at 10:14:05AM -0300, Fabiano Rosas wrote:
>> >> @@ -2003,21 +1997,7 @@ static bool migrate_prepare(MigrationState *s,
>> >> bool blk, bool resume,
>> >> }
>> >> }
>> >>
>> >> -if (blk) {
>> >> -if (migrate_colo()) {
>> >> -error_setg(errp, "No disk migration is required in COLO
>> >> mode");
>> >> -return false;
>> >> -}
>> >> -if (migrate_block()) {
>> >> -error_setg(errp, "Command options are incompatible with "
>> >> - "current migration capabilities");
>> >> -return false;
>> >> -}
>> >> -if (!migrate_cap_set(MIGRATION_CAPABILITY_BLOCK, true, errp)) {
>> >> -return false;
>> >> -}
>> >> -s->must_remove_block_options = true;
>> >> -}
>> >> +s->must_remove_block_options = true;
>> >
>> > Can we drop this var too? Perhaps with block_cleanup_parameters()?
>>
>> Yes, Markus mentioned it in v1 already. Take a look there. There's
>> several other declarations I missed. v3 is coming soon.
>
> Right, noticed that it's removed actually in the next patch.
>
> But iiuc it can already been removed in this patch. If we want to remove
> it in the next, logically we should set must_remove_block_options=false
> here, though.. So maybe easier to just drop it here.
Ah I see what you mean. I thought you're just asking for the removal
overall.
But block_cleanup_parameters sets the block capability to false and the
whole block migration only goes away in the next patch. I think we need
to keep this as true to preserve behavior. In theory, after this patch
people could still use the block migration just fine by setting the cap.
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 3/6] migration: Remove 'blk/-b' option from migrate commands
On Mon, Apr 29, 2024 at 02:18:57PM -0300, Fabiano Rosas wrote:
> Peter Xu writes:
>
> > On Fri, Apr 26, 2024 at 10:14:05AM -0300, Fabiano Rosas wrote:
> >> @@ -2003,21 +1997,7 @@ static bool migrate_prepare(MigrationState *s, bool
> >> blk, bool resume,
> >> }
> >> }
> >>
> >> -if (blk) {
> >> -if (migrate_colo()) {
> >> -error_setg(errp, "No disk migration is required in COLO
> >> mode");
> >> -return false;
> >> -}
> >> -if (migrate_block()) {
> >> -error_setg(errp, "Command options are incompatible with "
> >> - "current migration capabilities");
> >> -return false;
> >> -}
> >> -if (!migrate_cap_set(MIGRATION_CAPABILITY_BLOCK, true, errp)) {
> >> -return false;
> >> -}
> >> -s->must_remove_block_options = true;
> >> -}
> >> +s->must_remove_block_options = true;
> >
> > Can we drop this var too? Perhaps with block_cleanup_parameters()?
>
> Yes, Markus mentioned it in v1 already. Take a look there. There's
> several other declarations I missed. v3 is coming soon.
Right, noticed that it's removed actually in the next patch.
But iiuc it can already been removed in this patch. If we want to remove
it in the next, logically we should set must_remove_block_options=false
here, though.. So maybe easier to just drop it here.
--
Peter Xu
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 3/6] migration: Remove 'blk/-b' option from migrate commands
Peter Xu writes:
> On Fri, Apr 26, 2024 at 10:14:05AM -0300, Fabiano Rosas wrote:
>> @@ -2003,21 +1997,7 @@ static bool migrate_prepare(MigrationState *s, bool
>> blk, bool resume,
>> }
>> }
>>
>> -if (blk) {
>> -if (migrate_colo()) {
>> -error_setg(errp, "No disk migration is required in COLO mode");
>> -return false;
>> -}
>> -if (migrate_block()) {
>> -error_setg(errp, "Command options are incompatible with "
>> - "current migration capabilities");
>> -return false;
>> -}
>> -if (!migrate_cap_set(MIGRATION_CAPABILITY_BLOCK, true, errp)) {
>> -return false;
>> -}
>> -s->must_remove_block_options = true;
>> -}
>> +s->must_remove_block_options = true;
>
> Can we drop this var too? Perhaps with block_cleanup_parameters()?
Yes, Markus mentioned it in v1 already. Take a look there. There's
several other declarations I missed. v3 is coming soon.
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 3/6] migration: Remove 'blk/-b' option from migrate commands
On Fri, Apr 26, 2024 at 10:14:05AM -0300, Fabiano Rosas wrote:
> @@ -2003,21 +1997,7 @@ static bool migrate_prepare(MigrationState *s, bool
> blk, bool resume,
> }
> }
>
> -if (blk) {
> -if (migrate_colo()) {
> -error_setg(errp, "No disk migration is required in COLO mode");
> -return false;
> -}
> -if (migrate_block()) {
> -error_setg(errp, "Command options are incompatible with "
> - "current migration capabilities");
> -return false;
> -}
> -if (!migrate_cap_set(MIGRATION_CAPABILITY_BLOCK, true, errp)) {
> -return false;
> -}
> -s->must_remove_block_options = true;
> -}
> +s->must_remove_block_options = true;
Can we drop this var too? Perhaps with block_cleanup_parameters()?
--
Peter Xu
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
