Re: [libvirt] [PATCH v2 12/12] qemu: show disks stats for nbd migration

On Wed, Dec 28, 2016 at 17:39:21 +0300, Nikolay Shirokovskiy wrote: > There is no disks stats when migrating with VIR_MIGRATE_NON_SHARED_* > for qemu that supports nbd. The reason is that disks are copied via disk mirroring > and not in the scope of migration job itself. Below > a couble of issues of the implementation are described. > > 'total' field is set from 'end' field of block job info for the > sake of simplicity. This is true only when there is no guest disk > activity during migration. If there is an activity then 'end' will > grow This is exactly how memory migration works too. > while 'total' is an estimation that should stay constant. Nope. That's the reason why migration is an unbounded job, we never know what the total is and it is designed to change anytime during migration as we realize more data needs to be transferred. > I guess this can be fixed by setting 'total' to disk 'capacity'. Nothing to fix really. > There is also known possible corner case issue with this implementation. > There is a chance that client asking for stats at the process of the > mirroring stopping on successfull migration will see only part of mirroring disks > and thus will receive inconsisent partial info. > --- > docs/news.html.in | 4 ++++ > src/qemu/qemu_driver.c | 3 ++- > src/qemu/qemu_migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_migration.h | 5 +++++ > 4 files changed, 66 insertions(+), 1 deletion(-) > > diff --git a/docs/news.html.in b/docs/news.html.in > index 22611db..8036cf8 100644 > --- a/docs/news.html.in > +++ b/docs/news.html.in > @@ -42,6 +42,10 @@ > cpu cycles, stalled backend cpu cycles, and ref cpu > cycles by applications running on the platform > </li> > + <li>qemu: show disks stats for nbd disks migration<br/> > + Show disks stats in migrations stats in disks copy phase > + of migration with non-shared disks. > + </li> > </ul> > </li> > <li><strong>Bug fixes</strong> Since it took me so long to review your patches (I apologize for that), we started using news.xml instead of news.html.in. And it should be modified in a separate patch to avoid conflicts during backports. Otherwise the patch looks good and it's definitely cleaner than v1. However, I think you should also update disk migration statistics while stopping the mirror jobs so that the stats of a completed migration contain accurate data.