On Fri, Mar 24, 2017 at 12:40 AM, Kevin Wolf <kw...@redhat.com> wrote: > 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 <ashijeetacha...@gmail.com> > > 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.
Hmm, sorry about that. > > 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. Sure, I understand your point completely. I wasn't able to segregate things too, but now with the list of things you mentioned above, it certainly helps making things clearer. I will post a v2 as soon as I can, probably you will receive it first thing Monday. One thing I wanna ask is that were you able to take a look at my reply to the cover letter earlier. Will the test results using 'qemi-io' I posted there work? Ashijeet
