| Message ID | 1708622920-68779-15-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | allow cpr-reboot for vfio | expand |
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 <steven.sistare@oracle.com> Reviewed-by: Peter Xu <peterx@redhat.com>
Steve Sistare <steven.sistare@oracle.com> writes: > Fail the migration request if options are set that are incompatible > with cpr. > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > 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',
On 2/28/2024 2:21 AM, Markus Armbruster wrote: > Steve Sistare <steven.sistare@oracle.com> writes: > >> Fail the migration request if options are set that are incompatible >> with cpr. >> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >> --- >> 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
Steven Sistare <steven.sistare@oracle.com> writes: > On 2/28/2024 2:21 AM, Markus Armbruster wrote: >> Steve Sistare <steven.sistare@oracle.com> writes: >> >>> Fail the migration request if options are set that are incompatible >>> with cpr. >>> >>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> [...] >>> 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.
On 2/28/2024 11:05 AM, Markus Armbruster wrote: > Steven Sistare <steven.sistare@oracle.com> writes: > >> On 2/28/2024 2:21 AM, Markus Armbruster wrote: >>> Steve Sistare <steven.sistare@oracle.com> writes: >>> >>>> Fail the migration request if options are set that are incompatible >>>> with cpr. >>>> >>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > > [...] > >>>> 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
Steven Sistare <steven.sistare@oracle.com> writes: > On 2/28/2024 11:05 AM, Markus Armbruster wrote: >> Steven Sistare <steven.sistare@oracle.com> writes: >> >>> On 2/28/2024 2:21 AM, Markus Armbruster wrote: >>>> Steve Sistare <steven.sistare@oracle.com> writes: >>>> >>>>> Fail the migration request if options are set that are incompatible >>>>> with cpr. >>>>> >>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >> >> [...] >> >>>>> 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?
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-peterx@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,
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-peterx@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
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',
Fail the migration request if options are set that are incompatible with cpr. Signed-off-by: Steve Sistare <steven.sistare@oracle.com> --- migration/migration.c | 17 +++++++++++++++++ qapi/migration.json | 2 ++ 2 files changed, 19 insertions(+)