|
| From: | Vladimir Sementsov-Ogievskiy |
| Subject: | Re: [Qemu-devel] [PATCH 5/7] qcow2: refactor qcow2_co_pwritev: split out qcow2_co_do_pwritev |
| Date: | Mon, 1 Oct 2018 18:43:03 +0300 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 |
28.09.2018 17:23, Max Reitz wrote:
On 07.08.18 19:43, Vladimir Sementsov-Ogievskiy wrote:Split out block which will be reused in async scheme. Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden> --- block/qcow2.c | 138 ++++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 86 insertions(+), 52 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index a0df8d4e50..4d669432d1 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2210,6 +2210,85 @@ static bool merge_cow(uint64_t offset, unsigned bytes, return false; }+/* qcow2_co_do_pwritev+ * Called without s->lock unlocked + * hd_qiov - temp qiov for any use. It is initialized so it is empty and + * support adding up to qiov->niov + 2 elements + * l2meta - if not NULL, qcow2_co_do_pwritev() will consume it. Caller must not + * use it somehow after qcow2_co_do_pwritev() call + */ +static coroutine_fn int qcow2_co_do_pwritev(BlockDriverState *bs, + uint64_t file_cluster_offset, + uint64_t offset, + uint64_t bytes, + QEMUIOVector *qiov, + uint64_t qiov_offset, + QCowL2Meta *l2meta) +{ + int ret; + BDRVQcow2State *s = bs->opaque; + void *crypt_buf = NULL; + QEMUIOVector hd_qiov; + int offset_in_cluster = offset_into_cluster(s, offset); + + qemu_iovec_reset(&hd_qiov);This shouldn't be here.+ qemu_iovec_init(&hd_qiov, qiov->niov); + + if (bs->encrypted) { + assert(s->crypto); + assert(bytes <= QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size); + crypt_buf = qemu_try_blockalign(bs->file->bs, bytes); + qemu_iovec_to_buf(qiov, qiov_offset, crypt_buf, bytes); + + if (qcrypto_block_encrypt(s->crypto, + (s->crypt_physical_offset ? + file_cluster_offset + offset_in_cluster : + offset), + crypt_buf, + bytes, NULL) < 0) {Same question as in the read case: Can't we make do without the bounce buffer?
I think, we should not modify guest buffers..
+ ret = -EIO; + goto fail; + } + + qemu_iovec_add(&hd_qiov, crypt_buf, bytes); + } else { + qemu_iovec_concat(&hd_qiov, qiov, qiov_offset, bytes); + } + + /* If we need to do COW, check if it's possible to merge the + * writing of the guest data together with that of the COW regions. + * If it's not possible (or not necessary) then write the + * guest data now. */ + if (!merge_cow(offset, bytes, &hd_qiov, l2meta)) { + BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO); + trace_qcow2_writev_data(qemu_coroutine_self(), + file_cluster_offset + offset_in_cluster); + ret = bdrv_co_pwritev(bs->file, + file_cluster_offset + offset_in_cluster, + bytes, &hd_qiov, 0); + if (ret < 0) { + qemu_co_mutex_lock(&s->lock); + goto fail; + } + } + + qemu_co_mutex_lock(&s->lock);It looks a bit weird to only do some of the lock handling in this function. I don't quite like it, but as long as you keep it around the qcow2_handle_l2meta(), I won't object. A problem is that currently you don't keep it around qcow2_handle_l2meta(). Before going to the fail label, one has to manually lock the mutex (which you don't do in one of the above error paths, but that's fallout from patch 2). Maybe it would be nicer with a separate label that does the lock. Like: ... ret = qcow2_handle_l2meta(bs, &l2meta, true); goto done_locked; fail_unlocked: qemu_co_mutex_lock(&s->lock) done_locked: qcow2_handle_l2meta(bs, &l2meta, false); ...
ok
+ + ret = qcow2_handle_l2meta(bs, &l2meta, true); + if (ret) { + goto fail; + } + +fail: + qcow2_handle_l2meta(bs, &l2meta, false); + qemu_co_mutex_unlock(&s->lock); + + qemu_vfree(crypt_buf); + qemu_iovec_destroy(&hd_qiov); + + return ret; +} + static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags) @@ -2262,63 +2341,16 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,qemu_co_mutex_unlock(&s->lock); - qemu_iovec_reset(&hd_qiov);- qemu_iovec_concat(&hd_qiov, qiov, bytes_done, cur_bytes);- if (bs->encrypted) {- assert(s->crypto); - if (!cluster_data) { - cluster_data = qemu_try_blockalign(bs->file->bs, - QCOW_MAX_CRYPT_CLUSTERS - * s->cluster_size); - if (cluster_data == NULL) { - ret = -ENOMEM; - goto fail; - } - } - - assert(hd_qiov.size <= - QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size); - qemu_iovec_to_buf(&hd_qiov, 0, cluster_data, hd_qiov.size); - - if (qcrypto_block_encrypt(s->crypto, - (s->crypt_physical_offset ? - cluster_offset + offset_in_cluster : - offset), - cluster_data, - cur_bytes, NULL) < 0) { - ret = -EIO; - goto fail; - } - - qemu_iovec_reset(&hd_qiov); - qemu_iovec_add(&hd_qiov, cluster_data, cur_bytes); - } - - /* If we need to do COW, check if it's possible to merge the - * writing of the guest data together with that of the COW regions. - * If it's not possible (or not necessary) then write the - * guest data now. */ - if (!merge_cow(offset, cur_bytes, &hd_qiov, l2meta)) { - BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO); - trace_qcow2_writev_data(qemu_coroutine_self(), - cluster_offset + offset_in_cluster); - ret = bdrv_co_pwritev(bs->file, - cluster_offset + offset_in_cluster, - cur_bytes, &hd_qiov, 0); - if (ret < 0) { - qemu_co_mutex_lock(&s->lock); - goto fail; - } + ret = qcow2_co_do_pwritev(bs, cluster_offset, offset, cur_bytes, + qiov, bytes_done, l2meta); + l2meta = NULL; /* l2meta is consumed by qcow2_co_do_pwritev() */ + if (ret < 0) { + goto fail_nometa; }qemu_co_mutex_lock(&s->lock); - ret = qcow2_handle_l2meta(bs, &l2meta, true);- if (ret) { - goto fail; - } - bytes -= cur_bytes; offset += cur_bytes; bytes_done += cur_bytes; @@ -2331,6 +2363,8 @@ fail:qemu_co_mutex_unlock(&s->lock); +fail_nometa:+I'd remove the blank line (because we usually don't have blank lines below labels).qemu_iovec_destroy(&hd_qiov); qemu_vfree(cluster_data);cluster_data and hd_qiov are unused now. Maxtrace_qcow2_writev_done_req(qemu_coroutine_self(), ret);
-- Best regards, Vladimir
| [Prev in Thread] | Current Thread | [Next in Thread] |