Re: [Qemu-trivial] [Qemu-devel] [PATCH] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Max Reitz <mreitz@redhat.com>]

On 2014-10-23 at 09:03, Kevin Wolf wrote:
> Am 23.10.2014 um 00:18 hat Eric Blake geschrieben:
>> On 10/21/2014 03:24 AM, Max Reitz wrote:
>>> On 2014-10-21 at 10:04, Zhang Haoyu wrote:
>>>> Use local variable to bdrv_pwrite_sync L1 table,
>>>> needless to make conversion of cached L1 table between
>>>> big-endian and host style.
>>>>
>>>> Signed-off-by: Zhang Haoyu <zhanghy@sangfor.com>
>>>> ---
>>>>    block/qcow2-refcount.c | 22 +++++++---------------
>>>>    1 file changed, 7 insertions(+), 15 deletions(-)
>>>>
>> I know we're up to v5 and that Max already took it into his branch, but...
>>
>>
>>>>        l1_size2 = l1_size * sizeof(uint64_t);
>>>> +    l1_table = g_try_malloc0(align_offset(l1_size2, 512));
>>> I wanted to propose using qemu_try_blockalign(), but since it'd require
>>> a memset() afterwards, it gets rather ugly.
>> Not after this recent patch:
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2014-10/msg02499.html
> We can switch to qemu_try_blockalign0() in a follow-up patch.
>
> But this is far from being a fast path, so there is very little to gain
> anyway.

We don't need the 0 variant anyway. In v5, the one I applied, it's just 
qemu_try_blockalign(). The size does not have to be aligned to 
BDRV_SECTOR_SIZE, since any access index is below l1_size and only 
bdrv_pread() and bdrv_pwrite() are used, so we can first omit the 
align_offset(). Second, all the rest (0 .. l1_size - 1) is overwritten 
either by memcpy() or bdrv_pread(). Therefore, no 0-ing required and 
qemu_try_blockalign() was fine.

There are things we can do in follow-up patches to optimize v5 (getting 
read of the memcpy()s), but as Kevin said (and myself before that in 
reply to v5), this function is not performance-critical, so there's no 
real point in doing so (other than "Clearly unoptimized code makes my 
fingers twitch").

Max