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 <vsement...@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;
}
signature.asc
Description: PGP signature
