[GIT PULL] Writeback fix

[GIT PULL] Writeback fix

[Jens Axboe]

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(-)

-- 
Jens Axboe

Re: [GIT PULL] Writeback fix

[Linus Torvalds]

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

Re: [GIT PULL] Writeback fix

[Jens Axboe]

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?

-- 
Jens Axboe

Re: [GIT PULL] Writeback fix

[Linus Torvalds]

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

Re: [GIT PULL] Writeback fix

[Jens Axboe]

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.

-- 
Jens Axboe

Re: [GIT PULL] Writeback fix

[Jan Kara]

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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR