One criticism I have is that it does not seem as if you've coded in a way that promotes straight-forward review of this refactoring. Diff algorithms are not super-human, so extraneous changes or changes that are poorly placed can make a straight-forward review quite difficult for a revision of this size.
For example, it seems you have capriciously relocated zil_(lwb_)vdev_compare(). A second, much more consequential example is that you have refactored zil_commit_writer() but shifted all the code that has become zil_process_commit_list() above its former location in zil_commit_writer(), but then as well you have inserted your new logic of zil_prune_commit_list() and zil_commit_writer_stall() above zil_commit_writer() as well. The only reason I can think you chose to do this is because you have an aversion to forward declarations. In my opinion, you should have refactored zil_commit_writer() by extracting zil_process_commit_list() as a function immediately after zil_commit_writer() and added the necessary forward declaration rather than relocating all that code. I think as well that all your new functions [zil_prune_commit_list(), zil_commit_writer_stall(), zil_commit_waiter_timeout(), zil_commit_waiter(), etc.] should be placed further to the tail of the file — adding once again any forward declarations as necessary — in acknowledgement of the limitations of diff algorithms and in kindness to code reviewers. (I will say that I only determined the gist of your review after shifting functions around so that I could comprehend your changes when viewing the new diff.) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/447#issuecomment-326316331 ------------------------------------------ openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Tb8f4a0d0bed9b5eb-M646763eed86b1682f6634e30 Powered by Topicbox: https://topicbox.com
