From 34afc5c298fd4b4279aeec440603b8a1a13ab8c2 Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Wed, 11 Mar 2020 11:29:27 +0800 Subject: [PATCH 1/6] block/iscsi:use the flags in iscsi_open() prevent Clang warning Clang static code analyzer show warning: block/iscsi.c:1920:9: warning: Value stored to 'flags' is never read flags &= ~BDRV_O_RDWR; ^ ~~~~~~~~~~~~ In iscsi_allocmap_init() only checks BDRV_O_NOCACHE, which is the same in both of flags and bs->open_flags. We can use the flags instead bs->open_flags to prevent Clang warning. Reported-by: Euler Robot Signed-off-by: Chen Qun Reviewed-by: Kevin Wolf Message-Id: <20200311032927.35092-1-kuhn.chenqun@huawei.com> Signed-off-by: Kevin Wolf --- block/iscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/iscsi.c b/block/iscsi.c index 14680a436a..4e216bd8aa 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -2002,7 +2002,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, iscsilun->cluster_size = iscsilun->bl.opt_unmap_gran * iscsilun->block_size; if (iscsilun->lbprz) { - ret = iscsi_allocmap_init(iscsilun, bs->open_flags); + ret = iscsi_allocmap_init(iscsilun, flags); } } From 7a26df203c7b2e4a585fc6014358b57a948fb7e0 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 24 Mar 2020 18:59:21 +0300 Subject: [PATCH 2/6] block: fix bdrv_root_attach_child forget to unref child_bs bdrv_root_attach_child promises to drop child_bs reference on failure. It does it on first handled failure path, but not on the second. Fix that. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20200324155921.23822-1-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf --- block.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block.c b/block.c index af3faf664e..2e3905c99e 100644 --- a/block.c +++ b/block.c @@ -2617,6 +2617,7 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, error_propagate(errp, local_err); g_free(child); bdrv_abort_perm_update(child_bs); + bdrv_unref(child_bs); return NULL; } } From 6fcc859fc224b80982c94130a9c0e93554e6cbde Mon Sep 17 00:00:00 2001 From: Minwoo Im Date: Tue, 24 Mar 2020 23:06:46 +0900 Subject: [PATCH 3/6] nvme: Print 'cqid' for nvme_del_cq MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The given argument for this trace should be cqid, not sqid. Signed-off-by: Minwoo Im Message-Id: <20200324140646.8274-1-minwoo.im.dev@gmail.com> Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Stefano Garzarella Signed-off-by: Kevin Wolf --- hw/block/trace-events | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/block/trace-events b/hw/block/trace-events index f78939fa9d..bf6d11b58b 100644 --- a/hw/block/trace-events +++ b/hw/block/trace-events @@ -37,7 +37,7 @@ nvme_rw(const char *verb, uint32_t blk_count, uint64_t byte_count, uint64_t lba) nvme_create_sq(uint64_t addr, uint16_t sqid, uint16_t cqid, uint16_t qsize, uint16_t qflags) "create submission queue, addr=0x%"PRIx64", sqid=%"PRIu16", cqid=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16"" nvme_create_cq(uint64_t addr, uint16_t cqid, uint16_t vector, uint16_t size, uint16_t qflags, int ien) "create completion queue, addr=0x%"PRIx64", cqid=%"PRIu16", vector=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16", ien=%d" nvme_del_sq(uint16_t qid) "deleting submission queue sqid=%"PRIu16"" -nvme_del_cq(uint16_t cqid) "deleted completion queue, sqid=%"PRIu16"" +nvme_del_cq(uint16_t cqid) "deleted completion queue, cqid=%"PRIu16"" nvme_identify_ctrl(void) "identify controller" nvme_identify_ns(uint16_t ns) "identify namespace, nsid=%"PRIu16"" nvme_identify_nslist(uint16_t ns) "identify namespace list, nsid=%"PRIu16"" From 9178f4fe5f083064f5c91f04d98c815ce5a5af1c Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 26 Mar 2020 16:36:27 +0100 Subject: [PATCH 4/6] Revert "mirror: Don't let an operation wait for itself" This reverts commit 7e6c4ff792734e196c8ca82564c56b5e7c6288ca. The fix was incomplete as it only protected against requests waiting for themselves, but not against requests waiting for each other. We need a different solution. Signed-off-by: Kevin Wolf Message-Id: <20200326153628.4869-2-kwolf@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/mirror.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/block/mirror.c b/block/mirror.c index 6203e5946e..5879e63473 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -283,14 +283,11 @@ static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset, } static inline void coroutine_fn -mirror_wait_for_any_operation(MirrorBlockJob *s, MirrorOp *self, bool active) +mirror_wait_for_any_operation(MirrorBlockJob *s, bool active) { MirrorOp *op; QTAILQ_FOREACH(op, &s->ops_in_flight, next) { - if (self == op) { - continue; - } /* Do not wait on pseudo ops, because it may in turn wait on * some other operation to start, which may in fact be the * caller of this function. Since there is only one pseudo op @@ -305,10 +302,10 @@ mirror_wait_for_any_operation(MirrorBlockJob *s, MirrorOp *self, bool active) } static inline void coroutine_fn -mirror_wait_for_free_in_flight_slot(MirrorBlockJob *s, MirrorOp *self) +mirror_wait_for_free_in_flight_slot(MirrorBlockJob *s) { /* Only non-active operations use up in-flight slots */ - mirror_wait_for_any_operation(s, self, false); + mirror_wait_for_any_operation(s, false); } /* Perform a mirror copy operation. @@ -351,7 +348,7 @@ static void coroutine_fn mirror_co_read(void *opaque) while (s->buf_free_count < nb_chunks) { trace_mirror_yield_in_flight(s, op->offset, s->in_flight); - mirror_wait_for_free_in_flight_slot(s, op); + mirror_wait_for_free_in_flight_slot(s); } /* Now make a QEMUIOVector taking enough granularity-sized chunks @@ -558,7 +555,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) while (s->in_flight >= MAX_IN_FLIGHT) { trace_mirror_yield_in_flight(s, offset, s->in_flight); - mirror_wait_for_free_in_flight_slot(s, pseudo_op); + mirror_wait_for_free_in_flight_slot(s); } if (s->ret < 0) { @@ -612,7 +609,7 @@ static void mirror_free_init(MirrorBlockJob *s) static void coroutine_fn mirror_wait_for_all_io(MirrorBlockJob *s) { while (s->in_flight > 0) { - mirror_wait_for_free_in_flight_slot(s, NULL); + mirror_wait_for_free_in_flight_slot(s); } } @@ -810,7 +807,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s) if (s->in_flight >= MAX_IN_FLIGHT) { trace_mirror_yield(s, UINT64_MAX, s->buf_free_count, s->in_flight); - mirror_wait_for_free_in_flight_slot(s, NULL); + mirror_wait_for_free_in_flight_slot(s); continue; } @@ -963,7 +960,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) /* Do not start passive operations while there are active * writes in progress */ while (s->in_active_write_counter) { - mirror_wait_for_any_operation(s, NULL, true); + mirror_wait_for_any_operation(s, true); } if (s->ret < 0) { @@ -989,7 +986,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 || (cnt == 0 && s->in_flight > 0)) { trace_mirror_yield(s, cnt, s->buf_free_count, s->in_flight); - mirror_wait_for_free_in_flight_slot(s, NULL); + mirror_wait_for_free_in_flight_slot(s); continue; } else if (cnt != 0) { delay_ns = mirror_iteration(s); From ce8cabbd17cf738ddfc68384440c38e5dd2fdf97 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 26 Mar 2020 16:36:28 +0100 Subject: [PATCH 5/6] mirror: Wait only for in-flight operations mirror_wait_for_free_in_flight_slot() just picks a random operation to wait for. However, a MirrorOp is already in s->ops_in_flight when mirror_co_read() waits for free slots, so if not enough slots are immediately available, an operation can end up waiting for itself, or two or more operations can wait for each other to complete, which results in a hang. Fix this by adding a flag to MirrorOp that tells us if the request is already in flight (and therefore occupies slots that it will later free), and picking only such operations for waiting. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1794692 Signed-off-by: Kevin Wolf Message-Id: <20200326153628.4869-3-kwolf@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/mirror.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/block/mirror.c b/block/mirror.c index 5879e63473..c26fd9260d 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -102,6 +102,7 @@ struct MirrorOp { bool is_pseudo_op; bool is_active_write; + bool is_in_flight; CoQueue waiting_requests; Coroutine *co; @@ -293,7 +294,9 @@ mirror_wait_for_any_operation(MirrorBlockJob *s, bool active) * caller of this function. Since there is only one pseudo op * at any given time, we will always find some real operation * to wait on. */ - if (!op->is_pseudo_op && op->is_active_write == active) { + if (!op->is_pseudo_op && op->is_in_flight && + op->is_active_write == active) + { qemu_co_queue_wait(&op->waiting_requests, NULL); return; } @@ -367,6 +370,7 @@ static void coroutine_fn mirror_co_read(void *opaque) /* Copy the dirty cluster. */ s->in_flight++; s->bytes_in_flight += op->bytes; + op->is_in_flight = true; trace_mirror_one_iteration(s, op->offset, op->bytes); ret = bdrv_co_preadv(s->mirror_top_bs->backing, op->offset, op->bytes, @@ -382,6 +386,7 @@ static void coroutine_fn mirror_co_zero(void *opaque) op->s->in_flight++; op->s->bytes_in_flight += op->bytes; *op->bytes_handled = op->bytes; + op->is_in_flight = true; ret = blk_co_pwrite_zeroes(op->s->target, op->offset, op->bytes, op->s->unmap ? BDRV_REQ_MAY_UNMAP : 0); @@ -396,6 +401,7 @@ static void coroutine_fn mirror_co_discard(void *opaque) op->s->in_flight++; op->s->bytes_in_flight += op->bytes; *op->bytes_handled = op->bytes; + op->is_in_flight = true; ret = blk_co_pdiscard(op->s->target, op->offset, op->bytes); mirror_write_complete(op, ret); @@ -1319,6 +1325,7 @@ static MirrorOp *coroutine_fn active_write_prepare(MirrorBlockJob *s, .offset = offset, .bytes = bytes, .is_active_write = true, + .is_in_flight = true, }; qemu_co_queue_init(&op->waiting_requests); QTAILQ_INSERT_TAIL(&s->ops_in_flight, op, next); From df74b1d3dff80983e7a30db1346a4a05847d65fa Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 26 Mar 2020 18:07:57 +0100 Subject: [PATCH 6/6] qcow2: Remove unused fields from BDRVQcow2State MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These fields were already removed in commit c3c10f72, but then commit b58deb34 revived them probably due to bad merge conflict resolution. They are still unused, so remove them again. Fixes: b58deb344ddff3b9d8b265bf73a65274767ee5f4 Signed-off-by: Kevin Wolf Message-Id: <20200326170757.12344-1-kwolf@redhat.com> Reviewed-by: Eric Blake Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Kevin Wolf --- block/qcow2.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/block/qcow2.h b/block/qcow2.h index 0942126232..f4de0a27d5 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -301,9 +301,6 @@ typedef struct BDRVQcow2State { QEMUTimer *cache_clean_timer; unsigned cache_clean_interval; - uint8_t *cluster_cache; - uint8_t *cluster_data; - uint64_t cluster_cache_offset; QLIST_HEAD(, QCowL2Meta) cluster_allocs; uint64_t *refcount_table;