Den 2022-06-14 kl. 20:12, skrev Greg Kroah-Hartman:
On Tue, Jun 14, 2022 at 10:08:27AM -0700, Guenter Roeck wrote:
On Tue, Jun 14, 2022 at 08:36:08AM -0700, Guenter Roeck wrote:
On Mon, Jun 13, 2022 at 08:19:49PM +0200, Greg Kroah-Hartman wrote:
This is the start of the stable review cycle for the 5.15.47 release. There are 251 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know.
Responses should be made by Wed, 15 Jun 2022 18:18:03 +0000. Anything received after that time might be too late.
Build results: total: 159 pass: 159 fail: 0 Qemu test results: total: 488 pass: 488 fail: 0
I spoke a bit too early. I see the following backtrace in some qemu arm boot tests.
BUG: spinlock bad magic on CPU#0, kdevtmpfs/15 lock: noop_backing_dev_info+0x6c/0x3b0, .magic: 00000000, .owner: <none>/-1, .owner_cpu: 0 CPU: 0 PID: 15 Comm: kdevtmpfs Not tainted 5.15.47-rc2-00252-g677f0128d0ed #1 Hardware name: ARM RealView Machine (Device Tree Support) [<c01101d0>] (unwind_backtrace) from [<c010bc0c>] (show_stack+0x10/0x14) [<c010bc0c>] (show_stack) from [<c0a10ae4>] (dump_stack_lvl+0x68/0x90) [<c0a10ae4>] (dump_stack_lvl) from [<c0191250>] (do_raw_spin_lock+0xbc/0x124) [<c0191250>] (do_raw_spin_lock) from [<c02eb578>] (__mark_inode_dirty+0x1cc/0x704) [<c02eb578>] (__mark_inode_dirty) from [<c02e6a74>] (simple_setattr+0x44/0x5c) [<c02e6a74>] (simple_setattr) from [<c02d7a18>] (notify_change+0x400/0x45c) [<c02d7a18>] (notify_change) from [<c0a19ef8>] (devtmpfsd+0x1f8/0x2b8) [<c0a19ef8>] (devtmpfsd) from [<c014cf3c>] (kthread+0x150/0x17c) [<c014cf3c>] (kthread) from [<c0100120>] (ret_from_fork+0x14/0x34) Exception stack(0xd00dbfb0 to 0xd00dbff8) bfa0: 00000000 00000000 00000000 00000000 bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
This bisects to commit bc5d960d4e58 ("writeback: Fix inode->i_io_list not be protected by inode->i_lock error"). The problem is also seen in the mainline kernel. v5.15.y.queue and later are affected. Reverting the patch here and in mainline fixes the problem.
Thanks for letting me know. Hopefully it gets fixed in upstream...
I "think" this is the suggested fix:
https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git/commit/?h=...
-- Thomas
On Tue, Jun 14, 2022 at 11:20 AM Thomas Backlund tmb@tmb.nu wrote:
I "think" this is the suggested fix:
https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git/commit/?h=...
Ugh, this is just too ugly for words.
That's not a fix. That's a "hide the problem" patch.
Now, admittedly clearly the "hide the problem" code already existed, and was just moved earlier, but I really think this whole "we're calling __mark_inode_dirty() on an inode that isn't even *initialized* yet" is a much deeper issue, and shouldn't have some hacky work-around in __mark_inode_dirty() that just happens to make it work.
I don't mind that patch per se - moving the code is fine.
But I *do* mind the patch when the reason is to hide that wrong ordering of operations.
Now, maybe a proper fix might be to say that new_inode_pseudo() should always initialize i_state to I_DIRTY_ALL or something like that. The comment already says that they cannot participate in writeback, so maybe they should be disabled that way (ie a pseudo inode is always dirty and marking it dirty does nothing).
And then you get rid of the noop_backing_dev_info entirely.
Or just make sure that noop_backing_dev_info is fully initialized before it's used.
Because I think the real problem here is that things have a pointer to an uninitialized backing_dev_info.
Hmm? Jan?
Linus
On Tue, Jun 14, 2022 at 11:51 AM Linus Torvalds torvalds@linux-foundation.org wrote:
Or just make sure that noop_backing_dev_info is fully initialized before it's used.
I don't see any real reason why that
err = bdi_init(&noop_backing_dev_info);
couldn't just be done very early. Maybe as the first call in driver_init(), before the whole devtmpfs_init() etc.
But I might be missing some dependency there.
Linus
On Tue 14-06-22 12:00:22, Linus Torvalds wrote:
On Tue, Jun 14, 2022 at 11:51 AM Linus Torvalds torvalds@linux-foundation.org wrote:
Or just make sure that noop_backing_dev_info is fully initialized before it's used.
I don't see any real reason why that
err = bdi_init(&noop_backing_dev_info);couldn't just be done very early. Maybe as the first call in driver_init(), before the whole devtmpfs_init() etc.
I've checked the dependencies and cgroups (which are the only non-trivial dependency besides per-CPU infrastructure) are initialized early enough so it should work fine. So let's try that.
Honza
On Wed 15-06-22 13:00:22, Jan Kara wrote:
On Tue 14-06-22 12:00:22, Linus Torvalds wrote:
On Tue, Jun 14, 2022 at 11:51 AM Linus Torvalds torvalds@linux-foundation.org wrote:
Or just make sure that noop_backing_dev_info is fully initialized before it's used.
I don't see any real reason why that
err = bdi_init(&noop_backing_dev_info);couldn't just be done very early. Maybe as the first call in driver_init(), before the whole devtmpfs_init() etc.
I've checked the dependencies and cgroups (which are the only non-trivial dependency besides per-CPU infrastructure) are initialized early enough so it should work fine. So let's try that.
Attached patch boots for me. Guys, who was able to reproduce the failure: Can you please confirm this patch fixes your problem?
Honza
On Wed, Jun 15, 2022 at 03:38:45PM +0200, Jan Kara wrote:
On Wed 15-06-22 13:00:22, Jan Kara wrote:
On Tue 14-06-22 12:00:22, Linus Torvalds wrote:
On Tue, Jun 14, 2022 at 11:51 AM Linus Torvalds torvalds@linux-foundation.org wrote:
Or just make sure that noop_backing_dev_info is fully initialized before it's used.
I don't see any real reason why that
err = bdi_init(&noop_backing_dev_info);couldn't just be done very early. Maybe as the first call in driver_init(), before the whole devtmpfs_init() etc.
I've checked the dependencies and cgroups (which are the only non-trivial dependency besides per-CPU infrastructure) are initialized early enough so it should work fine. So let's try that.
Attached patch boots for me. Guys, who was able to reproduce the failure: Can you please confirm this patch fixes your problem?
It does for me.
Honza-- Jan Kara jack@suse.com SUSE Labs, CR
From 8f998b182be7563fc92aa8914cc7d21f75a3c20e Mon Sep 17 00:00:00 2001 From: Jan Kara jack@suse.cz Date: Wed, 15 Jun 2022 15:22:29 +0200 Subject: [PATCH] init: Initialized noop_backing_dev_info early
noop_backing_dev_info is used by superblocks of various pseudofilesystems such as kdevtmpfs. Initialize it before the filesystems get mounted.
Signed-off-by: Jan Kara jack@suse.cz
Tested-by: Guenter Roeck linux@roeck-us.net
Guenter
On Wed 15-06-22 11:00:26, Guenter Roeck wrote:
On Wed, Jun 15, 2022 at 03:38:45PM +0200, Jan Kara wrote:
On Wed 15-06-22 13:00:22, Jan Kara wrote:
On Tue 14-06-22 12:00:22, Linus Torvalds wrote:
On Tue, Jun 14, 2022 at 11:51 AM Linus Torvalds torvalds@linux-foundation.org wrote:
Or just make sure that noop_backing_dev_info is fully initialized before it's used.
I don't see any real reason why that
err = bdi_init(&noop_backing_dev_info);couldn't just be done very early. Maybe as the first call in driver_init(), before the whole devtmpfs_init() etc.
I've checked the dependencies and cgroups (which are the only non-trivial dependency besides per-CPU infrastructure) are initialized early enough so it should work fine. So let's try that.
Attached patch boots for me. Guys, who was able to reproduce the failure: Can you please confirm this patch fixes your problem?
It does for me.
Thanks for confirmation! I'll send the patch with proper tags etc. and also push it to Linus if nobody objects.
Honza
On 15/06/2022 14:38, Jan Kara wrote:
On Wed 15-06-22 13:00:22, Jan Kara wrote:
On Tue 14-06-22 12:00:22, Linus Torvalds wrote:
On Tue, Jun 14, 2022 at 11:51 AM Linus Torvalds torvalds@linux-foundation.org wrote:
Or just make sure that noop_backing_dev_info is fully initialized before it's used.
I don't see any real reason why that
err = bdi_init(&noop_backing_dev_info);couldn't just be done very early. Maybe as the first call in driver_init(), before the whole devtmpfs_init() etc.
I've checked the dependencies and cgroups (which are the only non-trivial dependency besides per-CPU infrastructure) are initialized early enough so it should work fine. So let's try that.
Attached patch boots for me. Guys, who was able to reproduce the failure: Can you please confirm this patch fixes your problem?
Honza
Works for me too
Tested-by: Suzuki K Poulose suzuki.poulose@arm.com
On Tue 14-06-22 11:51:35, Linus Torvalds wrote:
On Tue, Jun 14, 2022 at 11:20 AM Thomas Backlund tmb@tmb.nu wrote:
I "think" this is the suggested fix:
https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git/commit/?h=...
Ugh, this is just too ugly for words.
That's not a fix. That's a "hide the problem" patch.
I agree it is papering over the real problem. I consider that a stopgap solution so that machines can boot until we find a cleaner solution.
Now, admittedly clearly the "hide the problem" code already existed, and was just moved earlier, but I really think this whole "we're calling __mark_inode_dirty() on an inode that isn't even *initialized* yet" is a much deeper issue, and shouldn't have some hacky work-around in __mark_inode_dirty() that just happens to make it work.
I don't mind that patch per se - moving the code is fine.
But I *do* mind the patch when the reason is to hide that wrong ordering of operations.
Now, maybe a proper fix might be to say that new_inode_pseudo() should always initialize i_state to I_DIRTY_ALL or something like that. The comment already says that they cannot participate in writeback, so maybe they should be disabled that way (ie a pseudo inode is always dirty and marking it dirty does nothing).
Sadly it is not so simple. Firstly, new_inode_pseudo() gets used for all inodes (through new_inode()), secondly, tmpfs allocates fully standard inodes through new_inode() as any other filesystem. We could check writeback capabilities of the sb->s_bdi in new_inode_pseudo() but that would not work for inodes that will become block device inodes because blockdev_superblock has noop_backing_dev_info so we'd have to specialcase that. Overall it looks a bit hairy to my taste.
And then you get rid of the noop_backing_dev_info entirely.
And this would be even more difficult because there are other places that expect there's *some* bdi associated with each sb.
Or just make sure that noop_backing_dev_info is fully initialized before it's used.
Because I think the real problem here is that things have a pointer to an uninitialized backing_dev_info.
I fully agree with this. IMHO we need to initialize noop_backing_dev_info earlier but early init is not exactly my comfort zone so I have to verify whether various stuff in cgwb_bdi_init() is safe to call so early...
Honza
-
Guenter Roeck -
Jan Kara -
Linus Torvalds -
Suzuki K Poulose -
Thomas Backlund