[PATCH 4/6] progressmeter: protect with a mutex
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.
The simplest thing to do is to add a mutex to be able to provide
an accurate snapshot of the progress values to the caller.
Signed-off-by: Emanuele Giuseppe Esposito
---
blockjob.c| 33 +
include/qemu/progress_meter.h | 31 +++
job-qmp.c | 8 ++--
job.c | 3 +++
qemu-img.c| 9 ++---
5 files changed, 71 insertions(+), 13 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index 046c1bcd66..98bb12f015 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -306,18 +306,23 @@ int64_t block_job_ratelimit_get_delay(BlockJob *job,
uint64_t n)
BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
{
BlockJobInfo *info;
+uint64_t progress_current, progress_total;
if (block_job_is_internal(job)) {
error_setg(errp, "Cannot query QEMU internal jobs");
return NULL;
}
+
+progress_get_snapshot(&job->job.progress, &progress_current,
+ &progress_total);
+
info = g_new0(BlockJobInfo, 1);
info->type = g_strdup(job_type_str(&job->job));
info->device= g_strdup(job->job.id);
info->busy = qatomic_read(&job->job.busy);
info->paused= job->job.pause_count > 0;
-info->offset= job->job.progress.current;
-info->len = job->job.progress.total;
+info->offset= progress_current;
+info->len = progress_total;
info->speed = job->speed;
info->io_status = job->iostatus;
info->ready = job_is_ready(&job->job),
@@ -344,15 +349,19 @@ static void block_job_iostatus_set_err(BlockJob *job, int
error)
static void block_job_event_cancelled(Notifier *n, void *opaque)
{
BlockJob *job = opaque;
+uint64_t progress_current, progress_total;
if (block_job_is_internal(job)) {
return;
}
+progress_get_snapshot(&job->job.progress, &progress_current,
+ &progress_total);
+
qapi_event_send_block_job_cancelled(job_type(&job->job),
job->job.id,
-job->job.progress.total,
-job->job.progress.current,
+progress_total,
+progress_current,
job->speed);
}
@@ -360,6 +369,7 @@ static void block_job_event_completed(Notifier *n, void
*opaque)
{
BlockJob *job = opaque;
const char *msg = NULL;
+uint64_t progress_current, progress_total;
if (block_job_is_internal(job)) {
return;
@@ -369,10 +379,13 @@ static void block_job_event_completed(Notifier *n, void
*opaque)
msg = error_get_pretty(job->job.err);
}
+progress_get_snapshot(&job->job.progress, &progress_current,
+ &progress_total);
+
qapi_event_send_block_job_completed(job_type(&job->job),
job->job.id,
-job->job.progress.total,
-job->job.progress.current,
+progress_total,
+progress_current,
job->speed,
!!msg,
msg);
@@ -393,15 +406,19 @@ static void block_job_event_pending(Notifier *n, void
*opaque)
static void block_job_event_ready(Notifier *n, void *opaque)
{
BlockJob *job = opaque;
+uint64_t progress_current, progress_total;
if (block_job_is_internal(job)) {
return;
}
+progress_get_snapshot(&job->job.progress, &progress_current,
+ &progress_total);
+
qapi_event_send_block_job_ready(job_type(&job->job),
job->job.id,
-job->job.progress.total,
-job->job.progress.current,
+progress_total,
+progress_current,
job->speed);
}
diff --git a/include/qemu/progress_meter.h b/include/qemu/progress_meter.h
index 9a23ff071c..81f79770d2 100644
--- a/include/qemu/progress_meter.h
+++ b/include/qemu/progress_meter.h
@@ -27,6 +27,8 @@
#ifndef QEMU_PROGRESS_METER_H
#define QEMU_PROGRESS_METER_H
+#include "qemu/lockable.h"
+
typedef struct ProgressMeter {
/**
* Current progress. The unit is arbitrary as long as the ratio between
@@ -37,21 +39,50 @@ typedef
Re: [PATCH 4/6] progressmeter: protect with a mutex
10.05.2021 11:59, 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.
The simplest thing to do is to add a mutex to be able to provide
an accurate snapshot of the progress values to the caller.
Signed-off-by: Emanuele Giuseppe Esposito
---
blockjob.c| 33 +
[..]
--- a/include/qemu/progress_meter.h
+++ b/include/qemu/progress_meter.h
@@ -27,6 +27,8 @@
#ifndef QEMU_PROGRESS_METER_H
#define QEMU_PROGRESS_METER_H
+#include "qemu/lockable.h"
+
typedef struct ProgressMeter {
/**
* Current progress. The unit is arbitrary as long as the ratio between
@@ -37,21 +39,50 @@ typedef struct ProgressMeter {
/** Estimated current value at the completion of the process */
uint64_t total;
+
+QemuMutex lock;
} ProgressMeter;
+static inline void progress_init(ProgressMeter *pm)
+{
+qemu_mutex_init(&pm->lock);
+}
+
+static inline void progress_destroy(ProgressMeter *pm)
+{
+qemu_mutex_destroy(&pm->lock);
+}
Could we instead add a c file and add the structure private? Then we'll have
progress_new() and progress_free() APIs instead.
This way, it would be a lot simpler to control that nobady use structure fields
directly.
+
+static inline void progress_get_snapshot(ProgressMeter *pm,
+ uint64_t *current, uint64_t *total)
+{
+QEMU_LOCK_GUARD(&pm->lock);
+
+if (current) {
+*current = pm->current;
+}
+
+if (total) {
+*total = pm->total;
+}
+}
We don't have caller that pass only one pointer. So we can just do
*current = pm->current;
*total = pm->total;
implicitly requiring both pointers to be non NULL.
--
Best regards,
Vladimir
Re: [PATCH 4/6] progressmeter: protect with a mutex
On 10/05/2021 13:28, Vladimir Sementsov-Ogievskiy wrote:
10.05.2021 11:59, 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.
The simplest thing to do is to add a mutex to be able to provide
an accurate snapshot of the progress values to the caller.
Signed-off-by: Emanuele Giuseppe Esposito
---
blockjob.c | 33 +
[..]
--- a/include/qemu/progress_meter.h
+++ b/include/qemu/progress_meter.h
@@ -27,6 +27,8 @@
#ifndef QEMU_PROGRESS_METER_H
#define QEMU_PROGRESS_METER_H
+#include "qemu/lockable.h"
+
typedef struct ProgressMeter {
/**
* Current progress. The unit is arbitrary as long as the ratio
between
@@ -37,21 +39,50 @@ typedef struct ProgressMeter {
/** Estimated current value at the completion of the process */
uint64_t total;
+
+ QemuMutex lock;
} ProgressMeter;
+static inline void progress_init(ProgressMeter *pm)
+{
+ qemu_mutex_init(&pm->lock);
+}
+
+static inline void progress_destroy(ProgressMeter *pm)
+{
+ qemu_mutex_destroy(&pm->lock);
+}
Could we instead add a c file and add the structure private? Then we'll
have progress_new() and progress_free() APIs instead.
This way, it would be a lot simpler to control that nobady use structure
fields directly.
Makes sense, I'll do it.
+
+static inline void progress_get_snapshot(ProgressMeter *pm,
+ uint64_t *current, uint64_t
*total)
+{
+ QEMU_LOCK_GUARD(&pm->lock);
+
+ if (current) {
+ *current = pm->current;
+ }
+
+ if (total) {
+ *total = pm->total;
+ }
+}
We don't have caller that pass only one pointer. So we can just do
*current = pm->current;
*total = pm->total;
implicitly requiring both pointers to be non NULL.
Is it so performance critical that we need to skip these safety checks?
IMHO we can keep it as it is.
Thank you,
Emanuele
Re: [PATCH 4/6] progressmeter: protect with a mutex
10.05.2021 19:52, Emanuele Giuseppe Esposito wrote:
On 10/05/2021 13:28, Vladimir Sementsov-Ogievskiy wrote:
10.05.2021 11:59, 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.
The simplest thing to do is to add a mutex to be able to provide
an accurate snapshot of the progress values to the caller.
Signed-off-by: Emanuele Giuseppe Esposito
---
blockjob.c | 33 +
[..]
--- a/include/qemu/progress_meter.h
+++ b/include/qemu/progress_meter.h
@@ -27,6 +27,8 @@
#ifndef QEMU_PROGRESS_METER_H
#define QEMU_PROGRESS_METER_H
+#include "qemu/lockable.h"
+
typedef struct ProgressMeter {
/**
* Current progress. The unit is arbitrary as long as the ratio between
@@ -37,21 +39,50 @@ typedef struct ProgressMeter {
/** Estimated current value at the completion of the process */
uint64_t total;
+
+ QemuMutex lock;
} ProgressMeter;
+static inline void progress_init(ProgressMeter *pm)
+{
+ qemu_mutex_init(&pm->lock);
+}
+
+static inline void progress_destroy(ProgressMeter *pm)
+{
+ qemu_mutex_destroy(&pm->lock);
+}
Could we instead add a c file and add the structure private? Then we'll have
progress_new() and progress_free() APIs instead.
This way, it would be a lot simpler to control that nobady use structure fields
directly.
Makes sense, I'll do it.
+
+static inline void progress_get_snapshot(ProgressMeter *pm,
+ uint64_t *current, uint64_t *total)
+{
+ QEMU_LOCK_GUARD(&pm->lock);
+
+ if (current) {
+ *current = pm->current;
+ }
+
+ if (total) {
+ *total = pm->total;
+ }
+}
We don't have caller that pass only one pointer. So we can just do
*current = pm->current;
*total = pm->total;
implicitly requiring both pointers to be non NULL.
Is it so performance critical that we need to skip these safety checks?
IMHO we can keep it as it is.
Not performance. It's just less code. You propose more complex interface
(pointers may be valid pointers or NULL), implemented by more complex code
(extra if blocks). And there no users for this. So, I don't see any reason for
extra logic and code lines.
What kind of safety you want? All current callers pass non-zero pointers, it's
obvious. If someone will create new call, he should look first at function
itself, not blindly pass NULL pointers. And if not, he will get clean crash on
zero pointer dereference.
--
Best regards,
Vladimir
Re: [PATCH 4/6] progressmeter: protect with a mutex
We don't have caller that pass only one pointer. So we can just do *current = pm->current; *total = pm->total; implicitly requiring both pointers to be non NULL. Is it so performance critical that we need to skip these safety checks? IMHO we can keep it as it is. Not performance. It's just less code. You propose more complex interface (pointers may be valid pointers or NULL), implemented by more complex code (extra if blocks). And there no users for this. So, I don't see any reason for extra logic and code lines. What kind of safety you want? All current callers pass non-zero pointers, it's obvious. If someone will create new call, he should look first at function itself, not blindly pass NULL pointers. And if not, he will get clean crash on zero pointer dereference. Ok, makes sense. Will remove the ifs. Thank you, Emanuele
Re: [PATCH 4/6] progressmeter: protect with a mutex
On 10/05/21 13:28, Vladimir Sementsov-Ogievskiy wrote: Could we instead add a c file and add the structure private? Then we'll have progress_new() and progress_free() APIs instead. This way, it would be a lot simpler to control that nobady use structure fields directly. I don't know... I prefer embedding structs to heap allocation. Paolo
Re: [PATCH 4/6] progressmeter: protect with a mutex
11.05.2021 15:28, Paolo Bonzini wrote: On 10/05/21 13:28, Vladimir Sementsov-Ogievskiy wrote: Could we instead add a c file and add the structure private? Then we'll have progress_new() and progress_free() APIs instead. This way, it would be a lot simpler to control that nobady use structure fields directly. I don't know... I prefer embedding structs to heap allocation. I agree that embedding looks better from allocation point of view.. But IMHO encapsulation guarantee of private structure is better feature. Especially when we have getter/setter methods. Even the patch itself is example: to review this carefully, I should somehow check that every use of the structure is updated (grepping the code, or maybe change field name and recompile, to catch every occurrence of it).. And if patch makes structure private and adds setters/getters, it can be reviewed without applying. And we have to call _init/_destroy anyway, so it's not more complex to call _new/_free instead. -- Best regards, Vladimir
Re: [PATCH 4/6] progressmeter: protect with a mutex
On Mon, May 10, 2021 at 10:59:39AM +0200, 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. > > The simplest thing to do is to add a mutex to be able to provide > an accurate snapshot of the progress values to the caller. > > Signed-off-by: Emanuele Giuseppe Esposito > --- > blockjob.c| 33 + > include/qemu/progress_meter.h | 31 +++ > job-qmp.c | 8 ++-- > job.c | 3 +++ > qemu-img.c| 9 ++--- > 5 files changed, 71 insertions(+), 13 deletions(-) Unlike the next two patches where I had comments, here adding the lock makes sense to me - the point of the progress meter is to report numbers to the monitor thread so thread-safety is a key part of the API's value. Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
