[PATCH v2 0/5] block-copy: make helper APIs thread safe

[PATCH v2 0/5] block-copy: make helper APIs thread safe

[Emanuele Giuseppe Esposito]

This serie of patches bring thread safety to the smaller APIs used by
block-copy, namely ratelimit, progressmeter, co-shared-resource
and aiotask.
The end goal is to reduce the usage of AioContexlock in block-copy,
by introducing smaller granularity locks thus on making the block layer
thread safe. 

This serie depends on Paolo's coroutine_sleep API.

What's missing for block-copy to be fully thread-safe is fixing
the CoSleep API to allow cross-thread sleep and wakeup.
Paolo is working on it and will post the patches once his new
CoSleep API is accepted.

Patches 1-3 work on ratelimit (they are also based on the first
ratelimit patch sent by Paolo), 4 covers progressmeter and
5 co-shared-resources.

Based-on: <20210503112550.478521-1-pbonzini@redhat.com>
Based-on: <20210413125533.217440-1-pbonzini@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
v1 -> v2:
* Implement progressmeter as a separate .c file (was just an header),
  but keep the struct public, to avoid heap allocations [Vladimir, Paolo]
* Drop for now the Aiopool patches of v2 [Paolo, Stefan]

Emanuele Giuseppe Esposito (2):
  progressmeter: protect with a mutex
  co-shared-resource: protect with a mutex

Paolo Bonzini (3):
  ratelimit: treat zero speed as unlimited
  block-copy: let ratelimit handle a speed of 0
  blockjob: let ratelimit handle a speed of 0

 block/block-copy.c                | 28 ++++++--------
 block/meson.build                 |  1 +
 block/progress_meter.c            | 64 +++++++++++++++++++++++++++++++
 blockjob.c                        | 46 +++++++++++++---------
 include/qemu/co-shared-resource.h |  4 +-
 include/qemu/progress_meter.h     | 34 ++++++++--------
 include/qemu/ratelimit.h          | 12 +++++-
 job-qmp.c                         |  8 +++-
 job.c                             |  3 ++
 qemu-img.c                        |  9 +++--
 util/qemu-co-shared-resource.c    | 27 ++++++++++---
 11 files changed, 171 insertions(+), 65 deletions(-)
 create mode 100644 block/progress_meter.c

-- 
2.30.2

[PATCH v2 1/5] ratelimit: treat zero speed as unlimited

[Emanuele Giuseppe Esposito]

From: Paolo Bonzini <pbonzini@redhat.com>

Both users of RateLimit, block-copy.c and blockjob.c, treat
a speed of zero as unlimited, while RateLimit treats it as
"as slow as possible".  The latter is nicer from the code
point of view but pretty useless, so disable rate limiting
if a speed of zero is provided.

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 include/qemu/ratelimit.h | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/qemu/ratelimit.h b/include/qemu/ratelimit.h
index 003ea6d5a3..48bf59e857 100644
--- a/include/qemu/ratelimit.h
+++ b/include/qemu/ratelimit.h
@@ -43,7 +43,11 @@ static inline int64_t ratelimit_calculate_delay(RateLimit *limit, uint64_t n)
     double delay_slices;
 
     QEMU_LOCK_GUARD(&limit->lock);
-    assert(limit->slice_quota && limit->slice_ns);
+    if (!limit->slice_quota) {
+        /* Throttling disabled.  */
+        return 0;
+    }
+    assert(limit->slice_ns);
 
     if (limit->slice_end_time < now) {
         /* Previous, possibly extended, time slice finished; reset the
@@ -83,7 +87,11 @@ static inline void ratelimit_set_speed(RateLimit *limit, uint64_t speed,
 {
     QEMU_LOCK_GUARD(&limit->lock);
     limit->slice_ns = slice_ns;
-    limit->slice_quota = MAX(((double)speed * slice_ns) / 1000000000ULL, 1);
+    if (speed == 0) {
+        limit->slice_quota = 0;
+    } else {
+        limit->slice_quota = MAX(((double)speed * slice_ns) / 1000000000ULL, 1);
+    }
 }
 
 #endif
-- 
2.30.2

[PATCH v2 2/5] block-copy: let ratelimit handle a speed of 0

[Emanuele Giuseppe Esposito]

From: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/block-copy.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index c2e5090412..37c8e8504b 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -113,7 +113,6 @@ typedef struct BlockCopyState {
 
     SharedResource *mem;
 
-    uint64_t speed;
     RateLimit rate_limit;
 } BlockCopyState;
 
@@ -619,21 +618,19 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
         }
         task->zeroes = ret & BDRV_BLOCK_ZERO;
 
-        if (s->speed) {
-            if (!call_state->ignore_ratelimit) {
-                uint64_t ns = ratelimit_calculate_delay(&s->rate_limit, 0);
-                if (ns > 0) {
-                    block_copy_task_end(task, -EAGAIN);
-                    g_free(task);
-                    qemu_co_sleep_ns_wakeable(&call_state->sleep,
-                                              QEMU_CLOCK_REALTIME, ns);
-                    continue;
-                }
+        if (!call_state->ignore_ratelimit) {
+            uint64_t ns = ratelimit_calculate_delay(&s->rate_limit, 0);
+            if (ns > 0) {
+                block_copy_task_end(task, -EAGAIN);
+                g_free(task);
+                qemu_co_sleep_ns_wakeable(&call_state->sleep,
+                                          QEMU_CLOCK_REALTIME, ns);
+                continue;
             }
-
-            ratelimit_calculate_delay(&s->rate_limit, task->bytes);
         }
 
+        ratelimit_calculate_delay(&s->rate_limit, task->bytes);
+
         trace_block_copy_process(s, task->offset);
 
         co_get_from_shres(s->mem, task->bytes);
@@ -825,10 +822,7 @@ void block_copy_set_skip_unallocated(BlockCopyState *s, bool skip)
 
 void block_copy_set_speed(BlockCopyState *s, uint64_t speed)
 {
-    s->speed = speed;
-    if (speed > 0) {
-        ratelimit_set_speed(&s->rate_limit, speed, BLOCK_COPY_SLICE_TIME);
-    }
+    ratelimit_set_speed(&s->rate_limit, speed, BLOCK_COPY_SLICE_TIME);
 
     /*
      * Note: it's good to kick all call states from here, but it should be done
-- 
2.30.2

[PATCH v2 3/5] blockjob: let ratelimit handle a speed of 0

[Emanuele Giuseppe Esposito]

From: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 blockjob.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index dc1d9e0e46..22e5bb9b1f 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -300,10 +300,6 @@ bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
 
 int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n)
 {
-    if (!job->speed) {
-        return 0;
-    }
-
     return ratelimit_calculate_delay(&job->limit, n);
 }
 
@@ -472,12 +468,9 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
     blk_set_disable_request_queuing(blk, true);
     blk_set_allow_aio_context_change(blk, true);
 
-    /* Only set speed when necessary to avoid NotSupported error */
-    if (speed != 0) {
-        if (!block_job_set_speed(job, speed, errp)) {
-            job_early_fail(&job->job);
-            return NULL;
-        }
+    if (!block_job_set_speed(job, speed, errp)) {
+        job_early_fail(&job->job);
+        return NULL;
     }
 
     return job;
-- 
2.30.2

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

[Emanuele Giuseppe Esposito]

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>
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 @@
+/*
+ * Helper functionality for some process progress tracking.
+ *
+ * Copyright (c) 2011 IBM Corp.
+ * Copyright (c) 2012, 2018 Red Hat, Inc.
+ * Copyright (c) 2020 Virtuozzo International GmbH
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#include "qemu/osdep.h"
+#include "qemu/progress_meter.h"
+
+void progress_init(ProgressMeter *pm)
+{
+    qemu_mutex_init(&pm->lock);
+}
+
+void progress_destroy(ProgressMeter *pm)
+{
+    qemu_mutex_destroy(&pm->lock);
+}
+
+void progress_get_snapshot(ProgressMeter *pm, uint64_t *current,
+                           uint64_t *total)
+{
+    QEMU_LOCK_GUARD(&pm->lock);
+
+    *current = pm->current;
+    *total = pm->total;
+}
+
+void progress_work_done(ProgressMeter *pm, uint64_t done)
+{
+    QEMU_LOCK_GUARD(&pm->lock);
+    pm->current += done;
+}
+
+void progress_set_remaining(ProgressMeter *pm, uint64_t remaining)
+{
+    QEMU_LOCK_GUARD(&pm->lock);
+    pm->total = pm->current + remaining;
+}
+
+void progress_increase_remaining(ProgressMeter *pm, uint64_t delta)
+{
+    QEMU_LOCK_GUARD(&pm->lock);
+    pm->total += delta;
+}
diff --git a/blockjob.c b/blockjob.c
index 22e5bb9b1f..4bad1408cb 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..dadf822bbf 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,22 +39,24 @@ typedef struct ProgressMeter {
 
     /** Estimated current value at the completion of the process */
     uint64_t total;
+
+    QemuMutex lock; /* protects concurrent access to above fields */
 } ProgressMeter;
 
-static inline void progress_work_done(ProgressMeter *pm, uint64_t done)
-{
-    pm->current += done;
-}
-
-static inline void progress_set_remaining(ProgressMeter *pm, uint64_t remaining)
-{
-    pm->total = pm->current + remaining;
-}
-
-static inline void progress_increase_remaining(ProgressMeter *pm,
-                                               uint64_t delta)
-{
-    pm->total += delta;
-}
+void progress_init(ProgressMeter *pm);
+void progress_destroy(ProgressMeter *pm);
+
+/* Get a snapshot of internal current and total values  */
+void progress_get_snapshot(ProgressMeter *pm, uint64_t *current,
+                           uint64_t *total);
+
+/* Increases the amount of work done so far by @done */
+void progress_work_done(ProgressMeter *pm, uint64_t done);
+
+/* Sets how much work has to be done to complete to @remaining */
+void progress_set_remaining(ProgressMeter *pm, uint64_t remaining);
+
+/* Increases the total work to do by @delta */
+void progress_increase_remaining(ProgressMeter *pm, uint64_t delta);
 
 #endif /* QEMU_PROGRESS_METER_H */
diff --git a/job-qmp.c b/job-qmp.c
index 34c4da094f..829a28aa70 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -144,16 +144,20 @@ void qmp_job_dismiss(const char *id, Error **errp)
 static JobInfo *job_query_single(Job *job, Error **errp)
 {
     JobInfo *info;
+    uint64_t progress_current;
+    uint64_t progress_total;
 
     assert(!job_is_internal(job));
+    progress_get_snapshot(&job->progress, &progress_current,
+                          &progress_total);
 
     info = g_new(JobInfo, 1);
     *info = (JobInfo) {
         .id                 = g_strdup(job->id),
         .type               = job_type(job),
         .status             = job->status,
-        .current_progress   = job->progress.current,
-        .total_progress     = job->progress.total,
+        .current_progress   = progress_current,
+        .total_progress     = progress_total,
         .has_error          = !!job->err,
         .error              = job->err ? \
                               g_strdup(error_get_pretty(job->err)) : NULL,
diff --git a/job.c b/job.c
index 4aff13d95a..00b0560052 100644
--- a/job.c
+++ b/job.c
@@ -339,6 +339,8 @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn,
     job->cb            = cb;
     job->opaque        = opaque;
 
+    progress_init(&job->progress);
+
     notifier_list_init(&job->on_finalize_cancelled);
     notifier_list_init(&job->on_finalize_completed);
     notifier_list_init(&job->on_pending);
@@ -382,6 +384,7 @@ void job_unref(Job *job)
 
         QLIST_REMOVE(job, job_list);
 
+        progress_destroy(&job->progress);
         error_free(job->err);
         g_free(job->id);
         g_free(job);
diff --git a/qemu-img.c b/qemu-img.c
index a5993682aa..7956a89965 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -900,6 +900,7 @@ static void common_block_job_cb(void *opaque, int ret)
 
 static void run_block_job(BlockJob *job, Error **errp)
 {
+    uint64_t progress_current, progress_total;
     AioContext *aio_context = blk_get_aio_context(job->blk);
     int ret = 0;
 
@@ -908,9 +909,11 @@ static void run_block_job(BlockJob *job, Error **errp)
     do {
         float progress = 0.0f;
         aio_poll(aio_context, true);
-        if (job->job.progress.total) {
-            progress = (float)job->job.progress.current /
-                       job->job.progress.total * 100.f;
+
+        progress_get_snapshot(&job->job.progress, &progress_current,
+                              &progress_total);
+        if (progress_total) {
+            progress = (float)progress_current / progress_total * 100.f;
         }
         qemu_progress_print(progress, 0);
     } while (!job_is_ready(&job->job) && !job_is_completed(&job->job));
-- 
2.30.2

[PATCH v2 5/5] co-shared-resource: protect with a mutex

[Emanuele Giuseppe Esposito]

co-shared-resource is currently not thread-safe, as also reported
in co-shared-resource.h. Add a QemuMutex because co_try_get_from_shres
can also be invoked from non-coroutine context.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 include/qemu/co-shared-resource.h |  4 +---
 util/qemu-co-shared-resource.c    | 27 ++++++++++++++++++++++-----
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/include/qemu/co-shared-resource.h b/include/qemu/co-shared-resource.h
index 4e4503004c..78ca5850f8 100644
--- a/include/qemu/co-shared-resource.h
+++ b/include/qemu/co-shared-resource.h
@@ -26,15 +26,13 @@
 #ifndef QEMU_CO_SHARED_RESOURCE_H
 #define QEMU_CO_SHARED_RESOURCE_H
 
-
+/* Accesses to co-shared-resource API are thread-safe */
 typedef struct SharedResource SharedResource;
 
 /*
  * Create SharedResource structure
  *
  * @total: total amount of some resource to be shared between clients
- *
- * Note: this API is not thread-safe.
  */
 SharedResource *shres_create(uint64_t total);
 
diff --git a/util/qemu-co-shared-resource.c b/util/qemu-co-shared-resource.c
index 1c83cd9d29..bb875a86be 100644
--- a/util/qemu-co-shared-resource.c
+++ b/util/qemu-co-shared-resource.c
@@ -28,10 +28,13 @@
 #include "qemu/co-shared-resource.h"
 
 struct SharedResource {
-    uint64_t total;
-    uint64_t available;
+    uint64_t total; /* Set in shres_create() and not changed anymore */
 
+    /* State fields protected by lock */
+    uint64_t available;
     CoQueue queue;
+
+    QemuMutex lock;
 };
 
 SharedResource *shres_create(uint64_t total)
@@ -40,6 +43,7 @@ SharedResource *shres_create(uint64_t total)
 
     s->total = s->available = total;
     qemu_co_queue_init(&s->queue);
+    qemu_mutex_init(&s->lock);
 
     return s;
 }
@@ -47,10 +51,12 @@ SharedResource *shres_create(uint64_t total)
 void shres_destroy(SharedResource *s)
 {
     assert(s->available == s->total);
+    qemu_mutex_destroy(&s->lock);
     g_free(s);
 }
 
-bool co_try_get_from_shres(SharedResource *s, uint64_t n)
+/* Called with lock held. */
+static bool co_try_get_from_shres_locked(SharedResource *s, uint64_t n)
 {
     if (s->available >= n) {
         s->available -= n;
@@ -60,16 +66,27 @@ bool co_try_get_from_shres(SharedResource *s, uint64_t n)
     return false;
 }
 
+bool co_try_get_from_shres(SharedResource *s, uint64_t n)
+{
+    bool res;
+    QEMU_LOCK_GUARD(&s->lock);
+    res = co_try_get_from_shres_locked(s, n);
+
+    return res;
+}
+
 void coroutine_fn co_get_from_shres(SharedResource *s, uint64_t n)
 {
     assert(n <= s->total);
-    while (!co_try_get_from_shres(s, n)) {
-        qemu_co_queue_wait(&s->queue, NULL);
+    QEMU_LOCK_GUARD(&s->lock);
+    while (!co_try_get_from_shres_locked(s, n)) {
+        qemu_co_queue_wait(&s->queue, &s->lock);
     }
 }
 
 void coroutine_fn co_put_to_shres(SharedResource *s, uint64_t n)
 {
+    QEMU_LOCK_GUARD(&s->lock);
     assert(s->total - s->available >= n);
     s->available += n;
     qemu_co_queue_restart_all(&s->queue);
-- 
2.30.2

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

[Vladimir Sementsov-Ogievskiy]

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.


> 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 @@
> +/*
> + * Helper functionality for some process progress tracking.
> + *
> + * Copyright (c) 2011 IBM Corp.
> + * Copyright (c) 2012, 2018 Red Hat, Inc.
> + * Copyright (c) 2020 Virtuozzo International GmbH
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +#include "qemu/osdep.h"
> +#include "qemu/progress_meter.h"
> +
> +void progress_init(ProgressMeter *pm)
> +{
> +    qemu_mutex_init(&pm->lock);
> +}
> +
> +void progress_destroy(ProgressMeter *pm)
> +{
> +    qemu_mutex_destroy(&pm->lock);
> +}
> +
> +void progress_get_snapshot(ProgressMeter *pm, uint64_t *current,
> +                           uint64_t *total)
> +{
> +    QEMU_LOCK_GUARD(&pm->lock);
> +
> +    *current = pm->current;
> +    *total = pm->total;
> +}
> +
> +void progress_work_done(ProgressMeter *pm, uint64_t done)
> +{
> +    QEMU_LOCK_GUARD(&pm->lock);
> +    pm->current += done;
> +}
> +
> +void progress_set_remaining(ProgressMeter *pm, uint64_t remaining)
> +{
> +    QEMU_LOCK_GUARD(&pm->lock);
> +    pm->total = pm->current + remaining;
> +}
> +
> +void progress_increase_remaining(ProgressMeter *pm, uint64_t delta)
> +{
> +    QEMU_LOCK_GUARD(&pm->lock);
> +    pm->total += delta;
> +}

That's not what I meant. 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





-- 
Best regards,
Vladimir

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

Re: [PATCH v2 5/5] co-shared-resource: protect with a mutex

[Vladimir Sementsov-Ogievskiy]

18.05.2021 12:40, Emanuele Giuseppe Esposito wrote:
> co-shared-resource is currently not thread-safe, as also reported
> in co-shared-resource.h. Add a QemuMutex because co_try_get_from_shres
> can also be invoked from non-coroutine context.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   include/qemu/co-shared-resource.h |  4 +---
>   util/qemu-co-shared-resource.c    | 27 ++++++++++++++++++++++-----
>   2 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/include/qemu/co-shared-resource.h b/include/qemu/co-shared-resource.h
> index 4e4503004c..78ca5850f8 100644
> --- a/include/qemu/co-shared-resource.h
> +++ b/include/qemu/co-shared-resource.h
> @@ -26,15 +26,13 @@
>   #ifndef QEMU_CO_SHARED_RESOURCE_H
>   #define QEMU_CO_SHARED_RESOURCE_H
>   
> -
> +/* Accesses to co-shared-resource API are thread-safe */
>   typedef struct SharedResource SharedResource;
>   
>   /*
>    * Create SharedResource structure
>    *
>    * @total: total amount of some resource to be shared between clients
> - *
> - * Note: this API is not thread-safe.
>    */
>   SharedResource *shres_create(uint64_t total);
>   
> diff --git a/util/qemu-co-shared-resource.c b/util/qemu-co-shared-resource.c
> index 1c83cd9d29..bb875a86be 100644
> --- a/util/qemu-co-shared-resource.c
> +++ b/util/qemu-co-shared-resource.c
> @@ -28,10 +28,13 @@
>   #include "qemu/co-shared-resource.h"
>   
>   struct SharedResource {
> -    uint64_t total;
> -    uint64_t available;
> +    uint64_t total; /* Set in shres_create() and not changed anymore */
>   
> +    /* State fields protected by lock */
> +    uint64_t available;
>       CoQueue queue;
> +
> +    QemuMutex lock;
>   };
>   
>   SharedResource *shres_create(uint64_t total)
> @@ -40,6 +43,7 @@ SharedResource *shres_create(uint64_t total)
>   
>       s->total = s->available = total;
>       qemu_co_queue_init(&s->queue);
> +    qemu_mutex_init(&s->lock);
>   
>       return s;
>   }
> @@ -47,10 +51,12 @@ SharedResource *shres_create(uint64_t total)
>   void shres_destroy(SharedResource *s)
>   {
>       assert(s->available == s->total);
> +    qemu_mutex_destroy(&s->lock);
>       g_free(s);
>   }
>   
> -bool co_try_get_from_shres(SharedResource *s, uint64_t n)
> +/* Called with lock held. */
> +static bool co_try_get_from_shres_locked(SharedResource *s, uint64_t n)
>   {
>       if (s->available >= n) {
>           s->available -= n;
> @@ -60,16 +66,27 @@ bool co_try_get_from_shres(SharedResource *s, uint64_t n)
>       return false;
>   }
>   
> +bool co_try_get_from_shres(SharedResource *s, uint64_t n)
> +{
> +    bool res;
> +    QEMU_LOCK_GUARD(&s->lock);
> +    res = co_try_get_from_shres_locked(s, n);
> +
> +    return res;
> +}

it can be two lines:

QEMU_LOCK_GUARD(&s->lock);
return co_try_get_from_shres_locked(s, n);

> +
>   void coroutine_fn co_get_from_shres(SharedResource *s, uint64_t n)
>   {
>       assert(n <= s->total);
> -    while (!co_try_get_from_shres(s, n)) {
> -        qemu_co_queue_wait(&s->queue, NULL);
> +    QEMU_LOCK_GUARD(&s->lock);
> +    while (!co_try_get_from_shres_locked(s, n)) {
> +        qemu_co_queue_wait(&s->queue, &s->lock);
>       }
>   }
>   
>   void coroutine_fn co_put_to_shres(SharedResource *s, uint64_t n)
>   {
> +    QEMU_LOCK_GUARD(&s->lock);
>       assert(s->total - s->available >= n);
>       s->available += n;
>       qemu_co_queue_restart_all(&s->queue);
> 

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1906 bytes --]

On Tue, May 18, 2021 at 12:14:17PM +0200, Emanuele Giuseppe Esposito wrote:
> 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).

I took a look again:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Regarding hiding the struct, in a library with a public API the authors
need to be very careful about exposing struct fields in order to prevent
ABI breakage or applications making use of internal fields.

However, in QEMU we're less strict since we have full control over the
codebase (including internal API consumers). If other factors outweigh
the need for strict encapsulation, then you can put the struct
definition in the header file. Grepping for "private" in QEMU shows lots
of examples and there are probably many more without an explicit
"private" comment. Code reviewers can reject patches that touch private
struct fields or patches can be submitted to remove existing access to
private fields, so we don't have the limitations that library authors
have.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]