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

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

[James Bottomley]

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. A The question is should we (or at least should we
adhere to some minimal standards). A The standard we've been trying to
hold to in SCSI is one review per accepted non-trivial patch. A 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. A 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.

2. Handling Internal Conflict

My observation here is that actually most conflict is generated by the
review process (I know, if we increase reviews as I propose in 1. we'll
increase conflict on the lists on the basis of this observation), so
I've been thinking about ways to de-escalate it. A The principle issue
is that a review which doesn't just say the patch is fine (or fine
except for nitpicks) can be taken as criticism and criticism is often
processed personally. A The way you phrase criticism can have a great
bearing on the amount of personal insult taken by the other party.
A Corny as it sounds, the 0day bot response "Hi Z,A I love your patch!
Perhaps something to improve:" is specifically targetted at this
problem and seems actually to work. A I think we could all benefit from
discussing how to give and receive criticism in the form of patch
reviews responsibly, especially as not everyone's native language in
English and certain common linguistic phrasings in other languages can
come off as rude when directly translated to English (Russian springs
immediately to mind for some reason here). A Also Note, I think fixing
the review problem would solve most of the issues, so I'm not proposing
anything more formal like the code of conflict stuff in the main
kernel.

We could lump both of these under a single "Community Discussion" topic
if the organizers prefer ... especially if anyone has any other
community type issues they'd like to bring up.

James

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Mike Kravetz]

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/

-- 
Mike Kravetz

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Bart Van Assche]

On Wed, 2018-01-24 at 11:05 -0800, James Bottomley wrote:
> 2. Handling Internal Conflict
> 
> My observation here is that actually most conflict is generated by the
> review process (I know, if we increase reviews as I propose in 1. we'll
> increase conflict on the lists on the basis of this observation), so
> I've been thinking about ways to de-escalate it.  The principle issue
> is that a review which doesn't just say the patch is fine (or fine
> except for nitpicks) can be taken as criticism and criticism is often
> processed personally.  The way you phrase criticism can have a great
> bearing on the amount of personal insult taken by the other party.
>  Corny as it sounds, the 0day bot response "Hi Z, I love your patch!
> Perhaps something to improve:" is specifically targetted at this
> problem and seems actually to work.  I think we could all benefit from
> discussing how to give and receive criticism in the form of patch
> reviews responsibly, especially as not everyone's native language in
> English and certain common linguistic phrasings in other languages can
> come off as rude when directly translated to English (Russian springs
> immediately to mind for some reason here).  Also Note, I think fixing
> the review problem would solve most of the issues, so I'm not proposing
> anything more formal like the code of conflict stuff in the main
> kernel.
> 
> We could lump both of these under a single "Community Discussion" topic
> if the organizers prefer ... especially if anyone has any other
> community type issues they'd like to bring up.

Hello James,

How about discussing the following two additional topics during the same or
another session:
* We all want a concensus about the code and the algorithms in the Linux
  kernel. However, some contributors are not interested in trying to strive
  towards a concensus. If some contributors e.g. receive a request to rework
  their patches, if they don't like that request and if the reviewer is
  working for the same employer sometimes they try to use the corporate
  hierarchy to make the reviewer shut up. I think this is behavior that works
  against the long-term interests of the Linux kernel.
* Some other contributors are not interested in achieving a consensus and do
  not attempt to address reviewer feedback but instead keep arguing or do what
  they can to insult the reviewer.

Thanks,

Bart.

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

[James Bottomley]

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.A A The question is should we (or at
> > least should we adhere to some minimal standards).A A The standard
> > we've been trying to hold to in SCSI is one review per accepted
> > non-trivial patch.A A 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.A A 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. A Michal did say that XFS didn't have the
problem, however there not being XFS people in the room, discussion
stopped there. A 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.

James

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[James Bottomley]

On Wed, 2018-01-24 at 19:26 +0000, Bart Van Assche wrote:
> On Wed, 2018-01-24 at 11:05 -0800, James Bottomley wrote:
> > 
> > 2. Handling Internal Conflict
> > 
> > My observation here is that actually most conflict is generated by
> > the review process (I know, if we increase reviews as I propose in
> > 1. we'll increase conflict on the lists on the basis of this
> > observation), so I've been thinking about ways to de-escalate
> > it.A A The principle issue is that a review which doesn't just say
> > the patch is fine (or fine except for nitpicks) can be taken as
> > criticism and criticism is often processed personally.A A The way you
> > phrase criticism can have a great bearing on the amount of personal
> > insult taken by the other party. A Corny as it sounds, the 0day bot
> > response "Hi Z, I love your patch! Perhaps something to improve:"
> > is specifically targetted at this problem and seems actually to
> > work.A A I think we could all benefit from discussing how to give and
> > receive criticism in the form of patch reviews responsibly,
> > especially as not everyone's native language in English and certain
> > common linguistic phrasings in other languages can come off as rude
> > when directly translated to English (Russian springs immediately to
> > mind for some reason here).A A Also Note, I think fixing the review
> > problem would solve most of the issues, so I'm not proposing
> > anything more formal like the code of conflict stuff in the main
> > kernel.
> > 
> > We could lump both of these under a single "Community Discussion"
> > topic if the organizers prefer ... especially if anyone has any
> > other community type issues they'd like to bring up.
> 
> Hello James,
> 
> How about discussing the following two additional topics during the
> same or another session:
> * We all want a concensus about the code and the algorithms in the
> Linux
> A  kernel. However, some contributors are not interested in trying to
> strive
> A  towards a concensus. If some contributors e.g. receive a request to
> rework
> A  their patches, if they don't like that request and if the reviewer
> is
> A  working for the same employer sometimes they try to use the
> corporate
> A  hierarchy to make the reviewer shut up. I think this is behavior
> that works
> A  against the long-term interests of the Linux kernel.

Trying to intimidate people into doing what you want is a well known
social tool. A The particular problem here is that it's the corporate
power structure that's used to effect the intimidation. A That's pretty
much outside of our control (unless we work for the same company), so
there's not much we can do to stop it except say you shouldn't try to
intimidate reviewers. A I suppose one practical policy could be
demanding reviews from independent (meaning outside the corporate power
structure) people.

> * Some other contributors are not interested in achieving a consensus
> and do
> A  not attempt to address reviewer feedback but instead keep arguing
> or do what
> A  they can to insult the reviewer.

Well, I think this fits neatly in the giving and receiving and
receiving criticism bucket. The first rule of interacting with others
is "never attribute to malice something which could possibly be
accidental". A In the end it's up to the maintainer to arbitrate based
on their opinion of the merits of the review.

James

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Darrick J. Wong]

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
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Jan Kara]

On Wed 24-01-18 19:26:20, Bart Van Assche wrote:
> On Wed, 2018-01-24 at 11:05 -0800, James Bottomley wrote:
> > 2. Handling Internal Conflict
> > 
> > My observation here is that actually most conflict is generated by the
> > review process (I know, if we increase reviews as I propose in 1. we'll
> > increase conflict on the lists on the basis of this observation), so
> > I've been thinking about ways to de-escalate it.  The principle issue
> > is that a review which doesn't just say the patch is fine (or fine
> > except for nitpicks) can be taken as criticism and criticism is often
> > processed personally.  The way you phrase criticism can have a great
> > bearing on the amount of personal insult taken by the other party.
> >  Corny as it sounds, the 0day bot response "Hi Z, I love your patch!
> > Perhaps something to improve:" is specifically targetted at this
> > problem and seems actually to work.  I think we could all benefit from
> > discussing how to give and receive criticism in the form of patch
> > reviews responsibly, especially as not everyone's native language in
> > English and certain common linguistic phrasings in other languages can
> > come off as rude when directly translated to English (Russian springs
> > immediately to mind for some reason here).  Also Note, I think fixing
> > the review problem would solve most of the issues, so I'm not proposing
> > anything more formal like the code of conflict stuff in the main
> > kernel.
> > 
> > We could lump both of these under a single "Community Discussion" topic
> > if the organizers prefer ... especially if anyone has any other
> > community type issues they'd like to bring up.
> 
> Hello James,
> 
> How about discussing the following two additional topics during the same or
> another session:
> * We all want a concensus about the code and the algorithms in the Linux
>   kernel. However, some contributors are not interested in trying to strive
>   towards a concensus. If some contributors e.g. receive a request to rework
>   their patches, if they don't like that request and if the reviewer is
>   working for the same employer sometimes they try to use the corporate
>   hierarchy to make the reviewer shut up. I think this is behavior that works
>   against the long-term interests of the Linux kernel.

Well, it probably is but using corporate hierarchy to make reviewer shut
up is a problem of that corporation. So if particular examples are brought
to LF TAB attention maybe they could apply some pressure but that's all. In
the end it is a company internal thing. Change your boss / employer if they
do stuff like this to you. E.g. I personally never experienced anything
like this.

> * Some other contributors are not interested in achieving a consensus and do
>   not attempt to address reviewer feedback but instead keep arguing or do what
>   they can to insult the reviewer.

Yes, then I think it is up to the maintainer to weight in with his
opinion...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Jan Kara]

On Wed 24-01-18 11:05:44, 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, stuff like fs/reiserfs, fs/udf, fs/isofs, or fs/quota are also parts
of filesystem space but good luck with finding reviewers for those. 99% of
patches I sent in last 10 years were just met with silence (usually there's
0-1 developer interested in that code) so I just push them to have the bug
fixed... I don't feel that as a big problem since the code is reasonably
simple, can be tested, change rate is very low. I just wanted to give that
as an example that above rule does not work for everybody.

For larger filesystems I agree 'at least one reviewer' is a good rule. XFS
is known for this, I believe btrfs pretty much enforces it as well, Ted is
not enforcing this rule for ext4 AFAIK and often it is up to him to review
patches but larger / more complex stuff generally does get reviewed. So
IMO ext4 could use some improvement but I'll leave up to Ted to decide
what's better for ext4.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Goldwyn Rodrigues]

On 01/24/2018 01:05 PM, 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. A The question is should we (or at least should we
> adhere to some minimal standards). A The standard we've been trying to
> hold to in SCSI is one review per accepted non-trivial patch. A 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. A 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.
> 
> 2. Handling Internal Conflict
> 
> My observation here is that actually most conflict is generated by the
> review process (I know, if we increase reviews as I propose in 1. we'll
> increase conflict on the lists on the basis of this observation), so
> I've been thinking about ways to de-escalate it. A The principle issue
> is that a review which doesn't just say the patch is fine (or fine
> except for nitpicks) can be taken as criticism and criticism is often
> processed personally. A The way you phrase criticism can have a great
> bearing on the amount of personal insult taken by the other party.
> A Corny as it sounds, the 0day bot response "Hi Z,A I love your patch!
> Perhaps something to improve:" is specifically targetted at this
> problem and seems actually to work. A I think we could all benefit from
> discussing how to give and receive criticism in the form of patch
> reviews responsibly, especially as not everyone's native language in
> English and certain common linguistic phrasings in other languages can
> come off as rude when directly translated to English (Russian springs
> immediately to mind for some reason here). A Also Note, I think fixing
> the review problem would solve most of the issues, so I'm not proposing
> anything more formal like the code of conflict stuff in the main
> kernel.

The problem I have faced is that reviewers usually (not generalizing)
review by saying "This is bad",  "This is not acceptable" or "This will
not work" and leaving it there. Instead a reviewer should be focused on
providing the alternates and/or reasons like "This is bad because ..."
or "This will not work. Instead you should be doing ..." In short
towards constructive criticism, providing what reviewers think how it
should be done or resolved to move forward. While providing too much
information may be called spoon-feeding, there is always a balance..


> 
> We could lump both of these under a single "Community Discussion" topic
> if the organizers prefer ... especially if anyone has any other
> community type issues they'd like to bring up.
> 
> James
> 
> 

-- 
Goldwyn

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Eric Sandeen]

On 1/24/18 5:43 PM, Darrick J. Wong wrote:
> 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.A A The question is should we (or at
>>>> least should we adhere to some minimal standards).A A The standard
>>>> we've been trying to hold to in SCSI is one review per accepted
>>>> non-trivial patch.A A 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.A A 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. A 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.

<everything Darrick says sounds pretty much spot on and more eloquent
than I'm likely to provide, but here goes... >

Mandating reviews certainly can slow down patch acceptance, though I'd
expect that any good maintainer will be doing at least cursory review
before commit; when the maintainer writes patches themselves, they /are/
then at the mercy of others for an RVB: tag.  That hasn't in general
been a huge problem for us, though things do sometimes require a bit of
poking and prodding.  But I think that's a feature not a bug.  Obtaining
at least one meaningful review means that someone else has at least
some familiarity with the new code.

In the XFS community, in reality we have only about 4 kernelspace
reviewers, with a /very/ long tail of onesey-twosies; since v4.12:

<lots of 1's>
      2     Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
      2     Reviewed-by: Eric Sandeen <sandeen@redhat.com>
      3     Reviewed-by: Amir Goldstein <amir73il@gmail.com>
      4     Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>
      6     Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
      6     Reviewed-by: Jan Kara <jack@suse.cz>
     10     Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
     60     Reviewed-by: Christoph Hellwig <hch@lst.de>
    104     Reviewed-by: Dave Chinner <dchinner@redhat.com>
    109     Reviewed-by: Brian Foster <bfoster@redhat.com>
    208     Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

In userspace things look a little different in the same time period:

      1     Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
      1     Reviewed-by: Bill O'Donnell <billodo@redhat.com>
      1     Reviewed-by: Eric Sandeen <sandeen@sandeen.net>
      3     Reviewed-by: Dave Chinner <dchinner@redhat.com>
     11     Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
     12     Reviewed-by: Christoph Hellwig <hch@lst.de>
     25     Reviewed-by: Brian Foster <bfoster@redhat.com>
     37     Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
     44     Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Unsurprisingly(?) the maintainers still bear a lot of the review burden, but
the same workhorse rock stars are clearly present.  In reality it's something
we need to work on, to try to get more people participating in meaningful review,
both to speed up the cycle and to grow community knowledge.

Another thing that Darrick and I have bounced around a little bit is
the adequacy of email for significant review of large feature patchsets.
On the one hand, we'd like centralized review with archives, because
that's useful to future code archaeologists.  On the other hand, I can't
help but think that something like Github's ability to mark up 
comments line by line would have some advantages, particularly for
brand new code.



As for the question of conflict, I'm not sure what to say...  The XFS
development team has been lucky(?) to have been living in relative peace
and harmony for the past few years.  Speaking for myself, I try to
be aware of getting too nitpicky or enforcing preferences vs. requirements,
and I make an effort to reach out and check in with patch submitters
to keep things calibrated.  Having the dedicated #xfs channel helps here,
I think, for higher bandwidth communication about issues when needed.
I have no doubt that I've annoyed Darrick or Dave or Brian from time to
time (Dave usually makes this very obvious ;)) but we try to talk to each
other like humans and it seems to work out ok in the long run.

An expectation of 100% review surely helps here as well; if only 20%
of patches get reviewed, the reviews may stick out like criticism.  If
the expectation is that everything is meaningfully reviewed, nobody is
surprised by feedback when it comes.

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

As would I (if I'm invited) :)  As xfsprogs maintainer I probably have
some useful insights to our submit/review/commit cycle as well.

Thanks,
-Eric

> --D
> 
>> James
>>
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>