Re: [PATCH v2 4/5] progressmeter: protect with a mutex

[Emanuele Giuseppe Esposito]

On 18/05/2021 12:00, Vladimir Sementsov-Ogievskiy wrote:
> 18.05.2021 12:40, Emanuele Giuseppe Esposito wrote:
> > Progressmeter is protected by the AioContext mutex, which
> > is taken by the block jobs and their caller (like blockdev).
> >
> > We would like to remove the dependency of block layer code on the
> > AioContext mutex, since most drivers and the core I/O code are already
> > not relying on it.
> >
> > Create a new C file to implement the ProgressMeter API, but keep the
> > struct as public, to avoid forcing allocation on the heap.
> >
> > Also add a mutex to be able to provide an accurate snapshot of the
> > progress values to the caller.
> >
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>
> patch changed a lot, so you can't keep Stefan's r-b. r-b should be kept 
> if patch is unchanged.

Sorry, my bad. Will remove it, if we keep these changes (see below).

> > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > ---
> >   block/meson.build             |  1 +
> >   block/progress_meter.c        | 64 +++++++++++++++++++++++++++++++++++
> >   blockjob.c                    | 33 +++++++++++++-----
> >   include/qemu/progress_meter.h | 34 +++++++++++--------
> >   job-qmp.c                     |  8 +++--
> >   job.c                         |  3 ++
> >   qemu-img.c                    |  9 +++--
> >   7 files changed, 124 insertions(+), 28 deletions(-)
> >   create mode 100644 block/progress_meter.c
> >
> > diff --git a/block/meson.build b/block/meson.build
> > index d21990ec95..90efd21ecf 100644
> > --- a/block/meson.build
> > +++ b/block/meson.build
> > @@ -13,6 +13,7 @@ block_ss.add(files(
> >     'commit.c',
> >     'copy-on-read.c',
> >     'preallocate.c',
> > +  'progress_meter.c',
> >     'create.c',
> >     'crypto.c',
> >     'dirty-bitmap.c',
> > diff --git a/block/progress_meter.c b/block/progress_meter.c
> > new file mode 100644
> > index 0000000000..aa2e60248c
> > --- /dev/null
> > +++ b/block/progress_meter.c
> > @@ -0,0 +1,64 @@
> > +/*
[...]>
> That's not what I meant.

I know it's not what you meant. But since Paolo also pointed out that it 
would be better to avoid heap allocations, I went for something in 
between :) In the end it doesn't look so bad to have a separate .c file.

 If we move only function to c file, this
> doesn't make real sense. If we want encapsulation, we should move the 
> structure definition itself to the c file too. Than, you'll need _new / 
> _free functions instead of _init / _destroy, because we'll move to heap 
> allocation for progress meter. Also, if we go this way, please make at 
> least two patches:
>
> 1. move progress meter (functions and structure) to own c file, modify 
> meson.build
>
> 2. add thread-safety

Let's see how this goes, and then if we keep the .c file I will split 
the patches. But I guess this requires a v3 anyways.

Emanuele