On Sun, May 19, 2024 at 07:39:38PM -0700, Kees Cook wrote: > On Sun, May 19, 2024 at 12:14:34PM -0400, Kent Overstreet wrote: > > [...] > > bcachefs changes for 6.10-rc1 > > [...] > > bcachefs: bch2_btree_path_to_text() > > Hi Kent, > > I've asked after this before[1], but there continues to be a lot of > bcachefs development going on that is only visible when it appears in > -next or during the merge window. I cannot find the above commit on > any mailing list on lore.kernel.org[2]. The rules for -next are clear: > patches _must_ appear on a list _somewhere_ before they land in -next > (much less Linus's tree). The point is to get additional reviews, and > to serve as a focal point for any discussions that pop up over a given > change. Please adjust the bcachefs development workflow to address this. Over the course of my career, I've found the kind of workflow and level of review you seem to asking for to be at best not useful, and at worst harmful to productive functioning of a team - to my ability to teach people and get them happy and productive. The reality has just been that no one has ever been able to keep up with the rate at which I work and write code [0], and attempting to do code review of every patch means no one else gets anything done and we get sidetracked on irrelevant details. When I do post my patches to the list, the majority of what I get ends up being spelling fixes or at best the kinds of bugs that shake out quickly in real testing. In short, I've had to learn to write code without anyone looking over my shoulder, and I take pride in debugging my own code and not saddling other people with that. So instead, I prioritize: - real discussion over the work being done, which does tend to happen person to person or in meetings (getting more of that on the list would not be a bad idea; I do need to be spending more time writing documentation and design docs, especially at this point). - good effective test infrastructure - heavy and thoughtful use of assertions; there's a real art to effective use of assertions, where you think about what the correctness proof would look like and write assertions for the invariants (and assertions should be on _state_, not _logic_) I also do (try to) post patches to the list that are doing something interesting and worth discussion; the vast majority this cycle has been boring syzbot crap... IOW, I'm not trying to _flout_ process here, even if I do things somewhat differently; I've got quite a few people I'm actively teaching and bringing in and that's where most of my energy is going. And we do spend a lot of time going over code together, the meetings I run (especially with the younger guys) are very much code-and-workflow focused. You'll also find I'm quite responsive, on IRC and the list, should you have anything you wish to complain or yell about. (btw, there's also been some discussions in fs land about other people changing their workflows to something that looks more like mine; get the important stuff on the list, make the list less spammy, work with each other on a quicker timeline than that. They're not quite doing what I'm doing, but I do think there's room for the /way/ we do code review and the expectations around it to evolve a bit. Personally, I mostly just want code to be readable). I personally approach code review as being primarily about mentorship... I don't want people to have the expectation that I'm going to pore over their code and find their bugs; I'm not going to do that. I expect people to be adults, and take as much time as they need to to get it right; if there's something they're not sure about, I expect _them_ to bring it up. I personally feel that this mindset teaches more responsibility and the "right" kind of defensiveness that it takes to write reliable code. > Anyway, in reference to the above commit, please scrub bcachefs of its > %px format string uses. Neither it nor %p should be used[3][4] in new > code. (Which is, again, something checkpatch.pl will warn about.) So that particular code is used in debugfs (root only) or in dumps when we're panicing; the reason it's %px and not hashed is because I not uncommonly do things like grab addresses from the introspection and then use kgdb for further inspection. Does that alleviate your concern, if it's only exposed that way? If not, perhaps some sort of config option is appropriate. [0]: except Linus, who quite frequently leaves my jaw hanging with how quickly he can read code and spot real issues.
