| Message ID | 169786962623.1265253.5321166241579915281.stg-ugh@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT,PULL] iomap: bug fixes for 6.6-rc7 | expand |
On Fri, 20 Oct 2023 at 23:27, Darrick J. Wong <djwong@kernel.org> wrote: > > Please pull this branch with changes for iomap for 6.6-rc7. > > As usual, I did a test-merge with the main upstream branch as of a few > minutes ago, and didn't see any conflicts. Please let me know if you > encounter any problems. .. and as usual, the branch you point to does not actually exist. Because you *again* pointed to the wrong tree. This time I remembered what the mistake was last time, and picked out the right tree by hand, but *please* just fix your completely broken scripts or workflow. > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git iomap-6.6-fixes-5 No. It's pub/scm/fs/xfs/xfs-linux, once again. Linus
The pull request you sent on Fri, 20 Oct 2023 23:27:44 -0700:
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git iomap-6.6-fixes-5
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/5722119f674d81eb88d51463ece8096855d94cc0
Thank you!
On Sat, Oct 21, 2023 at 09:46:35AM -0700, Linus Torvalds wrote: > On Fri, 20 Oct 2023 at 23:27, Darrick J. Wong <djwong@kernel.org> wrote: > > > > Please pull this branch with changes for iomap for 6.6-rc7. > > > > As usual, I did a test-merge with the main upstream branch as of a few > > minutes ago, and didn't see any conflicts. Please let me know if you > > encounter any problems. > > .. and as usual, the branch you point to does not actually exist. > > Because you *again* pointed to the wrong tree. > > This time I remembered what the mistake was last time, and picked out > the right tree by hand, but *please* just fix your completely broken > scripts or workflow. > > > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git iomap-6.6-fixes-5 > > No. > > It's pub/scm/fs/xfs/xfs-linux, once again. Sorry about that. After reviewing the output of git request-pull, I have learned that if you provide a $url argument that does not point to a repo containing $start, it will print a warning to stderr and emit a garbage pull request to stdout anyway. No --force required or anything. Piping stdout to mutt without checking the return code is therefore a bad idea. I have now updated my wrapper script to buffer the entire pull request contents and check the return value before proceeding. It is a poor workman who blames his tools, so I declare publicly that you have an idiot for a maintainer. Christian: Do you have the bandwidth to take over fs/iomap/? --D > > Linus
On Mon, Oct 23, 2023 at 03:38:10PM -0700, Darrick J. Wong wrote: > On Sat, Oct 21, 2023 at 09:46:35AM -0700, Linus Torvalds wrote: > > On Fri, 20 Oct 2023 at 23:27, Darrick J. Wong <djwong@kernel.org> wrote: > > > > > > Please pull this branch with changes for iomap for 6.6-rc7. > > > > > > As usual, I did a test-merge with the main upstream branch as of a few > > > minutes ago, and didn't see any conflicts. Please let me know if you > > > encounter any problems. > > > > .. and as usual, the branch you point to does not actually exist. > > > > Because you *again* pointed to the wrong tree. > > > > This time I remembered what the mistake was last time, and picked out > > the right tree by hand, but *please* just fix your completely broken > > scripts or workflow. > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git iomap-6.6-fixes-5 > > > > No. > > > > It's pub/scm/fs/xfs/xfs-linux, once again. > > Sorry about that. After reviewing the output of git request-pull, I > have learned that if you provide a $url argument that does not point to > a repo containing $start, it will print a warning to stderr and emit a > garbage pull request to stdout anyway. No --force required or anything. > Piping stdout to mutt without checking the return code is therefore a > bad idea. > > I have now updated my wrapper script to buffer the entire pull request > contents and check the return value before proceeding. > > It is a poor workman who blames his tools, so I declare publicly that > you have an idiot for a maintainer. > > Christian: Do you have the bandwidth to take over fs/iomap/? If this helps you I will take iomap over but only if you and Christoph stay around as main reviewers. There's not much point in me pretending I can meaningfully review fs/iomap/ and I don't have the bandwith even if I could. So not without clear reviewers. But, - and I'm sorry if I may overstep bounds a little bit - I think this self-castigation is really unwarranted. And we all very much know that you definitely aren't an idiot. And personally I think we shouldn't give the impression that we expect this sort of repentance when we make mistakes. In other words, if the sole reason you're proposing this is an objectively false belief then I would suggest to reconsider.
On Tue, Oct 24, 2023 at 01:47:17PM +0200, Christian Brauner wrote: > On Mon, Oct 23, 2023 at 03:38:10PM -0700, Darrick J. Wong wrote: > > On Sat, Oct 21, 2023 at 09:46:35AM -0700, Linus Torvalds wrote: > > > On Fri, 20 Oct 2023 at 23:27, Darrick J. Wong <djwong@kernel.org> wrote: > > > > > > > > Please pull this branch with changes for iomap for 6.6-rc7. > > > > > > > > As usual, I did a test-merge with the main upstream branch as of a few > > > > minutes ago, and didn't see any conflicts. Please let me know if you > > > > encounter any problems. > > > > > > .. and as usual, the branch you point to does not actually exist. > > > > > > Because you *again* pointed to the wrong tree. > > > > > > This time I remembered what the mistake was last time, and picked out > > > the right tree by hand, but *please* just fix your completely broken > > > scripts or workflow. > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git iomap-6.6-fixes-5 > > > > > > No. > > > > > > It's pub/scm/fs/xfs/xfs-linux, once again. > > > > Sorry about that. After reviewing the output of git request-pull, I > > have learned that if you provide a $url argument that does not point to > > a repo containing $start, it will print a warning to stderr and emit a > > garbage pull request to stdout anyway. No --force required or anything. > > Piping stdout to mutt without checking the return code is therefore a > > bad idea. > > > > I have now updated my wrapper script to buffer the entire pull request > > contents and check the return value before proceeding. > > > > It is a poor workman who blames his tools, so I declare publicly that > > you have an idiot for a maintainer. > > > > Christian: Do you have the bandwidth to take over fs/iomap/? > > If this helps you I will take iomap over but only if you and Christoph > stay around as main reviewers. I can't speak for Christoph, but I am very much willing to continue developing patches for fs/iomap, running the QA farm to make sure it's working properly, and reviewing everyone else's patches. Same as I do now. What I would like to concentrate on in the future are: (a) improving documentation and cleanups that other fs maintainers have been asking for and I haven't had time to work on (b) helping interested fs maintainers port their fs to iomap for better performance (c) figuring out how to integrate smoothly with things like fsverity and fscrypt (d) not stepping on *your* toes every time you want to change something in the vfs only to have it collide with iomap changes that you didn't see Similar to what we just did with XFS, I propose breaking up the iomap Maintainer role into pieces that are more manageable by a single person. As RM, all you'd have to do is integrate reviewed patches and pull requests into one of your work branches. That gives you final say over what goes in and how it goes in, instead of letting branches collide in for-next without warning. You can still forward on the review requests and bug reports to me. That part isn't changing. I've enjoyed working with you and hope that'll continue well into the future. :) > There's not much point in me pretending I > can meaningfully review fs/iomap/ and I don't have the bandwith even if > I could. So not without clear reviewers. I hope the above assuades your concerns/fears! > But, - and I'm sorry if I may overstep bounds a little bit - I think > this self-castigation is really unwarranted. And we all very much know > that you definitely aren't an idiot. And personally I think we shouldn't > give the impression that we expect this sort of repentance when we make > mistakes. > > In other words, if the sole reason you're proposing this is an > objectively false belief then I would suggest to reconsider. Quite the opposite, these are changes that I've been wanting to make for months. :) --D
On Wed, Oct 25, 2023 at 08:13:25PM -0700, Darrick J. Wong wrote: > On Tue, Oct 24, 2023 at 01:47:17PM +0200, Christian Brauner wrote: > > On Mon, Oct 23, 2023 at 03:38:10PM -0700, Darrick J. Wong wrote: > > > On Sat, Oct 21, 2023 at 09:46:35AM -0700, Linus Torvalds wrote: > > > > On Fri, 20 Oct 2023 at 23:27, Darrick J. Wong <djwong@kernel.org> wrote: > > > > > > > > > > Please pull this branch with changes for iomap for 6.6-rc7. > > > > > > > > > > As usual, I did a test-merge with the main upstream branch as of a few > > > > > minutes ago, and didn't see any conflicts. Please let me know if you > > > > > encounter any problems. > > > > > > > > .. and as usual, the branch you point to does not actually exist. > > > > > > > > Because you *again* pointed to the wrong tree. > > > > > > > > This time I remembered what the mistake was last time, and picked out > > > > the right tree by hand, but *please* just fix your completely broken > > > > scripts or workflow. > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git iomap-6.6-fixes-5 > > > > > > > > No. > > > > > > > > It's pub/scm/fs/xfs/xfs-linux, once again. > > > > > > Sorry about that. After reviewing the output of git request-pull, I > > > have learned that if you provide a $url argument that does not point to > > > a repo containing $start, it will print a warning to stderr and emit a > > > garbage pull request to stdout anyway. No --force required or anything. > > > Piping stdout to mutt without checking the return code is therefore a > > > bad idea. > > > > > > I have now updated my wrapper script to buffer the entire pull request > > > contents and check the return value before proceeding. > > > > > > It is a poor workman who blames his tools, so I declare publicly that > > > you have an idiot for a maintainer. > > > > > > Christian: Do you have the bandwidth to take over fs/iomap/? > > > > If this helps you I will take iomap over but only if you and Christoph > > stay around as main reviewers. > > I can't speak for Christoph, but I am very much willing to continue > developing patches for fs/iomap, running the QA farm to make sure it's > working properly, and reviewing everyone else's patches. Same as I do > now. > > What I would like to concentrate on in the future are: > > (a) improving documentation and cleanups that other fs maintainers have > been asking for and I haven't had time to work on > > (b) helping interested fs maintainers port their fs to iomap for better > performance > > (c) figuring out how to integrate smoothly with things like fsverity and > fscrypt > > (d) not stepping on *your* toes every time you want to change something > in the vfs only to have it collide with iomap changes that you > didn't see > > Similar to what we just did with XFS, I propose breaking up the iomap > Maintainer role into pieces that are more manageable by a single person. Sounds good. > As RM, all you'd have to do is integrate reviewed patches and pull > requests into one of your work branches. That gives you final say over > what goes in and how it goes in, instead of letting branches collide in > for-next without warning. > > You can still forward on the review requests and bug reports to me. Ok, cool. > That part isn't changing. I've enjoyed working with you and hope > that'll continue well into the future. :) Thanks. That's good to hear and right back at you. > > > There's not much point in me pretending I > > can meaningfully review fs/iomap/ and I don't have the bandwith even if > > I could. So not without clear reviewers. > > I hope the above assuades your concerns/fears! Yes. > > > But, - and I'm sorry if I may overstep bounds a little bit - I think > > this self-castigation is really unwarranted. And we all very much know > > that you definitely aren't an idiot. And personally I think we shouldn't > > give the impression that we expect this sort of repentance when we make > > mistakes. > > > > In other words, if the sole reason you're proposing this is an > > objectively false belief then I would suggest to reconsider. > > Quite the opposite, these are changes that I've been wanting to make for > months. :) In that case I would propose you send a patch to Linus for MAINTAINERS updating the tree and the entries for iomap. I believe it's customary for the current maintainer to do this. Thanks! Christian
On Wed, 25 Oct 2023 at 17:13, Darrick J. Wong <djwong@kernel.org> wrote: > > Similar to what we just did with XFS, I propose breaking up the iomap > Maintainer role into pieces that are more manageable by a single person. > As RM, all you'd have to do is integrate reviewed patches and pull > requests into one of your work branches. That gives you final say over > what goes in and how it goes in, instead of letting branches collide in > for-next without warning. I _think_ what you are saying is that you'd like to avoid being both a maintainer and a developer. Now, I'm probably hugely biased and going by personal experience, but I do think that doing double duty is the worst of both worlds, and pointlessly stressful. As a maintainer, you have to worry about the big picture (things like release timing, even if it's just a "is this a fix for this release, or should it get queued for the next one") but also code-related things like "we have two different things going on, let's sort them out separately". Christian had that kind of issue just a couple of days ago with the btrfs tree. But then, as a developer, those are distractions and just add stress and worry, and distract from whatever you're working on. As a developer, the last thing you want to worry about is something else than the actual technical issue you're trying to solve. And obviously, there's a lot of overlap. A maintainer needs to be _able_ to be a developer just to make good choices. And the whole "maintainer vs developer" doesn't have to be two different people, somebody might shift from one to the other simply because maybe they enjoy both roles. Just not at the same time, all the time, having both things putting different stress on you. You can *kind* of see the difference in our git tree if you do git rev-list --count --author=XYZ --no-merges --since=1.year HEAD to see "code authorship" (aka developer), vs git rev-list --count --committer=XYZ --since=1.year HEAD which shows some kind of approximation of "maintainership". Obviously there is overlap (potentially a lot of it) and the above isn't some absolute thing, but you can see some patterns. I personally wish we had more people who are maintainers _without_ having to worry too much about developing new code. One of the issues that keeps coming up is that companies don't always seem to appreciate maintainership (which is a bit strange - the same companies may then _love_ appreciateing managers, which is something very different but has some of the same flavour to it). And btw, I don't want to make that "I wish we had more maintainers" be a value judgement. It's not that maintainers are somehow more important than developers. I just think they are two fairly different roles, and I think one person doing both puts unnecessary stress on that person. Linus
On Thu, 2023-10-26 at 08:10 -1000, Linus Torvalds wrote: > On Wed, 25 Oct 2023 at 17:13, Darrick J. Wong <djwong@kernel.org> wrote: > > > > Similar to what we just did with XFS, I propose breaking up the iomap > > Maintainer role into pieces that are more manageable by a single person. > > As RM, all you'd have to do is integrate reviewed patches and pull > > requests into one of your work branches. That gives you final say over > > what goes in and how it goes in, instead of letting branches collide in > > for-next without warning. > > I _think_ what you are saying is that you'd like to avoid being both a > maintainer and a developer. > > Now, I'm probably hugely biased and going by personal experience, but > I do think that doing double duty is the worst of both worlds, and > pointlessly stressful. > > As a maintainer, you have to worry about the big picture (things like > release timing, even if it's just a "is this a fix for this release, > or should it get queued for the next one") but also code-related > things like "we have two different things going on, let's sort them > out separately". Christian had that kind of issue just a couple of > days ago with the btrfs tree. > > But then, as a developer, those are distractions and just add stress > and worry, and distract from whatever you're working on. As a > developer, the last thing you want to worry about is something else > than the actual technical issue you're trying to solve. > > And obviously, there's a lot of overlap. A maintainer needs to be > _able_ to be a developer just to make good choices. And the whole > "maintainer vs developer" doesn't have to be two different people, > somebody might shift from one to the other simply because maybe they > enjoy both roles. Just not at the same time, all the time, having both > things putting different stress on you. > > You can *kind* of see the difference in our git tree if you do > > git rev-list --count --author=XYZ --no-merges --since=1.year HEAD > > to see "code authorship" (aka developer), vs > > git rev-list --count --committer=XYZ --since=1.year HEAD > > which shows some kind of approximation of "maintainership". Obviously > there is overlap (potentially a lot of it) and the above isn't some > absolute thing, but you can see some patterns. > > I personally wish we had more people who are maintainers _without_ > having to worry too much about developing new code. One of the issues > that keeps coming up is that companies don't always seem to appreciate > maintainership (which is a bit strange - the same companies may then > _love_ appreciateing managers, which is something very different but > has some of the same flavour to it). > > And btw, I don't want to make that "I wish we had more maintainers" be > a value judgement. It's not that maintainers are somehow more > important than developers. I just think they are two fairly different > roles, and I think one person doing both puts unnecessary stress on > that person. > Personally, I wouldn't want to give up doing development work altogether. I don't mind doing some maintainership duties, and helping out other maintainers where I can, but maintainership takes a lot more time than most people realize. You're sort of obligated to review everything that comes across your plate. I know Trond and Anna have been successful alternating the handling of pull requests and linux-next. I think it makes a lot of sense to have maintainership teams, at least for larger subsystems. We sort of have them informally now, but maybe we should be aiming to have a group of people who are all nominally maintainers on larger subsystems. They can then rotate between themselves on some schedule (every year? or every 5 releases? Whatever works...). That provides some extra incentive for other team members to help out with reviews, and it may make it simpler for someone else to step in when the current maintainer needs to step away. It does rely on a high degree of trust between team members, but most of us seem to get along.
On Thu, Oct 26, 2023 at 01:54:29PM +0200, Christian Brauner wrote: > On Wed, Oct 25, 2023 at 08:13:25PM -0700, Darrick J. Wong wrote: > > On Tue, Oct 24, 2023 at 01:47:17PM +0200, Christian Brauner wrote: > > > On Mon, Oct 23, 2023 at 03:38:10PM -0700, Darrick J. Wong wrote: > > > > On Sat, Oct 21, 2023 at 09:46:35AM -0700, Linus Torvalds wrote: > > > > > On Fri, 20 Oct 2023 at 23:27, Darrick J. Wong <djwong@kernel.org> wrote: > > > > > > > > > > > > Please pull this branch with changes for iomap for 6.6-rc7. > > > > > > > > > > > > As usual, I did a test-merge with the main upstream branch as of a few > > > > > > minutes ago, and didn't see any conflicts. Please let me know if you > > > > > > encounter any problems. > > > > > > > > > > .. and as usual, the branch you point to does not actually exist. > > > > > > > > > > Because you *again* pointed to the wrong tree. > > > > > > > > > > This time I remembered what the mistake was last time, and picked out > > > > > the right tree by hand, but *please* just fix your completely broken > > > > > scripts or workflow. > > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git iomap-6.6-fixes-5 > > > > > > > > > > No. > > > > > > > > > > It's pub/scm/fs/xfs/xfs-linux, once again. > > > > > > > > Sorry about that. After reviewing the output of git request-pull, I > > > > have learned that if you provide a $url argument that does not point to > > > > a repo containing $start, it will print a warning to stderr and emit a > > > > garbage pull request to stdout anyway. No --force required or anything. > > > > Piping stdout to mutt without checking the return code is therefore a > > > > bad idea. > > > > > > > > I have now updated my wrapper script to buffer the entire pull request > > > > contents and check the return value before proceeding. > > > > > > > > It is a poor workman who blames his tools, so I declare publicly that > > > > you have an idiot for a maintainer. > > > > > > > > Christian: Do you have the bandwidth to take over fs/iomap/? > > > > > > If this helps you I will take iomap over but only if you and Christoph > > > stay around as main reviewers. > > > > I can't speak for Christoph, but I am very much willing to continue > > developing patches for fs/iomap, running the QA farm to make sure it's > > working properly, and reviewing everyone else's patches. Same as I do > > now. > > > > What I would like to concentrate on in the future are: > > > > (a) improving documentation and cleanups that other fs maintainers have > > been asking for and I haven't had time to work on > > > > (b) helping interested fs maintainers port their fs to iomap for better > > performance > > > > (c) figuring out how to integrate smoothly with things like fsverity and > > fscrypt > > > > (d) not stepping on *your* toes every time you want to change something > > in the vfs only to have it collide with iomap changes that you > > didn't see > > > > Similar to what we just did with XFS, I propose breaking up the iomap > > Maintainer role into pieces that are more manageable by a single person. > > Sounds good. > > > As RM, all you'd have to do is integrate reviewed patches and pull > > requests into one of your work branches. That gives you final say over > > what goes in and how it goes in, instead of letting branches collide in > > for-next without warning. > > > > You can still forward on the review requests and bug reports to me. > > Ok, cool. > > > That part isn't changing. I've enjoyed working with you and hope > > that'll continue well into the future. :) > > Thanks. That's good to hear and right back at you. Thank you! I enjoyed reading that! :) > > > > > There's not much point in me pretending I > > > can meaningfully review fs/iomap/ and I don't have the bandwith even if > > > I could. So not without clear reviewers. > > > > I hope the above assuades your concerns/fears! > > Yes. > > > > > > But, - and I'm sorry if I may overstep bounds a little bit - I think > > > this self-castigation is really unwarranted. And we all very much know > > > that you definitely aren't an idiot. And personally I think we shouldn't > > > give the impression that we expect this sort of repentance when we make > > > mistakes. > > > > > > In other words, if the sole reason you're proposing this is an > > > objectively false belief then I would suggest to reconsider. > > > > Quite the opposite, these are changes that I've been wanting to make for > > months. :) > > In that case I would propose you send a patch to Linus for MAINTAINERS > updating the tree and the entries for iomap. I believe it's customary > for the current maintainer to do this. Will do. I'll change the M: entry to you, and add myself as an R:. --D > > Thanks! > Christian
On Thu, Oct 26, 2023 at 08:10:01AM -1000, Linus Torvalds wrote: > On Wed, 25 Oct 2023 at 17:13, Darrick J. Wong <djwong@kernel.org> wrote: > > > > Similar to what we just did with XFS, I propose breaking up the iomap > > Maintainer role into pieces that are more manageable by a single person. > > As RM, all you'd have to do is integrate reviewed patches and pull > > requests into one of your work branches. That gives you final say over > > what goes in and how it goes in, instead of letting branches collide in > > for-next without warning. > > I _think_ what you are saying is that you'd like to avoid being both a > maintainer and a developer. > > Now, I'm probably hugely biased and going by personal experience, but > I do think that doing double duty is the worst of both worlds, and > pointlessly stressful. > > As a maintainer, you have to worry about the big picture (things like > release timing, even if it's just a "is this a fix for this release, > or should it get queued for the next one") but also code-related > things like "we have two different things going on, let's sort them > out separately". Christian had that kind of issue just a couple of > days ago with the btrfs tree. > > But then, as a developer, those are distractions and just add stress > and worry, and distract from whatever you're working on. As a > developer, the last thing you want to worry about is something else > than the actual technical issue you're trying to solve. > > And obviously, there's a lot of overlap. A maintainer needs to be > _able_ to be a developer just to make good choices. And the whole > "maintainer vs developer" doesn't have to be two different people, > somebody might shift from one to the other simply because maybe they > enjoy both roles. Just not at the same time, all the time, having both > things putting different stress on you. > > You can *kind* of see the difference in our git tree if you do > > git rev-list --count --author=XYZ --no-merges --since=1.year HEAD > > to see "code authorship" (aka developer), vs > > git rev-list --count --committer=XYZ --since=1.year HEAD > > which shows some kind of approximation of "maintainership". Obviously > there is overlap (potentially a lot of it) and the above isn't some > absolute thing, but you can see some patterns. > > I personally wish we had more people who are maintainers _without_ > having to worry too much about developing new code. One of the issues > that keeps coming up is that companies don't always seem to appreciate > maintainership (which is a bit strange - the same companies may then > _love_ appreciateing managers, which is something very different but > has some of the same flavour to it). > > And btw, I don't want to make that "I wish we had more maintainers" be > a value judgement. It's not that maintainers are somehow more > important than developers. I just think they are two fairly different > roles, and I think one person doing both puts unnecessary stress on > that person. I think most of us enjoy both. Personally, I always try to carve out time where I can work on stuff that I find interesting. It helps to stay sane and it helps with providing good reviews. It also helps being reminded how easy it is to code up bugs. When there's an interesting series that morphes into larger subsystem massaging that's always great because that's a chance to collaborate and different people can take on different parts. And it usually means one does get a few patches in as well. In a smaller subsystem it's probably ok to be main developer, reviewer, and developer at the same time. But in a busy subsystem that trias isn't sustainable. Let alone scaling that to multiple subsystems. One of the critical parts is review. Good reviews are often insanely expensive and they are very much a factor in burning people out. If one only ever reviews and the load never ends that's going to fsck with you in the long run. A reviewer must be technically great, have design, context, maintainability, and wider context and future of the subsystem in mind, and not be afraid to have code changed or removed that they wrote (That last part really matters.), and also be ok with being publicly wrong in front of a whole bunch of smart people (Which I want to stress is absolutely fine and almost unavoidable.). But once you have a couple of good reviewers in a subsystem that you can really rely on that if they give an ack/rvb it's solid enough (not necessarily bug free even) that's really good. It not just takes away pressure it also means that it's possible for other reviewers/maintainers to go develop stuff themselves.
On Fri, 27 Oct 2023 at 08:46, Christian Brauner <brauner@kernel.org> wrote: > > One of the critical parts is review. Good reviews are often insanely > expensive and they are very much a factor in burning people out. If one > only ever reviews and the load never ends that's going to fsck with you > in the long run. I absolutely despise the review requirement that several companies have. I very much understand why it happens, but I think it's actively detrimental to the workflow. It's not just that reviewing is hard, the review requirement tends to be a serialization point where now you as a developer are waiting for others to review it, and those others are not nearly as motivated to do so or are easily going to be nitpicking about the non-critical things. So it's not just the reviewers that get burned out, I think the process ends up being horrific for developers too, and easily leads to the "let's send out version 17 of this patch based on previous review". At which point everybody is completely fed up with the whole process. And if it doesn't get to version 17, it's because the reviewers too have gotten so fed up that by version three they go "whatever, I've seen this before, they fixed the obvious thing I noticed, I'll mark it reviewed". The other dynamic with reviews is that you end up getting review-cliques, either due to company pressure or just a very natural "you review mine, I review yours" back-scratching. Don't get me wrong - it can work, and it can even work well, but I think the times it works really well is when people have gotten so used to each others, and know each other's quirks and workflows and they just work well together. But that also means that some people are having a much easier time getting reviews, because they are part of that "this group works well together" crowd. Maybe it's a necessary evil. I certainly do *not* think the "lone developer goes his own way" model works all that well. But the reason I said that I wish we had more maintainers, is that I think we would often be better off with not a "review process" back-and-forth. but a _pipeline_ through a few levels of maintainers. Not the "hold things up and send it back to the developer" kind of thing, but "Oh, this looks fine, I'll just send it on - possibly with the fixes I think are needed". So I think a pipeline of "Signed-off-by" (or just merges) might be something to strive for as at least a partial replacement for reviews. Sure, you might get Acked-by's or Reviewed-by's or Tested-by's along the way *too*, or - when people are just merging directly through git - you'd just get a merge commit with commentary and perhaps extra stuff on top. Back when we started doing the whole "Signed-off-by" - for legal reasons, not development reasons - the big requirement for me was that "it needs to work as a pipeline, not as some kind of back-and-forth that holds up development". And I think the whole sign-off chain has worked really well, and we've never needed to use it for the original legal purposes (and hopefully just by virtue of it existing, we never will), but it has been a huge success from a development standpoint. When something goes wrong, I think it's been great to have that whole chain of how it got merged, and in fact one of my least favorite parts of git ended up being how we never made it easy to see the merge chain after it's been committed (you can technically get it with "git name-rev", but it sure is not obvious). I dunno. Maybe I'm just dreaming. But the complaints about reviews - or lack of them - do tend to come up a lot, and I feel like the whole review process is a very big part of the problem. Linus
On Fri, Oct 27, 2023 at 01:30:00PM -1000, Linus Torvalds wrote: > On Fri, 27 Oct 2023 at 08:46, Christian Brauner <brauner@kernel.org> wrote: > > > > One of the critical parts is review. Good reviews are often insanely > > expensive and they are very much a factor in burning people out. If one > > only ever reviews and the load never ends that's going to fsck with you > > in the long run. > > I absolutely despise the review requirement that several companies > have. I very much understand why it happens, but I think it's actively > detrimental to the workflow. > > It's not just that reviewing is hard, the review requirement tends to > be a serialization point where now you as a developer are waiting for > others to review it, and those others are not nearly as motivated to > do so or are easily going to be nitpicking about the non-critical > things. > > So it's not just the reviewers that get burned out, I think the > process ends up being horrific for developers too, and easily leads to > the "let's send out version 17 of this patch based on previous > review". At which point everybody is completely fed up with the whole > process. > > And if it doesn't get to version 17, it's because the reviewers too > have gotten so fed up that by version three they go "whatever, I've > seen this before, they fixed the obvious thing I noticed, I'll mark it > reviewed". You're talking about companies having review requirements for their internal project stuff, right? I haven't heard that there's any sort of review requirement with respect to the upstream kernel from companies. Sure, if you make it a requirement then it sucks because it's just another chore. I think we lack more reviewers for a multitude of reasons but to me one of the biggest is that we don't encourage the people to do it and there's no inherent reward to it. Sure, you get that RVB or ACK in the changelog but whatever. That neither gets you a job nor does it get you on LWN. So why bother. So often our reviewers are the people who have some sort of sense of ownership with respect to the subsystem. That's mostly the maintainers. You occassionally get the author of a patch series that sticks around and reviews stuff that they wrote. But that's not enough and then we're back to the problem that you can't effectively be maintainer, reviewer, and main developer. And yes, nitpicky review isn't really helpful and the goal shouldn't be to push a series to pointless version numbers before it can be merged. > > The other dynamic with reviews is that you end up getting > review-cliques, either due to company pressure or just a very natural > "you review mine, I review yours" back-scratching. > > Don't get me wrong - it can work, and it can even work well, but I > think the times it works really well is when people have gotten so > used to each others, and know each other's quirks and workflows and > they just work well together. But that also means that some people are > having a much easier time getting reviews, because they are part of > that "this group works well together" crowd. Yes, we certainly see that happening. And I really think that's overall a good thing. It shouldn't become an in-group out-group thing obviously so that needs to be carefully handled. But long-term trust between core developers and maintainers is key to subsystem health imho. And such quick review bounces are a sign of that. I don't think we actively see useless reviews that are just "scratch my back". The people who review often do it with sufficient fervor (for better or worse sometimes). That said, yes, it needs to be fair. But it's just natural that you feel more likely to throw a ACK or RVB to something someone did you know usually does good work and you know will come back to help you put out the fire they started. > > Maybe it's a necessary evil. I certainly do *not* think the "lone I think it's just a natural development and it's on use to make sure it doesn't become some sort of group thing where it's five people from the same company that push their stuff upstream. > developer goes his own way" model works all that well. But the reason Yeah, so I do think that this is actually the bigger problem long term. Lone-wolf models are terrible. But I didn't grow up with lone-wolf projects so I have no strong attachment to such models in the first place. Maybe I judge that more harshly simply because of that. > I said that I wish we had more maintainers, is that I think we would > often be better off with not a "review process" back-and-forth. but a > _pipeline_ through a few levels of maintainers. Not the "hold things > up and send it back to the developer" kind of thing, but "Oh, this > looks fine, I'll just send it on - possibly with the fixes I think are > needed". > > So I think a pipeline of "Signed-off-by" (or just merges) might be > something to strive for as at least a partial replacement for reviews. > > Sure, you might get Acked-by's or Reviewed-by's or Tested-by's along > the way *too*, or - when people are just merging directly through git > - you'd just get a merge commit with commentary and perhaps extra > stuff on top. And we kinda do that in some subsystems rather consequently. Networking does it at least and BPF does it with their subtrees and then BPF and its subtrees bubble up into networking and then into mainline. And that model seems to work well, yes. And it takes pressure of because it formalizes the whole maintenance thing a lot more. Idk if you've seen that but what we do for new stuff that gets added to vfs is that we have maintainership entries ala BPF so e.g., FILESYSTEMS [EXPORTFS] - that patch is still in -next and the PR for that won't get sent before next week and then it lists the maintainers. But it's all part of vfs.git and they just bubble up the patches. We could just do that via sub merges even. > > Back when we started doing the whole "Signed-off-by" - for legal > reasons, not development reasons - the big requirement for me was that > "it needs to work as a pipeline, not as some kind of back-and-forth > that holds up development". And I think the whole sign-off chain has > worked really well, and we've never needed to use it for the original > legal purposes (and hopefully just by virtue of it existing, we never > will), but it has been a huge success from a development standpoint. > When something goes wrong, I think it's been great to have that whole > chain of how it got merged, and in fact one of my least favorite parts > of git ended up being how we never made it easy to see the merge chain > after it's been committed (you can technically get it with "git > name-rev", but it sure is not obvious). > > I dunno. Maybe I'm just dreaming. But the complaints about reviews - > or lack of them - do tend to come up a lot, and I feel like the whole > review process is a very big part of the problem. Yeah, certainly.
Hi Linus, Please pull this branch with changes for iomap for 6.6-rc7. As usual, I did a test-merge with the main upstream branch as of a few minutes ago, and didn't see any conflicts. Please let me know if you encounter any problems. --D The following changes since commit 684f7e6d28e8087502fc8efdb6c9fe82400479dd: iomap: Spelling s/preceeding/preceding/g (2023-09-28 09:26:58 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git iomap-6.6-fixes-5 for you to fetch changes up to 3ac974796e5d94509b85a403449132ea660127c2: iomap: fix short copy in iomap_write_iter() (2023-10-19 09:41:36 -0700) ---------------------------------------------------------------- Bug fixes for 6.6-rc6: * Fix a bug where a writev consisting of a bunch of sub-fsblock writes where the last buffer address is invalid could lead to an infinite loop. Signed-off-by: Darrick J. Wong <djwong@kernel.org> ---------------------------------------------------------------- Jan Stancek (1): iomap: fix short copy in iomap_write_iter() fs/iomap/buffered-io.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)