On 2018/8/20 下午10:17, David Sterba wrote: > On Wed, Jul 25, 2018 at 04:12:42PM +0800, Qu Wenruo wrote: >> extent-tree.c::find_free_extent() could be one of the most >> ill-structured function, it has at least 4 non-exit tags and jumps >> between them. >> >> To make it a little easier to understand, extract that big functions >> into 4 parts: >> >> 1) find_free_extent() >> Do the block group iterations, still the main function. >> >> 2) find_free_extent_clustered() >> Do the clustered allocation and cluster refill if needed. >> >> 3) find_free_extent_unclustered() >> Do the unclustered allocation and free space cache update if needed. >> >> 4) find_free_extent_update_loop() >> Do the corner-case black magic handling, like allocating new chunk >> and update loop condition. >> >> Also, for internal communication, created a new internal structure >> find_free_extent_ctrl, to make the parameter list shorter for function >> 2~4. >> >> Please note that this patch will try to avoid any functional/logical >> modification during the refactor, so still a lot of bad practice like in >> find_free_extent() we still use search/have_block_group/loop tags to >> implement a custom loop. >> >> But at least, this should provide the basis for later cleanup. > > I'd prefer to do the cleanup in smaller pieces, this patch is too big > for review. > >> --- >> I know such large patch is not a good practice, and there is choice to >> split the patch into 4 patches (one patch for each helper function), but >> that will make later rebase more error prone since new function will never >> conflict with newer changes. > > Small patches can be refreshed and updated easily, also each refresh > reviewed that the change is still correct. > > The big blob patches are the worst thing one can find in git history, > hunting a clustered allocation bug and ending up in a cleanup patch. No > amount of written assurance or "it worked on my machine" can change > that.
Well, if error really happens, it will still ends up in a cleanup patch. As even with patch split into small ones, it will still be the final patch to touch the real function. > > Finding the right way to split the patch and isolate nice and reviewable > changes requires more thinking which is not a bad thing on itself. > > I know everybody hates that because it's more work for them, but > consider the work on both sides This makes sense, but it still doesn't solve the problem that changes in new helper functions won't cause any conflict, thus rebasing or applying older version patch could still leads to frustrating bugs. I'll change to small patches, but I really hope to get some clue to solve above problems. Thanks, Qu
signature.asc
Description: OpenPGP digital signature
