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. 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. <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@xxxxxxxxxx> 2 Reviewed-by: Eric Sandeen <sandeen@xxxxxxxxxx> 3 Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx> 4 Reviewed-by: Ross Zwisler <ross.zwisler@xxxxxxxxxxxxxxx> 6 Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> 6 Reviewed-by: Jan Kara <jack@xxxxxxx> 10 Reviewed-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx> 60 Reviewed-by: Christoph Hellwig <hch@xxxxxx> 104 Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx> 109 Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx> 208 Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx> In userspace things look a little different in the same time period: 1 Reviewed-by: Allison Henderson <allison.henderson@xxxxxxxxxx> 1 Reviewed-by: Bill O'Donnell <billodo@xxxxxxxxxx> 1 Reviewed-by: Eric Sandeen <sandeen@xxxxxxxxxxx> 3 Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx> 11 Reviewed-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx> 12 Reviewed-by: Christoph Hellwig <hch@xxxxxx> 25 Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx> 37 Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx> 44 Reviewed-by: Eric Sandeen <sandeen@xxxxxxxxxx> 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@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
