Re: [Qemu-devel] [PATCH 2/2] block/io: refactor padding

[Stefan Hajnoczi]

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 <address@hidden>
> ---
>  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