Message ID | zhtllemg2gcex7hwybjzoavzrsnrwheuxtswqyo3mn2dlhsxbx@dkfnr5zx3r2x (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [GIT,PULL] bcachefs updates fro 6.10-rc1 | expand |
The pull request you sent on Sun, 19 May 2024 12:14:34 -0400:
> https://evilpiepirate.org/git/bcachefs.git tags/bcachefs-2024-05-19
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/16dbfae867cdeb32f3d24cea81193793d5decc61
Thank you!
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. 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.) Thanks! -Kees [1] https://lore.kernel.org/all/202401101525.112E8234@keescook/ [2] https://lore.kernel.org/linux-bcachefs/?q=bch2_btree_path_to_text [3] https://docs.kernel.org/process/deprecated.html#p-format-specifier [4] https://lore.kernel.org/lkml/CA+55aFwQEd_d40g4mUCSsVRZzrFPUJt74vc6PPpb675hYNXcKw@mail.gmail.com/
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.
On Sun, 2024-05-19 at 23:52 -0400, Kent Overstreet wrote: > 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. I get that in your head you have a superior development methodology but if I look at the reasons you advance below: > > 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... you still don't say what problem not posting most patches solves? You imply it would slow you down, but getting git-send-email to post to a mailing list can actually be automated through a pre-push commit hook with no slowdown in the awesome rate at which you apply patches to your own tree. Linux kernel process exists because it's been found to work over time. That's not to say it can't be changed, but it usually requires at least some stab at a reason before that happens. > 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. These are echo chamber reviews. Even if you echo chamber happens produce good reviews there's still no harm in getting them from outside it as well. Plus we can't make this generic workflow because too many less awesome contributors would take advantage of the laxity it offers. > 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. Just because you won't check others' code for bugs doesn't mean that others won't check your code for bugs... > 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. This is an unknown unknown problem: you can't bring up to others things you don't know about yourself. > I personally feel that this mindset teaches more responsibility and > the "right" kind of defensiveness that it takes to write reliable > code. So you didn't answer whether you ran checkpatch and ignored the warning (in which case a commit comment explaining why would have been useful) or didn't run checkpatch (in which case why not?). James
Hi Kent, On Mon, May 20, 2024 at 4:39 AM Kees Cook <keescook@chromium.org> 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. This morning, the kisskb build service informed me about several build failures on m68k (e.g. [1]). In fact, the kernel test robot had already detected them on multiple 32-bit platforms 4 days ago: - Subject: [bcachefs:bcachefs-testing 21/23] fs/bcachefs/btree_io.c:542:7: warning: format specifies type 'size_t' (aka 'unsigned int') but the argument has type 'unsigned long'[2] - Subject: [bcachefs:bcachefs-testing 21/23] fs/bcachefs/btree_io.c:541:33: warning: format '%zu' expects argument of type 'size_t', but argument 3 has type 'long unsigned int'[3] These are caused by commit 1d34085cde461893 ("bcachefs: Plumb bkey into __btree_err()"), which is nowhere to be found on any public mailing list archived by lore. + prt_printf(out, " bset byte offset %zu", + (unsigned long)(void *)k - + ((unsigned long)(void *)i & ~511UL)); Please stop committing private unreviewed patches to linux-next, as I have asked before [4]. Thank you! [1]http://kisskb.ellerman.id.au/kisskb/buildresult/15176487/ [2] https://lore.kernel.org/all/202405250948.JeFBsoxZ-lkp@intel.com/ [3] https://lore.kernel.org/all/202405250757.U8G0SdUV-lkp@intel.com/ [4] https://lore.kernel.org/all/CAMuHMdX04q-af8BVWqGgeG5gkZrrDJWsnrJh5j=XG97vrdTQrg@mail.gmail.com/ Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, May 28, 2024 at 09:18:24AM +0200, Geert Uytterhoeven wrote: > Hi Kent, > > On Mon, May 20, 2024 at 4:39 AM Kees Cook <keescook@chromium.org> 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. > > This morning, the kisskb build service informed me about several build > failures on m68k (e.g. [1]). > > In fact, the kernel test robot had already detected them on multiple 32-bit > platforms 4 days ago: > - Subject: [bcachefs:bcachefs-testing 21/23] fs/bcachefs/btree_io.c:542:7: > warning: format specifies type 'size_t' (aka 'unsigned int') but the > argument has type 'unsigned long'[2] > - Subject: [bcachefs:bcachefs-testing 21/23] fs/bcachefs/btree_io.c:541:33: > warning: format '%zu' expects argument of type 'size_t', but argument > 3 has type 'long unsigned int'[3] > > These are caused by commit 1d34085cde461893 ("bcachefs: > Plumb bkey into __btree_err()"), which is nowhere to be found on > any public mailing list archived by lore. > > + prt_printf(out, " bset byte offset %zu", > + (unsigned long)(void *)k - > + ((unsigned long)(void *)i & ~511UL)); > > Please stop committing private unreviewed patches to linux-next, > as I have asked before [4]. > Thank you! You seem to be complaining about test infrastructur eissues - you don't seriously expect code review to be catching 32 bit build isues, do you? 0day takes awhile to run, so I don't always see these right away. I'll add some 32 bit builds to my own test infrastructure.
Hi Kent, On Tue, May 28, 2024 at 4:57 PM Kent Overstreet <kent.overstreet@linux.dev> wrote: > On Tue, May 28, 2024 at 09:18:24AM +0200, Geert Uytterhoeven wrote: > > These are caused by commit 1d34085cde461893 ("bcachefs: > > Plumb bkey into __btree_err()"), which is nowhere to be found on > > any public mailing list archived by lore. > > > > + prt_printf(out, " bset byte offset %zu", > > + (unsigned long)(void *)k - > > + ((unsigned long)(void *)i & ~511UL)); > > > > Please stop committing private unreviewed patches to linux-next, > > as I have asked before [4]. > > Thank you! > > You seem to be complaining about test infrastructur eissues - you don't > seriously expect code review to be catching 32 bit build isues, do you? I do expect code review to catch (a) wrong printf format specifiers (especially when lots of casts are involved), (b) hardcoded constants, and (c) opportunities for introducing helper macros, Gr{oetje,eeting}s, Geert
On Mon, May 20, 2024 at 12:10:31PM -0400, James Bottomley wrote: > On Sun, 2024-05-19 at 23:52 -0400, Kent Overstreet wrote: > > 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... > you still don't say what problem not posting most patches solves? You > imply it would slow you down, but getting git-send-email to post to a > mailing list can actually be automated through a pre-push commit hook > with no slowdown in the awesome rate at which you apply patches to your > own tree. > Linux kernel process exists because it's been found to work over time. > That's not to say it can't be changed, but it usually requires at least > some stab at a reason before that happens. Even if no meaningful review ever happens on the actual posts there's still utility in having the patches on a list and findable in lore, since everything is normally on the list people end up with workflows that assume that they'll be able to find things there. For example it's common for test people who identify which patch introduces an issue to grab the patch from lore in order to review any discussion of the patch, then report by replying to the patch to help with context for their report and get some help with figuring out a CC list. Posting costs very little and makes people's lives easier.
On Sat, Jun 01, 2024 at 12:33:31PM +0100, Mark Brown wrote: > On Mon, May 20, 2024 at 12:10:31PM -0400, James Bottomley wrote: > > On Sun, 2024-05-19 at 23:52 -0400, Kent Overstreet wrote: > > > > 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... > > > you still don't say what problem not posting most patches solves? You > > imply it would slow you down, but getting git-send-email to post to a > > mailing list can actually be automated through a pre-push commit hook > > with no slowdown in the awesome rate at which you apply patches to your > > own tree. > > > Linux kernel process exists because it's been found to work over time. > > That's not to say it can't be changed, but it usually requires at least > > some stab at a reason before that happens. > > Even if no meaningful review ever happens on the actual posts there's > still utility in having the patches on a list and findable in lore, > since everything is normally on the list people end up with workflows > that assume that they'll be able to find things there. For example it's > common for test people who identify which patch introduces an issue to > grab the patch from lore in order to review any discussion of the patch, > then report by replying to the patch to help with context for their > report and get some help with figuring out a CC list. Posting costs > very little and makes people's lives easier. Exactly. This is the standard workflow that everyone depends on. So, for example, for my -next trees, I only ever add patches to them via "b4 am lore-url-here...". If I've got patches to add to -next from some devel tree, I don't cherry-pick them to my -next tree: I send them to lore, and then pull them back down. But the point is: send your stuff to lore. :)