Eric Blake <ebl...@redhat.com> wrote:
> On Thu, Apr 20, 2023 at 12:41:25PM +0200, Juan Quintela wrote:
>> Eric Blake <ebl...@redhat.com> wrote:
>
> ...lots of lines...
>
>> > ---
>> > migration/migration.c | 5 ++---
>> > 1 file changed, 2 insertions(+), 3 deletions(-)
>
> ...describing a tiny change ;)
>
>> >
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index bda47891933..cb0d42c0610 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -3444,13 +3444,11 @@ static void migration_completion(MigrationState *s)
>> > MIGRATION_STATUS_DEVICE);
>> > }
>> > if (ret >= 0) {
>> > + s->block_inactive = inactivate;
>> > qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
>> > ret = qemu_savevm_state_complete_precopy(s->to_dst_file,
>> > false,
>> > inactivate);
>> > }
>> > - if (inactivate && ret >= 0) {
>> > - s->block_inactive = true;
>> > - }
>> > }
>> > qemu_mutex_unlock_iothread();
>>
>> And I still have to look at the file to understand this "simple" patch.
>> (simple in size, not in what it means).
>
> Indeed - hence the long commit message!
>
>>
>> I will add this to my queue, but if you are in the "mood", I would like
>> to remove the declaration of inactivate and change this to something like:
>>
>> if (ret >= 0) {
>> /* Colo don't stop disks in normal operation */
>> s->block_inactive = !migrate_colo_enabled();
>> qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
>> ret = qemu_savevm_state_complete_precopy(s->to_dst_file,
>> false,
>> s->block_inactive);
>> }
>>
>> Or something around that lines?
>
> Yes, that looks like a trivial refactoring that preserves the same
> semantics.
>
>>
>> > @@ -3522,6 +3520,7 @@ fail_invalidate:
>> > bdrv_activate_all(&local_err);
>> > if (local_err) {
>> > error_report_err(local_err);
>> > + s->block_inactive = true;
>> > } else {
>> > s->block_inactive = false;
>> > }
>> > base-commit: 7dbd6f8a27e30fe14adb3d5869097cddf24038d6
>>
>> Just wondering, what git magic creates this line?
>
> git send-email --base=COMMIT_ID
>
> or even 'git config format.useAutoBase whenAble' to try and automate
> the use of this. (If my own git habits were cleaner, of always
> sticking patches in fresh branches, --base=auto is handy; but in
> practice, I tend to send one-off patches like this in the middle of
> 'git rebase' of a larger series, at which point I'm not on a clean
> branch where --base=auto works, so I end up having to manually specify
> one at the command line. Either way, including the base-commit: info
> can be very informative for applying a patch at the branch point then
> rebasing it locally, when attempting to apply the patch sent through
> email hits merge conflicts when applying it directly to changes on
> master in the meantime; I believe 'git am -3' is even able to exploit
> the comment when present to make smarter decisions about which parent
> commit it tries for doing 3-way patch resolution)
Thanks a lot.
It does the right thing for "trivial" stuff, i.e. when I sent a single
patch or a series against qemu/master.
I am not completely sure that it does the right thing when I send a
series on top of my previous pull request.
097387873b (HEAD -> atomic_counters) migration: Make dirty_bytes_last_sync
atomic
3f699a13b2 migration: Make dirty_pages_rate atomic
276a275895 (next) migration: Move qmp_migrate_set_parameters() to options.c
ab13b47801 migration: Move migrate_use_tls() to options.c
ecf5c18eac MAINTAINERS: Add Leonardo and Peter as reviewers
6e5dda696c migration: Disable postcopy + multifd migration
ac5f7bf8e2 (qemu/staging, qemu/master, qemu/HEAD) Merge tag
'migration-20230424-pull-request' of https://gitlab.com/juan.quintela/qemu into
staging
where the branchs are:
qemu/master: obvious
next: whatever is going to be on next migration PULL request, I will
rename this to migration-$date and send this series to the list
1st. I.e. assume they are on list but still not on master.
HEAD/atomic_counters: This is the series that I am sending
I have done:
>> ret = qemu_savevm_state_complete_precopy(s->to_dst_file,
>> false,
>> s->block_inactive);
>> }
>>
>> Or something around that lines?
>
> Yes, that looks like a trivial refactoring that preserves the same
> semantics.
>
>>
>> > @@ -3522,6 +3520,7 @@ fail_invalidate:
>> > bdrv_activate_all(&local_err);
>> > if (local_err) {
>> > error_report_err(local_err);
>> > + s->block_inactive = true;
>> > } else {
>> > s->block_inactive = false;
>> > }
>> > base-commit: 7dbd6f8a27e30fe14adb3d5869097cddf24038d6
>>
>> Just wondering, what git magic creates this line?
>
> git send-email --base=COMMIT_ID
>
> or even 'git config format.useAutoBase whenAble' to try and automate
> the use of this. (If my own git habits were cleaner, of always
> sticking patches in fresh branches, --base=auto is handy; but in
> practice, I tend to send one-off patches like this in the middle of
> 'git rebase' of a larger series, at which point I'm not on a clean
> branch where --base=auto works, so I end up having to manually specify
> one at the command line. Either way, including the base-commit: info
> can be very informative for applying a patch at the branch point then
> rebasing it locally, when attempting to apply the patch sent through
> email hits merge conflicts when applying it directly to changes on
> master in the meantime; I believe 'git am -3' is even able to exploit
> the comment when present to make smarter decisions about which parent
> commit it tries for doing 3-way patch resolution)
Thanks a lot.
It does the right thing for "trivial" stuff, i.e. when I sent a single
patch or a series against qemu/master.
I am not completely sure that it does the right thing when I send a
series on top of my previous pull request.
097387873b (HEAD -> atomic_counters) migration: Make dirty_bytes_last_sync
atomic
3f699a13b2 migration: Make dirty_pages_rate atomic
276a275895 (next) migration: Move qmp_migrate_set_parameters() to options.c
ab13b47801 migration: Move migrate_use_tls() to options.c
ecf5c18eac MAINTAINERS: Add Leonardo and Peter as reviewers
6e5dda696c migration: Disable postcopy + multifd migration
ac5f7bf8e2 (qemu/staging, qemu/master, qemu/HEAD) Merge tag
'migration-20230424-pull-request' of https://gitlab.com/juan.quintela/qemu into
staging
where the branchs are:
qemu/master: obvious
next: whatever is going to be on next migration PULL request, I will
rename this to migration-$date and send this series to the list
1st. I.e. assume they are on list but still not on master.
HEAD/atomic_counters: This is the series that I am sending
I have done first:
git branch --set-upstream-to=qemu/master
Because that is what the "real" master is, migration-$date is just an
intermediate "state".
git format-patch --cover-letter next -o /tmp/kk
In this case both useAutoBase=whenAble and useAutoBase=true do the same
"right" thing.
>From 097387873b2ef1894d5713fdfda8a7b2492476e5 Mon Sep 17 00:00:00 2001
...
Juan Quintela (2):
migration: Make dirty_pages_rate atomic
migration: Make dirty_bytes_last_sync atomic
migration/migration.c | 10 +++++++---
migration/ram.c | 8 +++++---
migration/ram.h | 4 ++--
3 files changed, 14 insertions(+), 8 deletions(-)
base-commit: ac5f7bf8e208cd7893dbb1a9520559e569a4677c
prerequisite-patch-id: 08dd37c2ffd8463398e00cade80765b017200b68
prerequisite-patch-id: 3a1d25d5e4f1f615b6e2c6749dcf891959ca48b5
prerequisite-patch-id: 5607c75cc228370df8953987c390682de3093b65
prerequisite-patch-id: ccb4d94973bd111380e4b50f781eeb6cfa7ce5ff
In obvious cases I do the rebase on top of qemu/master, but that is when
there is no dependencies with the PULL request, and that is not the
"interesting" case.
Thanks again, Juan.
