* False lockdep completion splats with loop device @ 2017-12-05 3:03 Theodore Ts'o 2017-12-05 5:16 ` Byungchul Park 0 siblings, 1 reply; 21+ messages in thread From: Theodore Ts'o @ 2017-12-05 3:03 UTC (permalink / raw) To: fstests; +Cc: linux-fsdevel, Byungchul Park, peterz, mingo 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-05 3:03 False lockdep completion splats with loop device Theodore Ts'o @ 2017-12-05 5:16 ` Byungchul Park 2017-12-05 15:07 ` Theodore Ts'o 0 siblings, 1 reply; 21+ messages in thread From: Byungchul Park @ 2017-12-05 5:16 UTC (permalink / raw) To: Theodore Ts'o, fstests; +Cc: linux-fsdevel, peterz, mingo, kernel-team 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-05 5:16 ` Byungchul Park @ 2017-12-05 15:07 ` Theodore Ts'o 2017-12-05 21:09 ` Dave Chinner 2017-12-06 6:23 ` Byungchul Park 0 siblings, 2 replies; 21+ messages in thread From: Theodore Ts'o @ 2017-12-05 15:07 UTC (permalink / raw) To: Byungchul Park; +Cc: fstests, linux-fsdevel, peterz, mingo, kernel-team 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-05 15:07 ` Theodore Ts'o @ 2017-12-05 21:09 ` Dave Chinner 2017-12-06 5:01 ` Byungchul Park 2017-12-06 6:23 ` Byungchul Park 1 sibling, 1 reply; 21+ messages in thread From: Dave Chinner @ 2017-12-05 21:09 UTC (permalink / raw) To: Theodore Ts'o Cc: Byungchul Park, fstests, linux-fsdevel, peterz, mingo, kernel-team 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-05 21:09 ` Dave Chinner @ 2017-12-06 5:01 ` Byungchul Park 2017-12-06 6:08 ` Dave Chinner 2017-12-06 6:31 ` Amir Goldstein 0 siblings, 2 replies; 21+ messages in thread From: Byungchul Park @ 2017-12-06 5:01 UTC (permalink / raw) To: Dave Chinner, Theodore Ts'o Cc: fstests, linux-fsdevel, peterz, mingo, kernel-team 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-06 5:01 ` Byungchul Park @ 2017-12-06 6:08 ` Dave Chinner 2017-12-06 6:31 ` Amir Goldstein 1 sibling, 0 replies; 21+ messages in thread From: Dave Chinner @ 2017-12-06 6:08 UTC (permalink / raw) To: Byungchul Park Cc: Theodore Ts'o, fstests, linux-fsdevel, peterz, mingo, kernel-team 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-06 5:01 ` Byungchul Park 2017-12-06 6:08 ` Dave Chinner @ 2017-12-06 6:31 ` Amir Goldstein 2017-12-06 7:01 ` Byungchul Park 1 sibling, 1 reply; 21+ messages in thread From: Amir Goldstein @ 2017-12-06 6:31 UTC (permalink / raw) To: Byungchul Park Cc: Dave Chinner, Theodore Ts'o, fstests, linux-fsdevel, Peter Zijlstra, mingo, kernel-team 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-06 6:31 ` Amir Goldstein @ 2017-12-06 7:01 ` Byungchul Park 2017-12-07 2:46 ` Amir Goldstein 0 siblings, 1 reply; 21+ messages in thread From: Byungchul Park @ 2017-12-06 7:01 UTC (permalink / raw) To: Amir Goldstein Cc: Dave Chinner, Theodore Ts'o, fstests, linux-fsdevel, Peter Zijlstra, mingo, kernel-team 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-06 7:01 ` Byungchul Park @ 2017-12-07 2:46 ` Amir Goldstein 2017-12-07 4:18 ` Amir Goldstein 2017-12-07 23:59 ` Dave Chinner 0 siblings, 2 replies; 21+ messages in thread From: Amir Goldstein @ 2017-12-07 2:46 UTC (permalink / raw) To: Byungchul Park Cc: Dave Chinner, Theodore Ts'o, fstests, linux-fsdevel, Peter Zijlstra, mingo, kernel-team 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-07 2:46 ` Amir Goldstein @ 2017-12-07 4:18 ` Amir Goldstein 2017-12-07 14:33 ` Theodore Ts'o 2017-12-08 1:51 ` Byungchul Park 2017-12-07 23:59 ` Dave Chinner 1 sibling, 2 replies; 21+ messages in thread From: Amir Goldstein @ 2017-12-07 4:18 UTC (permalink / raw) To: Byungchul Park Cc: Dave Chinner, Theodore Ts'o, fstests, linux-fsdevel, Peter Zijlstra, mingo, kernel-team 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-07 4:18 ` Amir Goldstein @ 2017-12-07 14:33 ` Theodore Ts'o 2017-12-07 14:53 ` Peter Zijlstra 2017-12-08 1:51 ` Byungchul Park 1 sibling, 1 reply; 21+ messages in thread From: Theodore Ts'o @ 2017-12-07 14:33 UTC (permalink / raw) To: Amir Goldstein Cc: Byungchul Park, Dave Chinner, fstests, linux-fsdevel, Peter Zijlstra, mingo, kernel-team 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-07 14:33 ` Theodore Ts'o @ 2017-12-07 14:53 ` Peter Zijlstra 0 siblings, 0 replies; 21+ messages in thread From: Peter Zijlstra @ 2017-12-07 14:53 UTC (permalink / raw) To: Theodore Ts'o Cc: Amir Goldstein, Byungchul Park, Dave Chinner, fstests, linux-fsdevel, mingo, kernel-team 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... ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-07 4:18 ` Amir Goldstein 2017-12-07 14:33 ` Theodore Ts'o @ 2017-12-08 1:51 ` Byungchul Park 1 sibling, 0 replies; 21+ messages in thread From: Byungchul Park @ 2017-12-08 1:51 UTC (permalink / raw) To: Amir Goldstein Cc: Dave Chinner, Theodore Ts'o, fstests, linux-fsdevel, Peter Zijlstra, mingo, kernel-team 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-07 2:46 ` Amir Goldstein 2017-12-07 4:18 ` Amir Goldstein @ 2017-12-07 23:59 ` Dave Chinner 2017-12-08 0:13 ` Al Viro 1 sibling, 1 reply; 21+ messages in thread From: Dave Chinner @ 2017-12-07 23:59 UTC (permalink / raw) To: Amir Goldstein Cc: Byungchul Park, Theodore Ts'o, fstests, linux-fsdevel, Peter Zijlstra, mingo, kernel-team 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-07 23:59 ` Dave Chinner @ 2017-12-08 0:13 ` Al Viro 2017-12-08 8:15 ` Amir Goldstein 0 siblings, 1 reply; 21+ messages in thread From: Al Viro @ 2017-12-08 0:13 UTC (permalink / raw) To: Dave Chinner Cc: Amir Goldstein, Byungchul Park, Theodore Ts'o, fstests, linux-fsdevel, Peter Zijlstra, mingo, kernel-team 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-08 0:13 ` Al Viro @ 2017-12-08 8:15 ` Amir Goldstein 2017-12-08 22:57 ` Dave Chinner 0 siblings, 1 reply; 21+ messages in thread From: Amir Goldstein @ 2017-12-08 8:15 UTC (permalink / raw) To: Al Viro Cc: Dave Chinner, Byungchul Park, Theodore Ts'o, fstests, linux-fsdevel, Peter Zijlstra, mingo, kernel-team 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-08 8:15 ` Amir Goldstein @ 2017-12-08 22:57 ` Dave Chinner 2017-12-09 8:44 ` Amir Goldstein 0 siblings, 1 reply; 21+ messages in thread From: Dave Chinner @ 2017-12-08 22:57 UTC (permalink / raw) To: Amir Goldstein Cc: Al Viro, Byungchul Park, Theodore Ts'o, fstests, linux-fsdevel, Peter Zijlstra, mingo, kernel-team 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-08 22:57 ` Dave Chinner @ 2017-12-09 8:44 ` Amir Goldstein 2017-12-09 16:02 ` Theodore Ts'o 0 siblings, 1 reply; 21+ messages in thread From: Amir Goldstein @ 2017-12-09 8:44 UTC (permalink / raw) To: Dave Chinner Cc: Al Viro, Byungchul Park, Theodore Ts'o, fstests, linux-fsdevel, Peter Zijlstra, mingo, kernel-team 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-09 8:44 ` Amir Goldstein @ 2017-12-09 16:02 ` Theodore Ts'o 2017-12-09 20:08 ` Amir Goldstein 0 siblings, 1 reply; 21+ messages in thread From: Theodore Ts'o @ 2017-12-09 16:02 UTC (permalink / raw) To: Amir Goldstein Cc: Dave Chinner, Al Viro, Byungchul Park, fstests, linux-fsdevel, Peter Zijlstra, mingo, kernel-team 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-09 16:02 ` Theodore Ts'o @ 2017-12-09 20:08 ` Amir Goldstein 0 siblings, 0 replies; 21+ messages in thread From: Amir Goldstein @ 2017-12-09 20:08 UTC (permalink / raw) To: Theodore Ts'o Cc: Dave Chinner, Al Viro, Byungchul Park, fstests, linux-fsdevel, Peter Zijlstra, mingo, kernel-team 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: False lockdep completion splats with loop device 2017-12-05 15:07 ` Theodore Ts'o 2017-12-05 21:09 ` Dave Chinner @ 2017-12-06 6:23 ` Byungchul Park 1 sibling, 0 replies; 21+ messages in thread From: Byungchul Park @ 2017-12-06 6:23 UTC (permalink / raw) To: Theodore Ts'o; +Cc: fstests, linux-fsdevel, peterz, mingo, kernel-team 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-12-09 20:08 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-05 3:03 False lockdep completion splats with loop device Theodore Ts'o 2017-12-05 5:16 ` Byungchul Park 2017-12-05 15:07 ` Theodore Ts'o 2017-12-05 21:09 ` Dave Chinner 2017-12-06 5:01 ` Byungchul Park 2017-12-06 6:08 ` Dave Chinner 2017-12-06 6:31 ` Amir Goldstein 2017-12-06 7:01 ` Byungchul Park 2017-12-07 2:46 ` Amir Goldstein 2017-12-07 4:18 ` Amir Goldstein 2017-12-07 14:33 ` Theodore Ts'o 2017-12-07 14:53 ` Peter Zijlstra 2017-12-08 1:51 ` Byungchul Park 2017-12-07 23:59 ` Dave Chinner 2017-12-08 0:13 ` Al Viro 2017-12-08 8:15 ` Amir Goldstein 2017-12-08 22:57 ` Dave Chinner 2017-12-09 8:44 ` Amir Goldstein 2017-12-09 16:02 ` Theodore Ts'o 2017-12-09 20:08 ` Amir Goldstein 2017-12-06 6:23 ` Byungchul Park
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.