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

[Vladimir Sementsov-Ogievskiy]

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