| Message ID | 9f6a8d1a-aa05-626d-6764-99c376722ed7@kernel.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT,PULL] Writeback fix | expand |
On Sat, Dec 10, 2022 at 7:36 AM Jens Axboe <axboe@kernel.dk> wrote: > > Just a single writeback fix from Jan, for sanity checking adding freed > inodes to lists. That's what the commit message says too, but that's not what the patch actually *does*. It also does that unexplained + if (inode->i_state & I_FREEING) { + list_del_init(&inode->i_io_list); + wb_io_lists_depopulated(wb); + return; + } that is new. And yes, it has a link: in the commit message. And yes, I followed the link in case it had some background. And dammit, it's ANOTHER of those stupid pointless and worthless links that just links to the patch submission, and has NO ADDITIONAL INFORMATION. Those links are actively detrimental. Stop it. I just wasted time hoping that there would be some information about why the patch was sent to me this late in the game. Instead, I just wated time on it. I pulled this and then unpulled it. I'm very very annoyed. This patch has an actively misleading commit message, has no explanation for why it's so critical that it needs to be sent, and has a useless link to garbage. Fix the damn explanation to actually match the change. Fix the damn link to point to something *useful* like the error report or something. And STOP WASTING EVERYBODY'S TIME with these annoying links that I keep hoping would explain something and give useful background to the change and instead just are a source of constant disappointment. Linus
On 12/10/22 10:44?AM, Linus Torvalds wrote: > On Sat, Dec 10, 2022 at 7:36 AM Jens Axboe <axboe@kernel.dk> wrote: >> >> Just a single writeback fix from Jan, for sanity checking adding freed >> inodes to lists. > > That's what the commit message says too, but that's not what the patch > actually *does*. > > It also does that unexplained > > + if (inode->i_state & I_FREEING) { > + list_del_init(&inode->i_io_list); > + wb_io_lists_depopulated(wb); > + return; > + } > > that is new. > > And yes, it has a link: in the commit message. And yes, I followed the > link in case it had some background. > > And dammit, it's ANOTHER of those stupid pointless and worthless links > that just links to the patch submission, and has NO ADDITIONAL > INFORMATION. I agree that sometimes they are useless, but sometimes there's discussion on the patch as well in that link. And ideally the patch itself, when sent to the list, should include the link to the report, if any. Then you'd get both. > Those links are actively detrimental. Stop it. I just wasted time > hoping that there would be some information about why the patch was > sent to me this late in the game. Instead, I just wated time on it. > > I pulled this and then unpulled it. I'm very very annoyed. This patch > has an actively misleading commit message, has no explanation for why > it's so critical that it needs to be sent, and has a useless link to > garbage. Just to be clear, this was deliberately held for the 6.2 merge window, but I can also see that I completely missed that in the pull request. Sorry about that, that should've been clear. > Fix the damn explanation to actually match the change. Fix the damn > link to point to something *useful* like the error report or > something. I'll let Jan resubmit this one, just disregard this pull request and we'll send a new one during the merge window. > And STOP WASTING EVERYBODY'S TIME with these annoying links that I > keep hoping would explain something and give useful background to the > change and instead just are a source of constant disappointment. For me, applying patches is done by using a script, which is why all patches get the link. I do think it's worth having the link, because some of them will indeed have useful discussion. Is it worth it to manually have to deal with that, in case there's nothing there?
On Sat, Dec 10, 2022 at 10:11 AM Jens Axboe <axboe@kernel.dk> wrote: > > Just to be clear, this was deliberately held for the 6.2 merge window, > but I can also see that I completely missed that in the pull request. > Sorry about that, that should've been clear. Oh, it looked very much like a "lastminute single fix for 6.1". Your other pull requests are in my "for 6.2" queue. Linus
On 12/10/22 11:14 AM, Linus Torvalds wrote: > On Sat, Dec 10, 2022 at 10:11 AM Jens Axboe <axboe@kernel.dk> wrote: >> >> Just to be clear, this was deliberately held for the 6.2 merge window, >> but I can also see that I completely missed that in the pull request. >> Sorry about that, that should've been clear. > > Oh, it looked very much like a "lastminute single fix for 6.1". Yeah understandably, that was my fault. > Your other pull requests are in my "for 6.2" queue. Great, thanks.
On Sat 10-12-22 09:44:16, Linus Torvalds wrote: > On Sat, Dec 10, 2022 at 7:36 AM Jens Axboe <axboe@kernel.dk> wrote: > > > > Just a single writeback fix from Jan, for sanity checking adding freed > > inodes to lists. > > That's what the commit message says too, but that's not what the patch > actually *does*. > > It also does that unexplained > > + if (inode->i_state & I_FREEING) { > + list_del_init(&inode->i_io_list); > + wb_io_lists_depopulated(wb); > + return; > + } > > that is new. Yeah, I should have explained that. Now added to commit description and also to a comment before the if. I'll send new version to Jens. Honza
Hi Linus, Just a single writeback fix from Jan, for sanity checking adding freed inodes to lists. Please pull! The following changes since commit eb7081409f94a9a8608593d0fb63a1aa3d6f95d8: Linux 6.1-rc6 (2022-11-20 16:02:16 -0800) are available in the Git repository at: git://git.kernel.dk/linux.git tags/writeback-2022-12-08 for you to fetch changes up to d6798bc243fabfcb86c1d39168f1619304d2b9f9: writeback: Add asserts for adding freed inode to lists (2022-11-24 07:21:51 -0700) ---------------------------------------------------------------- writeback-2022-12-08 ---------------------------------------------------------------- Jan Kara (1): writeback: Add asserts for adding freed inode to lists fs/fs-writeback.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)