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

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

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