Re: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag

[Mathieu Desnoyers]

----- Original Message -----
> From: "Dave Chinner" <david@xxxxxxxxxxxxx>
> To: "Steven Rostedt" <rostedt@xxxxxxxxxxx>
> Cc: josh@xxxxxxxxxxxxxxxx, "Joe Perches" <joe@xxxxxxxxxxx>, paulmck@xxxxxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx,
> mingo@xxxxxxxxxx, laijs@xxxxxxxxxxxxxx, dipankar@xxxxxxxxxx, akpm@xxxxxxxxxxxxxxxxxxxx, "mathieu desnoyers"
> <mathieu.desnoyers@xxxxxxxxxxxx>, niv@xxxxxxxxxx, tglx@xxxxxxxxxxxxx, peterz@xxxxxxxxxxxxx, dhowells@xxxxxxxxxx,
> edumazet@xxxxxxxxxx, dvhart@xxxxxxxxxxxxxxx, fweisbec@xxxxxxxxx, oleg@xxxxxxxxxx, sbw@xxxxxxx
> Sent: Tuesday, June 3, 2014 3:16:54 AM
> Subject: Re: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
> 
> On Mon, Jun 02, 2014 at 09:30:45PM -0400, Steven Rostedt wrote:
> > On Tue, 3 Jun 2014 11:11:25 +1000
> > Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> > 
> > 
> > > You've ignored the (c).(2) "free of known issues" criteria there.
> > > You cannot say a patch is free of issues if you haven't applied,
> > > compiled and tested it.
> > > 
> > > > We should not, for instance, prevent someone from providing a
> > > > Reviewed-by (as opposed to an Acked-by) for a driver whose hardware few
> > > > people actually have.  There's significant value in code review even
> > > > without the ability to test.
> > > 
> > > I don't disagree with you that there's value in code review, but
> > > that's not the only part of what "reviewed-by" means.
> > > 
> > > You can test that the code is free of known issues without reviewing
> > > it (i.e. tested-by). You can read the code and note that you can't
> > > see any technical issues without testing it (Acked-by).
> > 
> > Unless you run every test imaginable on all existing hardware, you are
> > not stating that it is free of known issues. I say your logic is flawed
> > right there.
> 
> If you take it to an extremes. Think about what you can test in 15
> minutes. Or for larger patchsets, how long it takes you to read the
> patchset?
> 
> IMO, every reviewer has their own developement environment and they
> should be at least testing that the change they are reviewing
> doesn't cause problems in that environment, just like they do for
> their own code before they post it for review.
> 
> Code being reviewed should pass the same testing bar that the
> developer uses for code they write and send for review. A maintainer
> quickly learns whose test environments are up to scratch or not. :/
> 
> > > But you can't say that is it both free of techical and known
> > > issues without both reading the code and testing it (Reviewed-by).
> > 
> > I disagree. Testing only tests what you run. It's useless otherwise.
> > Most code I review, and find bugs for in that review, will not be
> > caught by tests unless you ran it on a 1024 CPU box for a week.
> > 
> > I value looking hard at code much more than booting it and running some
> > insignificant micro test.
> 
> Running "insignficant micro tests" is exactly that - insignificant -
> and it's not a robust code review if that is all that is done.
> Runing *regression tests*, OTOH....
> 
> I know from experience that a "quick" 15 minute run on xfstests on a
> ramdisk will catch 99% of typical problems a filesystem patch might
> introduce. Code coverage reporting (done recently by RH QE
> engineers) tells me that this covers between 50-70% of the
> filesystem, VFS and MM subsystems (numbers vary with fs being
> tested), and so that's a pretty good, fast smoke test that I can run
> on any patch or patchset that is sent for review.
> 
> Any significant patch set requires longer to read and digest than a
> full xfstests run across all my test machines (about 80 minutes for
> a single mkfs/mount option configuration). So by the time I've
> actually read the code and commented on it, it's been through a full
> test cycle and it's pretty clear if there are problems or not..
> 
> And so when I say "reviewed-by" I'm pretty certain that there aren't
> any known issues.  Sure, it's not going to catch the "ran it on a
> 1024 CPU box for a week" type of bug, but that's the repsonsibility
> of the bug reporter to give you a "tested-by" for that specific
> problem.
> 
> And really, that points out *exactly* the how "reviewed-by" is a far
> more onerous than "tested-by". Tested-by only means the patch fixes
> the *specific problem* that was reported.  Reviewed-by means that,
> as far as the reviewer can determine, it doesn't cause regressions
> or other problems. It may or may not imply "tested-by" depending on
> the nature of the bug being fixed, but it certainly implies "no
> obvious regressions".  They are two very different meanings, and
> reviewed-by has a much, much wider scope than "tested-by".
> 
> > > And, yes, this is the definition we've been using for "reviewed-by"
> > > for XFS code since, well, years before the "reviewed-by" tag even
> > > existed...
> > 
> > Fine, just like all else. That is up to the maintainer to decide. You
> > may require people to run and test it as their review, but I require
> > that people understand the code I write and look for those flaws that
> > 99% of tests wont catch.
> > 
> > I run lots of specific tests on the code I write, I don't expect those
> > that review my code to do the same. In fact that's never what I even
> > ask for when I ask someone to review my code. Note, I do ask for
> > testers when I want people to test it, but those are not the same
> > people that review my code.
> 
> Very few subsystems have dedicated testers and hence rely on the
> test environments that the subsystem developers use every day to
> test their own code. IOWs, in most cases "tester" and the "reviewer"
> roles are performed by the same people.
> 
> > I find the reviewers of my code to be the worse testers. That's because
> > those that I ask to review my code know what it's suppose to do, and
> > those are the people that are not going to stumble over bugs. It's the
> > people that have no idea how your code works that will trigger the most
> > bugs in testing it. My best testers would be my worse reviewers.
> 
> "Reviewers are the worst testers".
> 
> Yet you accept the code they write as sufficiently well tested for
> merging? :/
> 
> > What do you require as a test anyway? I could boot your patches, but
> > since I don't have an XFS filesystem, I doubt that would be much use
> > for you.
> 
> I don't want you to review XFS code - you have no domain specific
> knowledge nor a test environment for XFS changes. IOWs, you meet
> none of the requirements to give a "reviewed-by" for an XFS change,
> and so to say you reviewed a change makes a mockery of the
> expectations outlines in the SubmittingPatches guidelines.
> 
> Similarly, I would expect any other maintainer to give me the same
> "go read the guidelines - you know better than that" line if I did
> the same thing for a patch that I clearly don't have a clue about or
> are able to test.
> 
> Reviewed-by only has value when it is backed by process and domain
> specific knowledge. If the person giving that tag doesn't have
> either of these, then it's worthless and they need to be taught what
> the correct thing to do is. Most people (even projects) don't learn
> proper software engineering processes until after they have been at
> the pointy end of a pile of crap at least once. :/

Hi Dave,

For various kernel subsystems, the situation appears to be like this:
there are few reviewers who have the technical ability to understand
the code. The reason why this email thread started is indeed the
"difficulty recruiting and retaining reviewers" (quoting Paul E.
McKenney).

On the other hand, testing can be automated, baby-sitted in a
continuous integration infrastructure. Code review cannot be automated
in that way.

You argue that anyone doing "review" should be running tests in order
to abide by the Reviewer's statement of oversight. Even if your
understanding of the Documentation/SubmittingPatches wording was
accurate, it sounds counter-productive to me, because it would keep
away people who have the technical knowledge to review code, but limited
time and hardware available to test it.

I also disagree with your interpretation that "free of known issues
which would argue against its inclusion" imply that testing needs to
be performed by the reviewer. It merely states that if unresolved issues
were brought to the knowledge of the reviewer, then the patch would not
be "free of known issues". It does not imply any active involvement in
testing, semantically speaking.

Thanks,

Mathieu

> 
> Cheers,
> 
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/