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 <vsement...@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
