Re: [PATCH 1/4] btrfs: use NOFS context when getting inodes during logging and log replay

[David Sterba <dsterba@suse.cz>]

On Fri, Jun 14, 2024 at 07:51:44AM +0930, Qu Wenruo wrote:
> >>>       do_iter_readv_writev+0x504/0x780 fs/read_write.c:741
> >>>       vfs_writev+0x36f/0xde0 fs/read_write.c:971
> >>>       do_pwritev+0x1b2/0x260 fs/read_write.c:1072
> >>>       __do_compat_sys_pwritev2 fs/read_write.c:1218 [inline]
> >>>       __se_compat_sys_pwritev2 fs/read_write.c:1210 [inline]
> >>>       __ia32_compat_sys_pwritev2+0x121/0x1b0 fs/read_write.c:1210
> >>>       do_syscall_32_irqs_on arch/x86/entry/common.c:165 [inline]
> >>>       __do_fast_syscall_32+0x73/0x120 arch/x86/entry/common.c:386
> >>>       do_fast_syscall_32+0x32/0x80 arch/x86/entry/common.c:411
> >>>       entry_SYSENTER_compat_after_hwframe+0x84/0x8e
> >>>      RIP: 0023:0xf7334579
> >>>      Code: b8 01 10 06 03 (...)
> >>>      RSP: 002b:00000000f5f265ac EFLAGS: 00000292 ORIG_RAX: 000000000000017b
> >>>      RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00000000200002c0
> >>>      RDX: 0000000000000001 RSI: 0000000000000000 RDI: 0000000000000000
> >>>      RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
> >>>      R10: 0000000000000000 R11: 0000000000000292 R12: 0000000000000000
> >>>      R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> >>>       </TASK>
> >>>
> >>> Fix this by ensuring we are under a NOFS scope whenever we call
> >>> btrfs_iget() during inode logging and log replay.
> >>>
> >>> Reported-by: syzbot+8576cfa84070dce4d59b@syzkaller.appspotmail.com
> >>> Link: https://lore.kernel.org/linux-btrfs/000000000000274a3a061abbd928@google.com/
> >>> Fixes: 712e36c5f2a7 ("btrfs: use GFP_KERNEL in btrfs_alloc_inode")
> >>
> >> I'm wondering if logging is the only location where we can trigger the
> >> deadlock.
> >>
> >> Would regular inode_get() causing such deadlock?
> >
> > What is inode_get()? I can't find anything with that exact name.
> 
> My bad, I mean iget().
> 
> >
> > If it's some path with a transaction handle open that can trigger
> > btrfs_alloc_inode() then yes, otherwise it depends on what locks are
> > held if any.
> >
> 
> Then would it be safer to revert the offending commit, aka make
> btrfs_alloc_inode() to use the old GFP_NOFS?
> 
> I just checked ext4_alloc_inode() and f2fs_alloc_inode(), both are still
> using NOFS instead.
> 
> Thus I'm wondering if it's really a good idea to go GFP_KERNEL in the
> first place.

In the long run we want to use the scoped NOFS and GFP_KERNEL. All easy
cases have been done (with occasional bugs like this patch fixes).