False lockdep completion splats with loop device

False lockdep completion splats with loop device

[Theodore Ts'o]

There are a ton of false lockdep splats that were introduced with
commit 2dcd5adfb740: "locking/lockdep: Remove the BROKEN flag from
CONFIG_LOCKDEP_CROSSRELEASE and CONFIG_LOCKDEP_COMPLETIONS".

I'm seeing this triggered by a number of xfstests tests, including
shared/298.  I see there was some discussion about this here:

  https://www.spinics.net/lists/linux-xfs/msg10832.html

... but I don't see any solution yet.  Is anyone working on this?

Failing that can we please make the completion lockdep something which
is optional since the false positives are incredibly !@#@!?! annoying?
That way I can still use most of lockdep, instead of being forced to
completely disable lockdep?

Thanks,

						- Ted

Re: False lockdep completion splats with loop device

[Byungchul Park]

On 12/5/2017 12:03 PM, Theodore Ts'o wrote:
> There are a ton of false lockdep splats that were introduced with
> commit 2dcd5adfb740: "locking/lockdep: Remove the BROKEN flag from
> CONFIG_LOCKDEP_CROSSRELEASE and CONFIG_LOCKDEP_COMPLETIONS".
> 
> I'm seeing this triggered by a number of xfstests tests, including
> shared/298.  I see there was some discussion about this here:
> 
>    https://www.spinics.net/lists/linux-xfs/msg10832.html
> 
> ... but I don't see any solution yet.  Is anyone working on this?
> 

Hello,

I believe that the commit e319e1fbd9d42 "block, locking/lockdep: Assign
a lock_class per gendisk used for wait_for_completion()" solved the
false positive.

Could you tell me if it doesn't handle it, with the report? Then, I
will follow up and try to solve it.

> Failing that can we please make the completion lockdep something which
> is optional since the false positives are incredibly !@#@!?! annoying?
> That way I can still use most of lockdep, instead of being forced to
> completely disable lockdep?
> 
> Thanks,
> 
> 						- Ted
> 

-- 
Thanks,
Byungchul

Re: False lockdep completion splats with loop device

[Theodore Ts'o]

On Tue, Dec 05, 2017 at 02:16:45PM +0900, Byungchul Park wrote:
> 
> Hello,
> 
> I believe that the commit e319e1fbd9d42 "block, locking/lockdep: Assign
> a lock_class per gendisk used for wait_for_completion()" solved the
> false positive.
> 
> Could you tell me if it doesn't handle it, with the report? Then, I
> will follow up and try to solve it.

No, it doesn't handle it.  And there was some discussion in the linked
thread on the xfs mailing list that seemed to indicate that it was not
a complete fix.

The following is a run of shared/298[1] using the dev branch of ext4.git
on kernel.org.  The kvm-xfstests appliance can be found at [2], with
instructions on how to use it at [3].

[1] https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/tree/tests/shared/298
[2] https://www.kernel.org/pub/linux/kernel/people/tytso/kvm-xfstests
[3] https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md

							- Ted

KERNEL: kernel	4.15.0-rc2-xfstests-00002-ge243560f12d7 #439 SMP Tue Dec 5 09:30:08 EST 2017 i686
FSTESTVER: e2fsprogs	v1.43.6-85-g7595699d0 (Wed, 6 Sep 2017 22:04:14 -0400)
FSTESTVER: fio		fio-3.2 (Fri, 3 Nov 2017 15:23:49 -0600)
FSTESTVER: quota		4d81e8b (Mon, 16 Oct 2017 09:42:44 +0200)
FSTESTVER: stress-ng	977ae35 (Wed, 6 Sep 2017 23:45:03 -0400)
FSTESTVER: xfsprogs	v4.14.0-rc2-1-g19ca9b0b (Mon, 27 Nov 2017 10:56:21 -0600)
FSTESTVER: xfstests-bld	0b27af9 (Tue, 28 Nov 2017 16:28:51 -0500)
FSTESTVER: xfstests	linux-v3.8-1797-g4f1eaa02 (Tue, 28 Nov 2017 15:49:06 -0500)
FSTESTCFG: "4k"
FSTESTSET: "shared/298"
FSTESTEXC: ""
FSTESTOPT: "aex"
MNTOPTS: ""
CPUS: "2"
MEM: "2008.27"
              total        used        free      shared  buff/cache   available
Mem:           2008          29        1926           8          51        1904
Swap:             0           0           0
BEGIN TEST 4k (1 test): Ext4 4k block Tue Dec  5 10:01:43 EST 2017
DEVICE: /dev/vdd
EXT_MKFS_OPTIONS: -b 4096
EXT_MOUNT_OPTIONS: -o block_validity
FSTYP         -- ext4
PLATFORM      -- Linux/i686 kvm-xfstests 4.15.0-rc2-xfstests-00002-ge243560f12d7
MKFS_OPTIONS  -- -b 4096 /dev/vdc
MOUNT_OPTIONS -- -o acl,user_xattr -o block_validity /dev/vdc /vdc

shared/298 7s ...	[10:01:45][   11.697717] run fstests shared/298 at 2017-12-05 10:01:45
[   15.151631] EXT4-fs (loop0): mounted filesystem with ordered data mode. Opts: (null)
[   16.183394] 
[   16.184011] ======================================================
[   16.186040] WARNING: possible circular locking dependency detected
[   16.187693] 4.15.0-rc2-xfstests-00002-ge243560f12d7 #439 Not tainted
[   16.189044] ------------------------------------------------------
[   16.190198] ext4lazyinit/648 is trying to acquire lock:
[   16.191201]  ((gendisk_completion)1 << part_shift(NUMA_NO_NODE)){+.+.}, at: [<8a1ebe9d>] wait_for_completion_io+0x12/0x20
[   16.193226] 
[   16.193226] but task is already holding lock:
[   16.194316]  (&meta_group_info[i]->alloc_sem){++++}, at: [<6495768c>] ext4_init_inode_table+0xb3/0x340
[   16.196090] 
[   16.196090] which lock already depends on the new lock.
[   16.196090] 
[   16.197603] 
[   16.197603] the existing dependency chain (in reverse order) is:
[   16.198989] 
[   16.198989] -> #2 (&meta_group_info[i]->alloc_sem){++++}:
[   16.200270]        lock_acquire+0x7d/0x1a0
[   16.201067]        down_read+0x34/0x90
[   16.201783]        __ext4_new_inode+0xcc2/0x14b0
[   16.202853]        ext4_create+0x8f/0x170
[   16.203613]        lookup_open+0x284/0x600
[   16.204409]        path_openat+0x33a/0xa40
[   16.205230]        do_filp_open+0x5e/0xb0
[   16.205990]        do_sys_open+0x1a4/0x230
[   16.206766]        SyS_open+0x1d/0x20
[   16.207459]        do_fast_syscall_32+0x85/0x2d0
[   16.208326]        entry_SYSENTER_32+0x4c/0x7b
[   16.209161] 
[   16.209161] -> #1 (jbd2_handle){++++}:
[   16.210142]        page_address+0x69/0xd0
[   16.211048]        copy_page_to_iter+0x32/0x3e0
[   16.212524]        generic_file_buffered_read+0x308/0x760
[   16.214279]        generic_file_read_iter+0x15e/0x1f0
[   16.215957]        ext4_file_read_iter+0x43/0xe0
[   16.217478] 
[   16.217478] -> #0 ((gendisk_completion)1 << part_shift(NUMA_NO_NODE)){+.+.}:
[   16.220266]        __lock_acquire+0x1235/0x1300
[   16.221758]        lock_acquire+0x7d/0x1a0
[   16.223102]        wait_for_common_io.constprop.0+0x3e/0x150
[   16.224893]        wait_for_completion_io+0x12/0x20
[   16.225793]        submit_bio_wait+0x6f/0x80
[   16.226574]        blkdev_issue_zeroout+0xa6/0x1e0
[   16.227445]        ext4_init_inode_table+0x173/0x340
[   16.228347]        ext4_lazyinit_thread+0x263/0x320
[   16.229233]        kthread+0x12e/0x150
[   16.229919]        ret_from_fork+0x19/0x24
[   16.230665] 
[   16.230665] other info that might help us debug this:
[   16.230665] 
[   16.232104] Chain exists of:
[   16.232104]   (gendisk_completion)1 << part_shift(NUMA_NO_NODE) --> jbd2_handle --> &meta_group_info[i]->alloc_sem
[   16.232104] 
[   16.234726]  Possible unsafe locking scenario:
[   16.234726] 
[   16.236617]        CPU0                    CPU1
[   16.238109]        ----                    ----
[   16.239598]   lock(&meta_group_info[i]->alloc_sem);
[   16.241208]                                lock(jbd2_handle);
[   16.243061]                                lock(&meta_group_info[i]->alloc_sem);
[   16.245496]   lock((gendisk_completion)1 << part_shift(NUMA_NO_NODE));
[   16.247642] 
[   16.247642]  *** DEADLOCK ***
[   16.247642] 
[   16.249587] 4 locks held by ext4lazyinit/648:
[   16.250970]  #0:  (&type->s_umount_key#26){++++}, at: [<8c13b129>] ext4_lazyinit_thread+0x6b/0x320
[   16.252630]  #1:  (sb_writers#3){.+.+}, at: [<4cd23367>] ext4_lazyinit_thread+0x14f/0x320
[   16.254109]  #2:  (jbd2_handle){++++}, at: [<327c6fca>] start_this_handle+0x303/0x790
[   16.256235]  #3:  (&meta_group_info[i]->alloc_sem){++++}, at: [<6495768c>] ext4_init_inode_table+0xb3/0x340
[   16.259407] 
[   16.259407] stack backtrace:
[   16.260839] CPU: 0 PID: 648 Comm: ext4lazyinit Not tainted 4.15.0-rc2-xfstests-00002-ge243560f12d7 #439
[   16.263789] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
[   16.266438] Call Trace:
[   16.267068]  dump_stack+0x7d/0xb2
[   16.267685]  print_circular_bug+0x1d2/0x340
[   16.268453]  check_prev_add+0x310/0x6b0
[   16.269161]  ? kvm_sched_clock_read+0x1f/0x40
[   16.269961]  ? sched_clock+0x9/0x10
[   16.270607]  __lock_acquire+0x1235/0x1300
[   16.271345]  ? copy_trace+0x90/0x90
[   16.272131]  lock_acquire+0x7d/0x1a0
[   16.273269]  ? wait_for_completion_io+0x12/0x20
[   16.274777]  wait_for_common_io.constprop.0+0x3e/0x150
[   16.276452]  ? wait_for_completion_io+0x12/0x20
[   16.277933]  ? lockdep_init_map+0x12/0x20
[   16.279246]  wait_for_completion_io+0x12/0x20
[   16.280682]  submit_bio_wait+0x6f/0x80
[   16.281910]  ? _cond_resched+0x17/0x30
[   16.283150]  ? blkdev_issue_zeroout+0x91/0x1e0
[   16.284576]  blkdev_issue_zeroout+0xa6/0x1e0
[   16.285976]  ext4_init_inode_table+0x173/0x340
[   16.287434]  ext4_lazyinit_thread+0x263/0x320
[   16.288858]  kthread+0x12e/0x150
[   16.289936]  ? ext4_unregister_li_request.isra.89+0x60/0x60
[   16.291770]  ? __kthread_create_on_node+0x170/0x170
[   16.293370]  ret_from_fork+0x19/0x24
 [10:01:55] 10s
[   21.970265] EXT4-fs (vdd): mounted filesystem with ordered data mode. Opts: acl,user_xattr,block_validity
_check_dmesg: something found in dmesg (see /results/ext4/results-4k/shared/298.dmesg)
Ran: shared/298
Failures: shared/298
Failed 1 of 1 tests
Xunit report: /results/ext4/results-4k/result.xml

[   22.876380] runtests.sh (230): drop_caches: 3
              total        used        free      shared  buff/cache   available
Mem:           2008          35        1939           8          33        1899
Swap:             0           0           0
END TEST: Ext4 4k block Tue Dec  5 10:01:56 EST 2017

Re: False lockdep completion splats with loop device

[Dave Chinner]

On Tue, Dec 05, 2017 at 10:07:41AM -0500, Theodore Ts'o wrote:
> On Tue, Dec 05, 2017 at 02:16:45PM +0900, Byungchul Park wrote:
> > 
> > Hello,
> > 
> > I believe that the commit e319e1fbd9d42 "block, locking/lockdep: Assign
> > a lock_class per gendisk used for wait_for_completion()" solved the
> > false positive.
> > 
> > Could you tell me if it doesn't handle it, with the report? Then, I
> > will follow up and try to solve it.
> 
> No, it doesn't handle it.  And there was some discussion in the linked
> thread on the xfs mailing list that seemed to indicate that it was not
> a complete fix.

Well, it uses a static key hidden inside the macro
alloc_disk_node(), so every disk allocated from the same callsite
points to the same static lockdep map. IOWs, every caller of
alloc_disk() (i.e. the vast majority of storage devices) are
configured to point at the same static lockdep map, regardless of
their location in the storage stack.

The loop device uses alloc_disk().

IOWs, it doesn't address the problem of false positives due to
layering in the IO stack at all because we can still have
filesystems both above and below the lockdep map that has been
attached to the devices...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: False lockdep completion splats with loop device

[Byungchul Park]

On 12/6/2017 6:09 AM, Dave Chinner wrote:
> On Tue, Dec 05, 2017 at 10:07:41AM -0500, Theodore Ts'o wrote:
>> On Tue, Dec 05, 2017 at 02:16:45PM +0900, Byungchul Park wrote:
>>>
>>> Hello,
>>>
>>> I believe that the commit e319e1fbd9d42 "block, locking/lockdep: Assign
>>> a lock_class per gendisk used for wait_for_completion()" solved the
>>> false positive.
>>>
>>> Could you tell me if it doesn't handle it, with the report? Then, I
>>> will follow up and try to solve it.
>>
>> No, it doesn't handle it.  And there was some discussion in the linked
>> thread on the xfs mailing list that seemed to indicate that it was not
>> a complete fix.
> 
> Well, it uses a static key hidden inside the macro
> alloc_disk_node(), so every disk allocated from the same callsite
> points to the same static lockdep map. IOWs, every caller of
> alloc_disk() (i.e. the vast majority of storage devices) are
> configured to point at the same static lockdep map, regardless of
> their location in the storage stack.
> 
> The loop device uses alloc_disk().
> 
> IOWs, it doesn't address the problem of false positives due to
> layering in the IO stack at all because we can still have
> filesystems both above and below the lockdep map that has been
> attached to the devices...

Hello,

What if we assign different classes to different loop devices? Do
you think it helps to avoid such false positives?

I'm not sure since I'm unfamiliar with the stacking stuff. It would
be appriciated if you let me know your opinion about it.

-- 
Thanks,
Byungchul

Re: False lockdep completion splats with loop device

[Dave Chinner]

On Wed, Dec 06, 2017 at 02:01:51PM +0900, Byungchul Park wrote:
> On 12/6/2017 6:09 AM, Dave Chinner wrote:
> >On Tue, Dec 05, 2017 at 10:07:41AM -0500, Theodore Ts'o wrote:
> >>On Tue, Dec 05, 2017 at 02:16:45PM +0900, Byungchul Park wrote:
> >>>
> >>>Hello,
> >>>
> >>>I believe that the commit e319e1fbd9d42 "block, locking/lockdep: Assign
> >>>a lock_class per gendisk used for wait_for_completion()" solved the
> >>>false positive.
> >>>
> >>>Could you tell me if it doesn't handle it, with the report? Then, I
> >>>will follow up and try to solve it.
> >>
> >>No, it doesn't handle it.  And there was some discussion in the linked
> >>thread on the xfs mailing list that seemed to indicate that it was not
> >>a complete fix.
> >
> >Well, it uses a static key hidden inside the macro
> >alloc_disk_node(), so every disk allocated from the same callsite
> >points to the same static lockdep map. IOWs, every caller of
> >alloc_disk() (i.e. the vast majority of storage devices) are
> >configured to point at the same static lockdep map, regardless of
> >their location in the storage stack.
> >
> >The loop device uses alloc_disk().
> >
> >IOWs, it doesn't address the problem of false positives due to
> >layering in the IO stack at all because we can still have
> >filesystems both above and below the lockdep map that has been
> >attached to the devices...
> 
> Hello,
> 
> What if we assign different classes to different loop devices? Do
> you think it helps to avoid such false positives?

It might, but that doesn't address all the other types of device
that can also be stacked in arbitrary orders, too. Loop devices are
just the obvious, easy to blame messenger.

As I said right at the start: this is a generic device layering
issue, not a loop device specific issue.

> I'm not sure since I'm unfamiliar with the stacking stuff. It would
> be appriciated if you let me know your opinion about it.

I don't have an answer to the problem, and I haven't spent any time
trying to think one up - I rarely use lockdep because I only ever
get false positives from it. I've got other problems I need to spend
my time on...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: False lockdep completion splats with loop device

[Byungchul Park]

On 12/6/2017 12:07 AM, Theodore Ts'o wrote:
> On Tue, Dec 05, 2017 at 02:16:45PM +0900, Byungchul Park wrote:
>>
>> Hello,
>>
>> I believe that the commit e319e1fbd9d42 "block, locking/lockdep: Assign
>> a lock_class per gendisk used for wait_for_completion()" solved the
>> false positive.
>>
>> Could you tell me if it doesn't handle it, with the report? Then, I
>> will follow up and try to solve it.
> 
> No, it doesn't handle it.  And there was some discussion in the linked
> thread on the xfs mailing list that seemed to indicate that it was not
> a complete fix.
> 
> The following is a run of shared/298[1] using the dev branch of ext4.git
> on kernel.org.  The kvm-xfstests appliance can be found at [2], with
> instructions on how to use it at [3].
> 
> [1] https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/tree/tests/shared/298
> [2] https://www.kernel.org/pub/linux/kernel/people/tytso/kvm-xfstests
> [3] https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md

OK, I will check it.

Thanks for informing the way to test.

> 
> 							- Ted
> 
> KERNEL: kernel	4.15.0-rc2-xfstests-00002-ge243560f12d7 #439 SMP Tue Dec 5 09:30:08 EST 2017 i686
> FSTESTVER: e2fsprogs	v1.43.6-85-g7595699d0 (Wed, 6 Sep 2017 22:04:14 -0400)
> FSTESTVER: fio		fio-3.2 (Fri, 3 Nov 2017 15:23:49 -0600)
> FSTESTVER: quota		4d81e8b (Mon, 16 Oct 2017 09:42:44 +0200)
> FSTESTVER: stress-ng	977ae35 (Wed, 6 Sep 2017 23:45:03 -0400)
> FSTESTVER: xfsprogs	v4.14.0-rc2-1-g19ca9b0b (Mon, 27 Nov 2017 10:56:21 -0600)
> FSTESTVER: xfstests-bld	0b27af9 (Tue, 28 Nov 2017 16:28:51 -0500)
> FSTESTVER: xfstests	linux-v3.8-1797-g4f1eaa02 (Tue, 28 Nov 2017 15:49:06 -0500)
> FSTESTCFG: "4k"
> FSTESTSET: "shared/298"
> FSTESTEXC: ""
> FSTESTOPT: "aex"
> MNTOPTS: ""
> CPUS: "2"
> MEM: "2008.27"
>                total        used        free      shared  buff/cache   available
> Mem:           2008          29        1926           8          51        1904
> Swap:             0           0           0
> BEGIN TEST 4k (1 test): Ext4 4k block Tue Dec  5 10:01:43 EST 2017
> DEVICE: /dev/vdd
> EXT_MKFS_OPTIONS: -b 4096
> EXT_MOUNT_OPTIONS: -o block_validity
> FSTYP         -- ext4
> PLATFORM      -- Linux/i686 kvm-xfstests 4.15.0-rc2-xfstests-00002-ge243560f12d7
> MKFS_OPTIONS  -- -b 4096 /dev/vdc
> MOUNT_OPTIONS -- -o acl,user_xattr -o block_validity /dev/vdc /vdc
> 
> shared/298 7s ...	[10:01:45][   11.697717] run fstests shared/298 at 2017-12-05 10:01:45
> [   15.151631] EXT4-fs (loop0): mounted filesystem with ordered data mode. Opts: (null)
> [   16.183394]
> [   16.184011] ======================================================
> [   16.186040] WARNING: possible circular locking dependency detected
> [   16.187693] 4.15.0-rc2-xfstests-00002-ge243560f12d7 #439 Not tainted
> [   16.189044] ------------------------------------------------------
> [   16.190198] ext4lazyinit/648 is trying to acquire lock:
> [   16.191201]  ((gendisk_completion)1 << part_shift(NUMA_NO_NODE)){+.+.}, at: [<8a1ebe9d>] wait_for_completion_io+0x12/0x20
> [   16.193226]
> [   16.193226] but task is already holding lock:
> [   16.194316]  (&meta_group_info[i]->alloc_sem){++++}, at: [<6495768c>] ext4_init_inode_table+0xb3/0x340
> [   16.196090]
> [   16.196090] which lock already depends on the new lock.
> [   16.196090]
> [   16.197603]
> [   16.197603] the existing dependency chain (in reverse order) is:
> [   16.198989]
> [   16.198989] -> #2 (&meta_group_info[i]->alloc_sem){++++}:
> [   16.200270]        lock_acquire+0x7d/0x1a0
> [   16.201067]        down_read+0x34/0x90
> [   16.201783]        __ext4_new_inode+0xcc2/0x14b0
> [   16.202853]        ext4_create+0x8f/0x170
> [   16.203613]        lookup_open+0x284/0x600
> [   16.204409]        path_openat+0x33a/0xa40
> [   16.205230]        do_filp_open+0x5e/0xb0
> [   16.205990]        do_sys_open+0x1a4/0x230
> [   16.206766]        SyS_open+0x1d/0x20
> [   16.207459]        do_fast_syscall_32+0x85/0x2d0
> [   16.208326]        entry_SYSENTER_32+0x4c/0x7b
> [   16.209161]
> [   16.209161] -> #1 (jbd2_handle){++++}:
> [   16.210142]        page_address+0x69/0xd0
> [   16.211048]        copy_page_to_iter+0x32/0x3e0
> [   16.212524]        generic_file_buffered_read+0x308/0x760
> [   16.214279]        generic_file_read_iter+0x15e/0x1f0
> [   16.215957]        ext4_file_read_iter+0x43/0xe0
> [   16.217478]
> [   16.217478] -> #0 ((gendisk_completion)1 << part_shift(NUMA_NO_NODE)){+.+.}:
> [   16.220266]        __lock_acquire+0x1235/0x1300
> [   16.221758]        lock_acquire+0x7d/0x1a0
> [   16.223102]        wait_for_common_io.constprop.0+0x3e/0x150
> [   16.224893]        wait_for_completion_io+0x12/0x20
> [   16.225793]        submit_bio_wait+0x6f/0x80
> [   16.226574]        blkdev_issue_zeroout+0xa6/0x1e0
> [   16.227445]        ext4_init_inode_table+0x173/0x340
> [   16.228347]        ext4_lazyinit_thread+0x263/0x320
> [   16.229233]        kthread+0x12e/0x150
> [   16.229919]        ret_from_fork+0x19/0x24
> [   16.230665]
> [   16.230665] other info that might help us debug this:
> [   16.230665]
> [   16.232104] Chain exists of:
> [   16.232104]   (gendisk_completion)1 << part_shift(NUMA_NO_NODE) --> jbd2_handle --> &meta_group_info[i]->alloc_sem
> [   16.232104]
> [   16.234726]  Possible unsafe locking scenario:
> [   16.234726]
> [   16.236617]        CPU0                    CPU1
> [   16.238109]        ----                    ----
> [   16.239598]   lock(&meta_group_info[i]->alloc_sem);
> [   16.241208]                                lock(jbd2_handle);
> [   16.243061]                                lock(&meta_group_info[i]->alloc_sem);
> [   16.245496]   lock((gendisk_completion)1 << part_shift(NUMA_NO_NODE));
> [   16.247642]
> [   16.247642]  *** DEADLOCK ***
> [   16.247642]
> [   16.249587] 4 locks held by ext4lazyinit/648:
> [   16.250970]  #0:  (&type->s_umount_key#26){++++}, at: [<8c13b129>] ext4_lazyinit_thread+0x6b/0x320
> [   16.252630]  #1:  (sb_writers#3){.+.+}, at: [<4cd23367>] ext4_lazyinit_thread+0x14f/0x320
> [   16.254109]  #2:  (jbd2_handle){++++}, at: [<327c6fca>] start_this_handle+0x303/0x790
> [   16.256235]  #3:  (&meta_group_info[i]->alloc_sem){++++}, at: [<6495768c>] ext4_init_inode_table+0xb3/0x340
> [   16.259407]
> [   16.259407] stack backtrace:
> [   16.260839] CPU: 0 PID: 648 Comm: ext4lazyinit Not tainted 4.15.0-rc2-xfstests-00002-ge243560f12d7 #439
> [   16.263789] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
> [   16.266438] Call Trace:
> [   16.267068]  dump_stack+0x7d/0xb2
> [   16.267685]  print_circular_bug+0x1d2/0x340
> [   16.268453]  check_prev_add+0x310/0x6b0
> [   16.269161]  ? kvm_sched_clock_read+0x1f/0x40
> [   16.269961]  ? sched_clock+0x9/0x10
> [   16.270607]  __lock_acquire+0x1235/0x1300
> [   16.271345]  ? copy_trace+0x90/0x90
> [   16.272131]  lock_acquire+0x7d/0x1a0
> [   16.273269]  ? wait_for_completion_io+0x12/0x20
> [   16.274777]  wait_for_common_io.constprop.0+0x3e/0x150
> [   16.276452]  ? wait_for_completion_io+0x12/0x20
> [   16.277933]  ? lockdep_init_map+0x12/0x20
> [   16.279246]  wait_for_completion_io+0x12/0x20
> [   16.280682]  submit_bio_wait+0x6f/0x80
> [   16.281910]  ? _cond_resched+0x17/0x30
> [   16.283150]  ? blkdev_issue_zeroout+0x91/0x1e0
> [   16.284576]  blkdev_issue_zeroout+0xa6/0x1e0
> [   16.285976]  ext4_init_inode_table+0x173/0x340
> [   16.287434]  ext4_lazyinit_thread+0x263/0x320
> [   16.288858]  kthread+0x12e/0x150
> [   16.289936]  ? ext4_unregister_li_request.isra.89+0x60/0x60
> [   16.291770]  ? __kthread_create_on_node+0x170/0x170
> [   16.293370]  ret_from_fork+0x19/0x24
>   [10:01:55] 10s
> [   21.970265] EXT4-fs (vdd): mounted filesystem with ordered data mode. Opts: acl,user_xattr,block_validity
> _check_dmesg: something found in dmesg (see /results/ext4/results-4k/shared/298.dmesg)
> Ran: shared/298
> Failures: shared/298
> Failed 1 of 1 tests
> Xunit report: /results/ext4/results-4k/result.xml
> 
> [   22.876380] runtests.sh (230): drop_caches: 3
>                total        used        free      shared  buff/cache   available
> Mem:           2008          35        1939           8          33        1899
> Swap:             0           0           0
> END TEST: Ext4 4k block Tue Dec  5 10:01:56 EST 2017
> 

-- 
Thanks,
Byungchul

Re: False lockdep completion splats with loop device

[Amir Goldstein]

On Wed, Dec 6, 2017 at 7:01 AM, Byungchul Park <byungchul.park@lge.com> wrote:
> On 12/6/2017 6:09 AM, Dave Chinner wrote:
>>
>> On Tue, Dec 05, 2017 at 10:07:41AM -0500, Theodore Ts'o wrote:
>>>
>>> On Tue, Dec 05, 2017 at 02:16:45PM +0900, Byungchul Park wrote:
>>>>
>>>>
>>>> Hello,
>>>>
>>>> I believe that the commit e319e1fbd9d42 "block, locking/lockdep: Assign
>>>> a lock_class per gendisk used for wait_for_completion()" solved the
>>>> false positive.
>>>>
>>>> Could you tell me if it doesn't handle it, with the report? Then, I
>>>> will follow up and try to solve it.
>>>
>>>
>>> No, it doesn't handle it.  And there was some discussion in the linked
>>> thread on the xfs mailing list that seemed to indicate that it was not
>>> a complete fix.
>>
>>
>> Well, it uses a static key hidden inside the macro
>> alloc_disk_node(), so every disk allocated from the same callsite
>> points to the same static lockdep map. IOWs, every caller of
>> alloc_disk() (i.e. the vast majority of storage devices) are
>> configured to point at the same static lockdep map, regardless of
>> their location in the storage stack.
>>
>> The loop device uses alloc_disk().
>>
>> IOWs, it doesn't address the problem of false positives due to
>> layering in the IO stack at all because we can still have
>> filesystems both above and below the lockdep map that has been
>> attached to the devices...
>
>
> Hello,
>
> What if we assign different classes to different loop devices? Do
> you think it helps to avoid such false positives?
>
> I'm not sure since I'm unfamiliar with the stacking stuff. It would
> be appriciated if you let me know your opinion about it.
>

I believe the problem in the tests is that loop is mistaken for the same
class/key as another (underlying) blockdev, where the fs of the loop file
resides.

For the first order of resolution, it would be enough to assign a different
key to different types of block devices, similar to the struct file_system_type
lock class keys.

This would be enough to cover to common case of ext4 over loop inside
ext4 over real disk (at least for the completion lockdep warnings).

For the second order of resolution it may be desired to assign a different
key/class to an instance of loop devices and other stackable block devices
(e.g. dm, nbd)  to cover the case of a deeper nested blockdev stack,
such as: ext4 over loop inside ext4 over loop inside ext4 over real disk.

That should be enough to cover the completion lockdep validation,
but it still leaves to problem of the file_system_type lock key, which
is still the same for the 2 instances of ext4 in the stack above.
For that I suggested a solution similar to
b1eaa950f7e9 ovl: lockdep annotate of nested stacked overlayfs inode lock
but that will require propagating a "bdev_stack_depth" throughout the
block and fs stack and increment it when creating loop or nbd device.

Cheers,
Amir.

Re: False lockdep completion splats with loop device

[Byungchul Park]

On 12/6/2017 3:31 PM, Amir Goldstein wrote:
> On Wed, Dec 6, 2017 at 7:01 AM, Byungchul Park <byungchul.park@lge.com> wrote:
>> On 12/6/2017 6:09 AM, Dave Chinner wrote:
>>>
>>> On Tue, Dec 05, 2017 at 10:07:41AM -0500, Theodore Ts'o wrote:
>>>>
>>>> On Tue, Dec 05, 2017 at 02:16:45PM +0900, Byungchul Park wrote:
>>>>>
>>>>>
>>>>> Hello,
>>>>>
>>>>> I believe that the commit e319e1fbd9d42 "block, locking/lockdep: Assign
>>>>> a lock_class per gendisk used for wait_for_completion()" solved the
>>>>> false positive.
>>>>>
>>>>> Could you tell me if it doesn't handle it, with the report? Then, I
>>>>> will follow up and try to solve it.
>>>>
>>>>
>>>> No, it doesn't handle it.  And there was some discussion in the linked
>>>> thread on the xfs mailing list that seemed to indicate that it was not
>>>> a complete fix.
>>>
>>>
>>> Well, it uses a static key hidden inside the macro
>>> alloc_disk_node(), so every disk allocated from the same callsite
>>> points to the same static lockdep map. IOWs, every caller of
>>> alloc_disk() (i.e. the vast majority of storage devices) are
>>> configured to point at the same static lockdep map, regardless of
>>> their location in the storage stack.
>>>
>>> The loop device uses alloc_disk().
>>>
>>> IOWs, it doesn't address the problem of false positives due to
>>> layering in the IO stack at all because we can still have
>>> filesystems both above and below the lockdep map that has been
>>> attached to the devices...
>>
>>
>> Hello,
>>
>> What if we assign different classes to different loop devices? Do
>> you think it helps to avoid such false positives?
>>
>> I'm not sure since I'm unfamiliar with the stacking stuff. It would
>> be appriciated if you let me know your opinion about it.
>>

Hello Amir,

Thanks a lot for your opinion! See the below.

> I believe the problem in the tests is that loop is mistaken for the same
> class/key as another (underlying) blockdev, where the fs of the loop file
> resides.
> 
> For the first order of resolution, it would be enough to assign a different
> key to different types of block devices, similar to the struct file_system_type
> lock class keys.
> 
> This would be enough to cover to common case of ext4 over loop inside
> ext4 over real disk (at least for the completion lockdep warnings).

This was already done by the commit e319e1fbd9d42 "block,
locking/lockdep: Assign a lock_class per gendisk used for
wait_for_completion()".

> For the second order of resolution it may be desired to assign a different
> key/class to an instance of loop devices and other stackable block devices
> (e.g. dm, nbd)  to cover the case of a deeper nested blockdev stack,
> such as: ext4 over loop inside ext4 over loop inside ext4 over real disk.

This is what I should do.

> That should be enough to cover the completion lockdep validation,
> but it still leaves to problem of the file_system_type lock key, which
> is still the same for the 2 instances of ext4 in the stack above.
> For that I suggested a solution similar to
> b1eaa950f7e9 ovl: lockdep annotate of nested stacked overlayfs inode lock
> but that will require propagating a "bdev_stack_depth" throughout the
> block and fs stack and increment it when creating loop or nbd device.
> 
> Cheers,
> Amir.
> 

-- 
Thanks,
Byungchul

Re: False lockdep completion splats with loop device

[Amir Goldstein]

On Wed, Dec 6, 2017 at 9:01 AM, Byungchul Park <byungchul.park@lge.com> wrote:
> On 12/6/2017 3:31 PM, Amir Goldstein wrote:
>>
>> On Wed, Dec 6, 2017 at 7:01 AM, Byungchul Park <byungchul.park@lge.com>
>> wrote:
>>>
>>> On 12/6/2017 6:09 AM, Dave Chinner wrote:
>>>>
>>>>
>>>> On Tue, Dec 05, 2017 at 10:07:41AM -0500, Theodore Ts'o wrote:
>>>>>
>>>>>
>>>>> On Tue, Dec 05, 2017 at 02:16:45PM +0900, Byungchul Park wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> I believe that the commit e319e1fbd9d42 "block, locking/lockdep:
>>>>>> Assign
>>>>>> a lock_class per gendisk used for wait_for_completion()" solved the
>>>>>> false positive.
>>>>>>
>>>>>> Could you tell me if it doesn't handle it, with the report? Then, I
>>>>>> will follow up and try to solve it.
>>>>>
>>>>>
>>>>>
>>>>> No, it doesn't handle it.  And there was some discussion in the linked
>>>>> thread on the xfs mailing list that seemed to indicate that it was not
>>>>> a complete fix.
>>>>
>>>>
>>>>
>>>> Well, it uses a static key hidden inside the macro
>>>> alloc_disk_node(), so every disk allocated from the same callsite
>>>> points to the same static lockdep map. IOWs, every caller of
>>>> alloc_disk() (i.e. the vast majority of storage devices) are
>>>> configured to point at the same static lockdep map, regardless of
>>>> their location in the storage stack.
>>>>
>>>> The loop device uses alloc_disk().
>>>>
>>>> IOWs, it doesn't address the problem of false positives due to
>>>> layering in the IO stack at all because we can still have
>>>> filesystems both above and below the lockdep map that has been
>>>> attached to the devices...
>>>
>>>
>>>
>>> Hello,
>>>
>>> What if we assign different classes to different loop devices? Do
>>> you think it helps to avoid such false positives?
>>>
>>> I'm not sure since I'm unfamiliar with the stacking stuff. It would
>>> be appriciated if you let me know your opinion about it.
>>>
>
> Hello Amir,
>
> Thanks a lot for your opinion! See the below.
>
>> I believe the problem in the tests is that loop is mistaken for the same
>> class/key as another (underlying) blockdev, where the fs of the loop file
>> resides.
>>
>> For the first order of resolution, it would be enough to assign a
>> different
>> key to different types of block devices, similar to the struct
>> file_system_type
>> lock class keys.
>>
>> This would be enough to cover to common case of ext4 over loop inside
>> ext4 over real disk (at least for the completion lockdep warnings).
>
>
> This was already done by the commit e319e1fbd9d42 "block,
> locking/lockdep: Assign a lock_class per gendisk used for
> wait_for_completion()".
>

Right...

>> For the second order of resolution it may be desired to assign a different
>> key/class to an instance of loop devices and other stackable block devices
>> (e.g. dm, nbd)  to cover the case of a deeper nested blockdev stack,
>> such as: ext4 over loop inside ext4 over loop inside ext4 over real disk.
>
>
> This is what I should do.
>

But that won't help Ted. The test use case doesn't include doubly nested loop,
very few setups will do that. Maybe xfstests running within a container over
loop image. I doubt it that setup is common, so until there are nested container
implementations out there that are using loop image mount, this use case
is not very interesting.

>
>> That should be enough to cover the completion lockdep validation,
>> but it still leaves to problem of the file_system_type lock key, which
>> is still the same for the 2 instances of ext4 in the stack above.
>> For that I suggested a solution similar to
>> b1eaa950f7e9 ovl: lockdep annotate of nested stacked overlayfs inode lock
>> but that will require propagating a "bdev_stack_depth" throughout the
>> block and fs stack and increment it when creating loop or nbd device.
>>

So this in the problem Ted is reporting and it needs to be solved inside
ext4 and any other fs that can be loop mounted on a file inside same fs type.

The lockdep chain is: ext4/loop -> loop completion -> ext4/real
but all the internal ext4 locks (i.e. &meta_group_info[i]->alloc_sem) are
annotated statically for the ext4 fs type, so to fix that need:
1. ext4 to know it is mounted over loop/nbd
2. ext4 to annotate all internal locks that surround blockdev I/O as
    "loop nested"/"not loop nested" like overlayfs does with the vfs locks
3. if multi nested looped fs is important (not really) then loop/nbd will
    need to know if its file is on a looped fs and propagate nesting level
    to ext4

Cheers,
Amir.

Re: False lockdep completion splats with loop device

[Amir Goldstein]

On Thu, Dec 7, 2017 at 4:46 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Dec 6, 2017 at 9:01 AM, Byungchul Park <byungchul.park@lge.com> wrote:
>> On 12/6/2017 3:31 PM, Amir Goldstein wrote:
>>>
>>> On Wed, Dec 6, 2017 at 7:01 AM, Byungchul Park <byungchul.park@lge.com>
>>> wrote:
>>>>
>>>> On 12/6/2017 6:09 AM, Dave Chinner wrote:
>>>>>
>>>>>
>>>>> On Tue, Dec 05, 2017 at 10:07:41AM -0500, Theodore Ts'o wrote:
>>>>>>
>>>>>>
>>>>>> On Tue, Dec 05, 2017 at 02:16:45PM +0900, Byungchul Park wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> I believe that the commit e319e1fbd9d42 "block, locking/lockdep:
>>>>>>> Assign
>>>>>>> a lock_class per gendisk used for wait_for_completion()" solved the
>>>>>>> false positive.
>>>>>>>
>>>>>>> Could you tell me if it doesn't handle it, with the report? Then, I
>>>>>>> will follow up and try to solve it.
>>>>>>
>>>>>>
>>>>>>
>>>>>> No, it doesn't handle it.  And there was some discussion in the linked
>>>>>> thread on the xfs mailing list that seemed to indicate that it was not
>>>>>> a complete fix.
>>>>>
>>>>>
>>>>>
>>>>> Well, it uses a static key hidden inside the macro
>>>>> alloc_disk_node(), so every disk allocated from the same callsite
>>>>> points to the same static lockdep map. IOWs, every caller of
>>>>> alloc_disk() (i.e. the vast majority of storage devices) are
>>>>> configured to point at the same static lockdep map, regardless of
>>>>> their location in the storage stack.
>>>>>
>>>>> The loop device uses alloc_disk().
>>>>>
>>>>> IOWs, it doesn't address the problem of false positives due to
>>>>> layering in the IO stack at all because we can still have
>>>>> filesystems both above and below the lockdep map that has been
>>>>> attached to the devices...
>>>>
>>>>
>>>>
>>>> Hello,
>>>>
>>>> What if we assign different classes to different loop devices? Do
>>>> you think it helps to avoid such false positives?
>>>>
>>>> I'm not sure since I'm unfamiliar with the stacking stuff. It would
>>>> be appriciated if you let me know your opinion about it.
>>>>
>>
>> Hello Amir,
>>
>> Thanks a lot for your opinion! See the below.
>>
>>> I believe the problem in the tests is that loop is mistaken for the same
>>> class/key as another (underlying) blockdev, where the fs of the loop file
>>> resides.
>>>
>>> For the first order of resolution, it would be enough to assign a
>>> different
>>> key to different types of block devices, similar to the struct
>>> file_system_type
>>> lock class keys.
>>>
>>> This would be enough to cover to common case of ext4 over loop inside
>>> ext4 over real disk (at least for the completion lockdep warnings).
>>
>>
>> This was already done by the commit e319e1fbd9d42 "block,
>> locking/lockdep: Assign a lock_class per gendisk used for
>> wait_for_completion()".
>>
>
> Right...
>
>>> For the second order of resolution it may be desired to assign a different
>>> key/class to an instance of loop devices and other stackable block devices
>>> (e.g. dm, nbd)  to cover the case of a deeper nested blockdev stack,
>>> such as: ext4 over loop inside ext4 over loop inside ext4 over real disk.
>>
>>
>> This is what I should do.
>>
>
> But that won't help Ted. The test use case doesn't include doubly nested loop,
> very few setups will do that. Maybe xfstests running within a container over
> loop image. I doubt it that setup is common, so until there are nested container
> implementations out there that are using loop image mount, this use case
> is not very interesting.
>
>>
>>> That should be enough to cover the completion lockdep validation,
>>> but it still leaves to problem of the file_system_type lock key, which
>>> is still the same for the 2 instances of ext4 in the stack above.
>>> For that I suggested a solution similar to
>>> b1eaa950f7e9 ovl: lockdep annotate of nested stacked overlayfs inode lock
>>> but that will require propagating a "bdev_stack_depth" throughout the
>>> block and fs stack and increment it when creating loop or nbd device.
>>>
>
> So this in the problem Ted is reporting and it needs to be solved inside
> ext4 and any other fs that can be loop mounted on a file inside same fs type.
>
> The lockdep chain is: ext4/loop -> loop completion -> ext4/real
> but all the internal ext4 locks (i.e. &meta_group_info[i]->alloc_sem) are
> annotated statically for the ext4 fs type, so to fix that need:
> 1. ext4 to know it is mounted over loop/nbd
> 2. ext4 to annotate all internal locks that surround blockdev I/O as
>     "loop nested"/"not loop nested" like overlayfs does with the vfs locks
> 3. if multi nested looped fs is important (not really) then loop/nbd will
>     need to know if its file is on a looped fs and propagate nesting level
>     to ext4
>

So maybe a macro for filesystems to annotate internal locks with
underlying blockdev's gendisk lockdep_map?

Does that make sense?

Amir.

Re: False lockdep completion splats with loop device

[Theodore Ts'o]

Let me ask a potentially stupid question, Can someone give an example
of the problem this completion lockdep is actually trying to *catch*.
The lockdep message is sufficiently confusing it's not even clear what
*good* it is in the first place.

Is there an example of a bug that this lockdep feature hasf actually
caught in real life, as opposed to false positives?

							- Ted

Re: False lockdep completion splats with loop device

[Peter Zijlstra]

On Thu, Dec 07, 2017 at 09:33:29AM -0500, Theodore Ts'o wrote:
> Let me ask a potentially stupid question, Can someone give an example
> of the problem this completion lockdep is actually trying to *catch*.
> The lockdep message is sufficiently confusing it's not even clear what
> *good* it is in the first place.


	lock(A)			lock(A)
	wait_for_completion(C)	complete(C);


> Is there an example of a bug that this lockdep feature hasf actually
> caught in real life, as opposed to false positives?

Yes, it caught a fair bunch of real deadlocks in cpu hotplug, watchdog,
and other places I can't remember.

Here's one in the mm:

  https://lkml.kernel.org/r/20171030152200.ayfnewoqkxbuk4zh@hirez.programming.kicks-ass.net

I have at least one open issue in perf that I've no real idea what to do
about:

  https://lkml.kernel.org/r/20171027153336.GC3857@worktop


Yes lockdep is a pain...

Re: False lockdep completion splats with loop device

[Dave Chinner]

On Thu, Dec 07, 2017 at 04:46:52AM +0200, Amir Goldstein wrote:
> >> For the second order of resolution it may be desired to assign a different
> >> key/class to an instance of loop devices and other stackable block devices
> >> (e.g. dm, nbd)  to cover the case of a deeper nested blockdev stack,
> >> such as: ext4 over loop inside ext4 over loop inside ext4 over real disk.
> >
> >
> > This is what I should do.
> >
> 
> But that won't help Ted. The test use case doesn't include doubly nested loop,
> very few setups will do that. Maybe xfstests running within a container over
> loop image. I doubt it that setup is common, so until there are nested container
> implementations out there that are using loop image mount, this use case
> is not very interesting.

xfs/073 nests xfs/loop/xfs/loop/xfs/scratchdev

And I know that people are running with loop devices on XFS as their
test/scratch devices. That gives us filesystems nested 4 layers deep
before we even consider what block device the host filesystem is
sitting on top of.

> The lockdep chain is: ext4/loop -> loop completion -> ext4/real
> but all the internal ext4 locks (i.e. &meta_group_info[i]->alloc_sem) are
> annotated statically for the ext4 fs type, so to fix that need:
> 1. ext4 to know it is mounted over loop/nbd
> 2. ext4 to annotate all internal locks that surround blockdev I/O as
>     "loop nested"/"not loop nested" like overlayfs does with the vfs locks

*cough*

And so every filesystem would need to do this annotation, because
they can all sit above/below a loop device. Essentially you are
suggesting we use superblock-private lockdep class keys rather
than static global maps as we use now.

That's possible, but it could be messy and we'd have to change all
the internal locking setup in every filesystem to do this.

And we'd have to do the same thing for all the block devices that
use completions internally, too because they can also get stacked
above/below other block devices.

> 3. if multi nested looped fs is important (not really) then loop/nbd will
>     need to know if its file is on a looped fs and propagate nesting level
>     to ext4

This functionality is definitely used and needs to be supported by
the annotations.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: False lockdep completion splats with loop device

[Al Viro]

On Fri, Dec 08, 2017 at 10:59:22AM +1100, Dave Chinner wrote:

> > 3. if multi nested looped fs is important (not really) then loop/nbd will
> >     need to know if its file is on a looped fs and propagate nesting level
> >     to ext4
> 
> This functionality is definitely used and needs to be supported by
> the annotations.

FWIW, in addition to loop, there's md.  So should loop need to know if its
backing file lives on a filesystem that does, for example, have an extrnal
journal sitting on RAID1 with one of the components hosted in a file on
some other fs, brought into the array via another loop device?

Oh, and to make life even more fun, backing file can be flipped under
configured /dev/loop.  IOW, those nesting depths are not static at all.

Re: False lockdep completion splats with loop device

[Byungchul Park]

On 12/7/2017 1:18 PM, Amir Goldstein wrote:
> On Thu, Dec 7, 2017 at 4:46 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Wed, Dec 6, 2017 at 9:01 AM, Byungchul Park <byungchul.park@lge.com> wrote:
>>> On 12/6/2017 3:31 PM, Amir Goldstein wrote:
>>>>
>>>> On Wed, Dec 6, 2017 at 7:01 AM, Byungchul Park <byungchul.park@lge.com>
>>>> wrote:
>>>>>
>>>>> On 12/6/2017 6:09 AM, Dave Chinner wrote:
>>>>>>
>>>>>>
>>>>>> On Tue, Dec 05, 2017 at 10:07:41AM -0500, Theodore Ts'o wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Dec 05, 2017 at 02:16:45PM +0900, Byungchul Park wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> I believe that the commit e319e1fbd9d42 "block, locking/lockdep:
>>>>>>>> Assign
>>>>>>>> a lock_class per gendisk used for wait_for_completion()" solved the
>>>>>>>> false positive.
>>>>>>>>
>>>>>>>> Could you tell me if it doesn't handle it, with the report? Then, I
>>>>>>>> will follow up and try to solve it.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> No, it doesn't handle it.  And there was some discussion in the linked
>>>>>>> thread on the xfs mailing list that seemed to indicate that it was not
>>>>>>> a complete fix.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Well, it uses a static key hidden inside the macro
>>>>>> alloc_disk_node(), so every disk allocated from the same callsite
>>>>>> points to the same static lockdep map. IOWs, every caller of
>>>>>> alloc_disk() (i.e. the vast majority of storage devices) are
>>>>>> configured to point at the same static lockdep map, regardless of
>>>>>> their location in the storage stack.
>>>>>>
>>>>>> The loop device uses alloc_disk().
>>>>>>
>>>>>> IOWs, it doesn't address the problem of false positives due to
>>>>>> layering in the IO stack at all because we can still have
>>>>>> filesystems both above and below the lockdep map that has been
>>>>>> attached to the devices...
>>>>>
>>>>>
>>>>>
>>>>> Hello,
>>>>>
>>>>> What if we assign different classes to different loop devices? Do
>>>>> you think it helps to avoid such false positives?
>>>>>
>>>>> I'm not sure since I'm unfamiliar with the stacking stuff. It would
>>>>> be appriciated if you let me know your opinion about it.
>>>>>
>>>
>>> Hello Amir,
>>>
>>> Thanks a lot for your opinion! See the below.
>>>
>>>> I believe the problem in the tests is that loop is mistaken for the same
>>>> class/key as another (underlying) blockdev, where the fs of the loop file
>>>> resides.
>>>>
>>>> For the first order of resolution, it would be enough to assign a
>>>> different
>>>> key to different types of block devices, similar to the struct
>>>> file_system_type
>>>> lock class keys.
>>>>
>>>> This would be enough to cover to common case of ext4 over loop inside
>>>> ext4 over real disk (at least for the completion lockdep warnings).
>>>
>>>
>>> This was already done by the commit e319e1fbd9d42 "block,
>>> locking/lockdep: Assign a lock_class per gendisk used for
>>> wait_for_completion()".
>>>
>>
>> Right...
>>
>>>> For the second order of resolution it may be desired to assign a different
>>>> key/class to an instance of loop devices and other stackable block devices
>>>> (e.g. dm, nbd)  to cover the case of a deeper nested blockdev stack,
>>>> such as: ext4 over loop inside ext4 over loop inside ext4 over real disk.
>>>
>>>
>>> This is what I should do.
>>>
>>
>> But that won't help Ted. The test use case doesn't include doubly nested loop,
>> very few setups will do that. Maybe xfstests running within a container over
>> loop image. I doubt it that setup is common, so until there are nested container
>> implementations out there that are using loop image mount, this use case
>> is not very interesting.
>>
>>>
>>>> That should be enough to cover the completion lockdep validation,
>>>> but it still leaves to problem of the file_system_type lock key, which
>>>> is still the same for the 2 instances of ext4 in the stack above.
>>>> For that I suggested a solution similar to
>>>> b1eaa950f7e9 ovl: lockdep annotate of nested stacked overlayfs inode lock
>>>> but that will require propagating a "bdev_stack_depth" throughout the
>>>> block and fs stack and increment it when creating loop or nbd device.
>>>>
>>
>> So this in the problem Ted is reporting and it needs to be solved inside
>> ext4 and any other fs that can be loop mounted on a file inside same fs type.
>>
>> The lockdep chain is: ext4/loop -> loop completion -> ext4/real
>> but all the internal ext4 locks (i.e. &meta_group_info[i]->alloc_sem) are
>> annotated statically for the ext4 fs type, so to fix that need:
>> 1. ext4 to know it is mounted over loop/nbd
>> 2. ext4 to annotate all internal locks that surround blockdev I/O as
>>      "loop nested"/"not loop nested" like overlayfs does with the vfs locks
>> 3. if multi nested looped fs is important (not really) then loop/nbd will
>>      need to know if its file is on a looped fs and propagate nesting level
>>      to ext4
>>
> 
> So maybe a macro for filesystems to annotate internal locks with
> underlying blockdev's gendisk lockdep_map?
> 
> Does that make sense?

Hello,

I think it can be a way. But I'm thinking about a more general way..

Thanks for the proposal.

-- 
Thanks,
Byungchul

Re: False lockdep completion splats with loop device

[Amir Goldstein]

On Fri, Dec 8, 2017 at 2:13 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, Dec 08, 2017 at 10:59:22AM +1100, Dave Chinner wrote:
>
>> > 3. if multi nested looped fs is important (not really) then loop/nbd will
>> >     need to know if its file is on a looped fs and propagate nesting level
>> >     to ext4
>>
>> This functionality is definitely used and needs to be supported by
>> the annotations.
>
> FWIW, in addition to loop, there's md.

Do you mean md can sit on a file directly without loop/nbd?
I am referring to loop/nbd, because those are the only ones I know
of that can be used to nest (non stackable) fs over fs.

> So should loop need to know if its
> backing file lives on a filesystem that does, for example, have an extrnal
> journal sitting on RAID1 with one of the components hosted in a file on
> some other fs, brought into the array via another loop device?
>

The nested depth accounting scheme I proposed is unneeded.
I think it should be enough to use a macro to annotate locks that
can go from fs into io for underlying blockdev,
e.g. (for Ted's lockdep splat):
    init_rwsem(&meta_group_info[i]->alloc_sem);
    lockdep_fsio(&meta_group_info[i]->alloc_sem, sb);

That can help isolate the annotation to a lockdep map attached
to the underlying blockdev/gendisk.

As you pointed out, this does not cover all the cases of fs that uses
multi blockdev for external journal or whatever, but even if we isolate
lockdep by the main (or any) blockdev of a filesystem, assuming this
blockdev is used exclusively by that fs instance, this would be enough
to mitigate the false positives from loop nested setups.

> Oh, and to make life even more fun, backing file can be flipped under
> configured /dev/loop.  IOW, those nesting depths are not static at all.

In that case, lockdep would have been very right to warn us, because
we can actually create a deadlock by looping loop into a file that sits
inside fs on the loop device.

Amir.

Re: False lockdep completion splats with loop device

[Dave Chinner]

On Fri, Dec 08, 2017 at 10:15:07AM +0200, Amir Goldstein wrote:
> On Fri, Dec 8, 2017 at 2:13 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Fri, Dec 08, 2017 at 10:59:22AM +1100, Dave Chinner wrote:
> >
> >> > 3. if multi nested looped fs is important (not really) then loop/nbd will
> >> >     need to know if its file is on a looped fs and propagate nesting level
> >> >     to ext4
> >>
> >> This functionality is definitely used and needs to be supported by
> >> the annotations.
> >
> > FWIW, in addition to loop, there's md.
> 
> Do you mean md can sit on a file directly without loop/nbd?
> I am referring to loop/nbd, because those are the only ones I know
> of that can be used to nest (non stackable) fs over fs.

The problem is one of stacked completions, not loop/nbd devices. The
loop and nbd devices are just the simplest way to stack completions.
MD can sit on MD and other devices in complex stacks, so if there's
completion in MD, we have the same completion layering problem.

i.e. this is *not specifically a filesystem problem*. This is a
completion stacking problem, and lots of different layers in the
storage stack use completions and can be layered in arbitrary
orders. MD and DM are jsut two more virtual block devices that can
stack in random orders.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: False lockdep completion splats with loop device

[Amir Goldstein]

On Sat, Dec 9, 2017 at 12:57 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Fri, Dec 08, 2017 at 10:15:07AM +0200, Amir Goldstein wrote:
>> On Fri, Dec 8, 2017 at 2:13 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> > On Fri, Dec 08, 2017 at 10:59:22AM +1100, Dave Chinner wrote:
>> >
>> >> > 3. if multi nested looped fs is important (not really) then loop/nbd will
>> >> >     need to know if its file is on a looped fs and propagate nesting level
>> >> >     to ext4
>> >>
>> >> This functionality is definitely used and needs to be supported by
>> >> the annotations.
>> >
>> > FWIW, in addition to loop, there's md.
>>
>> Do you mean md can sit on a file directly without loop/nbd?
>> I am referring to loop/nbd, because those are the only ones I know
>> of that can be used to nest (non stackable) fs over fs.
>
> The problem is one of stacked completions, not loop/nbd devices. The
> loop and nbd devices are just the simplest way to stack completions.
> MD can sit on MD and other devices in complex stacks, so if there's
> completion in MD, we have the same completion layering problem.
>
> i.e. this is *not specifically a filesystem problem*. This is a
> completion stacking problem, and lots of different layers in the
> storage stack use completions and can be layered in arbitrary
> orders. MD and DM are jsut two more virtual block devices that can
> stack in random orders.
>

The way I see it, there are two different stacking problems, one is
specifically a fs problem, the other is not.

1. completion stacking annotation problem.

commit e319e1fbd9d42 "block, locking/lockdep: Assign
a lock_class per gendisk used for wait_for_completion()"
is a partial solution, because it solves only case of stacking different
types of gendisk. It should probably be extended to assign a lock_class
per gendisk instance.

2. loop nested fs stacking annotation problem

This depends on a specific fs and a specific lock.
In the ext4 example, IIUC, IO is submitted inside meta_group_info[i]->alloc_sem
of loop mounted fs.
If backing loop file is not pre-allocated, that IO can lead to block
allocation in
underlying ext4 and take meta_group_info[i]->alloc_sem.
That lock is in a different fs instance, but uses the same static allocation.

If solution to problem 1 above is acceptable, then going the same route for
problem 2 could be solved by assigning a lock class for sb instance for
locks of that sort.

The honest truth is that loop nested fs can *really* cause a deadlock with
silly setups.
completion lockdep is the unlucky messenger that was trying to bring that to
our attention.
We told completion lockdep that we are aware of that potential deadlock and
that our setup is not silly and beheaded the messenger.

Byungchul,

Seeing how your changes to lockdep completion has caused pain to fs developers,
I recommend that when checking your changes you run Ted's kvm-xfstests to
verify it is not causing any regressions w.r.t. false positive lockdep warnings.
This test VM is specifically designed for other subsystem developers to
easily run the fs subsystem testsuite.

Specifically, when working on a fix for the problem Ted reported, you
should at least
run the test shared/298 on ext4 and you should also run the test
xfs/073 on xfs, where
Dave indicated there is a nested loop fs setup.

Cheers,
Amir.

Re: False lockdep completion splats with loop device

[Theodore Ts'o]

On Sat, Dec 09, 2017 at 10:44:33AM +0200, Amir Goldstein wrote:
> The honest truth is that loop nested fs can *really* cause a
> deadlock with silly setups.  completion lockdep is the unlucky
> messenger that was trying to bring that to our attention.

What is the "silly setup" (I'm not saying kernel bugs, but rather
things which a sysadmin might want to set up) that would cause
deadlock?  In the example you gave, the block allocation is happenig
in two different file system instances, so if lockdep whinges, that's
another example of a false positive.

> Byungchul,
> 
> Seeing how your changes to lockdep completion has caused pain to fs developers,
> I recommend that when checking your changes you run Ted's kvm-xfstests to
> verify it is not causing any regressions w.r.t. false positive lockdep warnings.
> This test VM is specifically designed for other subsystem developers to
> easily run the fs subsystem testsuite.

How about a Kconfig to disable completion lockdep?  It's clearly not
ready for prime time.

At the *minimum*, block devices and struct supers need to have the
ability to have unique id's, and there needs to be a trivial way to
mix that into lockdep key that doesn't require every single subsystem
and device driver author to use undocumented lockdep interfaces to
make the right thing happen.  Instead, we need abstractions that make
the right thing happen by default.

Until this happens, we either need to disable lockdep completions, or
disable lockdep altogether.  I'd prefer the former --- and I'll even
offer to run a Kconfig fixup patch through the ext4 tree to do this,
"free of charge" --- but if you NACK such a change, I'll probably end
up following Dave Chinner's wise counsel and suggest to all ext4
developers to disable lockdep.

							- Ted

Re: False lockdep completion splats with loop device

[Amir Goldstein]

On Sat, Dec 9, 2017 at 6:02 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Sat, Dec 09, 2017 at 10:44:33AM +0200, Amir Goldstein wrote:
>> The honest truth is that loop nested fs can *really* cause a
>> deadlock with silly setups.  completion lockdep is the unlucky
>> messenger that was trying to bring that to our attention.
>
> What is the "silly setup" (I'm not saying kernel bugs, but rather
> things which a sysadmin might want to set up) that would cause
> deadlock?  In the example you gave, the block allocation is happenig
> in two different file system instances, so if lockdep whinges, that's
> another example of a false positive.
>

The "silly setup" is not something that any sane admin will do,
but there a lot of crazy stuff you can do with loop, like:
- create a file in FS1 on VG1/LV1
- setup loop dev
- add the loop dev to VG1
- extend LV1 onto loopdev
- extend FS1 onto loopdev

That setup is possible. That setup can deadlock.

I am not trying to defend lockdep developers regressing our tests
and "forcing" the burden of annotations on fs developers. That is wrong.
The burden of proof should be on lockdep developers and IMO is was
premature to force completion lockdep.

But for full disclosure, I am stating a fact that lockdep is not meant to
tell us that we hit a deadlock. lockdep is meant to tell us that a deadlock
is possible with current code.
And what do you know - deadlock is possible, just not with your setup.
It has always been possible - completion lockdep just exposed that.

Going forward, it is clear that with completion lockdep, the static lockdep
annotations for fs locks are not sufficient. We need to have a very simple
way of annotating a lock that is associated with an fs instance rather
than an fs type.

Amir.