Re: [f2fs-dev] circular locking dependency warning in f2fs

[Guenter Roeck <linux@roeck-us.net>]

On Wed, Aug 16, 2023 at 08:52:19PM -0700, Jaegeuk Kim wrote:
> On Wed, Aug 16, 2023 at 7:11 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On Wed, Aug 16, 2023 at 10:25:06AM -0700, Jaegeuk Kim wrote:
> > > Hi,
> > >
> > > On Tue, Aug 15, 2023 at 10:09 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > >
> > > > Hi,
> > > >
> > > > when trying to boot from an f2fs file system with lock debugging enabled,
> > > > I get the attached circular locking dependency warning. Is this a known
> > > > problem ?
> > > >
> > > > Thanks,
> > > > Guenter
> > > >
> > > > ---
> > > > [   10.315522] ======================================================
> > > > [   10.315620] WARNING: possible circular locking dependency detected
> > > > [   10.315753] 6.5.0-rc6-00027-g91aa6c412d7f #1 Tainted: G                 N
> > > > [   10.315843] ------------------------------------------------------
> > > > [   10.315922] seedrng/717 is trying to acquire lock:
> > > > [   10.316010] ffff8e0e02c6b868 (&fi->i_sem){+.+.}-{3:3}, at: f2fs_add_inline_entry+0x134/0x2d0
> > > > [   10.316315]
> > > > [   10.316315] but task is already holding lock:
> > > > [   10.316391] ffff8e0e02c6b278 (&fi->i_xattr_sem){.+.+}-{3:3}, at: f2fs_add_dentry+0x41/0xc0
> > > > [   10.316543]
> > > > [   10.316543] which lock already depends on the new lock.
> > > > [   10.316543]
> > > > [   10.316641]
> > > > [   10.316641] the existing dependency chain (in reverse order) is:
> > > > [   10.316745]
> > > > [   10.316745] -> #1 (&fi->i_xattr_sem){.+.+}-{3:3}:
> > > > [   10.316883]        down_read+0x3d/0x170
> > > > [   10.316973]        f2fs_getxattr+0x370/0x440
> > > > [   10.317036]        __f2fs_get_acl+0x38/0x1e0
> > > > [   10.317094]        f2fs_init_acl+0x69/0x420
> > > > [   10.317151]        f2fs_init_inode_metadata+0x72/0x450
> > > > [   10.317218]        f2fs_add_regular_entry+0x372/0x510
> > > > [   10.317283]        f2fs_add_dentry+0xb8/0xc0
> > > > [   10.317340]        f2fs_do_add_link+0xd9/0x110
> > > > [   10.317399]        f2fs_mkdir+0xdf/0x180
> > > > [   10.317453]        vfs_mkdir+0x142/0x220
> > > > [   10.317508]        do_mkdirat+0x7c/0x120
> > > > [   10.317561]        __x64_sys_mkdir+0x47/0x70
> > > > [   10.317617]        do_syscall_64+0x3f/0x90
> > > > [   10.317673]        entry_SYSCALL_64_after_hwframe+0x73/0xdd
> > > > [   10.317757]
> > > > [   10.317757] -> #0 (&fi->i_sem){+.+.}-{3:3}:
> > > > [   10.317843]        __lock_acquire+0x16bf/0x2860
> > > > [   10.317907]        lock_acquire+0xcc/0x2c0
> > > > [   10.317962]        down_write+0x3a/0xc0
> > > > [   10.318014]        f2fs_add_inline_entry+0x134/0x2d0
> > > > [   10.318077]        f2fs_add_dentry+0x55/0xc0
> > > > [   10.318141]        f2fs_do_add_link+0xd9/0x110
> > > > [   10.318201]        f2fs_create+0xe8/0x170
> > > > [   10.318255]        lookup_open.isra.0+0x58e/0x6c0
> > > > [   10.318317]        path_openat+0x2af/0xa40
> > > > [   10.318372]        do_filp_open+0xb1/0x160
> > > > [   10.318428]        do_sys_openat2+0x91/0xc0
> > > > [   10.318485]        __x64_sys_open+0x54/0xa0
> > > > [   10.318541]        do_syscall_64+0x3f/0x90
> > > > [   10.318597]        entry_SYSCALL_64_after_hwframe+0x73/0xdd
> > > > [   10.318676]
> > > > [   10.318676] other info that might help us debug this:
> > > > [   10.318676]
> > > > [   10.318799]  Possible unsafe locking scenario:
> > > > [   10.318799]
> > > > [   10.318875]        CPU0                    CPU1
> > > > [   10.318935]        ----                    ----
> > > > [   10.318999]   rlock(&fi->i_xattr_sem);
> > > > [   10.319068]                                lock(&fi->i_sem);
> > > > [   10.319166]                                lock(&fi->i_xattr_sem);
> > > > [   10.319264]   lock(&fi->i_sem);
> > >
> > > It looks like the same one reported by syzbot.
> > > https://syzkaller.appspot.com/bug?extid=a4976ce949df66b1ddf1
> > >
> > > However, I suspect it's a false alarm, since I'm interpreting the loop as below.
> > >
> > > CPU0                                         CPU1
> > > lock(new_inode#1->i_xattr_sem)
> > >                                                    lock(new_inode#2->i_sem)
> > >                                                    lock(dir->i_xattr_sem)
> > > lock(new_inode#1->i_sem)
> > >
> > > This looks fine to me.
> > >
> >
> > Based on your feedback, am I correct assuming that you don't plan
> > to fix this ?
> 
> I'm quite open to something that I may miss. Chao, what do you think?
> 

Just to give you a background: Recently I started boot testing from f2fs.
Apparently my test bed is the only one testing f2fs, meaning I was the
only one reporting that f2fs crashes in v6.1.y. I then extended my j2fs
test coverage to multiple architectures, only to discover that a recent
commit introduced this lockdep splatch.

My tests always run with lockdep debugging enabled. At the same time,
I can not tolerate persistent warnings because those would hide real
problems. As consequence, I disable any feature generating such warnings
if the reponsible maintainer(s) tell me that they won't fix it. In this
context, it doesn't matter if the warning is a false positive or not.

Ultimately I am quite neutral in this regard; it is up to you if you are
going to address the warning or not. I would just like to know if the
above warning is going to be fixed so I can adjust my test coverage
accordingly.

Side question: Does this possibly need down_read_nested() ?

Thanks,
Guenter


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel