Re: [Qemu-devel] [RFC PATCH 03/19] block: Don't use guest sector size for qemu_blockalign()

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2013/12/10 17:42, Kevin Wolf 写道:
> Am 10.12.2013 um 04:18 hat Wenchao Xia geschrieben:
>> 于 2013/12/7 1:22, Kevin Wolf 写道:
>>> bs->buffer_alignment is set by the device emulation and contains the
>>> logical block size of the guest device. This isn't something that the
>>> block layer should know, and even less something to use for determining
>>> the right alignment of buffers to be used for the host.
>>>
>>> The new function bdrv_opt_mem_align() allows for hooks in a BlockDriver
>>> so that it can tell the qemu block layer the optimal alignment to be
>>> used so that no bounce buffer must be used in the driver.
>>>
>>> This patch may change the buffer alignment from 4k to 512 for all
>>> callers that used qemu_blockalign() with the top-level image format
>>> BlockDriverState. The value was never propagated to other levels in the
>>> tree, so in particular raw-posix never required anything else than 512.
>>>
>>> While on disks with 4k sectors direct I/O requires a 4k alignment,
>>> memory may still be okay when aligned to 512 byte boundaries. This is
>>> what must have happened in practice, because otherwise this would
>>> already have failed earlier. Therefore I don't expect regressions even
>>> with this intermediate state. Later, raw-posix can implement the hook
>>> and expose a different memory alignment requirement.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>    block.c                   | 32 +++++++++++++++++++++++++++++---
>>>    include/block/block.h     |  1 +
>>>    include/block/block_int.h |  4 ++++
>>>    3 files changed, 34 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 613201b..669793b 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -213,6 +213,31 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs,
>>>        qemu_co_queue_next(&bs->throttled_reqs[is_write]);
>>>    }
>>>
>>> +size_t bdrv_opt_mem_align(BlockDriverState *bs)
>>> +{
>>> +    size_t alignment;
>>> +
>>> +    if (!bs || !bs->drv) {
>>> +        /* 4k should be on the safe side */
>>> +        return 4096;
>>> +    }
>>> +
>>> +    if (bs->drv->bdrv_opt_mem_align) {
>>> +        return bs->drv->bdrv_opt_mem_align(bs);
>>> +    }
>>> +
>>> +    if (bs->file) {
>>> +        alignment = bdrv_opt_mem_align(bs->file);
>>> +    } else {
>>> +        alignment = 512;
>>> +    }
>>> +
>>> +    if (bs->backing_hd) {
>>> +        alignment = MAX(alignment, bdrv_opt_mem_align(bs->backing_hd));
>>> +    }
>>
>>    Maybe I didn't understand the commit message correctly, does this code
>> intend to get MAX alignment value in a chain? For example:
>> base(4096)->mid(512)->top(1024) results: 4096
>> The condition to traver the backing files seems complex.
>
> Yes, you want to align any newly allocated buffer such that no matter
> what direction it takes on its way through the block layer, it will
> never be misaligned. This means that you need to use the highest
> alignment restriction.
>
> Kevin
>

   The condition to travel and compare, seems complex, since when
"return" is executed, then travers would stop. for example:
base(4096)->mid(512)->top(1024), when top's
bs->drv->bdrv_opt_mem_align != NULL, the result is 1024.
So I wonder whether this is on purpose, or the code should be:

size_t bdrv_opt_mem_align(BlockDriverState *bs)
{
     size_t alignment;

     if (!bs || !bs->drv) {
         /* 4k should be on the safe side */
         alignment = 4096;
         goto compare;
     }

     if (bs->drv->bdrv_opt_mem_align) {
         alignment = bs->drv->bdrv_opt_mem_align(bs);
         goto compare;
     }

     if (bs->file) {
         alignment = bdrv_opt_mem_align(bs->file);
     } else {
         alignment = 512;
     }

  compare:
     if (bs->backing_hd) {
         alignment = MAX(alignment, bdrv_opt_mem_align(bs->backing_hd));
     }
     return alignment;
}