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

[Max Reitz]

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