On 2019-06-03 12:19 p.m., Bart Van Assche wrote:
On 5/24/19 11:47 AM, Douglas Gilbert wrote:
This patchset is big and can be regarded as a driver rewrite. The
number of lines increases from around 3000 to over 6000.
Every SCSI reviewer I know is too busy to review a patch series that involves so
many changes. You may have to split this series into a number of smaller series
if you want to encourage reviewers to have a look.
Cutting a patchset that touches around 1500 lines of a 3000 line
driver, then adds new functionality amounting to an extra 3000
lines of code (and comments), according to the "one change per
patch" rule would result in a patchset with hundreds of patches.
Further if they are to be bisectable then they must not only
compile and build, but run properly. Of course that is impossible
for new functionality as there is little to test the new
functionality against. Then there is the case of a driver maintainer
who wants to clean up code that has "rusted" over time, including
being weakened by hundreds of well-meaning but silly janitor type
patches. Should a maintainer be discouraged from doing that?
Looking around at current patchsets, LWN keeps a helpful list at
the bottom of this page (from last Thursday):
https://lwn.net/Articles/789234/
[non-subscribers need to wait until next Thursday to view that].
I looked at the "Device Drivers" section. Most patchsets there
had between 10 and 20 patches, one had 33. One, the SIW Infiniband
driver, is over 10,000 lines long, and contains 'only' 12 patches.
Reviewers are obviously a scarce resource, but making their live's
easier shouldn't be a goal in itself.
Further, the utility of bottom up reviews by humans reduces with
time as the automatic tools to do that get better. Of the responses
to my "version 1" patchset 10 days ago, I would regard only one of
your responses "useful", and all but one of the kbuild robot's
responses useful. By "useful" I mean that they lead to better code.
If new functionality is being proposed, surely it is better to
check that it is documented and that test code exists. Then the
design and high level details of the implementation should be
assessed. And if the code doesn't involve special hardware,
shouldn't the test code be run as part of the review process?
Code reviews have their place, but over the last 10 years, bugs in
the sg and scsi_debug drivers have been found by _using_ those
drivers. Also with those drivers I and others have found bugs in
the block layer and the SCSI mid-level. To date I have had no
feedback about design document describing this patchset:
http://sg.danny.cz/sg/sg_v40.html
even though I refer to sections of it in about half of the patches
in this patchset. Plus I have written extensive test code and made
it available; again no feedback on that.
In the article: "A ring buffer for epoll":
https://lwn.net/Articles/789603/ [May 30, 2019]
Jonathan Corbet discusses these issues in the final section
titled: "Some closing grumbles".
Doug Gilbert
