Re: [LSF/MM TOPIC] Patch Submission process and Handling Internal Conflict

Darrick J. Wong
2018-01-24T23:43:47Z

On Wed, Jan 24, 2018 at 01:36:00PM -0800, James Bottomley wrote:
> On Wed, 2018-01-24 at 11:20 -0800, Mike Kravetz wrote:
> > On 01/24/2018 11:05 AM, James Bottomley wrote:
> > >
> > > I've got two community style topics, which should probably be
> > > discussed
> > > in the plenary
> > >
> > > 1. Patch Submission Process
> > >
> > > Today we don't have a uniform patch submission process across
> > > Storage, Filesystems and MM.ï¿½ï¿½The question is should we (or at
> > > least should we adhere to some minimal standards).ï¿½ï¿½The standard
> > > we've been trying to hold to in SCSI is one review per accepted
> > > non-trivial patch.ï¿½ï¿½For us, it's useful because it encourages
> > > driver writers to review each other's patches rather than just
> > > posting and then complaining their patch hasn't gone in.ï¿½ï¿½I can
> > > certainly think of a couple of bugs I've had to chase in mm where
> > > the underlying patches would have benefited from review, so I'd
> > > like to discuss making the one review per non-trival patch our base
> > > minimum standard across the whole of LSF/MM; it would certainly
> > > serve to improve our Reviewed-by statistics.
> >
> > Well, the mm track at least has some discussion of this last year:
> > https://lwn.net/Articles/718212/
>
> The pushback in your session was mandating reviews would mean slowing
> patch acceptance or possibly causing the dropping of patches that
> couldn't get reviewed. ï¿½Michal did say that XFS didn't have the
> problem, however there not being XFS people in the room, discussion
> stopped there.

I actually /was/ lurking in the session, but a year later I have more
thoughts:

Now that I've been maintainer for more than a year I feel more confident
in actually talking about our review processes, though I can only speak
about my own experiences and hope the other xfs developers chime in if
they choose.

In xfs we are fortunate enough that most of the codebase is at least
one software layer up from the raw hardware, which means that anybody
can build xfs with all kconfig options enabled and use it to try to
create all possible metadata structures, which means that the ability to
review a given patch and try it out isn't restricted to the subset of
people with a particular hardware device. This means that there aren't
any patches that cannot be reviewed, which is not something I'm so sure
of for the mm layer.

Requiring review on the vast majority of non-maintainer patches that
goes into xfs (and xfsprogs) doesn't has the effect of increasing the
time to upstream acceptance, since the fact that it was committed at all
implies that the maintainer probably looked at it.

The dangerous part of course is when the maintainer commits non-trivial
code without a review -- did they look at it, or just commit whatever
made the symptoms go away? So that's argument #1 for creating a group
norm that yes, everyone should be involved in review on a semi regular
basis. Certainly if they're also *submitting* patches.

Argument #2 is that encouraging review of everything most likely reduces
the overall time it takes for a feature to mature because that means
that at least one of the regular participants in the group have taken
the time to read and understand how the patches mesh with the existing
systems and will ask questions when they see ill-fitting pieces. It
definitely reduces code churn from not having to walk back bad patches
and rushed microcode updates. That said, I've no data to back up this
assertion, merely my observations of the past decade.

My third argument is that the most time consuming part of
maintainership isn't gluing patches onto a git tree and running tests,
it's reviewing the patches. It's a big help to know that other people
who are more familiar with various subcomponents of xfs review patches
regularly, so I don't feel as much pressure to know all things at all
times, and I worry less about blind spots because we work as a group of
people who don't see every xfs component in exactly the same way.

(Granted it helps that Dave Chinner is a fountain of historical context
indexing...)

That said, I also get reeeeally itchy to commit my own patches at times,
especially things that look like trivial one-liners. However, I find
that nothing in xfs is simple, and moreover the reviewers are
knowledgeable enough that even trivial patches can get reviewed quickly.

For bigger things like new features or large refactorings, there's a
strong need for updating documentation like the disk format
specification, developing a test plan, and integrating new tests into
xfstests. That's where review is most useful, because it is the
submitter's opportunity to increase everyone's knowledge levels. It is
also the reviewers' chance to anticipate design problems when it is
easy/cheap to fix them, and for everyone to build confidence about the
code that's going in.

The challenge for everyone, then, is to get together to decide on a
reasonable target for the amount and the levels on which review happen;
and to figure out how to make that reviewing a group norm to avoid
regression to 'building hacks on other hacks'. My uneducated guess as
to a good starting point is to start by trying to build a rough
consensus about how the memory manager actually works, after which you
can then review the high level design of these large patchsets that come
in.

Unfortunately, I'm not sufficiently familiar with the mm community to
know if I've just asked for the moon. That's where LSFMM comes in. :)

>ï¿½Having this as a plenary would allow people outside mm
> to describe their experiences and for us to look at process based
> solutions using our shared experience.

I'd show up, so long as this wasn't scheduled against something else.
(IOWs, yes please.)

--D

> James