| Message ID | 20221103161620.13120-4-avihaih@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/migration: Implement VFIO migration protocol v2 | expand |
On 11/3/22 19:16, Avihai Horon wrote: > From: Juan Quintela <quintela@redhat.com> > > And it appears that what is wrong is the code. During bulk stage we > need to make sure that some block is dirty, but no games with > max_size at all. :) That made me interested in, why we need this one block, so I decided to search through the history. And what I see? Haha, that was my commit 04636dc410b163c "migration/block: fix pending() return value" [1], which you actually revert with this patch. So, at least we should note, that it's a revert of [1]. Still that this will reintroduce the bug fixed by [1]. As I understand the problem is (was) that in block_save_complete() we finalize only dirty blocks, but don't finalize the bulk phase if it's not finalized yet. So, we can fix block_save_complete() to finalize the bulk phase, instead of hacking with pending in [1]. Interesting, why we need this one block, described in the comment you refer to? Was it an incomplete workaround for the same problem, described in [1]? If so, we can fix block_save_complete() and remove this if() together with the comment from block_save_pending(). > > Signed-off-by: Juan Quintela <quintela@redhat.com> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > migration/block.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/migration/block.c b/migration/block.c > index b3d680af75..39ce4003c6 100644 > --- a/migration/block.c > +++ b/migration/block.c > @@ -879,8 +879,8 @@ static void block_save_pending(void *opaque, uint64_t max_size, > blk_mig_unlock(); > > /* Report at least one block pending during bulk phase */ > - if (pending <= max_size && !block_mig_state.bulk_completed) { > - pending = max_size + BLK_MIG_BLOCK_SIZE; > + if (!pending && !block_mig_state.bulk_completed) { > + pending = BLK_MIG_BLOCK_SIZE; > } > > trace_migration_block_save_pending(pending);
On 11/8/22 21:36, Vladimir Sementsov-Ogievskiy wrote: > On 11/3/22 19:16, Avihai Horon wrote: >> From: Juan Quintela <quintela@redhat.com> >> >> And it appears that what is wrong is the code. During bulk stage we >> need to make sure that some block is dirty, but no games with >> max_size at all. > > :) That made me interested in, why we need this one block, so I decided to search through the history. > > And what I see? Haha, that was my commit 04636dc410b163c "migration/block: fix pending() return value" [1], which you actually revert with this patch. > > So, at least we should note, that it's a revert of [1]. > > Still that this will reintroduce the bug fixed by [1]. > > As I understand the problem is (was) that in block_save_complete() we finalize only dirty blocks, but don't finalize the bulk phase if it's not finalized yet. So, we can fix block_save_complete() to finalize the bulk phase, instead of hacking with pending in [1]. > > Interesting, why we need this one block, described in the comment you refer to? Was it an incomplete workaround for the same problem, described in [1]? If so, we can fix block_save_complete() and remove this if() together with the comment from block_save_pending(). > PS: Don't we want to deprecate block migration? Is it really used in production? block-mirror is a recommended way to migrate block devices.
On 08/11/2022 20:36, Vladimir Sementsov-Ogievskiy wrote: > External email: Use caution opening links or attachments > > > On 11/3/22 19:16, Avihai Horon wrote: >> From: Juan Quintela <quintela@redhat.com> >> >> And it appears that what is wrong is the code. During bulk stage we >> need to make sure that some block is dirty, but no games with >> max_size at all. > > :) That made me interested in, why we need this one block, so I > decided to search through the history. > > And what I see? Haha, that was my commit 04636dc410b163c > "migration/block: fix pending() return value" [1], which you actually > revert with this patch. > > So, at least we should note, that it's a revert of [1]. > > Still that this will reintroduce the bug fixed by [1]. > > As I understand the problem is (was) that in block_save_complete() we > finalize only dirty blocks, but don't finalize the bulk phase if it's > not finalized yet. So, we can fix block_save_complete() to finalize > the bulk phase, instead of hacking with pending in [1]. > > Interesting, why we need this one block, described in the comment you > refer to? Was it an incomplete workaround for the same problem, > described in [1]? If so, we can fix block_save_complete() and remove > this if() together with the comment from block_save_pending(). > I am not familiar with block migration. I can drop this patch in next version. Juan/Stefan, could you help here? >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >> --- >> migration/block.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/migration/block.c b/migration/block.c >> index b3d680af75..39ce4003c6 100644 >> --- a/migration/block.c >> +++ b/migration/block.c >> @@ -879,8 +879,8 @@ static void block_save_pending(void *opaque, >> uint64_t max_size, >> blk_mig_unlock(); >> >> /* Report at least one block pending during bulk phase */ >> - if (pending <= max_size && !block_mig_state.bulk_completed) { >> - pending = max_size + BLK_MIG_BLOCK_SIZE; >> + if (!pending && !block_mig_state.bulk_completed) { >> + pending = BLK_MIG_BLOCK_SIZE; >> } >> >> trace_migration_block_save_pending(pending); > > -- > Best regards, > Vladimir >
Ping. On 10/11/2022 15:38, Avihai Horon wrote: > > On 08/11/2022 20:36, Vladimir Sementsov-Ogievskiy wrote: >> External email: Use caution opening links or attachments >> >> >> On 11/3/22 19:16, Avihai Horon wrote: >>> From: Juan Quintela <quintela@redhat.com> >>> >>> And it appears that what is wrong is the code. During bulk stage we >>> need to make sure that some block is dirty, but no games with >>> max_size at all. >> >> :) That made me interested in, why we need this one block, so I >> decided to search through the history. >> >> And what I see? Haha, that was my commit 04636dc410b163c >> "migration/block: fix pending() return value" [1], which you actually >> revert with this patch. >> >> So, at least we should note, that it's a revert of [1]. >> >> Still that this will reintroduce the bug fixed by [1]. >> >> As I understand the problem is (was) that in block_save_complete() we >> finalize only dirty blocks, but don't finalize the bulk phase if it's >> not finalized yet. So, we can fix block_save_complete() to finalize >> the bulk phase, instead of hacking with pending in [1]. >> >> Interesting, why we need this one block, described in the comment you >> refer to? Was it an incomplete workaround for the same problem, >> described in [1]? If so, we can fix block_save_complete() and remove >> this if() together with the comment from block_save_pending(). >> > I am not familiar with block migration. > I can drop this patch in next version. > > Juan/Stefan, could you help here? > >>> >>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >>> --- >>> migration/block.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/migration/block.c b/migration/block.c >>> index b3d680af75..39ce4003c6 100644 >>> --- a/migration/block.c >>> +++ b/migration/block.c >>> @@ -879,8 +879,8 @@ static void block_save_pending(void *opaque, >>> uint64_t max_size, >>> blk_mig_unlock(); >>> >>> /* Report at least one block pending during bulk phase */ >>> - if (pending <= max_size && !block_mig_state.bulk_completed) { >>> - pending = max_size + BLK_MIG_BLOCK_SIZE; >>> + if (!pending && !block_mig_state.bulk_completed) { >>> + pending = BLK_MIG_BLOCK_SIZE; >>> } >>> >>> trace_migration_block_save_pending(pending); >> >> -- >> Best regards, >> Vladimir >>
diff --git a/migration/block.c b/migration/block.c index b3d680af75..39ce4003c6 100644 --- a/migration/block.c +++ b/migration/block.c @@ -879,8 +879,8 @@ static void block_save_pending(void *opaque, uint64_t max_size, blk_mig_unlock(); /* Report at least one block pending during bulk phase */ - if (pending <= max_size && !block_mig_state.bulk_completed) { - pending = max_size + BLK_MIG_BLOCK_SIZE; + if (!pending && !block_mig_state.bulk_completed) { + pending = BLK_MIG_BLOCK_SIZE; } trace_migration_block_save_pending(pending);