Re: [Qemu-block] [PATCH 1/2] vmdk: Optimize I/O by allocating multiple clusters

[Kevin Wolf]

Am 11.03.2017 um 12:54 hat Ashijeet Acharya geschrieben:
> The vmdk driver in block/vmdk.c used to allocate cluster by cluster
> which slowed down its I/O performance.
> 
> Make vmdk driver allocate multiple clusters at once to reduce the
> overhead costs of multiple separate cluster allocation calls. The number
> of clusters allocated at once depends on the L2 table boundaries. Also
> the first and the last clusters are allocated separately as we may need
> to perform COW for them
> 
> Signed-off-by: Ashijeet Acharya <address@hidden>

Ashijeet, this is pretty hard to review because you're doing too many
things in a single patch. The rule is to do only one logical change in
each patch whenever it's possible. In particular, pure code motion (or
renames) need to be separated from code modification because when you
move code and modify it at the same time, it is very hard to see the
actual difference.

Do you think you could split this into multiple patches, like this:

1. Move vmdk_find_offset_in_cluster() to the top
2. Rename get_whole_cluster() to do_alloc_cluster_offset()
   (To be honest, both names aren't great. Something like qcow2's
   perform_cow() would describe better what's going on there.)
3. Factor out some code from get_cluster_offset() into handle_alloc()
4. Rename get_cluster_offset() into vmdk_get_cluster_offset()
...
n. Handle multiple clusters at once

Obviously, I haven't taken the time to fully read and understand all of
your patch, so don't take this too literally, but you see what kind of
granularity the individual patches should be. If you do it like this,
each change becomes really simple and can be reviewed and discussed on
its own. And if we later find a bug in VMDK, git bisect can point out
exactly which step introduced the problem rather than pointing at a
single big commit that is hard to understand.

Good splitting of patches is one of the tricks to get quicker reviews.
Admittedly, it is often not easy, but it is worth spending some thought
on how to make a series really easy to read for others.

Kevin