* [Qemu-devel] [PATCH 0/2] block/io: refactor padding @ 2019-05-28 8:45 Vladimir Sementsov-Ogievskiy 2019-05-28 8:45 ` [Qemu-devel] [PATCH 1/2] util/iov: introduce qemu_iovec_init_extended Vladimir Sementsov-Ogievskiy 2019-05-28 8:45 ` [Qemu-devel] [PATCH 2/2] block/io: refactor padding Vladimir Sementsov-Ogievskiy 0 siblings, 2 replies; 8+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-05-28 8:45 UTC (permalink / raw) To: qemu-devel, qemu-block; +Cc: kwolf, fam, stefanha, mreitz Hi all! Here is unifying of padding code in block/io.c, the main patch is 02. Vladimir Sementsov-Ogievskiy (2): util/iov: introduce qemu_iovec_init_extended block/io: refactor padding include/qemu/iov.h | 5 + block/io.c | 344 +++++++++++++++++++++++---------------------- util/iov.c | 89 ++++++++++++ 3 files changed, 273 insertions(+), 165 deletions(-) -- 2.18.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 1/2] util/iov: introduce qemu_iovec_init_extended 2019-05-28 8:45 [Qemu-devel] [PATCH 0/2] block/io: refactor padding Vladimir Sementsov-Ogievskiy @ 2019-05-28 8:45 ` Vladimir Sementsov-Ogievskiy 2019-05-31 8:33 ` Stefan Hajnoczi 2019-05-28 8:45 ` [Qemu-devel] [PATCH 2/2] block/io: refactor padding Vladimir Sementsov-Ogievskiy 1 sibling, 1 reply; 8+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-05-28 8:45 UTC (permalink / raw) To: qemu-devel, qemu-block; +Cc: kwolf, fam, stefanha, mreitz Introduce new initialization API, to create requests with padding. Will be used in the following patch. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/qemu/iov.h | 5 +++ util/iov.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/include/qemu/iov.h b/include/qemu/iov.h index 48b45987b7..1c5be66102 100644 --- a/include/qemu/iov.h +++ b/include/qemu/iov.h @@ -199,6 +199,11 @@ static inline void *qemu_iovec_buf(QEMUIOVector *qiov) void qemu_iovec_init(QEMUIOVector *qiov, int alloc_hint); void qemu_iovec_init_external(QEMUIOVector *qiov, struct iovec *iov, int niov); +void qemu_iovec_init_extended( + QEMUIOVector *qiov, + void *left, size_t left_len, + QEMUIOVector *middle, size_t middle_offset, size_t middle_len, + void *right, size_t right_len); void qemu_iovec_add(QEMUIOVector *qiov, void *base, size_t len); void qemu_iovec_concat(QEMUIOVector *dst, QEMUIOVector *src, size_t soffset, size_t sbytes); diff --git a/util/iov.c b/util/iov.c index 74e6ca8ed7..6bfd609998 100644 --- a/util/iov.c +++ b/util/iov.c @@ -353,6 +353,95 @@ void qemu_iovec_concat(QEMUIOVector *dst, qemu_iovec_concat_iov(dst, src->iov, src->niov, soffset, sbytes); } +/* + * qiov_find_iov + * + * Return iov, where byte at @offset (in @qiov) is. + * Update @offset to be offset inside that iov to the smae byte. + */ +static struct iovec *qiov_find_iov(QEMUIOVector *qiov, size_t *offset) +{ + struct iovec *iov = qiov->iov; + + assert(*offset < qiov->size); + + while (*offset >= iov->iov_len) { + *offset -= iov->iov_len; + iov++; + } + + return iov; +} + +/* + * qiov_slice + * + * Fund subarray of iovec's, containing requested range. @head would + * be offset in first iov (retruned by the function), @tail would be + * count of extra bytes in last iov (returned iov + @niov - 1). + */ +static struct iovec *qiov_slice(QEMUIOVector *qiov, + size_t offset, size_t len, + size_t *head, size_t *tail, int *niov) +{ + struct iovec *iov = qiov_find_iov(qiov, &offset), *end_iov; + size_t end_offset; + + assert(offset + len <= qiov->size); + + end_offset = iov->iov_len; + end_iov = iov + 1; + + while (end_offset - offset < len) { + end_offset += end_iov->iov_len; + end_iov++; + } + + *niov = end_iov - iov; + *head = offset; + *tail = (end_offset - offset) - len; + + return iov; +} + +/* + * Compile new iovec, combining @head_buf buffer, sub-qiov of @mid_qiov, + * and @tail_buf buffer into new qiov. + */ +void qemu_iovec_init_extended( + QEMUIOVector *qiov, + void *head_buf, size_t head_len, + QEMUIOVector *mid_qiov, size_t mid_offset, size_t mid_len, + void *tail_buf, size_t tail_len) +{ + size_t mid_head, mid_tail; + int niov; + struct iovec *p, *mid_iov = qiov_slice(mid_qiov, mid_offset, mid_len, + &mid_head, &mid_tail, &niov); + + assert(niov); + qiov->niov = qiov->nalloc = niov + !!head_len + !!tail_len; + qiov->size = head_len + mid_len + tail_len; + + p = qiov->iov = g_new(struct iovec, qiov->niov); + if (head_len) { + p->iov_base = head_buf; + p->iov_len = head_len; + p++; + } + + memcpy(p, mid_iov, niov * sizeof(*p)); + p[0].iov_base = (uint8_t *)p[0].iov_base + mid_head; + p[0].iov_len -= mid_head; + p[niov - 1].iov_len -= mid_tail; + p += niov; + + if (tail_len) { + p->iov_base = tail_buf; + p->iov_len = tail_len; + } +} + /* * Check if the contents of the iovecs are all zero */ -- 2.18.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] util/iov: introduce qemu_iovec_init_extended 2019-05-28 8:45 ` [Qemu-devel] [PATCH 1/2] util/iov: introduce qemu_iovec_init_extended Vladimir Sementsov-Ogievskiy @ 2019-05-31 8:33 ` Stefan Hajnoczi 0 siblings, 0 replies; 8+ messages in thread From: Stefan Hajnoczi @ 2019-05-31 8:33 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy; +Cc: kwolf, fam, qemu-devel, qemu-block, mreitz [-- Attachment #1: Type: text/plain, Size: 2347 bytes --] On Tue, May 28, 2019 at 11:45:43AM +0300, Vladimir Sementsov-Ogievskiy wrote: > diff --git a/util/iov.c b/util/iov.c > index 74e6ca8ed7..6bfd609998 100644 > --- a/util/iov.c > +++ b/util/iov.c > @@ -353,6 +353,95 @@ void qemu_iovec_concat(QEMUIOVector *dst, > qemu_iovec_concat_iov(dst, src->iov, src->niov, soffset, sbytes); > } > > +/* > + * qiov_find_iov > + * > + * Return iov, where byte at @offset (in @qiov) is. > + * Update @offset to be offset inside that iov to the smae byte. s/smae/same/ > + */ > +static struct iovec *qiov_find_iov(QEMUIOVector *qiov, size_t *offset) > +{ > + struct iovec *iov = qiov->iov; > + > + assert(*offset < qiov->size); > + > + while (*offset >= iov->iov_len) { > + *offset -= iov->iov_len; > + iov++; > + } > + > + return iov; > +} > + > +/* > + * qiov_slice > + * > + * Fund subarray of iovec's, containing requested range. @head would s/Fund/Find/ > + * be offset in first iov (retruned by the function), @tail would be s/retruned/returned/ > + * count of extra bytes in last iov (returned iov + @niov - 1). > + */ > +static struct iovec *qiov_slice(QEMUIOVector *qiov, > + size_t offset, size_t len, > + size_t *head, size_t *tail, int *niov) > +{ > + struct iovec *iov = qiov_find_iov(qiov, &offset), *end_iov; > + size_t end_offset; > + > + assert(offset + len <= qiov->size); offset has already been modified by qiov_find_iov() in iov's initializer so this comparison is meaningless. Fix: struct iovec *iov; struct iovec *end_iov; size_t end_offset; assert(offset + len <= qiov->size); iov = qiov_find_iov(qiov, &offset); Perhaps qiov_find_iov() shouldn't reuse the offset argument for two different things (the offset from the beginning of qiov and the offset from the beginning of the returned iovec). This would eliminate this class of bugs. > + > + end_offset = iov->iov_len; > + end_iov = iov + 1; > + > + while (end_offset - offset < len) { > + end_offset += end_iov->iov_len; > + end_iov++; > + } Hmm...this looks like qiov_find_iov(). Can this function be implemented in less code using two calls to qiov_find_iov() to find the first and last iovecs? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 2/2] block/io: refactor padding 2019-05-28 8:45 [Qemu-devel] [PATCH 0/2] block/io: refactor padding Vladimir Sementsov-Ogievskiy 2019-05-28 8:45 ` [Qemu-devel] [PATCH 1/2] util/iov: introduce qemu_iovec_init_extended Vladimir Sementsov-Ogievskiy @ 2019-05-28 8:45 ` Vladimir Sementsov-Ogievskiy 2019-05-31 10:51 ` Stefan Hajnoczi 1 sibling, 1 reply; 8+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-05-28 8:45 UTC (permalink / raw) To: qemu-devel, qemu-block; +Cc: kwolf, fam, stefanha, mreitz We have similar padding code in bdrv_co_pwritev, bdrv_co_do_pwrite_zeroes and bdrv_co_preadv. Let's combine and unify it. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/io.c | 344 ++++++++++++++++++++++++++++------------------------- 1 file changed, 179 insertions(+), 165 deletions(-) diff --git a/block/io.c b/block/io.c index 3134a60a48..840e276269 100644 --- a/block/io.c +++ b/block/io.c @@ -1344,28 +1344,155 @@ out: } /* - * Handle a read request in coroutine context + * Request padding + * + * |<---- align ---->| |<---- align ---->| + * |<- head ->|<------------ bytes ------------>|<-- tail -->| + * | | | | | | + * -*----------$------*-------- ... --------*----$------------*--- + * | | | | | | + * | offset | | end | + * ALIGN_UP(offset) ALIGN_DOWN(offset) ALIGN_DOWN(end) ALIGN_UP(end) + * [buf ... ) [tail_buf ) + * + * @buf is an aligned allocation needed to store @head and @tail paddings. @head + * is placed at the beginning of @buf and @tail at the @end. + * + * @tail_buf is a pointer to sub-buffer, corresponding to align-sized chunk + * around tail, if tail exists. + * + * @merge_reads is true for small requests, + * if @buf_len == @head + bytes + @tail. In this case it is possible that both + * head and tail exist but @buf_len == align and @tail_buf == @buf. */ +typedef struct BdrvRequestPadding { + uint8_t *buf; + size_t buf_len; + uint8_t *tail_buf; + size_t head; + size_t tail; + bool merge_reads; + QEMUIOVector local_qiov; +} BdrvRequestPadding; + +static bool bdrv_init_padding(BlockDriverState *bs, + int64_t offset, int64_t bytes, + BdrvRequestPadding *pad) +{ + uint64_t align = bs->bl.request_alignment; + size_t sum; + + memset(pad, 0, sizeof(*pad)); + + pad->head = offset & (align - 1); + pad->tail = ((offset + bytes) & (align - 1)); + if (pad->tail) { + pad->tail = align - pad->tail; + } + + if ((!pad->head && !pad->tail) || !bytes) { + return false; + } + + sum = pad->head + bytes + pad->tail; + pad->buf_len = (sum > align && pad->head && pad->tail) ? 2 * align : align; + pad->buf = qemu_blockalign(bs, pad->buf_len); + pad->merge_reads = sum == pad->buf_len; + if (pad->tail) { + pad->tail_buf = pad->buf + pad->buf_len - align; + } + + return true; +} + +static int bdrv_padding_read(BdrvChild *child, + BdrvTrackedRequest *req, + BdrvRequestPadding *pad, + bool zero_middle) +{ + QEMUIOVector local_qiov; + BlockDriverState *bs = child->bs; + uint64_t align = bs->bl.request_alignment; + int ret; + + assert(req->serialising && pad->buf); + + if (pad->head || pad->merge_reads) { + uint64_t bytes = pad->merge_reads ? pad->buf_len : align; + + qemu_iovec_init_buf(&local_qiov, pad->buf, bytes); + + bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_HEAD); + ret = bdrv_aligned_preadv(child, req, req->overlap_offset, bytes, + align, &local_qiov, 0); + if (ret < 0) { + return ret; + } + bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD); + + if (pad->merge_reads) { + goto zero_mem; + } + } + + if (pad->tail) { + qemu_iovec_init_buf(&local_qiov, pad->tail_buf, align); + + bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_TAIL); + ret = bdrv_aligned_preadv( + child, req, + req->overlap_offset + req->overlap_bytes - align, + align, align, &local_qiov, 0); + if (ret < 0) { + return ret; + } + bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL); + } + +zero_mem: + if (zero_middle) { + memset(pad->buf + pad->head, 0, pad->buf_len - pad->head - pad->tail); + } + + return 0; +} + +static void bdrv_padding_destroy(BdrvRequestPadding *pad) +{ + if (pad->buf) { + qemu_vfree(pad->buf); + qemu_iovec_destroy(&pad->local_qiov); + } +} + +static QEMUIOVector *bdrv_pad_request(BlockDriverState *bs, QEMUIOVector *qiov, + int64_t *offset, unsigned int *bytes, + BdrvRequestPadding *pad) +{ + bdrv_init_padding(bs, *offset, *bytes, pad); + if (!pad->buf) { + return qiov; + } + + qemu_iovec_init_extended(&pad->local_qiov, pad->buf, pad->head, + qiov, 0, *bytes, + pad->buf + pad->buf_len - pad->tail, pad->tail); + *bytes += pad->head + pad->tail; + *offset -= pad->head; + + return &pad->local_qiov; +} + int coroutine_fn bdrv_co_preadv(BdrvChild *child, int64_t offset, unsigned int bytes, QEMUIOVector *qiov, BdrvRequestFlags flags) { BlockDriverState *bs = child->bs; - BlockDriver *drv = bs->drv; BdrvTrackedRequest req; - - uint64_t align = bs->bl.request_alignment; - uint8_t *head_buf = NULL; - uint8_t *tail_buf = NULL; - QEMUIOVector local_qiov; - bool use_local_qiov = false; + BdrvRequestPadding pad; int ret; - trace_bdrv_co_preadv(child->bs, offset, bytes, flags); - - if (!drv) { - return -ENOMEDIUM; - } + trace_bdrv_co_preadv(bs, offset, bytes, flags); ret = bdrv_check_byte_request(bs, offset, bytes); if (ret < 0) { @@ -1379,43 +1506,16 @@ int coroutine_fn bdrv_co_preadv(BdrvChild *child, flags |= BDRV_REQ_COPY_ON_READ; } - /* Align read if necessary by padding qiov */ - if (offset & (align - 1)) { - head_buf = qemu_blockalign(bs, align); - qemu_iovec_init(&local_qiov, qiov->niov + 2); - qemu_iovec_add(&local_qiov, head_buf, offset & (align - 1)); - qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); - use_local_qiov = true; - - bytes += offset & (align - 1); - offset = offset & ~(align - 1); - } - - if ((offset + bytes) & (align - 1)) { - if (!use_local_qiov) { - qemu_iovec_init(&local_qiov, qiov->niov + 1); - qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); - use_local_qiov = true; - } - tail_buf = qemu_blockalign(bs, align); - qemu_iovec_add(&local_qiov, tail_buf, - align - ((offset + bytes) & (align - 1))); - - bytes = ROUND_UP(bytes, align); - } + qiov = bdrv_pad_request(bs, qiov, &offset, &bytes, &pad); tracked_request_begin(&req, bs, offset, bytes, BDRV_TRACKED_READ); - ret = bdrv_aligned_preadv(child, &req, offset, bytes, align, - use_local_qiov ? &local_qiov : qiov, - flags); + ret = bdrv_aligned_preadv(child, &req, offset, bytes, + bs->bl.request_alignment, + qiov, flags); tracked_request_end(&req); bdrv_dec_in_flight(bs); - if (use_local_qiov) { - qemu_iovec_destroy(&local_qiov); - qemu_vfree(head_buf); - qemu_vfree(tail_buf); - } + bdrv_padding_destroy(&pad); return ret; } @@ -1711,44 +1811,34 @@ static int coroutine_fn bdrv_co_do_zero_pwritev(BdrvChild *child, BdrvTrackedRequest *req) { BlockDriverState *bs = child->bs; - uint8_t *buf = NULL; QEMUIOVector local_qiov; uint64_t align = bs->bl.request_alignment; - unsigned int head_padding_bytes, tail_padding_bytes; int ret = 0; + bool padding; + BdrvRequestPadding pad; - head_padding_bytes = offset & (align - 1); - tail_padding_bytes = (align - (offset + bytes)) & (align - 1); - - - assert(flags & BDRV_REQ_ZERO_WRITE); - if (head_padding_bytes || tail_padding_bytes) { - buf = qemu_blockalign(bs, align); - qemu_iovec_init_buf(&local_qiov, buf, align); - } - if (head_padding_bytes) { - uint64_t zero_bytes = MIN(bytes, align - head_padding_bytes); - - /* RMW the unaligned part before head. */ + padding = bdrv_init_padding(bs, offset, bytes, &pad); + if (padding) { mark_request_serialising(req, align); wait_serialising_requests(req); - bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_HEAD); - ret = bdrv_aligned_preadv(child, req, offset & ~(align - 1), align, - align, &local_qiov, 0); - if (ret < 0) { - goto fail; - } - bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD); - memset(buf + head_padding_bytes, 0, zero_bytes); - ret = bdrv_aligned_pwritev(child, req, offset & ~(align - 1), align, - align, &local_qiov, - flags & ~BDRV_REQ_ZERO_WRITE); - if (ret < 0) { - goto fail; + bdrv_padding_read(child, req, &pad, true); + + if (pad.head || pad.merge_reads) { + int64_t aligned_offset = offset & ~(align - 1); + int64_t write_bytes = pad.merge_reads ? pad.buf_len : align; + + qemu_iovec_init_buf(&local_qiov, pad.buf, write_bytes); + ret = bdrv_aligned_pwritev(child, req, aligned_offset, write_bytes, + align, &local_qiov, + flags & ~BDRV_REQ_ZERO_WRITE); + if (ret < 0 || pad.merge_reads) { + /* Error or all work is done */ + goto out; + } + offset += write_bytes - pad.head; + bytes -= write_bytes - pad.head; } - offset += zero_bytes; - bytes -= zero_bytes; } assert(!bytes || (offset & (align - 1)) == 0); @@ -1758,7 +1848,7 @@ static int coroutine_fn bdrv_co_do_zero_pwritev(BdrvChild *child, ret = bdrv_aligned_pwritev(child, req, offset, aligned_bytes, align, NULL, flags); if (ret < 0) { - goto fail; + goto out; } bytes -= aligned_bytes; offset += aligned_bytes; @@ -1766,26 +1856,17 @@ static int coroutine_fn bdrv_co_do_zero_pwritev(BdrvChild *child, assert(!bytes || (offset & (align - 1)) == 0); if (bytes) { - assert(align == tail_padding_bytes + bytes); - /* RMW the unaligned part after tail. */ - mark_request_serialising(req, align); - wait_serialising_requests(req); - bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_TAIL); - ret = bdrv_aligned_preadv(child, req, offset, align, - align, &local_qiov, 0); - if (ret < 0) { - goto fail; - } - bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL); + assert(align == pad.tail + bytes); - memset(buf, 0, bytes); + qemu_iovec_init_buf(&local_qiov, pad.tail_buf, align); ret = bdrv_aligned_pwritev(child, req, offset, align, align, &local_qiov, flags & ~BDRV_REQ_ZERO_WRITE); } -fail: - qemu_vfree(buf); - return ret; +out: + bdrv_padding_destroy(&pad); + + return ret; } /* @@ -1798,10 +1879,7 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child, BlockDriverState *bs = child->bs; BdrvTrackedRequest req; uint64_t align = bs->bl.request_alignment; - uint8_t *head_buf = NULL; - uint8_t *tail_buf = NULL; - QEMUIOVector local_qiov; - bool use_local_qiov = false; + BdrvRequestPadding pad; int ret; trace_bdrv_co_pwritev(child->bs, offset, bytes, flags); @@ -1828,86 +1906,22 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child, goto out; } - if (offset & (align - 1)) { - QEMUIOVector head_qiov; - + qiov = bdrv_pad_request(bs, qiov, &offset, &bytes, &pad); + if (pad.head || pad.tail) { mark_request_serialising(&req, align); wait_serialising_requests(&req); - - head_buf = qemu_blockalign(bs, align); - qemu_iovec_init_buf(&head_qiov, head_buf, align); - - bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_HEAD); - ret = bdrv_aligned_preadv(child, &req, offset & ~(align - 1), align, - align, &head_qiov, 0); - if (ret < 0) { - goto fail; - } - bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD); - - qemu_iovec_init(&local_qiov, qiov->niov + 2); - qemu_iovec_add(&local_qiov, head_buf, offset & (align - 1)); - qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); - use_local_qiov = true; - - bytes += offset & (align - 1); - offset = offset & ~(align - 1); - - /* We have read the tail already if the request is smaller - * than one aligned block. - */ - if (bytes < align) { - qemu_iovec_add(&local_qiov, head_buf + bytes, align - bytes); - bytes = align; - } - } - - if ((offset + bytes) & (align - 1)) { - QEMUIOVector tail_qiov; - size_t tail_bytes; - bool waited; - - mark_request_serialising(&req, align); - waited = wait_serialising_requests(&req); - assert(!waited || !use_local_qiov); - - tail_buf = qemu_blockalign(bs, align); - qemu_iovec_init_buf(&tail_qiov, tail_buf, align); - - bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_TAIL); - ret = bdrv_aligned_preadv(child, &req, (offset + bytes) & ~(align - 1), - align, align, &tail_qiov, 0); - if (ret < 0) { - goto fail; - } - bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL); - - if (!use_local_qiov) { - qemu_iovec_init(&local_qiov, qiov->niov + 1); - qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); - use_local_qiov = true; - } - - tail_bytes = (offset + bytes) & (align - 1); - qemu_iovec_add(&local_qiov, tail_buf + tail_bytes, align - tail_bytes); - - bytes = ROUND_UP(bytes, align); + bdrv_padding_read(child, &req, &pad, false); } ret = bdrv_aligned_pwritev(child, &req, offset, bytes, align, - use_local_qiov ? &local_qiov : qiov, - flags); + qiov, flags); -fail: + bdrv_padding_destroy(&pad); - if (use_local_qiov) { - qemu_iovec_destroy(&local_qiov); - } - qemu_vfree(head_buf); - qemu_vfree(tail_buf); out: tracked_request_end(&req); bdrv_dec_in_flight(bs); + return ret; } -- 2.18.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] block/io: refactor padding 2019-05-28 8:45 ` [Qemu-devel] [PATCH 2/2] block/io: refactor padding Vladimir Sementsov-Ogievskiy @ 2019-05-31 10:51 ` Stefan Hajnoczi 2019-05-31 14:10 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 8+ messages in thread From: Stefan Hajnoczi @ 2019-05-31 10:51 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy; +Cc: kwolf, fam, qemu-devel, qemu-block, mreitz [-- Attachment #1: Type: text/plain, Size: 5822 bytes --] On Tue, May 28, 2019 at 11:45:44AM +0300, Vladimir Sementsov-Ogievskiy wrote: > We have similar padding code in bdrv_co_pwritev, > bdrv_co_do_pwrite_zeroes and bdrv_co_preadv. Let's combine and unify > it. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/io.c | 344 ++++++++++++++++++++++++++++------------------------- Hmmm...this adds more lines than it removes. O_o Merging a change like this can still be useful but there's a risk of making the code harder to understand due to the additional layers of abstraction. > 1 file changed, 179 insertions(+), 165 deletions(-) > > diff --git a/block/io.c b/block/io.c > index 3134a60a48..840e276269 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -1344,28 +1344,155 @@ out: > } > > /* > - * Handle a read request in coroutine context > + * Request padding > + * > + * |<---- align ---->| |<---- align ---->| > + * |<- head ->|<------------ bytes ------------>|<-- tail -->| > + * | | | | | | > + * -*----------$------*-------- ... --------*----$------------*--- > + * | | | | | | > + * | offset | | end | > + * ALIGN_UP(offset) ALIGN_DOWN(offset) ALIGN_DOWN(end) ALIGN_UP(end) Are ALIGN_UP(offset) and ALIGN_DOWN(offset) in the wrong order? > + * [buf ... ) [tail_buf ) > + * > + * @buf is an aligned allocation needed to store @head and @tail paddings. @head > + * is placed at the beginning of @buf and @tail at the @end. > + * > + * @tail_buf is a pointer to sub-buffer, corresponding to align-sized chunk > + * around tail, if tail exists. > + * > + * @merge_reads is true for small requests, > + * if @buf_len == @head + bytes + @tail. In this case it is possible that both > + * head and tail exist but @buf_len == align and @tail_buf == @buf. > */ > +typedef struct BdrvRequestPadding { > + uint8_t *buf; > + size_t buf_len; > + uint8_t *tail_buf; > + size_t head; > + size_t tail; > + bool merge_reads; > + QEMUIOVector local_qiov; > +} BdrvRequestPadding; > + > +static bool bdrv_init_padding(BlockDriverState *bs, > + int64_t offset, int64_t bytes, > + BdrvRequestPadding *pad) > +{ > + uint64_t align = bs->bl.request_alignment; > + size_t sum; > + > + memset(pad, 0, sizeof(*pad)); > + > + pad->head = offset & (align - 1); > + pad->tail = ((offset + bytes) & (align - 1)); > + if (pad->tail) { > + pad->tail = align - pad->tail; > + } > + > + if ((!pad->head && !pad->tail) || !bytes) { > + return false; > + } > + > + sum = pad->head + bytes + pad->tail; > + pad->buf_len = (sum > align && pad->head && pad->tail) ? 2 * align : align; > + pad->buf = qemu_blockalign(bs, pad->buf_len); > + pad->merge_reads = sum == pad->buf_len; > + if (pad->tail) { > + pad->tail_buf = pad->buf + pad->buf_len - align; > + } > + > + return true; > +} > + > +static int bdrv_padding_read(BdrvChild *child, > + BdrvTrackedRequest *req, > + BdrvRequestPadding *pad, > + bool zero_middle) > +{ > + QEMUIOVector local_qiov; > + BlockDriverState *bs = child->bs; > + uint64_t align = bs->bl.request_alignment; > + int ret; > + > + assert(req->serialising && pad->buf); > + > + if (pad->head || pad->merge_reads) { > + uint64_t bytes = pad->merge_reads ? pad->buf_len : align; > + > + qemu_iovec_init_buf(&local_qiov, pad->buf, bytes); > + > + bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_HEAD); PWRITEV? That's unexpected for a function called bdrv_padding_read(). Please rename this to bdrv_padding_rmw_read() so it's clear this is part of a read-modify-write operation, not a regular read. > + ret = bdrv_aligned_preadv(child, req, req->overlap_offset, bytes, > + align, &local_qiov, 0); > + if (ret < 0) { > + return ret; > + } > + bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD); > + > + if (pad->merge_reads) { > + goto zero_mem; > + } > + } > + > + if (pad->tail) { > + qemu_iovec_init_buf(&local_qiov, pad->tail_buf, align); > + > + bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_TAIL); > + ret = bdrv_aligned_preadv( > + child, req, > + req->overlap_offset + req->overlap_bytes - align, > + align, align, &local_qiov, 0); > + if (ret < 0) { > + return ret; > + } > + bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL); > + } > + > +zero_mem: > + if (zero_middle) { > + memset(pad->buf + pad->head, 0, pad->buf_len - pad->head - pad->tail); > + } > + > + return 0; > +} > + > +static void bdrv_padding_destroy(BdrvRequestPadding *pad) > +{ > + if (pad->buf) { > + qemu_vfree(pad->buf); > + qemu_iovec_destroy(&pad->local_qiov); > + } > +} > + > +static QEMUIOVector *bdrv_pad_request(BlockDriverState *bs, QEMUIOVector *qiov, > + int64_t *offset, unsigned int *bytes, > + BdrvRequestPadding *pad) Doc comment missing? > +{ > + bdrv_init_padding(bs, *offset, *bytes, pad); > + if (!pad->buf) { > + return qiov; > + } I think there's no need to peek at pad->buf: if (!bdrv_init_padding(bs, *offset, *bytes, pad)) { return qiov; } [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] block/io: refactor padding 2019-05-31 10:51 ` Stefan Hajnoczi @ 2019-05-31 14:10 ` Vladimir Sementsov-Ogievskiy 2019-05-31 15:49 ` Kevin Wolf 0 siblings, 1 reply; 8+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-05-31 14:10 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: kwolf, fam, qemu-devel, qemu-block, mreitz 31.05.2019 13:51, Stefan Hajnoczi wrote: > On Tue, May 28, 2019 at 11:45:44AM +0300, Vladimir Sementsov-Ogievskiy wrote: >> We have similar padding code in bdrv_co_pwritev, >> bdrv_co_do_pwrite_zeroes and bdrv_co_preadv. Let's combine and unify >> it. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/io.c | 344 ++++++++++++++++++++++++++++------------------------- > > Hmmm...this adds more lines than it removes. O_o It's near to be the same size, and keep in mind big comment. > > Merging a change like this can still be useful but there's a risk of > making the code harder to understand due to the additional layers of > abstraction. It's a preparation for adding qiov_offset parameter to read/write path. Seems correct to unify similar things, which I'm going to change. And I really want to make code more understandable than it was.. But my view is not general of course. > >> 1 file changed, 179 insertions(+), 165 deletions(-) >> >> diff --git a/block/io.c b/block/io.c >> index 3134a60a48..840e276269 100644 >> --- a/block/io.c >> +++ b/block/io.c >> @@ -1344,28 +1344,155 @@ out: >> } >> >> /* >> - * Handle a read request in coroutine context >> + * Request padding >> + * >> + * |<---- align ---->| |<---- align ---->| >> + * |<- head ->|<------------ bytes ------------>|<-- tail -->| >> + * | | | | | | >> + * -*----------$------*-------- ... --------*----$------------*--- >> + * | | | | | | >> + * | offset | | end | >> + * ALIGN_UP(offset) ALIGN_DOWN(offset) ALIGN_DOWN(end) ALIGN_UP(end) > > Are ALIGN_UP(offset) and ALIGN_DOWN(offset) in the wrong order? yes :( > >> + * [buf ... ) [tail_buf ) >> + * >> + * @buf is an aligned allocation needed to store @head and @tail paddings. @head >> + * is placed at the beginning of @buf and @tail at the @end. >> + * >> + * @tail_buf is a pointer to sub-buffer, corresponding to align-sized chunk >> + * around tail, if tail exists. >> + * >> + * @merge_reads is true for small requests, >> + * if @buf_len == @head + bytes + @tail. In this case it is possible that both >> + * head and tail exist but @buf_len == align and @tail_buf == @buf. >> */ >> +typedef struct BdrvRequestPadding { >> + uint8_t *buf; >> + size_t buf_len; >> + uint8_t *tail_buf; >> + size_t head; >> + size_t tail; >> + bool merge_reads; >> + QEMUIOVector local_qiov; >> +} BdrvRequestPadding; >> + >> +static bool bdrv_init_padding(BlockDriverState *bs, >> + int64_t offset, int64_t bytes, >> + BdrvRequestPadding *pad) >> +{ >> + uint64_t align = bs->bl.request_alignment; >> + size_t sum; >> + >> + memset(pad, 0, sizeof(*pad)); >> + >> + pad->head = offset & (align - 1); >> + pad->tail = ((offset + bytes) & (align - 1)); >> + if (pad->tail) { >> + pad->tail = align - pad->tail; >> + } >> + >> + if ((!pad->head && !pad->tail) || !bytes) { >> + return false; >> + } >> + >> + sum = pad->head + bytes + pad->tail; >> + pad->buf_len = (sum > align && pad->head && pad->tail) ? 2 * align : align; >> + pad->buf = qemu_blockalign(bs, pad->buf_len); >> + pad->merge_reads = sum == pad->buf_len; >> + if (pad->tail) { >> + pad->tail_buf = pad->buf + pad->buf_len - align; >> + } >> + >> + return true; >> +} >> + >> +static int bdrv_padding_read(BdrvChild *child, >> + BdrvTrackedRequest *req, >> + BdrvRequestPadding *pad, >> + bool zero_middle) >> +{ >> + QEMUIOVector local_qiov; >> + BlockDriverState *bs = child->bs; >> + uint64_t align = bs->bl.request_alignment; >> + int ret; >> + >> + assert(req->serialising && pad->buf); >> + >> + if (pad->head || pad->merge_reads) { >> + uint64_t bytes = pad->merge_reads ? pad->buf_len : align; >> + >> + qemu_iovec_init_buf(&local_qiov, pad->buf, bytes); >> + >> + bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_HEAD); > > PWRITEV? That's unexpected for a function called bdrv_padding_read(). > Please rename this to bdrv_padding_rmw_read() so it's clear this is part > of a read-modify-write operation, not a regular read. > >> + ret = bdrv_aligned_preadv(child, req, req->overlap_offset, bytes, >> + align, &local_qiov, 0); >> + if (ret < 0) { >> + return ret; >> + } >> + bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD); >> + >> + if (pad->merge_reads) { >> + goto zero_mem; >> + } >> + } >> + >> + if (pad->tail) { >> + qemu_iovec_init_buf(&local_qiov, pad->tail_buf, align); >> + >> + bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_TAIL); >> + ret = bdrv_aligned_preadv( >> + child, req, >> + req->overlap_offset + req->overlap_bytes - align, >> + align, align, &local_qiov, 0); >> + if (ret < 0) { >> + return ret; >> + } >> + bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL); >> + } >> + >> +zero_mem: >> + if (zero_middle) { >> + memset(pad->buf + pad->head, 0, pad->buf_len - pad->head - pad->tail); >> + } >> + >> + return 0; >> +} >> + >> +static void bdrv_padding_destroy(BdrvRequestPadding *pad) >> +{ >> + if (pad->buf) { >> + qemu_vfree(pad->buf); >> + qemu_iovec_destroy(&pad->local_qiov); >> + } >> +} >> + >> +static QEMUIOVector *bdrv_pad_request(BlockDriverState *bs, QEMUIOVector *qiov, >> + int64_t *offset, unsigned int *bytes, >> + BdrvRequestPadding *pad) > > Doc comment missing? > >> +{ >> + bdrv_init_padding(bs, *offset, *bytes, pad); >> + if (!pad->buf) { >> + return qiov; >> + } > > I think there's no need to peek at pad->buf: > > if (!bdrv_init_padding(bs, *offset, *bytes, pad)) { > return qiov; > } > -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] block/io: refactor padding 2019-05-31 14:10 ` Vladimir Sementsov-Ogievskiy @ 2019-05-31 15:49 ` Kevin Wolf 2019-05-31 15:58 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 8+ messages in thread From: Kevin Wolf @ 2019-05-31 15:49 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy Cc: fam, qemu-block, qemu-devel, Stefan Hajnoczi, mreitz Am 31.05.2019 um 16:10 hat Vladimir Sementsov-Ogievskiy geschrieben: > 31.05.2019 13:51, Stefan Hajnoczi wrote: > > On Tue, May 28, 2019 at 11:45:44AM +0300, Vladimir Sementsov-Ogievskiy wrote: > >> We have similar padding code in bdrv_co_pwritev, > >> bdrv_co_do_pwrite_zeroes and bdrv_co_preadv. Let's combine and unify > >> it. > >> > >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > >> --- > >> block/io.c | 344 ++++++++++++++++++++++++++++------------------------- > > > > Hmmm...this adds more lines than it removes. O_o > > It's near to be the same size, and keep in mind big comment. If you take the whole series into account, it looks even less favourable, despite some comments: 3 files changed, 273 insertions(+), 165 deletions(-) > > > > Merging a change like this can still be useful but there's a risk of > > making the code harder to understand due to the additional layers of > > abstraction. > > It's a preparation for adding qiov_offset parameter to read/write path. Seems > correct to unify similar things, which I'm going to change. And I really want > to make code more understandable than it was.. But my view is not general > of course. Depending on the changes you're going to make (e.g. adding more users of the same functionality, or making the duplicated code much larger), this can be a good justification even if the code size increases. I'd suggest to add the explanation (like 'This is in preparation for ...') to the commit message then. Kevin ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] block/io: refactor padding 2019-05-31 15:49 ` Kevin Wolf @ 2019-05-31 15:58 ` Vladimir Sementsov-Ogievskiy 0 siblings, 0 replies; 8+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-05-31 15:58 UTC (permalink / raw) To: Kevin Wolf; +Cc: fam, qemu-block, qemu-devel, Stefan Hajnoczi, mreitz 31.05.2019 18:49, Kevin Wolf wrote: > Am 31.05.2019 um 16:10 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 31.05.2019 13:51, Stefan Hajnoczi wrote: >>> On Tue, May 28, 2019 at 11:45:44AM +0300, Vladimir Sementsov-Ogievskiy wrote: >>>> We have similar padding code in bdrv_co_pwritev, >>>> bdrv_co_do_pwrite_zeroes and bdrv_co_preadv. Let's combine and unify >>>> it. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> block/io.c | 344 ++++++++++++++++++++++++++++------------------------- >>> >>> Hmmm...this adds more lines than it removes. O_o >> >> It's near to be the same size, and keep in mind big comment. > > If you take the whole series into account, it looks even less > favourable, despite some comments: > > 3 files changed, 273 insertions(+), 165 deletions(-) > >>> >>> Merging a change like this can still be useful but there's a risk of >>> making the code harder to understand due to the additional layers of >>> abstraction. >> >> It's a preparation for adding qiov_offset parameter to read/write path. Seems >> correct to unify similar things, which I'm going to change. And I really want >> to make code more understandable than it was.. But my view is not general >> of course. > > Depending on the changes you're going to make (e.g. adding more users of > the same functionality, or making the duplicated code much larger), this > can be a good justification even if the code size increases. > > I'd suggest to add the explanation (like 'This is in preparation for > ...') to the commit message then. > > Kevin > I think, I'll resend the whole series with this preparation next week. -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-05-31 16:12 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-28 8:45 [Qemu-devel] [PATCH 0/2] block/io: refactor padding Vladimir Sementsov-Ogievskiy 2019-05-28 8:45 ` [Qemu-devel] [PATCH 1/2] util/iov: introduce qemu_iovec_init_extended Vladimir Sementsov-Ogievskiy 2019-05-31 8:33 ` Stefan Hajnoczi 2019-05-28 8:45 ` [Qemu-devel] [PATCH 2/2] block/io: refactor padding Vladimir Sementsov-Ogievskiy 2019-05-31 10:51 ` Stefan Hajnoczi 2019-05-31 14:10 ` Vladimir Sementsov-Ogievskiy 2019-05-31 15:49 ` Kevin Wolf 2019-05-31 15:58 ` Vladimir Sementsov-Ogievskiy
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.