Re: [PATCH V4 14/14] migration: options incompatible with cpr
On 2/29/2024 12:40 AM, Peter Xu wrote: > On Thu, Feb 29, 2024 at 06:31:26AM +0100, Markus Armbruster wrote: >> Hmm, perhaps Peter can still squash in the corrections before posting >> his PR. Peter? > > The PR was sent yesterday, it's already in PeterM's -staging. I worry it's > a bit late to call a stop now. > > https://lore.kernel.org/qemu-devel/20240228051315.400759-23-pet...@redhat.com > > Steve, could you provide a standalone patch for the update? Then I'll do > the best accordingly (e.g. if the PR failed to apply I'll squash when > resend v2; or I'll pick it up for the next). I sent the patch. I also re-wrapped all cpr-reboot paragraphs to 70 columns and addressed Markus' other comments on "migration: update cpr-reboot description". Peter, if you squash it, the last sentence "cpr-reboot may not be used ..." squashes into migration: options incompatible with cpr and everything else squashes into migration: update cpr-reboot description - Steve
Re: [PATCH V4 14/14] migration: options incompatible with cpr
On Thu, Feb 29, 2024 at 06:31:26AM +0100, Markus Armbruster wrote: > Hmm, perhaps Peter can still squash in the corrections before posting > his PR. Peter? The PR was sent yesterday, it's already in PeterM's -staging. I worry it's a bit late to call a stop now. https://lore.kernel.org/qemu-devel/20240228051315.400759-23-pet...@redhat.com Steve, could you provide a standalone patch for the update? Then I'll do the best accordingly (e.g. if the PR failed to apply I'll squash when resend v2; or I'll pick it up for the next). Thanks, -- Peter Xu
Re: [PATCH V4 14/14] migration: options incompatible with cpr
Steven Sistare writes: > On 2/28/2024 11:05 AM, Markus Armbruster wrote: >> Steven Sistare writes: >> >>> On 2/28/2024 2:21 AM, Markus Armbruster wrote: Steve Sistare writes: > Fail the migration request if options are set that are incompatible > with cpr. > > Signed-off-by: Steve Sistare >> >> [...] >> > diff --git a/qapi/migration.json b/qapi/migration.json > index 0990297..c6bfe2e 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -657,6 +657,8 @@ > # shared backend must be be non-volatile across reboot, such as by > backing > # it with a dax device. > # > +# cpr-reboot may not be used with postcopy, colo, or > background-snapshot. > +# @cpr-reboot COLO Wrap the line: # @cpr-reboot may not be used with postcopy, COLO, or # background-snapshot. This doesn't tell the reader what settings exactly do not work with @cpr-reboot. For instance "background-snapshot" is about enabling migration capability @background-snapshot. We could write something like "is incompatible with enabling migration capability @background-snapshot". Same for the other two. Worthwhile? >>> >>> I initially was more precise exactly as you suggest, but I realized that >>> postcopy encompasses multiple capabilities, and I did not want to enumerate >>> them, nor require new capabilities related to these 3 to be listed here >>> if/when they are created, so I generalized. A keyword search in this file >>> gives unambiguous matches. >>> >>> Peter pushed the patch a few hours before you sent this. >> >> Okay. >> >> A followup patch to correct @cpr-reboot, COLO and line wrapping would be >> nice. > > Will do - steve Hmm, perhaps Peter can still squash in the corrections before posting his PR. Peter?
Re: [PATCH V4 14/14] migration: options incompatible with cpr
On 2/28/2024 11:05 AM, Markus Armbruster wrote: > Steven Sistare writes: > >> On 2/28/2024 2:21 AM, Markus Armbruster wrote: >>> Steve Sistare writes: >>> Fail the migration request if options are set that are incompatible with cpr. Signed-off-by: Steve Sistare > > [...] > diff --git a/qapi/migration.json b/qapi/migration.json index 0990297..c6bfe2e 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -657,6 +657,8 @@ # shared backend must be be non-volatile across reboot, such as by backing # it with a dax device. # +# cpr-reboot may not be used with postcopy, colo, or background-snapshot. +# >>> >>> @cpr-reboot >>> >>> COLO >>> >>> Wrap the line: >>> >>># @cpr-reboot may not be used with postcopy, COLO, or >>># background-snapshot. >>> >>> This doesn't tell the reader what settings exactly do not work with >>> @cpr-reboot. >>> >>> For instance "background-snapshot" is about enabling migration >>> capability @background-snapshot. We could write something like "is >>> incompatible with enabling migration capability @background-snapshot". >>> >>> Same for the other two. Worthwhile? >> >> I initially was more precise exactly as you suggest, but I realized that >> postcopy encompasses multiple capabilities, and I did not want to enumerate >> them, nor require new capabilities related to these 3 to be listed here >> if/when they are created, so I generalized. A keyword search in this file >> gives unambiguous matches. >> >> Peter pushed the patch a few hours before you sent this. > > Okay. > > A followup patch to correct @cpr-reboot, COLO and line wrapping would be > nice. Will do - steve
Re: [PATCH V4 14/14] migration: options incompatible with cpr
Steven Sistare writes: > On 2/28/2024 2:21 AM, Markus Armbruster wrote: >> Steve Sistare writes: >> >>> Fail the migration request if options are set that are incompatible >>> with cpr. >>> >>> Signed-off-by: Steve Sistare [...] >>> diff --git a/qapi/migration.json b/qapi/migration.json >>> index 0990297..c6bfe2e 100644 >>> --- a/qapi/migration.json >>> +++ b/qapi/migration.json >>> @@ -657,6 +657,8 @@ >>> # shared backend must be be non-volatile across reboot, such as by >>> backing >>> # it with a dax device. >>> # >>> +# cpr-reboot may not be used with postcopy, colo, or >>> background-snapshot. >>> +# >> >> @cpr-reboot >> >> COLO >> >> Wrap the line: >> >># @cpr-reboot may not be used with postcopy, COLO, or >># background-snapshot. >> >> This doesn't tell the reader what settings exactly do not work with >> @cpr-reboot. >> >> For instance "background-snapshot" is about enabling migration >> capability @background-snapshot. We could write something like "is >> incompatible with enabling migration capability @background-snapshot". >> >> Same for the other two. Worthwhile? > > I initially was more precise exactly as you suggest, but I realized that > postcopy encompasses multiple capabilities, and I did not want to enumerate > them, nor require new capabilities related to these 3 to be listed here > if/when they are created, so I generalized. A keyword search in this file > gives unambiguous matches. > > Peter pushed the patch a few hours before you sent this. Okay. A followup patch to correct @cpr-reboot, COLO and line wrapping would be nice.
Re: [PATCH V4 14/14] migration: options incompatible with cpr
On 2/28/2024 2:21 AM, Markus Armbruster wrote:
> Steve Sistare writes:
>
>> Fail the migration request if options are set that are incompatible
>> with cpr.
>>
>> Signed-off-by: Steve Sistare
>> ---
>> migration/migration.c | 17 +
>> qapi/migration.json | 2 ++
>> 2 files changed, 19 insertions(+)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 90a9094..7652fd4 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1953,6 +1953,23 @@ static bool migrate_prepare(MigrationState *s, bool
>> blk, bool blk_inc,
>> return false;
>> }
>>
>> +if (migrate_mode_is_cpr(s)) {
>> +const char *conflict = NULL;
>> +
>> +if (migrate_postcopy()) {
>> +conflict = "postcopy";
>> +} else if (migrate_background_snapshot()) {
>> +conflict = "background snapshot";
>> +} else if (migrate_colo()) {
>> +conflict = "COLO";
>> +}
>> +
>> +if (conflict) {
>> +error_setg(errp, "Cannot use %s with CPR", conflict);
>> +return false;
>> +}
>> +}
>> +
>> if (blk || blk_inc) {
>> if (migrate_colo()) {
>> error_setg(errp, "No disk migration is required in COLO mode");
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 0990297..c6bfe2e 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -657,6 +657,8 @@
>> # shared backend must be be non-volatile across reboot, such as by
>> backing
>> # it with a dax device.
>> #
>> +# cpr-reboot may not be used with postcopy, colo, or
>> background-snapshot.
>> +#
>
> @cpr-reboot
>
> COLO
>
> Wrap the line:
>
># @cpr-reboot may not be used with postcopy, COLO, or
># background-snapshot.
>
> This doesn't tell the reader what settings exactly do not work with
> @cpr-reboot.
>
> For instance "background-snapshot" is about enabling migration
> capability @background-snapshot. We could write something like "is
> incompatible with enabling migration capability @background-snapshot".
>
> Same for the other two. Worthwhile?
I initially was more precise exactly as you suggest, but I realized that
postcopy encompasses multiple capabilities, and I did not want to enumerate
them, nor require new capabilities related to these 3 to be listed here
if/when they are created, so I generalized. A keyword search in this file
gives unambiguous matches.
Peter pushed the patch a few hours before you sent this.
- Steve
Re: [PATCH V4 14/14] migration: options incompatible with cpr
Steve Sistare writes:
> Fail the migration request if options are set that are incompatible
> with cpr.
>
> Signed-off-by: Steve Sistare
> ---
> migration/migration.c | 17 +
> qapi/migration.json | 2 ++
> 2 files changed, 19 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 90a9094..7652fd4 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1953,6 +1953,23 @@ static bool migrate_prepare(MigrationState *s, bool
> blk, bool blk_inc,
> return false;
> }
>
> +if (migrate_mode_is_cpr(s)) {
> +const char *conflict = NULL;
> +
> +if (migrate_postcopy()) {
> +conflict = "postcopy";
> +} else if (migrate_background_snapshot()) {
> +conflict = "background snapshot";
> +} else if (migrate_colo()) {
> +conflict = "COLO";
> +}
> +
> +if (conflict) {
> +error_setg(errp, "Cannot use %s with CPR", conflict);
> +return false;
> +}
> +}
> +
> if (blk || blk_inc) {
> if (migrate_colo()) {
> error_setg(errp, "No disk migration is required in COLO mode");
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 0990297..c6bfe2e 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -657,6 +657,8 @@
> # shared backend must be be non-volatile across reboot, such as by
> backing
> # it with a dax device.
> #
> +# cpr-reboot may not be used with postcopy, colo, or background-snapshot.
> +#
@cpr-reboot
COLO
Wrap the line:
# @cpr-reboot may not be used with postcopy, COLO, or
# background-snapshot.
This doesn't tell the reader what settings exactly do not work with
@cpr-reboot.
For instance "background-snapshot" is about enabling migration
capability @background-snapshot. We could write something like "is
incompatible with enabling migration capability @background-snapshot".
Same for the other two. Worthwhile?
> # (since 8.2)
> ##
> { 'enum': 'MigMode',
Re: [PATCH V4 14/14] migration: options incompatible with cpr
On Thu, Feb 22, 2024 at 09:28:40AM -0800, Steve Sistare wrote: > Fail the migration request if options are set that are incompatible > with cpr. > > Signed-off-by: Steve Sistare Reviewed-by: Peter Xu -- Peter Xu
[PATCH V4 14/14] migration: options incompatible with cpr
Fail the migration request if options are set that are incompatible
with cpr.
Signed-off-by: Steve Sistare
---
migration/migration.c | 17 +
qapi/migration.json | 2 ++
2 files changed, 19 insertions(+)
diff --git a/migration/migration.c b/migration/migration.c
index 90a9094..7652fd4 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1953,6 +1953,23 @@ static bool migrate_prepare(MigrationState *s, bool blk,
bool blk_inc,
return false;
}
+if (migrate_mode_is_cpr(s)) {
+const char *conflict = NULL;
+
+if (migrate_postcopy()) {
+conflict = "postcopy";
+} else if (migrate_background_snapshot()) {
+conflict = "background snapshot";
+} else if (migrate_colo()) {
+conflict = "COLO";
+}
+
+if (conflict) {
+error_setg(errp, "Cannot use %s with CPR", conflict);
+return false;
+}
+}
+
if (blk || blk_inc) {
if (migrate_colo()) {
error_setg(errp, "No disk migration is required in COLO mode");
diff --git a/qapi/migration.json b/qapi/migration.json
index 0990297..c6bfe2e 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -657,6 +657,8 @@
# shared backend must be be non-volatile across reboot, such as by backing
# it with a dax device.
#
+# cpr-reboot may not be used with postcopy, colo, or background-snapshot.
+#
# (since 8.2)
##
{ 'enum': 'MigMode',
--
1.8.3.1
