* [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
@ 2021-07-02 8:51 ` Sachin Sant
0 siblings, 0 replies; 35+ messages in thread
From: Sachin Sant @ 2021-07-02 8:51 UTC (permalink / raw)
To: linux-ext4, linux-fsdevel; +Cc: linuxppc-dev, yi.zhang, jack
While running LTP tests (chdir01) against 5.13.0-next20210701 booted on a Power server,
following crash is encountered.
[ 3051.182992] ext2 filesystem being mounted at /var/tmp/avocado_oau90dri/ltp-W0cFB5HtCy/lKhal5/mntpoint supports timestamps until 2038 (0x7fffffff)
[ 3051.621341] EXT4-fs (loop0): mounting ext3 file system using the ext4 subsystem
[ 3051.624645] EXT4-fs (loop0): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
[ 3051.624682] ext3 filesystem being mounted at /var/tmp/avocado_oau90dri/ltp-W0cFB5HtCy/lKhal5/mntpoint supports timestamps until 2038 (0x7fffffff)
[ 3051.629026] Kernel attempted to read user page (13fda70000) - exploit attempt? (uid: 0)
[ 3051.629074] BUG: Unable to handle kernel data access on read at 0x13fda70000
[ 3051.629103] Faulting instruction address: 0xc0000000006fa5cc
[ 3051.629118] Oops: Kernel access of bad area, sig: 11 [#1]
[ 3051.629130] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
[ 3051.629148] Modules linked in: vfat fat btrfs blake2b_generic xor zstd_compress raid6_pq xfs loop sctp ip6_udp_tunnel udp_tunnel libcrc32c rpadlpar_io rpaphp dm_mod bonding rfkill sunrpc pseries_rng xts vmx_crypto uio_pdrv_genirq uio sch_fq_codel ip_tables ext4 mbcache jbd2 sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp fuse [last unloaded: test_cpuidle_latency]
[ 3051.629270] CPU: 10 PID: 274044 Comm: chdir01 Tainted: G W OE 5.13.0-next-20210701 #1
[ 3051.629289] NIP: c0000000006fa5cc LR: c008000006949bc4 CTR: c0000000006fa5a0
[ 3051.629300] REGS: c000000f74de3660 TRAP: 0300 Tainted: G W OE (5.13.0-next-20210701)
[ 3051.629314] MSR: 800000000280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 24000288 XER: 20040000
[ 3051.629342] CFAR: c008000006957564 DAR: 00000013fda70000 DSISR: 40000000 IRQMASK: 0
[ 3051.629342] GPR00: c008000006949bc4 c000000f74de3900 c0000000029bc800 c000000f88f0ab80
[ 3051.629342] GPR04: ffffffffffffffff 0000000000000020 0000000024000282 0000000000000000
[ 3051.629342] GPR08: c00000110628c828 0000000000000000 00000013fda70000 c008000006957550
[ 3051.629342] GPR12: c0000000006fa5a0 c0000013ffffbe80 0000000000000000 0000000000000000
[ 3051.629342] GPR16: 0000000000000000 0000000000000000 00000000100555f8 0000000010050d40
[ 3051.629342] GPR20: 0000000000000000 0000000010026188 0000000010026160 c000000f88f0ac08
[ 3051.629342] GPR24: 0000000000000000 c000000f88f0a920 0000000000000000 0000000000000002
[ 3051.629342] GPR28: c000000f88f0ac50 c000000f88f0a800 c000000fc5577d00 c000000f88f0ab80
[ 3051.629468] NIP [c0000000006fa5cc] percpu_counter_add_batch+0x2c/0xf0
[ 3051.629493] LR [c008000006949bc4] __jbd2_journal_remove_checkpoint+0x9c/0x280 [jbd2]
[ 3051.629526] Call Trace:
[ 3051.629532] [c000000f74de3900] [c000000f88f0a84c] 0xc000000f88f0a84c (unreliable)
[ 3051.629547] [c000000f74de3940] [c008000006949bc4] __jbd2_journal_remove_checkpoint+0x9c/0x280 [jbd2]
[ 3051.629577] [c000000f74de3980] [c008000006949eb4] jbd2_log_do_checkpoint+0x10c/0x630 [jbd2]
[ 3051.629605] [c000000f74de3a40] [c0080000069547dc] jbd2_journal_destroy+0x1b4/0x4e0 [jbd2]
[ 3051.629636] [c000000f74de3ad0] [c00800000735d72c] ext4_put_super+0xb4/0x560 [ext4]
[ 3051.629703] [c000000f74de3b60] [c000000000484d64] generic_shutdown_super+0xc4/0x1d0
[ 3051.629720] [c000000f74de3bd0] [c000000000484f48] kill_block_super+0x38/0x90
[ 3051.629736] [c000000f74de3c00] [c000000000485120] deactivate_locked_super+0x80/0x100
[ 3051.629752] [c000000f74de3c30] [c0000000004bec1c] cleanup_mnt+0x10c/0x1d0
[ 3051.629767] [c000000f74de3c80] [c000000000188b08] task_work_run+0xf8/0x170
[ 3051.629783] [c000000f74de3cd0] [c000000000021a24] do_notify_resume+0x434/0x480
[ 3051.629800] [c000000f74de3d80] [c000000000032910] interrupt_exit_user_prepare_main+0x1a0/0x260
[ 3051.629816] [c000000f74de3de0] [c000000000032d08] syscall_exit_prepare+0x68/0x150
[ 3051.629830] [c000000f74de3e10] [c00000000000c770] system_call_common+0x100/0x258
[ 3051.629846] --- interrupt: c00 at 0x7fffa2b92ffc
[ 3051.629855] NIP: 00007fffa2b92ffc LR: 00007fffa2b92fcc CTR: 0000000000000000
[ 3051.629867] REGS: c000000f74de3e80 TRAP: 0c00 Tainted: G W OE (5.13.0-next-20210701)
[ 3051.629880] MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 24000474 XER: 00000000
[ 3051.629908] IRQMASK: 0
[ 3051.629908] GPR00: 0000000000000034 00007fffc0242e20 00007fffa2c77100 0000000000000000
[ 3051.629908] GPR04: 0000000000000000 0000000000000078 0000000000000000 0000000000000020
[ 3051.629908] GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[ 3051.629908] GPR12: 0000000000000000 00007fffa2d1a310 0000000000000000 0000000000000000
[ 3051.629908] GPR16: 0000000000000000 0000000000000000 00000000100555f8 0000000010050d40
[ 3051.629908] GPR20: 0000000000000000 0000000010026188 0000000010026160 00000000100288f0
[ 3051.629908] GPR24: 00007fffa2d13320 00000000000186a0 0000000010025dd8 0000000010055688
[ 3051.629908] GPR28: 0000000010024bb8 0000000000000001 0000000000000001 0000000000000000
[ 3051.630022] NIP [00007fffa2b92ffc] 0x7fffa2b92ffc
[ 3051.630032] LR [00007fffa2b92fcc] 0x7fffa2b92fcc
[ 3051.630041] --- interrupt: c00
[ 3051.630048] Instruction dump:
[ 3051.630057] 60000000 3c4c022c 38422260 7c0802a6 fbe1fff8 fba1ffe8 7c7f1b78 fbc1fff0
[ 3051.630078] f8010010 f821ffc1 e94d0030 e9230020 <7fca4aaa> 7fbe2214 7fa9fe76 7d2aea78
[ 3051.630102] ---[ end trace 83afe3a19212c333 ]---
[ 3051.633656]
[ 3052.633681] Kernel panic - not syncing: Fatal exception
5.13.0-next-20210630 was good. Bisect points to following patch:
commit 4ba3fcdde7e3
jbd2,ext4: add a shrinker to release checkpointed buffers
Reverting this patch allows the test to run successfully.
Thanks
-Sachin
^ permalink raw reply [flat|nested] 35+ messages in thread
* [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
@ 2021-07-02 8:51 ` Sachin Sant
0 siblings, 0 replies; 35+ messages in thread
From: Sachin Sant @ 2021-07-02 8:51 UTC (permalink / raw)
To: linux-ext4, linux-fsdevel; +Cc: jack, linuxppc-dev, yi.zhang
While running LTP tests (chdir01) against 5.13.0-next20210701 booted on a Power server,
following crash is encountered.
[ 3051.182992] ext2 filesystem being mounted at /var/tmp/avocado_oau90dri/ltp-W0cFB5HtCy/lKhal5/mntpoint supports timestamps until 2038 (0x7fffffff)
[ 3051.621341] EXT4-fs (loop0): mounting ext3 file system using the ext4 subsystem
[ 3051.624645] EXT4-fs (loop0): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
[ 3051.624682] ext3 filesystem being mounted at /var/tmp/avocado_oau90dri/ltp-W0cFB5HtCy/lKhal5/mntpoint supports timestamps until 2038 (0x7fffffff)
[ 3051.629026] Kernel attempted to read user page (13fda70000) - exploit attempt? (uid: 0)
[ 3051.629074] BUG: Unable to handle kernel data access on read at 0x13fda70000
[ 3051.629103] Faulting instruction address: 0xc0000000006fa5cc
[ 3051.629118] Oops: Kernel access of bad area, sig: 11 [#1]
[ 3051.629130] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
[ 3051.629148] Modules linked in: vfat fat btrfs blake2b_generic xor zstd_compress raid6_pq xfs loop sctp ip6_udp_tunnel udp_tunnel libcrc32c rpadlpar_io rpaphp dm_mod bonding rfkill sunrpc pseries_rng xts vmx_crypto uio_pdrv_genirq uio sch_fq_codel ip_tables ext4 mbcache jbd2 sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp fuse [last unloaded: test_cpuidle_latency]
[ 3051.629270] CPU: 10 PID: 274044 Comm: chdir01 Tainted: G W OE 5.13.0-next-20210701 #1
[ 3051.629289] NIP: c0000000006fa5cc LR: c008000006949bc4 CTR: c0000000006fa5a0
[ 3051.629300] REGS: c000000f74de3660 TRAP: 0300 Tainted: G W OE (5.13.0-next-20210701)
[ 3051.629314] MSR: 800000000280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 24000288 XER: 20040000
[ 3051.629342] CFAR: c008000006957564 DAR: 00000013fda70000 DSISR: 40000000 IRQMASK: 0
[ 3051.629342] GPR00: c008000006949bc4 c000000f74de3900 c0000000029bc800 c000000f88f0ab80
[ 3051.629342] GPR04: ffffffffffffffff 0000000000000020 0000000024000282 0000000000000000
[ 3051.629342] GPR08: c00000110628c828 0000000000000000 00000013fda70000 c008000006957550
[ 3051.629342] GPR12: c0000000006fa5a0 c0000013ffffbe80 0000000000000000 0000000000000000
[ 3051.629342] GPR16: 0000000000000000 0000000000000000 00000000100555f8 0000000010050d40
[ 3051.629342] GPR20: 0000000000000000 0000000010026188 0000000010026160 c000000f88f0ac08
[ 3051.629342] GPR24: 0000000000000000 c000000f88f0a920 0000000000000000 0000000000000002
[ 3051.629342] GPR28: c000000f88f0ac50 c000000f88f0a800 c000000fc5577d00 c000000f88f0ab80
[ 3051.629468] NIP [c0000000006fa5cc] percpu_counter_add_batch+0x2c/0xf0
[ 3051.629493] LR [c008000006949bc4] __jbd2_journal_remove_checkpoint+0x9c/0x280 [jbd2]
[ 3051.629526] Call Trace:
[ 3051.629532] [c000000f74de3900] [c000000f88f0a84c] 0xc000000f88f0a84c (unreliable)
[ 3051.629547] [c000000f74de3940] [c008000006949bc4] __jbd2_journal_remove_checkpoint+0x9c/0x280 [jbd2]
[ 3051.629577] [c000000f74de3980] [c008000006949eb4] jbd2_log_do_checkpoint+0x10c/0x630 [jbd2]
[ 3051.629605] [c000000f74de3a40] [c0080000069547dc] jbd2_journal_destroy+0x1b4/0x4e0 [jbd2]
[ 3051.629636] [c000000f74de3ad0] [c00800000735d72c] ext4_put_super+0xb4/0x560 [ext4]
[ 3051.629703] [c000000f74de3b60] [c000000000484d64] generic_shutdown_super+0xc4/0x1d0
[ 3051.629720] [c000000f74de3bd0] [c000000000484f48] kill_block_super+0x38/0x90
[ 3051.629736] [c000000f74de3c00] [c000000000485120] deactivate_locked_super+0x80/0x100
[ 3051.629752] [c000000f74de3c30] [c0000000004bec1c] cleanup_mnt+0x10c/0x1d0
[ 3051.629767] [c000000f74de3c80] [c000000000188b08] task_work_run+0xf8/0x170
[ 3051.629783] [c000000f74de3cd0] [c000000000021a24] do_notify_resume+0x434/0x480
[ 3051.629800] [c000000f74de3d80] [c000000000032910] interrupt_exit_user_prepare_main+0x1a0/0x260
[ 3051.629816] [c000000f74de3de0] [c000000000032d08] syscall_exit_prepare+0x68/0x150
[ 3051.629830] [c000000f74de3e10] [c00000000000c770] system_call_common+0x100/0x258
[ 3051.629846] --- interrupt: c00 at 0x7fffa2b92ffc
[ 3051.629855] NIP: 00007fffa2b92ffc LR: 00007fffa2b92fcc CTR: 0000000000000000
[ 3051.629867] REGS: c000000f74de3e80 TRAP: 0c00 Tainted: G W OE (5.13.0-next-20210701)
[ 3051.629880] MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 24000474 XER: 00000000
[ 3051.629908] IRQMASK: 0
[ 3051.629908] GPR00: 0000000000000034 00007fffc0242e20 00007fffa2c77100 0000000000000000
[ 3051.629908] GPR04: 0000000000000000 0000000000000078 0000000000000000 0000000000000020
[ 3051.629908] GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[ 3051.629908] GPR12: 0000000000000000 00007fffa2d1a310 0000000000000000 0000000000000000
[ 3051.629908] GPR16: 0000000000000000 0000000000000000 00000000100555f8 0000000010050d40
[ 3051.629908] GPR20: 0000000000000000 0000000010026188 0000000010026160 00000000100288f0
[ 3051.629908] GPR24: 00007fffa2d13320 00000000000186a0 0000000010025dd8 0000000010055688
[ 3051.629908] GPR28: 0000000010024bb8 0000000000000001 0000000000000001 0000000000000000
[ 3051.630022] NIP [00007fffa2b92ffc] 0x7fffa2b92ffc
[ 3051.630032] LR [00007fffa2b92fcc] 0x7fffa2b92fcc
[ 3051.630041] --- interrupt: c00
[ 3051.630048] Instruction dump:
[ 3051.630057] 60000000 3c4c022c 38422260 7c0802a6 fbe1fff8 fba1ffe8 7c7f1b78 fbc1fff0
[ 3051.630078] f8010010 f821ffc1 e94d0030 e9230020 <7fca4aaa> 7fbe2214 7fa9fe76 7d2aea78
[ 3051.630102] ---[ end trace 83afe3a19212c333 ]---
[ 3051.633656]
[ 3052.633681] Kernel panic - not syncing: Fatal exception
5.13.0-next-20210630 was good. Bisect points to following patch:
commit 4ba3fcdde7e3
jbd2,ext4: add a shrinker to release checkpointed buffers
Reverting this patch allows the test to run successfully.
Thanks
-Sachin
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
2021-07-02 8:51 ` Sachin Sant
@ 2021-07-02 9:38 ` Guoqing Jiang
-1 siblings, 0 replies; 35+ messages in thread
From: Guoqing Jiang @ 2021-07-02 9:38 UTC (permalink / raw)
To: Sachin Sant, linux-ext4, linux-fsdevel; +Cc: linuxppc-dev, yi.zhang, jack
On 7/2/21 4:51 PM, Sachin Sant wrote:
> While running LTP tests (chdir01) against 5.13.0-next20210701 booted on a Power server,
> following crash is encountered.
>
> [ 3051.182992] ext2 filesystem being mounted at /var/tmp/avocado_oau90dri/ltp-W0cFB5HtCy/lKhal5/mntpoint supports timestamps until 2038 (0x7fffffff)
> [ 3051.621341] EXT4-fs (loop0): mounting ext3 file system using the ext4 subsystem
> [ 3051.624645] EXT4-fs (loop0): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
> [ 3051.624682] ext3 filesystem being mounted at /var/tmp/avocado_oau90dri/ltp-W0cFB5HtCy/lKhal5/mntpoint supports timestamps until 2038 (0x7fffffff)
> [ 3051.629026] Kernel attempted to read user page (13fda70000) - exploit attempt? (uid: 0)
> [ 3051.629074] BUG: Unable to handle kernel data access on read at 0x13fda70000
> [ 3051.629103] Faulting instruction address: 0xc0000000006fa5cc
> [ 3051.629118] Oops: Kernel access of bad area, sig: 11 [#1]
> [ 3051.629130] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
> [ 3051.629148] Modules linked in: vfat fat btrfs blake2b_generic xor zstd_compress raid6_pq xfs loop sctp ip6_udp_tunnel udp_tunnel libcrc32c rpadlpar_io rpaphp dm_mod bonding rfkill sunrpc pseries_rng xts vmx_crypto uio_pdrv_genirq uio sch_fq_codel ip_tables ext4 mbcache jbd2 sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp fuse [last unloaded: test_cpuidle_latency]
> [ 3051.629270] CPU: 10 PID: 274044 Comm: chdir01 Tainted: G W OE 5.13.0-next-20210701 #1
> [ 3051.629289] NIP: c0000000006fa5cc LR: c008000006949bc4 CTR: c0000000006fa5a0
> [ 3051.629300] REGS: c000000f74de3660 TRAP: 0300 Tainted: G W OE (5.13.0-next-20210701)
> [ 3051.629314] MSR: 800000000280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 24000288 XER: 20040000
> [ 3051.629342] CFAR: c008000006957564 DAR: 00000013fda70000 DSISR: 40000000 IRQMASK: 0
> [ 3051.629342] GPR00: c008000006949bc4 c000000f74de3900 c0000000029bc800 c000000f88f0ab80
> [ 3051.629342] GPR04: ffffffffffffffff 0000000000000020 0000000024000282 0000000000000000
> [ 3051.629342] GPR08: c00000110628c828 0000000000000000 00000013fda70000 c008000006957550
> [ 3051.629342] GPR12: c0000000006fa5a0 c0000013ffffbe80 0000000000000000 0000000000000000
> [ 3051.629342] GPR16: 0000000000000000 0000000000000000 00000000100555f8 0000000010050d40
> [ 3051.629342] GPR20: 0000000000000000 0000000010026188 0000000010026160 c000000f88f0ac08
> [ 3051.629342] GPR24: 0000000000000000 c000000f88f0a920 0000000000000000 0000000000000002
> [ 3051.629342] GPR28: c000000f88f0ac50 c000000f88f0a800 c000000fc5577d00 c000000f88f0ab80
> [ 3051.629468] NIP [c0000000006fa5cc] percpu_counter_add_batch+0x2c/0xf0
> [ 3051.629493] LR [c008000006949bc4] __jbd2_journal_remove_checkpoint+0x9c/0x280 [jbd2]
> [ 3051.629526] Call Trace:
> [ 3051.629532] [c000000f74de3900] [c000000f88f0a84c] 0xc000000f88f0a84c (unreliable)
> [ 3051.629547] [c000000f74de3940] [c008000006949bc4] __jbd2_journal_remove_checkpoint+0x9c/0x280 [jbd2]
> [ 3051.629577] [c000000f74de3980] [c008000006949eb4] jbd2_log_do_checkpoint+0x10c/0x630 [jbd2]
> [ 3051.629605] [c000000f74de3a40] [c0080000069547dc] jbd2_journal_destroy+0x1b4/0x4e0 [jbd2]
> [ 3051.629636] [c000000f74de3ad0] [c00800000735d72c] ext4_put_super+0xb4/0x560 [ext4]
> [ 3051.629703] [c000000f74de3b60] [c000000000484d64] generic_shutdown_super+0xc4/0x1d0
> [ 3051.629720] [c000000f74de3bd0] [c000000000484f48] kill_block_super+0x38/0x90
> [ 3051.629736] [c000000f74de3c00] [c000000000485120] deactivate_locked_super+0x80/0x100
> [ 3051.629752] [c000000f74de3c30] [c0000000004bec1c] cleanup_mnt+0x10c/0x1d0
> [ 3051.629767] [c000000f74de3c80] [c000000000188b08] task_work_run+0xf8/0x170
> [ 3051.629783] [c000000f74de3cd0] [c000000000021a24] do_notify_resume+0x434/0x480
> [ 3051.629800] [c000000f74de3d80] [c000000000032910] interrupt_exit_user_prepare_main+0x1a0/0x260
> [ 3051.629816] [c000000f74de3de0] [c000000000032d08] syscall_exit_prepare+0x68/0x150
> [ 3051.629830] [c000000f74de3e10] [c00000000000c770] system_call_common+0x100/0x258
> [ 3051.629846] --- interrupt: c00 at 0x7fffa2b92ffc
> [ 3051.629855] NIP: 00007fffa2b92ffc LR: 00007fffa2b92fcc CTR: 0000000000000000
> [ 3051.629867] REGS: c000000f74de3e80 TRAP: 0c00 Tainted: G W OE (5.13.0-next-20210701)
> [ 3051.629880] MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 24000474 XER: 00000000
> [ 3051.629908] IRQMASK: 0
> [ 3051.629908] GPR00: 0000000000000034 00007fffc0242e20 00007fffa2c77100 0000000000000000
> [ 3051.629908] GPR04: 0000000000000000 0000000000000078 0000000000000000 0000000000000020
> [ 3051.629908] GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [ 3051.629908] GPR12: 0000000000000000 00007fffa2d1a310 0000000000000000 0000000000000000
> [ 3051.629908] GPR16: 0000000000000000 0000000000000000 00000000100555f8 0000000010050d40
> [ 3051.629908] GPR20: 0000000000000000 0000000010026188 0000000010026160 00000000100288f0
> [ 3051.629908] GPR24: 00007fffa2d13320 00000000000186a0 0000000010025dd8 0000000010055688
> [ 3051.629908] GPR28: 0000000010024bb8 0000000000000001 0000000000000001 0000000000000000
> [ 3051.630022] NIP [00007fffa2b92ffc] 0x7fffa2b92ffc
> [ 3051.630032] LR [00007fffa2b92fcc] 0x7fffa2b92fcc
> [ 3051.630041] --- interrupt: c00
> [ 3051.630048] Instruction dump:
> [ 3051.630057] 60000000 3c4c022c 38422260 7c0802a6 fbe1fff8 fba1ffe8 7c7f1b78 fbc1fff0
> [ 3051.630078] f8010010 f821ffc1 e94d0030 e9230020 <7fca4aaa> 7fbe2214 7fa9fe76 7d2aea78
> [ 3051.630102] ---[ end trace 83afe3a19212c333 ]---
> [ 3051.633656]
> [ 3052.633681] Kernel panic - not syncing: Fatal exception
>
> 5.13.0-next-20210630 was good. Bisect points to following patch:
>
> commit 4ba3fcdde7e3
> jbd2,ext4: add a shrinker to release checkpointed buffers
>
> Reverting this patch allows the test to run successfully.
I guess the problem is j_jh_shrink_count was destroyed in ext4_put_super
_> jbd2_journal_unregister_shrinker
which is before the path ext4_put_super -> jbd2_journal_destroy ->
jbd2_log_do_checkpoint to call
percpu_counter_dec(&journal->j_jh_shrink_count).
And since jbd2_journal_unregister_shrinker is already called inside
jbd2_journal_destroy, does it make sense
to do this?
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1176,7 +1176,6 @@ static void ext4_put_super(struct super_block *sb)
ext4_unregister_sysfs(sb);
if (sbi->s_journal) {
- jbd2_journal_unregister_shrinker(sbi->s_journal);
aborted = is_journal_aborted(sbi->s_journal);
err = jbd2_journal_destroy(sbi->s_journal);
sbi->s_journal = NULL;
Thanks,
Guoqing
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
@ 2021-07-02 9:38 ` Guoqing Jiang
0 siblings, 0 replies; 35+ messages in thread
From: Guoqing Jiang @ 2021-07-02 9:38 UTC (permalink / raw)
To: Sachin Sant, linux-ext4, linux-fsdevel; +Cc: jack, linuxppc-dev, yi.zhang
On 7/2/21 4:51 PM, Sachin Sant wrote:
> While running LTP tests (chdir01) against 5.13.0-next20210701 booted on a Power server,
> following crash is encountered.
>
> [ 3051.182992] ext2 filesystem being mounted at /var/tmp/avocado_oau90dri/ltp-W0cFB5HtCy/lKhal5/mntpoint supports timestamps until 2038 (0x7fffffff)
> [ 3051.621341] EXT4-fs (loop0): mounting ext3 file system using the ext4 subsystem
> [ 3051.624645] EXT4-fs (loop0): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
> [ 3051.624682] ext3 filesystem being mounted at /var/tmp/avocado_oau90dri/ltp-W0cFB5HtCy/lKhal5/mntpoint supports timestamps until 2038 (0x7fffffff)
> [ 3051.629026] Kernel attempted to read user page (13fda70000) - exploit attempt? (uid: 0)
> [ 3051.629074] BUG: Unable to handle kernel data access on read at 0x13fda70000
> [ 3051.629103] Faulting instruction address: 0xc0000000006fa5cc
> [ 3051.629118] Oops: Kernel access of bad area, sig: 11 [#1]
> [ 3051.629130] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
> [ 3051.629148] Modules linked in: vfat fat btrfs blake2b_generic xor zstd_compress raid6_pq xfs loop sctp ip6_udp_tunnel udp_tunnel libcrc32c rpadlpar_io rpaphp dm_mod bonding rfkill sunrpc pseries_rng xts vmx_crypto uio_pdrv_genirq uio sch_fq_codel ip_tables ext4 mbcache jbd2 sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp fuse [last unloaded: test_cpuidle_latency]
> [ 3051.629270] CPU: 10 PID: 274044 Comm: chdir01 Tainted: G W OE 5.13.0-next-20210701 #1
> [ 3051.629289] NIP: c0000000006fa5cc LR: c008000006949bc4 CTR: c0000000006fa5a0
> [ 3051.629300] REGS: c000000f74de3660 TRAP: 0300 Tainted: G W OE (5.13.0-next-20210701)
> [ 3051.629314] MSR: 800000000280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 24000288 XER: 20040000
> [ 3051.629342] CFAR: c008000006957564 DAR: 00000013fda70000 DSISR: 40000000 IRQMASK: 0
> [ 3051.629342] GPR00: c008000006949bc4 c000000f74de3900 c0000000029bc800 c000000f88f0ab80
> [ 3051.629342] GPR04: ffffffffffffffff 0000000000000020 0000000024000282 0000000000000000
> [ 3051.629342] GPR08: c00000110628c828 0000000000000000 00000013fda70000 c008000006957550
> [ 3051.629342] GPR12: c0000000006fa5a0 c0000013ffffbe80 0000000000000000 0000000000000000
> [ 3051.629342] GPR16: 0000000000000000 0000000000000000 00000000100555f8 0000000010050d40
> [ 3051.629342] GPR20: 0000000000000000 0000000010026188 0000000010026160 c000000f88f0ac08
> [ 3051.629342] GPR24: 0000000000000000 c000000f88f0a920 0000000000000000 0000000000000002
> [ 3051.629342] GPR28: c000000f88f0ac50 c000000f88f0a800 c000000fc5577d00 c000000f88f0ab80
> [ 3051.629468] NIP [c0000000006fa5cc] percpu_counter_add_batch+0x2c/0xf0
> [ 3051.629493] LR [c008000006949bc4] __jbd2_journal_remove_checkpoint+0x9c/0x280 [jbd2]
> [ 3051.629526] Call Trace:
> [ 3051.629532] [c000000f74de3900] [c000000f88f0a84c] 0xc000000f88f0a84c (unreliable)
> [ 3051.629547] [c000000f74de3940] [c008000006949bc4] __jbd2_journal_remove_checkpoint+0x9c/0x280 [jbd2]
> [ 3051.629577] [c000000f74de3980] [c008000006949eb4] jbd2_log_do_checkpoint+0x10c/0x630 [jbd2]
> [ 3051.629605] [c000000f74de3a40] [c0080000069547dc] jbd2_journal_destroy+0x1b4/0x4e0 [jbd2]
> [ 3051.629636] [c000000f74de3ad0] [c00800000735d72c] ext4_put_super+0xb4/0x560 [ext4]
> [ 3051.629703] [c000000f74de3b60] [c000000000484d64] generic_shutdown_super+0xc4/0x1d0
> [ 3051.629720] [c000000f74de3bd0] [c000000000484f48] kill_block_super+0x38/0x90
> [ 3051.629736] [c000000f74de3c00] [c000000000485120] deactivate_locked_super+0x80/0x100
> [ 3051.629752] [c000000f74de3c30] [c0000000004bec1c] cleanup_mnt+0x10c/0x1d0
> [ 3051.629767] [c000000f74de3c80] [c000000000188b08] task_work_run+0xf8/0x170
> [ 3051.629783] [c000000f74de3cd0] [c000000000021a24] do_notify_resume+0x434/0x480
> [ 3051.629800] [c000000f74de3d80] [c000000000032910] interrupt_exit_user_prepare_main+0x1a0/0x260
> [ 3051.629816] [c000000f74de3de0] [c000000000032d08] syscall_exit_prepare+0x68/0x150
> [ 3051.629830] [c000000f74de3e10] [c00000000000c770] system_call_common+0x100/0x258
> [ 3051.629846] --- interrupt: c00 at 0x7fffa2b92ffc
> [ 3051.629855] NIP: 00007fffa2b92ffc LR: 00007fffa2b92fcc CTR: 0000000000000000
> [ 3051.629867] REGS: c000000f74de3e80 TRAP: 0c00 Tainted: G W OE (5.13.0-next-20210701)
> [ 3051.629880] MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 24000474 XER: 00000000
> [ 3051.629908] IRQMASK: 0
> [ 3051.629908] GPR00: 0000000000000034 00007fffc0242e20 00007fffa2c77100 0000000000000000
> [ 3051.629908] GPR04: 0000000000000000 0000000000000078 0000000000000000 0000000000000020
> [ 3051.629908] GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [ 3051.629908] GPR12: 0000000000000000 00007fffa2d1a310 0000000000000000 0000000000000000
> [ 3051.629908] GPR16: 0000000000000000 0000000000000000 00000000100555f8 0000000010050d40
> [ 3051.629908] GPR20: 0000000000000000 0000000010026188 0000000010026160 00000000100288f0
> [ 3051.629908] GPR24: 00007fffa2d13320 00000000000186a0 0000000010025dd8 0000000010055688
> [ 3051.629908] GPR28: 0000000010024bb8 0000000000000001 0000000000000001 0000000000000000
> [ 3051.630022] NIP [00007fffa2b92ffc] 0x7fffa2b92ffc
> [ 3051.630032] LR [00007fffa2b92fcc] 0x7fffa2b92fcc
> [ 3051.630041] --- interrupt: c00
> [ 3051.630048] Instruction dump:
> [ 3051.630057] 60000000 3c4c022c 38422260 7c0802a6 fbe1fff8 fba1ffe8 7c7f1b78 fbc1fff0
> [ 3051.630078] f8010010 f821ffc1 e94d0030 e9230020 <7fca4aaa> 7fbe2214 7fa9fe76 7d2aea78
> [ 3051.630102] ---[ end trace 83afe3a19212c333 ]---
> [ 3051.633656]
> [ 3052.633681] Kernel panic - not syncing: Fatal exception
>
> 5.13.0-next-20210630 was good. Bisect points to following patch:
>
> commit 4ba3fcdde7e3
> jbd2,ext4: add a shrinker to release checkpointed buffers
>
> Reverting this patch allows the test to run successfully.
I guess the problem is j_jh_shrink_count was destroyed in ext4_put_super
_> jbd2_journal_unregister_shrinker
which is before the path ext4_put_super -> jbd2_journal_destroy ->
jbd2_log_do_checkpoint to call
percpu_counter_dec(&journal->j_jh_shrink_count).
And since jbd2_journal_unregister_shrinker is already called inside
jbd2_journal_destroy, does it make sense
to do this?
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1176,7 +1176,6 @@ static void ext4_put_super(struct super_block *sb)
ext4_unregister_sysfs(sb);
if (sbi->s_journal) {
- jbd2_journal_unregister_shrinker(sbi->s_journal);
aborted = is_journal_aborted(sbi->s_journal);
err = jbd2_journal_destroy(sbi->s_journal);
sbi->s_journal = NULL;
Thanks,
Guoqing
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
2021-07-02 9:38 ` Guoqing Jiang
@ 2021-07-02 13:13 ` Theodore Ts'o
-1 siblings, 0 replies; 35+ messages in thread
From: Theodore Ts'o @ 2021-07-02 13:13 UTC (permalink / raw)
To: Guoqing Jiang
Cc: Sachin Sant, linux-ext4, linux-fsdevel, linuxppc-dev, yi.zhang, jack
On Fri, Jul 02, 2021 at 05:38:10PM +0800, Guoqing Jiang wrote:
>
>
> I guess the problem is j_jh_shrink_count was destroyed in ext4_put_super _>
> jbd2_journal_unregister_shrinker
> which is before the path ext4_put_super -> jbd2_journal_destroy ->
> jbd2_log_do_checkpoint to call
> percpu_counter_dec(&journal->j_jh_shrink_count).
>
> And since jbd2_journal_unregister_shrinker is already called inside
> jbd2_journal_destroy, does it make sense
> to do this?
>
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1176,7 +1176,6 @@ static void ext4_put_super(struct super_block *sb)
> ext4_unregister_sysfs(sb);
>
> if (sbi->s_journal) {
> - jbd2_journal_unregister_shrinker(sbi->s_journal);
> aborted = is_journal_aborted(sbi->s_journal);
> err = jbd2_journal_destroy(sbi->s_journal);
> sbi->s_journal = NULL;
Good catch. There's another place where we call
jbd2_journal_unregister_shrinker(), in the failure path for
ext4_fill_super().
- Ted
P.S. Whatever outgoing mailer you are using, it's not preserving TAB
characters correctly. You might want to look into that before trying
to submit a patch.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
@ 2021-07-02 13:13 ` Theodore Ts'o
0 siblings, 0 replies; 35+ messages in thread
From: Theodore Ts'o @ 2021-07-02 13:13 UTC (permalink / raw)
To: Guoqing Jiang
Cc: Sachin Sant, jack, yi.zhang, linux-fsdevel, linux-ext4, linuxppc-dev
On Fri, Jul 02, 2021 at 05:38:10PM +0800, Guoqing Jiang wrote:
>
>
> I guess the problem is j_jh_shrink_count was destroyed in ext4_put_super _>
> jbd2_journal_unregister_shrinker
> which is before the path ext4_put_super -> jbd2_journal_destroy ->
> jbd2_log_do_checkpoint to call
> percpu_counter_dec(&journal->j_jh_shrink_count).
>
> And since jbd2_journal_unregister_shrinker is already called inside
> jbd2_journal_destroy, does it make sense
> to do this?
>
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1176,7 +1176,6 @@ static void ext4_put_super(struct super_block *sb)
> ext4_unregister_sysfs(sb);
>
> if (sbi->s_journal) {
> - jbd2_journal_unregister_shrinker(sbi->s_journal);
> aborted = is_journal_aborted(sbi->s_journal);
> err = jbd2_journal_destroy(sbi->s_journal);
> sbi->s_journal = NULL;
Good catch. There's another place where we call
jbd2_journal_unregister_shrinker(), in the failure path for
ext4_fill_super().
- Ted
P.S. Whatever outgoing mailer you are using, it's not preserving TAB
characters correctly. You might want to look into that before trying
to submit a patch.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
2021-07-02 9:38 ` Guoqing Jiang
@ 2021-07-02 13:23 ` Zhang Yi
-1 siblings, 0 replies; 35+ messages in thread
From: Zhang Yi @ 2021-07-02 13:23 UTC (permalink / raw)
To: Guoqing Jiang, Sachin Sant, linux-ext4, linux-fsdevel
Cc: linuxppc-dev, jack, Theodore Ts'o
On 2021/7/2 17:38, Guoqing Jiang wrote:
>
>
> On 7/2/21 4:51 PM, Sachin Sant wrote:
>> While running LTP tests (chdir01) against 5.13.0-next20210701 booted on a Power server,
>> following crash is encountered.
>>
>> [ 3051.182992] ext2 filesystem being mounted at /var/tmp/avocado_oau90dri/ltp-W0cFB5HtCy/lKhal5/mntpoint supports timestamps until 2038 (0x7fffffff)
>> [ 3051.621341] EXT4-fs (loop0): mounting ext3 file system using the ext4 subsystem
>> [ 3051.624645] EXT4-fs (loop0): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
>> [ 3051.624682] ext3 filesystem being mounted at /var/tmp/avocado_oau90dri/ltp-W0cFB5HtCy/lKhal5/mntpoint supports timestamps until 2038 (0x7fffffff)
>> [ 3051.629026] Kernel attempted to read user page (13fda70000) - exploit attempt? (uid: 0)
>> [ 3051.629074] BUG: Unable to handle kernel data access on read at 0x13fda70000
>> [ 3051.629103] Faulting instruction address: 0xc0000000006fa5cc
>> [ 3051.629118] Oops: Kernel access of bad area, sig: 11 [#1]
>> [ 3051.629130] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
>> [ 3051.629148] Modules linked in: vfat fat btrfs blake2b_generic xor zstd_compress raid6_pq xfs loop sctp ip6_udp_tunnel udp_tunnel libcrc32c rpadlpar_io rpaphp dm_mod bonding rfkill sunrpc pseries_rng xts vmx_crypto uio_pdrv_genirq uio sch_fq_codel ip_tables ext4 mbcache jbd2 sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp fuse [last unloaded: test_cpuidle_latency]
>> [ 3051.629270] CPU: 10 PID: 274044 Comm: chdir01 Tainted: G W OE 5.13.0-next-20210701 #1
>> [ 3051.629289] NIP: c0000000006fa5cc LR: c008000006949bc4 CTR: c0000000006fa5a0
>> [ 3051.629300] REGS: c000000f74de3660 TRAP: 0300 Tainted: G W OE (5.13.0-next-20210701)
>> [ 3051.629314] MSR: 800000000280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 24000288 XER: 20040000
>> [ 3051.629342] CFAR: c008000006957564 DAR: 00000013fda70000 DSISR: 40000000 IRQMASK: 0
>> [ 3051.629342] GPR00: c008000006949bc4 c000000f74de3900 c0000000029bc800 c000000f88f0ab80
>> [ 3051.629342] GPR04: ffffffffffffffff 0000000000000020 0000000024000282 0000000000000000
>> [ 3051.629342] GPR08: c00000110628c828 0000000000000000 00000013fda70000 c008000006957550
>> [ 3051.629342] GPR12: c0000000006fa5a0 c0000013ffffbe80 0000000000000000 0000000000000000
>> [ 3051.629342] GPR16: 0000000000000000 0000000000000000 00000000100555f8 0000000010050d40
>> [ 3051.629342] GPR20: 0000000000000000 0000000010026188 0000000010026160 c000000f88f0ac08
>> [ 3051.629342] GPR24: 0000000000000000 c000000f88f0a920 0000000000000000 0000000000000002
>> [ 3051.629342] GPR28: c000000f88f0ac50 c000000f88f0a800 c000000fc5577d00 c000000f88f0ab80
>> [ 3051.629468] NIP [c0000000006fa5cc] percpu_counter_add_batch+0x2c/0xf0
>> [ 3051.629493] LR [c008000006949bc4] __jbd2_journal_remove_checkpoint+0x9c/0x280 [jbd2]
>> [ 3051.629526] Call Trace:
>> [ 3051.629532] [c000000f74de3900] [c000000f88f0a84c] 0xc000000f88f0a84c (unreliable)
>> [ 3051.629547] [c000000f74de3940] [c008000006949bc4] __jbd2_journal_remove_checkpoint+0x9c/0x280 [jbd2]
>> [ 3051.629577] [c000000f74de3980] [c008000006949eb4] jbd2_log_do_checkpoint+0x10c/0x630 [jbd2]
>> [ 3051.629605] [c000000f74de3a40] [c0080000069547dc] jbd2_journal_destroy+0x1b4/0x4e0 [jbd2]
>> [ 3051.629636] [c000000f74de3ad0] [c00800000735d72c] ext4_put_super+0xb4/0x560 [ext4]
>> [ 3051.629703] [c000000f74de3b60] [c000000000484d64] generic_shutdown_super+0xc4/0x1d0
>> [ 3051.629720] [c000000f74de3bd0] [c000000000484f48] kill_block_super+0x38/0x90
>> [ 3051.629736] [c000000f74de3c00] [c000000000485120] deactivate_locked_super+0x80/0x100
>> [ 3051.629752] [c000000f74de3c30] [c0000000004bec1c] cleanup_mnt+0x10c/0x1d0
>> [ 3051.629767] [c000000f74de3c80] [c000000000188b08] task_work_run+0xf8/0x170
>> [ 3051.629783] [c000000f74de3cd0] [c000000000021a24] do_notify_resume+0x434/0x480
>> [ 3051.629800] [c000000f74de3d80] [c000000000032910] interrupt_exit_user_prepare_main+0x1a0/0x260
>> [ 3051.629816] [c000000f74de3de0] [c000000000032d08] syscall_exit_prepare+0x68/0x150
>> [ 3051.629830] [c000000f74de3e10] [c00000000000c770] system_call_common+0x100/0x258
>> [ 3051.629846] --- interrupt: c00 at 0x7fffa2b92ffc
>> [ 3051.629855] NIP: 00007fffa2b92ffc LR: 00007fffa2b92fcc CTR: 0000000000000000
>> [ 3051.629867] REGS: c000000f74de3e80 TRAP: 0c00 Tainted: G W OE (5.13.0-next-20210701)
>> [ 3051.629880] MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 24000474 XER: 00000000
>> [ 3051.629908] IRQMASK: 0
>> [ 3051.629908] GPR00: 0000000000000034 00007fffc0242e20 00007fffa2c77100 0000000000000000
>> [ 3051.629908] GPR04: 0000000000000000 0000000000000078 0000000000000000 0000000000000020
>> [ 3051.629908] GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> [ 3051.629908] GPR12: 0000000000000000 00007fffa2d1a310 0000000000000000 0000000000000000
>> [ 3051.629908] GPR16: 0000000000000000 0000000000000000 00000000100555f8 0000000010050d40
>> [ 3051.629908] GPR20: 0000000000000000 0000000010026188 0000000010026160 00000000100288f0
>> [ 3051.629908] GPR24: 00007fffa2d13320 00000000000186a0 0000000010025dd8 0000000010055688
>> [ 3051.629908] GPR28: 0000000010024bb8 0000000000000001 0000000000000001 0000000000000000
>> [ 3051.630022] NIP [00007fffa2b92ffc] 0x7fffa2b92ffc
>> [ 3051.630032] LR [00007fffa2b92fcc] 0x7fffa2b92fcc
>> [ 3051.630041] --- interrupt: c00
>> [ 3051.630048] Instruction dump:
>> [ 3051.630057] 60000000 3c4c022c 38422260 7c0802a6 fbe1fff8 fba1ffe8 7c7f1b78 fbc1fff0
>> [ 3051.630078] f8010010 f821ffc1 e94d0030 e9230020 <7fca4aaa> 7fbe2214 7fa9fe76 7d2aea78
>> [ 3051.630102] ---[ end trace 83afe3a19212c333 ]---
>> [ 3051.633656]
>> [ 3052.633681] Kernel panic - not syncing: Fatal exception
>>
>> 5.13.0-next-20210630 was good. Bisect points to following patch:
>>
>> commit 4ba3fcdde7e3
>> jbd2,ext4: add a shrinker to release checkpointed buffers
>>
>> Reverting this patch allows the test to run successfully.
>
> I guess the problem is j_jh_shrink_count was destroyed in ext4_put_super _> jbd2_journal_unregister_shrinker
> which is before the path ext4_put_super -> jbd2_journal_destroy -> jbd2_log_do_checkpoint to call
> percpu_counter_dec(&journal->j_jh_shrink_count).
>
> And since jbd2_journal_unregister_shrinker is already called inside jbd2_journal_destroy, does it make sense
> to do this?
>
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1176,7 +1176,6 @@ static void ext4_put_super(struct super_block *sb)
> ext4_unregister_sysfs(sb);
>
> if (sbi->s_journal) {
> - jbd2_journal_unregister_shrinker(sbi->s_journal);
> aborted = is_journal_aborted(sbi->s_journal);
> err = jbd2_journal_destroy(sbi->s_journal);
> sbi->s_journal = NULL;
>
Hi Guoqing,
Thanks for your analyze. This problem cannot reproduce on x86_64 but 100% reproduce on arm64,
it depends on the percpu counter code on different architecture.
Indeed, as you said, the real problem is invoke percpu_counter_dec(&journal->j_jh_shrink_count)
after it was destroyed during umount, and I'm afraid that it may also affect ocfs2
because it doesn't initialize the percpu counter before doing add/sub in
__jbd2_journal_[insert|remove]_checkpoint().
I think the quick fix could be:
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 152880c298ca..48c7e5d17b38 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1352,17 +1352,23 @@ static journal_t *journal_init_common(struct block_device *bdev,
if (!journal->j_wbuf)
goto err_cleanup;
+ err = percpu_counter_init(&journal->j_jh_shrink_count, 0, GFP_KERNEL);
+ if (err)
+ goto err_cleanup;
+
bh = getblk_unmovable(journal->j_dev, start, journal->j_blocksize);
if (!bh) {
pr_err("%s: Cannot get buffer for journal superblock\n",
__func__);
- goto err_cleanup;
+ goto err_cleanup_cnt;
}
journal->j_sb_buffer = bh;
journal->j_superblock = (journal_superblock_t *)bh->b_data;
return journal;
+err_cleanup_cnt:
+ percpu_counter_destroy(&journal->j_jh_shrink_count);
err_cleanup:
kfree(journal->j_wbuf);
jbd2_journal_destroy_revoke(journal);
@@ -2101,26 +2107,13 @@ static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
*/
int jbd2_journal_register_shrinker(journal_t *journal)
{
- int err;
-
journal->j_shrink_transaction = NULL;
-
- err = percpu_counter_init(&journal->j_jh_shrink_count, 0, GFP_KERNEL);
- if (err)
- return err;
-
journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
journal->j_shrinker.seeks = DEFAULT_SEEKS;
journal->j_shrinker.batch = journal->j_max_transaction_buffers;
- err = register_shrinker(&journal->j_shrinker);
- if (err) {
- percpu_counter_destroy(&journal->j_jh_shrink_count);
- return err;
- }
-
- return 0;
+ return register_shrinker(&journal->j_shrinker);
}
EXPORT_SYMBOL(jbd2_journal_register_shrinker);
@@ -2132,7 +2125,6 @@ EXPORT_SYMBOL(jbd2_journal_register_shrinker);
*/
void jbd2_journal_unregister_shrinker(journal_t *journal)
{
- percpu_counter_destroy(&journal->j_jh_shrink_count);
unregister_shrinker(&journal->j_shrinker);
}
EXPORT_SYMBOL(jbd2_journal_unregister_shrinker);
@@ -2209,8 +2201,6 @@ int jbd2_journal_destroy(journal_t *journal)
brelse(journal->j_sb_buffer);
}
- jbd2_journal_unregister_shrinker(journal);
-
if (journal->j_proc_entry)
jbd2_stats_proc_exit(journal);
iput(journal->j_inode);
@@ -2220,6 +2210,7 @@ int jbd2_journal_destroy(journal_t *journal)
crypto_free_shash(journal->j_chksum_driver);
kfree(journal->j_fc_wbuf);
kfree(journal->j_wbuf);
+ percpu_counter_destroy(&journal->j_jh_shrink_count);
kfree(journal);
return err;
Thanks,
Yi.
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
@ 2021-07-02 13:23 ` Zhang Yi
0 siblings, 0 replies; 35+ messages in thread
From: Zhang Yi @ 2021-07-02 13:23 UTC (permalink / raw)
To: Guoqing Jiang, Sachin Sant, linux-ext4, linux-fsdevel
Cc: jack, linuxppc-dev, Theodore Ts'o
On 2021/7/2 17:38, Guoqing Jiang wrote:
>
>
> On 7/2/21 4:51 PM, Sachin Sant wrote:
>> While running LTP tests (chdir01) against 5.13.0-next20210701 booted on a Power server,
>> following crash is encountered.
>>
>> [ 3051.182992] ext2 filesystem being mounted at /var/tmp/avocado_oau90dri/ltp-W0cFB5HtCy/lKhal5/mntpoint supports timestamps until 2038 (0x7fffffff)
>> [ 3051.621341] EXT4-fs (loop0): mounting ext3 file system using the ext4 subsystem
>> [ 3051.624645] EXT4-fs (loop0): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
>> [ 3051.624682] ext3 filesystem being mounted at /var/tmp/avocado_oau90dri/ltp-W0cFB5HtCy/lKhal5/mntpoint supports timestamps until 2038 (0x7fffffff)
>> [ 3051.629026] Kernel attempted to read user page (13fda70000) - exploit attempt? (uid: 0)
>> [ 3051.629074] BUG: Unable to handle kernel data access on read at 0x13fda70000
>> [ 3051.629103] Faulting instruction address: 0xc0000000006fa5cc
>> [ 3051.629118] Oops: Kernel access of bad area, sig: 11 [#1]
>> [ 3051.629130] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
>> [ 3051.629148] Modules linked in: vfat fat btrfs blake2b_generic xor zstd_compress raid6_pq xfs loop sctp ip6_udp_tunnel udp_tunnel libcrc32c rpadlpar_io rpaphp dm_mod bonding rfkill sunrpc pseries_rng xts vmx_crypto uio_pdrv_genirq uio sch_fq_codel ip_tables ext4 mbcache jbd2 sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp fuse [last unloaded: test_cpuidle_latency]
>> [ 3051.629270] CPU: 10 PID: 274044 Comm: chdir01 Tainted: G W OE 5.13.0-next-20210701 #1
>> [ 3051.629289] NIP: c0000000006fa5cc LR: c008000006949bc4 CTR: c0000000006fa5a0
>> [ 3051.629300] REGS: c000000f74de3660 TRAP: 0300 Tainted: G W OE (5.13.0-next-20210701)
>> [ 3051.629314] MSR: 800000000280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 24000288 XER: 20040000
>> [ 3051.629342] CFAR: c008000006957564 DAR: 00000013fda70000 DSISR: 40000000 IRQMASK: 0
>> [ 3051.629342] GPR00: c008000006949bc4 c000000f74de3900 c0000000029bc800 c000000f88f0ab80
>> [ 3051.629342] GPR04: ffffffffffffffff 0000000000000020 0000000024000282 0000000000000000
>> [ 3051.629342] GPR08: c00000110628c828 0000000000000000 00000013fda70000 c008000006957550
>> [ 3051.629342] GPR12: c0000000006fa5a0 c0000013ffffbe80 0000000000000000 0000000000000000
>> [ 3051.629342] GPR16: 0000000000000000 0000000000000000 00000000100555f8 0000000010050d40
>> [ 3051.629342] GPR20: 0000000000000000 0000000010026188 0000000010026160 c000000f88f0ac08
>> [ 3051.629342] GPR24: 0000000000000000 c000000f88f0a920 0000000000000000 0000000000000002
>> [ 3051.629342] GPR28: c000000f88f0ac50 c000000f88f0a800 c000000fc5577d00 c000000f88f0ab80
>> [ 3051.629468] NIP [c0000000006fa5cc] percpu_counter_add_batch+0x2c/0xf0
>> [ 3051.629493] LR [c008000006949bc4] __jbd2_journal_remove_checkpoint+0x9c/0x280 [jbd2]
>> [ 3051.629526] Call Trace:
>> [ 3051.629532] [c000000f74de3900] [c000000f88f0a84c] 0xc000000f88f0a84c (unreliable)
>> [ 3051.629547] [c000000f74de3940] [c008000006949bc4] __jbd2_journal_remove_checkpoint+0x9c/0x280 [jbd2]
>> [ 3051.629577] [c000000f74de3980] [c008000006949eb4] jbd2_log_do_checkpoint+0x10c/0x630 [jbd2]
>> [ 3051.629605] [c000000f74de3a40] [c0080000069547dc] jbd2_journal_destroy+0x1b4/0x4e0 [jbd2]
>> [ 3051.629636] [c000000f74de3ad0] [c00800000735d72c] ext4_put_super+0xb4/0x560 [ext4]
>> [ 3051.629703] [c000000f74de3b60] [c000000000484d64] generic_shutdown_super+0xc4/0x1d0
>> [ 3051.629720] [c000000f74de3bd0] [c000000000484f48] kill_block_super+0x38/0x90
>> [ 3051.629736] [c000000f74de3c00] [c000000000485120] deactivate_locked_super+0x80/0x100
>> [ 3051.629752] [c000000f74de3c30] [c0000000004bec1c] cleanup_mnt+0x10c/0x1d0
>> [ 3051.629767] [c000000f74de3c80] [c000000000188b08] task_work_run+0xf8/0x170
>> [ 3051.629783] [c000000f74de3cd0] [c000000000021a24] do_notify_resume+0x434/0x480
>> [ 3051.629800] [c000000f74de3d80] [c000000000032910] interrupt_exit_user_prepare_main+0x1a0/0x260
>> [ 3051.629816] [c000000f74de3de0] [c000000000032d08] syscall_exit_prepare+0x68/0x150
>> [ 3051.629830] [c000000f74de3e10] [c00000000000c770] system_call_common+0x100/0x258
>> [ 3051.629846] --- interrupt: c00 at 0x7fffa2b92ffc
>> [ 3051.629855] NIP: 00007fffa2b92ffc LR: 00007fffa2b92fcc CTR: 0000000000000000
>> [ 3051.629867] REGS: c000000f74de3e80 TRAP: 0c00 Tainted: G W OE (5.13.0-next-20210701)
>> [ 3051.629880] MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 24000474 XER: 00000000
>> [ 3051.629908] IRQMASK: 0
>> [ 3051.629908] GPR00: 0000000000000034 00007fffc0242e20 00007fffa2c77100 0000000000000000
>> [ 3051.629908] GPR04: 0000000000000000 0000000000000078 0000000000000000 0000000000000020
>> [ 3051.629908] GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> [ 3051.629908] GPR12: 0000000000000000 00007fffa2d1a310 0000000000000000 0000000000000000
>> [ 3051.629908] GPR16: 0000000000000000 0000000000000000 00000000100555f8 0000000010050d40
>> [ 3051.629908] GPR20: 0000000000000000 0000000010026188 0000000010026160 00000000100288f0
>> [ 3051.629908] GPR24: 00007fffa2d13320 00000000000186a0 0000000010025dd8 0000000010055688
>> [ 3051.629908] GPR28: 0000000010024bb8 0000000000000001 0000000000000001 0000000000000000
>> [ 3051.630022] NIP [00007fffa2b92ffc] 0x7fffa2b92ffc
>> [ 3051.630032] LR [00007fffa2b92fcc] 0x7fffa2b92fcc
>> [ 3051.630041] --- interrupt: c00
>> [ 3051.630048] Instruction dump:
>> [ 3051.630057] 60000000 3c4c022c 38422260 7c0802a6 fbe1fff8 fba1ffe8 7c7f1b78 fbc1fff0
>> [ 3051.630078] f8010010 f821ffc1 e94d0030 e9230020 <7fca4aaa> 7fbe2214 7fa9fe76 7d2aea78
>> [ 3051.630102] ---[ end trace 83afe3a19212c333 ]---
>> [ 3051.633656]
>> [ 3052.633681] Kernel panic - not syncing: Fatal exception
>>
>> 5.13.0-next-20210630 was good. Bisect points to following patch:
>>
>> commit 4ba3fcdde7e3
>> jbd2,ext4: add a shrinker to release checkpointed buffers
>>
>> Reverting this patch allows the test to run successfully.
>
> I guess the problem is j_jh_shrink_count was destroyed in ext4_put_super _> jbd2_journal_unregister_shrinker
> which is before the path ext4_put_super -> jbd2_journal_destroy -> jbd2_log_do_checkpoint to call
> percpu_counter_dec(&journal->j_jh_shrink_count).
>
> And since jbd2_journal_unregister_shrinker is already called inside jbd2_journal_destroy, does it make sense
> to do this?
>
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1176,7 +1176,6 @@ static void ext4_put_super(struct super_block *sb)
> ext4_unregister_sysfs(sb);
>
> if (sbi->s_journal) {
> - jbd2_journal_unregister_shrinker(sbi->s_journal);
> aborted = is_journal_aborted(sbi->s_journal);
> err = jbd2_journal_destroy(sbi->s_journal);
> sbi->s_journal = NULL;
>
Hi Guoqing,
Thanks for your analyze. This problem cannot reproduce on x86_64 but 100% reproduce on arm64,
it depends on the percpu counter code on different architecture.
Indeed, as you said, the real problem is invoke percpu_counter_dec(&journal->j_jh_shrink_count)
after it was destroyed during umount, and I'm afraid that it may also affect ocfs2
because it doesn't initialize the percpu counter before doing add/sub in
__jbd2_journal_[insert|remove]_checkpoint().
I think the quick fix could be:
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 152880c298ca..48c7e5d17b38 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1352,17 +1352,23 @@ static journal_t *journal_init_common(struct block_device *bdev,
if (!journal->j_wbuf)
goto err_cleanup;
+ err = percpu_counter_init(&journal->j_jh_shrink_count, 0, GFP_KERNEL);
+ if (err)
+ goto err_cleanup;
+
bh = getblk_unmovable(journal->j_dev, start, journal->j_blocksize);
if (!bh) {
pr_err("%s: Cannot get buffer for journal superblock\n",
__func__);
- goto err_cleanup;
+ goto err_cleanup_cnt;
}
journal->j_sb_buffer = bh;
journal->j_superblock = (journal_superblock_t *)bh->b_data;
return journal;
+err_cleanup_cnt:
+ percpu_counter_destroy(&journal->j_jh_shrink_count);
err_cleanup:
kfree(journal->j_wbuf);
jbd2_journal_destroy_revoke(journal);
@@ -2101,26 +2107,13 @@ static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
*/
int jbd2_journal_register_shrinker(journal_t *journal)
{
- int err;
-
journal->j_shrink_transaction = NULL;
-
- err = percpu_counter_init(&journal->j_jh_shrink_count, 0, GFP_KERNEL);
- if (err)
- return err;
-
journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
journal->j_shrinker.seeks = DEFAULT_SEEKS;
journal->j_shrinker.batch = journal->j_max_transaction_buffers;
- err = register_shrinker(&journal->j_shrinker);
- if (err) {
- percpu_counter_destroy(&journal->j_jh_shrink_count);
- return err;
- }
-
- return 0;
+ return register_shrinker(&journal->j_shrinker);
}
EXPORT_SYMBOL(jbd2_journal_register_shrinker);
@@ -2132,7 +2125,6 @@ EXPORT_SYMBOL(jbd2_journal_register_shrinker);
*/
void jbd2_journal_unregister_shrinker(journal_t *journal)
{
- percpu_counter_destroy(&journal->j_jh_shrink_count);
unregister_shrinker(&journal->j_shrinker);
}
EXPORT_SYMBOL(jbd2_journal_unregister_shrinker);
@@ -2209,8 +2201,6 @@ int jbd2_journal_destroy(journal_t *journal)
brelse(journal->j_sb_buffer);
}
- jbd2_journal_unregister_shrinker(journal);
-
if (journal->j_proc_entry)
jbd2_stats_proc_exit(journal);
iput(journal->j_inode);
@@ -2220,6 +2210,7 @@ int jbd2_journal_destroy(journal_t *journal)
crypto_free_shash(journal->j_chksum_driver);
kfree(journal->j_fc_wbuf);
kfree(journal->j_wbuf);
+ percpu_counter_destroy(&journal->j_jh_shrink_count);
kfree(journal);
return err;
Thanks,
Yi.
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
2021-07-02 13:23 ` Zhang Yi
@ 2021-07-02 13:52 ` Zhang Yi
-1 siblings, 0 replies; 35+ messages in thread
From: Zhang Yi @ 2021-07-02 13:52 UTC (permalink / raw)
To: Theodore Ts'o, Jan Kara
Cc: linuxppc-dev, Guoqing Jiang, Sachin Sant, Ext4 Developers List,
linux-fsdevel
On 2021/7/2 21:23, Zhang Yi wrote:
> On 2021/7/2 17:38, Guoqing Jiang wrote:
>>
>>
>> On 7/2/21 4:51 PM, Sachin Sant wrote:
>>> While running LTP tests (chdir01) against 5.13.0-next20210701 booted on a Power server,
>>> following crash is encountered.
>>>
>>> [ 3051.182992] ext2 filesystem being mounted at /var/tmp/avocado_oau90dri/ltp-W0cFB5HtCy/lKhal5/mntpoint supports timestamps until 2038 (0x7fffffff)
>>> [ 3051.621341] EXT4-fs (loop0): mounting ext3 file system using the ext4 subsystem
>>> [ 3051.624645] EXT4-fs (loop0): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
>>> [ 3051.624682] ext3 filesystem being mounted at /var/tmp/avocado_oau90dri/ltp-W0cFB5HtCy/lKhal5/mntpoint supports timestamps until 2038 (0x7fffffff)
>>> [ 3051.629026] Kernel attempted to read user page (13fda70000) - exploit attempt? (uid: 0)
>>> [ 3051.629074] BUG: Unable to handle kernel data access on read at 0x13fda70000
>>> [ 3051.629103] Faulting instruction address: 0xc0000000006fa5cc
>>> [ 3051.629118] Oops: Kernel access of bad area, sig: 11 [#1]
>>> [ 3051.629130] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
>>> [ 3051.629148] Modules linked in: vfat fat btrfs blake2b_generic xor zstd_compress raid6_pq xfs loop sctp ip6_udp_tunnel udp_tunnel libcrc32c rpadlpar_io rpaphp dm_mod bonding rfkill sunrpc pseries_rng xts vmx_crypto uio_pdrv_genirq uio sch_fq_codel ip_tables ext4 mbcache jbd2 sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp fuse [last unloaded: test_cpuidle_latency]
>>> [ 3051.629270] CPU: 10 PID: 274044 Comm: chdir01 Tainted: G W OE 5.13.0-next-20210701 #1
>>> [ 3051.629289] NIP: c0000000006fa5cc LR: c008000006949bc4 CTR: c0000000006fa5a0
>>> [ 3051.629300] REGS: c000000f74de3660 TRAP: 0300 Tainted: G W OE (5.13.0-next-20210701)
>>> [ 3051.629314] MSR: 800000000280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 24000288 XER: 20040000
>>> [ 3051.629342] CFAR: c008000006957564 DAR: 00000013fda70000 DSISR: 40000000 IRQMASK: 0
>>> [ 3051.629342] GPR00: c008000006949bc4 c000000f74de3900 c0000000029bc800 c000000f88f0ab80
>>> [ 3051.629342] GPR04: ffffffffffffffff 0000000000000020 0000000024000282 0000000000000000
>>> [ 3051.629342] GPR08: c00000110628c828 0000000000000000 00000013fda70000 c008000006957550
>>> [ 3051.629342] GPR12: c0000000006fa5a0 c0000013ffffbe80 0000000000000000 0000000000000000
>>> [ 3051.629342] GPR16: 0000000000000000 0000000000000000 00000000100555f8 0000000010050d40
>>> [ 3051.629342] GPR20: 0000000000000000 0000000010026188 0000000010026160 c000000f88f0ac08
>>> [ 3051.629342] GPR24: 0000000000000000 c000000f88f0a920 0000000000000000 0000000000000002
>>> [ 3051.629342] GPR28: c000000f88f0ac50 c000000f88f0a800 c000000fc5577d00 c000000f88f0ab80
>>> [ 3051.629468] NIP [c0000000006fa5cc] percpu_counter_add_batch+0x2c/0xf0
>>> [ 3051.629493] LR [c008000006949bc4] __jbd2_journal_remove_checkpoint+0x9c/0x280 [jbd2]
>>> [ 3051.629526] Call Trace:
>>> [ 3051.629532] [c000000f74de3900] [c000000f88f0a84c] 0xc000000f88f0a84c (unreliable)
>>> [ 3051.629547] [c000000f74de3940] [c008000006949bc4] __jbd2_journal_remove_checkpoint+0x9c/0x280 [jbd2]
>>> [ 3051.629577] [c000000f74de3980] [c008000006949eb4] jbd2_log_do_checkpoint+0x10c/0x630 [jbd2]
>>> [ 3051.629605] [c000000f74de3a40] [c0080000069547dc] jbd2_journal_destroy+0x1b4/0x4e0 [jbd2]
>>> [ 3051.629636] [c000000f74de3ad0] [c00800000735d72c] ext4_put_super+0xb4/0x560 [ext4]
>>> [ 3051.629703] [c000000f74de3b60] [c000000000484d64] generic_shutdown_super+0xc4/0x1d0
>>> [ 3051.629720] [c000000f74de3bd0] [c000000000484f48] kill_block_super+0x38/0x90
>>> [ 3051.629736] [c000000f74de3c00] [c000000000485120] deactivate_locked_super+0x80/0x100
>>> [ 3051.629752] [c000000f74de3c30] [c0000000004bec1c] cleanup_mnt+0x10c/0x1d0
>>> [ 3051.629767] [c000000f74de3c80] [c000000000188b08] task_work_run+0xf8/0x170
>>> [ 3051.629783] [c000000f74de3cd0] [c000000000021a24] do_notify_resume+0x434/0x480
>>> [ 3051.629800] [c000000f74de3d80] [c000000000032910] interrupt_exit_user_prepare_main+0x1a0/0x260
>>> [ 3051.629816] [c000000f74de3de0] [c000000000032d08] syscall_exit_prepare+0x68/0x150
>>> [ 3051.629830] [c000000f74de3e10] [c00000000000c770] system_call_common+0x100/0x258
>>> [ 3051.629846] --- interrupt: c00 at 0x7fffa2b92ffc
>>> [ 3051.629855] NIP: 00007fffa2b92ffc LR: 00007fffa2b92fcc CTR: 0000000000000000
>>> [ 3051.629867] REGS: c000000f74de3e80 TRAP: 0c00 Tainted: G W OE (5.13.0-next-20210701)
>>> [ 3051.629880] MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 24000474 XER: 00000000
>>> [ 3051.629908] IRQMASK: 0
>>> [ 3051.629908] GPR00: 0000000000000034 00007fffc0242e20 00007fffa2c77100 0000000000000000
>>> [ 3051.629908] GPR04: 0000000000000000 0000000000000078 0000000000000000 0000000000000020
>>> [ 3051.629908] GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>> [ 3051.629908] GPR12: 0000000000000000 00007fffa2d1a310 0000000000000000 0000000000000000
>>> [ 3051.629908] GPR16: 0000000000000000 0000000000000000 00000000100555f8 0000000010050d40
>>> [ 3051.629908] GPR20: 0000000000000000 0000000010026188 0000000010026160 00000000100288f0
>>> [ 3051.629908] GPR24: 00007fffa2d13320 00000000000186a0 0000000010025dd8 0000000010055688
>>> [ 3051.629908] GPR28: 0000000010024bb8 0000000000000001 0000000000000001 0000000000000000
>>> [ 3051.630022] NIP [00007fffa2b92ffc] 0x7fffa2b92ffc
>>> [ 3051.630032] LR [00007fffa2b92fcc] 0x7fffa2b92fcc
>>> [ 3051.630041] --- interrupt: c00
>>> [ 3051.630048] Instruction dump:
>>> [ 3051.630057] 60000000 3c4c022c 38422260 7c0802a6 fbe1fff8 fba1ffe8 7c7f1b78 fbc1fff0
>>> [ 3051.630078] f8010010 f821ffc1 e94d0030 e9230020 <7fca4aaa> 7fbe2214 7fa9fe76 7d2aea78
>>> [ 3051.630102] ---[ end trace 83afe3a19212c333 ]---
>>> [ 3051.633656]
>>> [ 3052.633681] Kernel panic - not syncing: Fatal exception
>>>
>>> 5.13.0-next-20210630 was good. Bisect points to following patch:
>>>
>>> commit 4ba3fcdde7e3
>>> jbd2,ext4: add a shrinker to release checkpointed buffers
>>>
>>> Reverting this patch allows the test to run successfully.
>>
>> I guess the problem is j_jh_shrink_count was destroyed in ext4_put_super _> jbd2_journal_unregister_shrinker
>> which is before the path ext4_put_super -> jbd2_journal_destroy -> jbd2_log_do_checkpoint to call
>> percpu_counter_dec(&journal->j_jh_shrink_count).
>>
>> And since jbd2_journal_unregister_shrinker is already called inside jbd2_journal_destroy, does it make sense
>> to do this?
>>
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -1176,7 +1176,6 @@ static void ext4_put_super(struct super_block *sb)
>> ext4_unregister_sysfs(sb);
>>
>> if (sbi->s_journal) {
>> - jbd2_journal_unregister_shrinker(sbi->s_journal);
>> aborted = is_journal_aborted(sbi->s_journal);
>> err = jbd2_journal_destroy(sbi->s_journal);
>> sbi->s_journal = NULL;
>>
>
> Hi Guoqing,
>
> Thanks for your analyze. This problem cannot reproduce on x86_64 but 100% reproduce on arm64,
> it depends on the percpu counter code on different architecture.
>
> Indeed, as you said, the real problem is invoke percpu_counter_dec(&journal->j_jh_shrink_count)
> after it was destroyed during umount, and I'm afraid that it may also affect ocfs2
> because it doesn't initialize the percpu counter before doing add/sub in
> __jbd2_journal_[insert|remove]_checkpoint().
>
> I think the quick fix could be:
>
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 152880c298ca..48c7e5d17b38 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1352,17 +1352,23 @@ static journal_t *journal_init_common(struct block_device *bdev,
> if (!journal->j_wbuf)
> goto err_cleanup;
>
> + err = percpu_counter_init(&journal->j_jh_shrink_count, 0, GFP_KERNEL);
> + if (err)
> + goto err_cleanup;
> +
> bh = getblk_unmovable(journal->j_dev, start, journal->j_blocksize);
> if (!bh) {
> pr_err("%s: Cannot get buffer for journal superblock\n",
> __func__);
> - goto err_cleanup;
> + goto err_cleanup_cnt;
> }
> journal->j_sb_buffer = bh;
> journal->j_superblock = (journal_superblock_t *)bh->b_data;
>
> return journal;
>
> +err_cleanup_cnt:
> + percpu_counter_destroy(&journal->j_jh_shrink_count);
> err_cleanup:
> kfree(journal->j_wbuf);
> jbd2_journal_destroy_revoke(journal);
> @@ -2101,26 +2107,13 @@ static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
> */
> int jbd2_journal_register_shrinker(journal_t *journal)
> {
> - int err;
> -
> journal->j_shrink_transaction = NULL;
> -
> - err = percpu_counter_init(&journal->j_jh_shrink_count, 0, GFP_KERNEL);
> - if (err)
> - return err;
> -
> journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
> journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
> journal->j_shrinker.seeks = DEFAULT_SEEKS;
> journal->j_shrinker.batch = journal->j_max_transaction_buffers;
>
> - err = register_shrinker(&journal->j_shrinker);
> - if (err) {
> - percpu_counter_destroy(&journal->j_jh_shrink_count);
> - return err;
> - }
> -
> - return 0;
> + return register_shrinker(&journal->j_shrinker);
> }
> EXPORT_SYMBOL(jbd2_journal_register_shrinker);
>
> @@ -2132,7 +2125,6 @@ EXPORT_SYMBOL(jbd2_journal_register_shrinker);
> */
> void jbd2_journal_unregister_shrinker(journal_t *journal)
> {
> - percpu_counter_destroy(&journal->j_jh_shrink_count);
> unregister_shrinker(&journal->j_shrinker);
> }
> EXPORT_SYMBOL(jbd2_journal_unregister_shrinker);
> @@ -2209,8 +2201,6 @@ int jbd2_journal_destroy(journal_t *journal)
> brelse(journal->j_sb_buffer);
> }
>
> - jbd2_journal_unregister_shrinker(journal);
> -
> if (journal->j_proc_entry)
> jbd2_stats_proc_exit(journal);
> iput(journal->j_inode);
> @@ -2220,6 +2210,7 @@ int jbd2_journal_destroy(journal_t *journal)
> crypto_free_shash(journal->j_chksum_driver);
> kfree(journal->j_fc_wbuf);
> kfree(journal->j_wbuf);
> + percpu_counter_destroy(&journal->j_jh_shrink_count);
> kfree(journal);
>
> return err;
>
Hi, Ted,
Sorry about not catching this problem, this fix is not format corrected,
if you think this fix is OK, I can send a patch after test.
Thanks,
Yi.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
@ 2021-07-02 13:52 ` Zhang Yi
0 siblings, 0 replies; 35+ messages in thread
From: Zhang Yi @ 2021-07-02 13:52 UTC (permalink / raw)
To: Theodore Ts'o, Jan Kara
Cc: Sachin Sant, linux-fsdevel, Ext4 Developers List, linuxppc-dev,
Guoqing Jiang
On 2021/7/2 21:23, Zhang Yi wrote:
> On 2021/7/2 17:38, Guoqing Jiang wrote:
>>
>>
>> On 7/2/21 4:51 PM, Sachin Sant wrote:
>>> While running LTP tests (chdir01) against 5.13.0-next20210701 booted on a Power server,
>>> following crash is encountered.
>>>
>>> [ 3051.182992] ext2 filesystem being mounted at /var/tmp/avocado_oau90dri/ltp-W0cFB5HtCy/lKhal5/mntpoint supports timestamps until 2038 (0x7fffffff)
>>> [ 3051.621341] EXT4-fs (loop0): mounting ext3 file system using the ext4 subsystem
>>> [ 3051.624645] EXT4-fs (loop0): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
>>> [ 3051.624682] ext3 filesystem being mounted at /var/tmp/avocado_oau90dri/ltp-W0cFB5HtCy/lKhal5/mntpoint supports timestamps until 2038 (0x7fffffff)
>>> [ 3051.629026] Kernel attempted to read user page (13fda70000) - exploit attempt? (uid: 0)
>>> [ 3051.629074] BUG: Unable to handle kernel data access on read at 0x13fda70000
>>> [ 3051.629103] Faulting instruction address: 0xc0000000006fa5cc
>>> [ 3051.629118] Oops: Kernel access of bad area, sig: 11 [#1]
>>> [ 3051.629130] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
>>> [ 3051.629148] Modules linked in: vfat fat btrfs blake2b_generic xor zstd_compress raid6_pq xfs loop sctp ip6_udp_tunnel udp_tunnel libcrc32c rpadlpar_io rpaphp dm_mod bonding rfkill sunrpc pseries_rng xts vmx_crypto uio_pdrv_genirq uio sch_fq_codel ip_tables ext4 mbcache jbd2 sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp fuse [last unloaded: test_cpuidle_latency]
>>> [ 3051.629270] CPU: 10 PID: 274044 Comm: chdir01 Tainted: G W OE 5.13.0-next-20210701 #1
>>> [ 3051.629289] NIP: c0000000006fa5cc LR: c008000006949bc4 CTR: c0000000006fa5a0
>>> [ 3051.629300] REGS: c000000f74de3660 TRAP: 0300 Tainted: G W OE (5.13.0-next-20210701)
>>> [ 3051.629314] MSR: 800000000280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 24000288 XER: 20040000
>>> [ 3051.629342] CFAR: c008000006957564 DAR: 00000013fda70000 DSISR: 40000000 IRQMASK: 0
>>> [ 3051.629342] GPR00: c008000006949bc4 c000000f74de3900 c0000000029bc800 c000000f88f0ab80
>>> [ 3051.629342] GPR04: ffffffffffffffff 0000000000000020 0000000024000282 0000000000000000
>>> [ 3051.629342] GPR08: c00000110628c828 0000000000000000 00000013fda70000 c008000006957550
>>> [ 3051.629342] GPR12: c0000000006fa5a0 c0000013ffffbe80 0000000000000000 0000000000000000
>>> [ 3051.629342] GPR16: 0000000000000000 0000000000000000 00000000100555f8 0000000010050d40
>>> [ 3051.629342] GPR20: 0000000000000000 0000000010026188 0000000010026160 c000000f88f0ac08
>>> [ 3051.629342] GPR24: 0000000000000000 c000000f88f0a920 0000000000000000 0000000000000002
>>> [ 3051.629342] GPR28: c000000f88f0ac50 c000000f88f0a800 c000000fc5577d00 c000000f88f0ab80
>>> [ 3051.629468] NIP [c0000000006fa5cc] percpu_counter_add_batch+0x2c/0xf0
>>> [ 3051.629493] LR [c008000006949bc4] __jbd2_journal_remove_checkpoint+0x9c/0x280 [jbd2]
>>> [ 3051.629526] Call Trace:
>>> [ 3051.629532] [c000000f74de3900] [c000000f88f0a84c] 0xc000000f88f0a84c (unreliable)
>>> [ 3051.629547] [c000000f74de3940] [c008000006949bc4] __jbd2_journal_remove_checkpoint+0x9c/0x280 [jbd2]
>>> [ 3051.629577] [c000000f74de3980] [c008000006949eb4] jbd2_log_do_checkpoint+0x10c/0x630 [jbd2]
>>> [ 3051.629605] [c000000f74de3a40] [c0080000069547dc] jbd2_journal_destroy+0x1b4/0x4e0 [jbd2]
>>> [ 3051.629636] [c000000f74de3ad0] [c00800000735d72c] ext4_put_super+0xb4/0x560 [ext4]
>>> [ 3051.629703] [c000000f74de3b60] [c000000000484d64] generic_shutdown_super+0xc4/0x1d0
>>> [ 3051.629720] [c000000f74de3bd0] [c000000000484f48] kill_block_super+0x38/0x90
>>> [ 3051.629736] [c000000f74de3c00] [c000000000485120] deactivate_locked_super+0x80/0x100
>>> [ 3051.629752] [c000000f74de3c30] [c0000000004bec1c] cleanup_mnt+0x10c/0x1d0
>>> [ 3051.629767] [c000000f74de3c80] [c000000000188b08] task_work_run+0xf8/0x170
>>> [ 3051.629783] [c000000f74de3cd0] [c000000000021a24] do_notify_resume+0x434/0x480
>>> [ 3051.629800] [c000000f74de3d80] [c000000000032910] interrupt_exit_user_prepare_main+0x1a0/0x260
>>> [ 3051.629816] [c000000f74de3de0] [c000000000032d08] syscall_exit_prepare+0x68/0x150
>>> [ 3051.629830] [c000000f74de3e10] [c00000000000c770] system_call_common+0x100/0x258
>>> [ 3051.629846] --- interrupt: c00 at 0x7fffa2b92ffc
>>> [ 3051.629855] NIP: 00007fffa2b92ffc LR: 00007fffa2b92fcc CTR: 0000000000000000
>>> [ 3051.629867] REGS: c000000f74de3e80 TRAP: 0c00 Tainted: G W OE (5.13.0-next-20210701)
>>> [ 3051.629880] MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 24000474 XER: 00000000
>>> [ 3051.629908] IRQMASK: 0
>>> [ 3051.629908] GPR00: 0000000000000034 00007fffc0242e20 00007fffa2c77100 0000000000000000
>>> [ 3051.629908] GPR04: 0000000000000000 0000000000000078 0000000000000000 0000000000000020
>>> [ 3051.629908] GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>> [ 3051.629908] GPR12: 0000000000000000 00007fffa2d1a310 0000000000000000 0000000000000000
>>> [ 3051.629908] GPR16: 0000000000000000 0000000000000000 00000000100555f8 0000000010050d40
>>> [ 3051.629908] GPR20: 0000000000000000 0000000010026188 0000000010026160 00000000100288f0
>>> [ 3051.629908] GPR24: 00007fffa2d13320 00000000000186a0 0000000010025dd8 0000000010055688
>>> [ 3051.629908] GPR28: 0000000010024bb8 0000000000000001 0000000000000001 0000000000000000
>>> [ 3051.630022] NIP [00007fffa2b92ffc] 0x7fffa2b92ffc
>>> [ 3051.630032] LR [00007fffa2b92fcc] 0x7fffa2b92fcc
>>> [ 3051.630041] --- interrupt: c00
>>> [ 3051.630048] Instruction dump:
>>> [ 3051.630057] 60000000 3c4c022c 38422260 7c0802a6 fbe1fff8 fba1ffe8 7c7f1b78 fbc1fff0
>>> [ 3051.630078] f8010010 f821ffc1 e94d0030 e9230020 <7fca4aaa> 7fbe2214 7fa9fe76 7d2aea78
>>> [ 3051.630102] ---[ end trace 83afe3a19212c333 ]---
>>> [ 3051.633656]
>>> [ 3052.633681] Kernel panic - not syncing: Fatal exception
>>>
>>> 5.13.0-next-20210630 was good. Bisect points to following patch:
>>>
>>> commit 4ba3fcdde7e3
>>> jbd2,ext4: add a shrinker to release checkpointed buffers
>>>
>>> Reverting this patch allows the test to run successfully.
>>
>> I guess the problem is j_jh_shrink_count was destroyed in ext4_put_super _> jbd2_journal_unregister_shrinker
>> which is before the path ext4_put_super -> jbd2_journal_destroy -> jbd2_log_do_checkpoint to call
>> percpu_counter_dec(&journal->j_jh_shrink_count).
>>
>> And since jbd2_journal_unregister_shrinker is already called inside jbd2_journal_destroy, does it make sense
>> to do this?
>>
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -1176,7 +1176,6 @@ static void ext4_put_super(struct super_block *sb)
>> ext4_unregister_sysfs(sb);
>>
>> if (sbi->s_journal) {
>> - jbd2_journal_unregister_shrinker(sbi->s_journal);
>> aborted = is_journal_aborted(sbi->s_journal);
>> err = jbd2_journal_destroy(sbi->s_journal);
>> sbi->s_journal = NULL;
>>
>
> Hi Guoqing,
>
> Thanks for your analyze. This problem cannot reproduce on x86_64 but 100% reproduce on arm64,
> it depends on the percpu counter code on different architecture.
>
> Indeed, as you said, the real problem is invoke percpu_counter_dec(&journal->j_jh_shrink_count)
> after it was destroyed during umount, and I'm afraid that it may also affect ocfs2
> because it doesn't initialize the percpu counter before doing add/sub in
> __jbd2_journal_[insert|remove]_checkpoint().
>
> I think the quick fix could be:
>
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 152880c298ca..48c7e5d17b38 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1352,17 +1352,23 @@ static journal_t *journal_init_common(struct block_device *bdev,
> if (!journal->j_wbuf)
> goto err_cleanup;
>
> + err = percpu_counter_init(&journal->j_jh_shrink_count, 0, GFP_KERNEL);
> + if (err)
> + goto err_cleanup;
> +
> bh = getblk_unmovable(journal->j_dev, start, journal->j_blocksize);
> if (!bh) {
> pr_err("%s: Cannot get buffer for journal superblock\n",
> __func__);
> - goto err_cleanup;
> + goto err_cleanup_cnt;
> }
> journal->j_sb_buffer = bh;
> journal->j_superblock = (journal_superblock_t *)bh->b_data;
>
> return journal;
>
> +err_cleanup_cnt:
> + percpu_counter_destroy(&journal->j_jh_shrink_count);
> err_cleanup:
> kfree(journal->j_wbuf);
> jbd2_journal_destroy_revoke(journal);
> @@ -2101,26 +2107,13 @@ static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
> */
> int jbd2_journal_register_shrinker(journal_t *journal)
> {
> - int err;
> -
> journal->j_shrink_transaction = NULL;
> -
> - err = percpu_counter_init(&journal->j_jh_shrink_count, 0, GFP_KERNEL);
> - if (err)
> - return err;
> -
> journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
> journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
> journal->j_shrinker.seeks = DEFAULT_SEEKS;
> journal->j_shrinker.batch = journal->j_max_transaction_buffers;
>
> - err = register_shrinker(&journal->j_shrinker);
> - if (err) {
> - percpu_counter_destroy(&journal->j_jh_shrink_count);
> - return err;
> - }
> -
> - return 0;
> + return register_shrinker(&journal->j_shrinker);
> }
> EXPORT_SYMBOL(jbd2_journal_register_shrinker);
>
> @@ -2132,7 +2125,6 @@ EXPORT_SYMBOL(jbd2_journal_register_shrinker);
> */
> void jbd2_journal_unregister_shrinker(journal_t *journal)
> {
> - percpu_counter_destroy(&journal->j_jh_shrink_count);
> unregister_shrinker(&journal->j_shrinker);
> }
> EXPORT_SYMBOL(jbd2_journal_unregister_shrinker);
> @@ -2209,8 +2201,6 @@ int jbd2_journal_destroy(journal_t *journal)
> brelse(journal->j_sb_buffer);
> }
>
> - jbd2_journal_unregister_shrinker(journal);
> -
> if (journal->j_proc_entry)
> jbd2_stats_proc_exit(journal);
> iput(journal->j_inode);
> @@ -2220,6 +2210,7 @@ int jbd2_journal_destroy(journal_t *journal)
> crypto_free_shash(journal->j_chksum_driver);
> kfree(journal->j_fc_wbuf);
> kfree(journal->j_wbuf);
> + percpu_counter_destroy(&journal->j_jh_shrink_count);
> kfree(journal);
>
> return err;
>
Hi, Ted,
Sorry about not catching this problem, this fix is not format corrected,
if you think this fix is OK, I can send a patch after test.
Thanks,
Yi.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
2021-07-02 13:52 ` Zhang Yi
@ 2021-07-02 16:11 ` Theodore Ts'o
-1 siblings, 0 replies; 35+ messages in thread
From: Theodore Ts'o @ 2021-07-02 16:11 UTC (permalink / raw)
To: Zhang Yi
Cc: Jan Kara, linuxppc-dev, Guoqing Jiang, Sachin Sant,
Ext4 Developers List, linux-fsdevel
On Fri, Jul 02, 2021 at 09:52:13PM +0800, Zhang Yi wrote:
>
> Sorry about not catching this problem, this fix is not format corrected,
> if you think this fix is OK, I can send a patch after test.
The issue I see with your approach, which removes the
jbd2_journal_unregister_shrinker() call from jbd2_destsroy_journal(),
is that means that *all* callers of jbd2_destroy_journal now have to
be responsible for calling jbd2_journal_unregister_shrinker() --- and
there a number of call sites to jbd2_journal_unregister_shrinker():
fs/ext4/super.c: err = jbd2_journal_destroy(sbi->s_journal);
fs/ext4/super.c: jbd2_journal_destroy(sbi->s_journal);
fs/ext4/super.c: jbd2_journal_destroy(journal);
fs/ext4/super.c: jbd2_journal_destroy(journal);
fs/ext4/super.c: jbd2_journal_destroy(journal);
fs/ocfs2/journal.c: if (!jbd2_journal_destroy(journal->j_journal) && !status) {
fs/ocfs2/journal.c: jbd2_journal_destroy(journal);
fs/ocfs2/journal.c: jbd2_journal_destroy(journal);
So it probably makes more sense to keep jbd2_journal_unregister_shrinker()
in jbd2_destroy_journal(), since arguably the fact that we are using a
shrinker is an internal implementation detail, and the users of jbd2
ideally shouldn't need to be expected to know they have unregister
jbd2's shirnkers.
Similarly, perhaps we should be moving jbd2_journal_register_shirnker()
into jbd2_journal_init_common(). We can un-export the register and
unshrink register functions, and declare them as static functions internal
to fs/jbd2/journal.c.
What do you think?
- Ted
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
@ 2021-07-02 16:11 ` Theodore Ts'o
0 siblings, 0 replies; 35+ messages in thread
From: Theodore Ts'o @ 2021-07-02 16:11 UTC (permalink / raw)
To: Zhang Yi
Cc: Sachin Sant, Jan Kara, Guoqing Jiang, linux-fsdevel,
Ext4 Developers List, linuxppc-dev
On Fri, Jul 02, 2021 at 09:52:13PM +0800, Zhang Yi wrote:
>
> Sorry about not catching this problem, this fix is not format corrected,
> if you think this fix is OK, I can send a patch after test.
The issue I see with your approach, which removes the
jbd2_journal_unregister_shrinker() call from jbd2_destsroy_journal(),
is that means that *all* callers of jbd2_destroy_journal now have to
be responsible for calling jbd2_journal_unregister_shrinker() --- and
there a number of call sites to jbd2_journal_unregister_shrinker():
fs/ext4/super.c: err = jbd2_journal_destroy(sbi->s_journal);
fs/ext4/super.c: jbd2_journal_destroy(sbi->s_journal);
fs/ext4/super.c: jbd2_journal_destroy(journal);
fs/ext4/super.c: jbd2_journal_destroy(journal);
fs/ext4/super.c: jbd2_journal_destroy(journal);
fs/ocfs2/journal.c: if (!jbd2_journal_destroy(journal->j_journal) && !status) {
fs/ocfs2/journal.c: jbd2_journal_destroy(journal);
fs/ocfs2/journal.c: jbd2_journal_destroy(journal);
So it probably makes more sense to keep jbd2_journal_unregister_shrinker()
in jbd2_destroy_journal(), since arguably the fact that we are using a
shrinker is an internal implementation detail, and the users of jbd2
ideally shouldn't need to be expected to know they have unregister
jbd2's shirnkers.
Similarly, perhaps we should be moving jbd2_journal_register_shirnker()
into jbd2_journal_init_common(). We can un-export the register and
unshrink register functions, and declare them as static functions internal
to fs/jbd2/journal.c.
What do you think?
- Ted
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
2021-07-02 16:11 ` Theodore Ts'o
@ 2021-07-02 22:11 ` Theodore Ts'o
-1 siblings, 0 replies; 35+ messages in thread
From: Theodore Ts'o @ 2021-07-02 22:11 UTC (permalink / raw)
To: Zhang Yi
Cc: Jan Kara, linuxppc-dev, Guoqing Jiang, Sachin Sant,
Ext4 Developers List, linux-fsdevel
On Fri, Jul 02, 2021 at 12:11:54PM -0400, Theodore Ts'o wrote:
> So it probably makes more sense to keep jbd2_journal_unregister_shrinker()
> in jbd2_destroy_journal(), since arguably the fact that we are using a
> shrinker is an internal implementation detail, and the users of jbd2
> ideally shouldn't need to be expected to know they have unregister
> jbd2's shirnkers.
>
> Similarly, perhaps we should be moving jbd2_journal_register_shirnker()
> into jbd2_journal_init_common(). We can un-export the register and
> unshrink register functions, and declare them as static functions internal
> to fs/jbd2/journal.c.
>
> What do you think?
Like this...
commit 8f9e16badb8fda3391e03146a47c93e76680efaf
Author: Theodore Ts'o <tytso@mit.edu>
Date: Fri Jul 2 18:05:03 2021 -0400
ext4: fix doubled call to jbd2_journal_unregister_shrinker()
On Power and ARM platforms this was causing kernel crash when
unmounting the file system, due to a percpu_counter getting destroyed
twice.
Fix this by cleaning how the jbd2 shrinker is initialized and
uninitiazed by making it solely the responsibility of
fs/jbd2/journal.c.
Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index b8ff0399e171..dfa09a277b56 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1184,7 +1184,6 @@ static void ext4_put_super(struct super_block *sb)
ext4_unregister_sysfs(sb);
if (sbi->s_journal) {
- jbd2_journal_unregister_shrinker(sbi->s_journal);
aborted = is_journal_aborted(sbi->s_journal);
err = jbd2_journal_destroy(sbi->s_journal);
sbi->s_journal = NULL;
@@ -5176,7 +5175,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
sbi->s_ea_block_cache = NULL;
if (sbi->s_journal) {
- jbd2_journal_unregister_shrinker(sbi->s_journal);
jbd2_journal_destroy(sbi->s_journal);
sbi->s_journal = NULL;
}
@@ -5502,12 +5500,6 @@ static int ext4_load_journal(struct super_block *sb,
ext4_commit_super(sb);
}
- err = jbd2_journal_register_shrinker(journal);
- if (err) {
- EXT4_SB(sb)->s_journal = NULL;
- goto err_out;
- }
-
return 0;
err_out:
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 152880c298ca..2595703aca51 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -99,6 +99,8 @@ EXPORT_SYMBOL(jbd2_journal_begin_ordered_truncate);
EXPORT_SYMBOL(jbd2_inode_cache);
static int jbd2_journal_create_slab(size_t slab_size);
+static int jbd2_journal_register_shrinker(journal_t *journal);
+static void jbd2_journal_unregister_shrinker(journal_t *journal);
#ifdef CONFIG_JBD2_DEBUG
void __jbd2_debug(int level, const char *file, const char *func,
@@ -2043,7 +2045,8 @@ int jbd2_journal_load(journal_t *journal)
goto recovery_error;
journal->j_flags |= JBD2_LOADED;
- return 0;
+
+ return jbd2_journal_register_shrinker(journal);
recovery_error:
printk(KERN_WARNING "JBD2: recovery failed\n");
@@ -2099,7 +2102,7 @@ static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
* Init a percpu counter to record the checkpointed buffers on the checkpoint
* list and register a shrinker to release their journal_head.
*/
-int jbd2_journal_register_shrinker(journal_t *journal)
+static int jbd2_journal_register_shrinker(journal_t *journal)
{
int err;
@@ -2122,7 +2125,6 @@ int jbd2_journal_register_shrinker(journal_t *journal)
return 0;
}
-EXPORT_SYMBOL(jbd2_journal_register_shrinker);
/**
* jbd2_journal_unregister_shrinker()
@@ -2130,12 +2132,13 @@ EXPORT_SYMBOL(jbd2_journal_register_shrinker);
*
* Unregister the checkpointed buffer shrinker and destroy the percpu counter.
*/
-void jbd2_journal_unregister_shrinker(journal_t *journal)
+static void jbd2_journal_unregister_shrinker(journal_t *journal)
{
- percpu_counter_destroy(&journal->j_jh_shrink_count);
- unregister_shrinker(&journal->j_shrinker);
+ if (journal->j_shrinker.flags & SHRINKER_REGISTERED) {
+ percpu_counter_destroy(&journal->j_jh_shrink_count);
+ unregister_shrinker(&journal->j_shrinker);
+ }
}
-EXPORT_SYMBOL(jbd2_journal_unregister_shrinker);
/**
* jbd2_journal_destroy() - Release a journal_t structure.
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 6cc035321562..632afbe4b18f 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1556,8 +1556,6 @@ extern int jbd2_journal_set_features
(journal_t *, unsigned long, unsigned long, unsigned long);
extern void jbd2_journal_clear_features
(journal_t *, unsigned long, unsigned long, unsigned long);
-extern int jbd2_journal_register_shrinker(journal_t *journal);
-extern void jbd2_journal_unregister_shrinker(journal_t *journal);
extern int jbd2_journal_load (journal_t *journal);
extern int jbd2_journal_destroy (journal_t *);
extern int jbd2_journal_recover (journal_t *journal);
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
@ 2021-07-02 22:11 ` Theodore Ts'o
0 siblings, 0 replies; 35+ messages in thread
From: Theodore Ts'o @ 2021-07-02 22:11 UTC (permalink / raw)
To: Zhang Yi
Cc: Sachin Sant, Jan Kara, Guoqing Jiang, linux-fsdevel,
Ext4 Developers List, linuxppc-dev
On Fri, Jul 02, 2021 at 12:11:54PM -0400, Theodore Ts'o wrote:
> So it probably makes more sense to keep jbd2_journal_unregister_shrinker()
> in jbd2_destroy_journal(), since arguably the fact that we are using a
> shrinker is an internal implementation detail, and the users of jbd2
> ideally shouldn't need to be expected to know they have unregister
> jbd2's shirnkers.
>
> Similarly, perhaps we should be moving jbd2_journal_register_shirnker()
> into jbd2_journal_init_common(). We can un-export the register and
> unshrink register functions, and declare them as static functions internal
> to fs/jbd2/journal.c.
>
> What do you think?
Like this...
commit 8f9e16badb8fda3391e03146a47c93e76680efaf
Author: Theodore Ts'o <tytso@mit.edu>
Date: Fri Jul 2 18:05:03 2021 -0400
ext4: fix doubled call to jbd2_journal_unregister_shrinker()
On Power and ARM platforms this was causing kernel crash when
unmounting the file system, due to a percpu_counter getting destroyed
twice.
Fix this by cleaning how the jbd2 shrinker is initialized and
uninitiazed by making it solely the responsibility of
fs/jbd2/journal.c.
Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index b8ff0399e171..dfa09a277b56 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1184,7 +1184,6 @@ static void ext4_put_super(struct super_block *sb)
ext4_unregister_sysfs(sb);
if (sbi->s_journal) {
- jbd2_journal_unregister_shrinker(sbi->s_journal);
aborted = is_journal_aborted(sbi->s_journal);
err = jbd2_journal_destroy(sbi->s_journal);
sbi->s_journal = NULL;
@@ -5176,7 +5175,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
sbi->s_ea_block_cache = NULL;
if (sbi->s_journal) {
- jbd2_journal_unregister_shrinker(sbi->s_journal);
jbd2_journal_destroy(sbi->s_journal);
sbi->s_journal = NULL;
}
@@ -5502,12 +5500,6 @@ static int ext4_load_journal(struct super_block *sb,
ext4_commit_super(sb);
}
- err = jbd2_journal_register_shrinker(journal);
- if (err) {
- EXT4_SB(sb)->s_journal = NULL;
- goto err_out;
- }
-
return 0;
err_out:
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 152880c298ca..2595703aca51 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -99,6 +99,8 @@ EXPORT_SYMBOL(jbd2_journal_begin_ordered_truncate);
EXPORT_SYMBOL(jbd2_inode_cache);
static int jbd2_journal_create_slab(size_t slab_size);
+static int jbd2_journal_register_shrinker(journal_t *journal);
+static void jbd2_journal_unregister_shrinker(journal_t *journal);
#ifdef CONFIG_JBD2_DEBUG
void __jbd2_debug(int level, const char *file, const char *func,
@@ -2043,7 +2045,8 @@ int jbd2_journal_load(journal_t *journal)
goto recovery_error;
journal->j_flags |= JBD2_LOADED;
- return 0;
+
+ return jbd2_journal_register_shrinker(journal);
recovery_error:
printk(KERN_WARNING "JBD2: recovery failed\n");
@@ -2099,7 +2102,7 @@ static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
* Init a percpu counter to record the checkpointed buffers on the checkpoint
* list and register a shrinker to release their journal_head.
*/
-int jbd2_journal_register_shrinker(journal_t *journal)
+static int jbd2_journal_register_shrinker(journal_t *journal)
{
int err;
@@ -2122,7 +2125,6 @@ int jbd2_journal_register_shrinker(journal_t *journal)
return 0;
}
-EXPORT_SYMBOL(jbd2_journal_register_shrinker);
/**
* jbd2_journal_unregister_shrinker()
@@ -2130,12 +2132,13 @@ EXPORT_SYMBOL(jbd2_journal_register_shrinker);
*
* Unregister the checkpointed buffer shrinker and destroy the percpu counter.
*/
-void jbd2_journal_unregister_shrinker(journal_t *journal)
+static void jbd2_journal_unregister_shrinker(journal_t *journal)
{
- percpu_counter_destroy(&journal->j_jh_shrink_count);
- unregister_shrinker(&journal->j_shrinker);
+ if (journal->j_shrinker.flags & SHRINKER_REGISTERED) {
+ percpu_counter_destroy(&journal->j_jh_shrink_count);
+ unregister_shrinker(&journal->j_shrinker);
+ }
}
-EXPORT_SYMBOL(jbd2_journal_unregister_shrinker);
/**
* jbd2_journal_destroy() - Release a journal_t structure.
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 6cc035321562..632afbe4b18f 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1556,8 +1556,6 @@ extern int jbd2_journal_set_features
(journal_t *, unsigned long, unsigned long, unsigned long);
extern void jbd2_journal_clear_features
(journal_t *, unsigned long, unsigned long, unsigned long);
-extern int jbd2_journal_register_shrinker(journal_t *journal);
-extern void jbd2_journal_unregister_shrinker(journal_t *journal);
extern int jbd2_journal_load (journal_t *journal);
extern int jbd2_journal_destroy (journal_t *);
extern int jbd2_journal_recover (journal_t *journal);
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
2021-07-02 16:11 ` Theodore Ts'o
@ 2021-07-03 3:05 ` Zhang Yi
-1 siblings, 0 replies; 35+ messages in thread
From: Zhang Yi @ 2021-07-03 3:05 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Jan Kara, linuxppc-dev, Guoqing Jiang, Sachin Sant,
Ext4 Developers List, linux-fsdevel
On 2021/7/3 0:11, Theodore Ts'o wrote:
> On Fri, Jul 02, 2021 at 09:52:13PM +0800, Zhang Yi wrote:
>>
>> Sorry about not catching this problem, this fix is not format corrected,
>> if you think this fix is OK, I can send a patch after test.
>
> The issue I see with your approach, which removes the
> jbd2_journal_unregister_shrinker() call from jbd2_destsroy_journal(),
> is that means that *all* callers of jbd2_destroy_journal now have to
> be responsible for calling jbd2_journal_unregister_shrinker() --- and
> there a number of call sites to jbd2_journal_unregister_shrinker():
>
> fs/ext4/super.c: err = jbd2_journal_destroy(sbi->s_journal);
> fs/ext4/super.c: jbd2_journal_destroy(sbi->s_journal);
> fs/ext4/super.c: jbd2_journal_destroy(journal);
> fs/ext4/super.c: jbd2_journal_destroy(journal);
> fs/ext4/super.c: jbd2_journal_destroy(journal);
> fs/ocfs2/journal.c: if (!jbd2_journal_destroy(journal->j_journal) && !status) {
> fs/ocfs2/journal.c: jbd2_journal_destroy(journal);
> fs/ocfs2/journal.c: jbd2_journal_destroy(journal);
>
Originally, I want to add this shrinker as a optional feature for jbd2 because
only ext4 use it now and I'm not sure does ocfs2 needs this feature. So I export
jbd2_journal_[un]register_shrinker(), ext4 could invoke them individually.
If with my fix, there is no responsible for calling
jbd2_journal_unregister_shrinker() before every jbd2_journal_destroy(). There
are only two places that need to do this, one is the error path after
ext4_load_journal() because we have already register the shrinker, other one
is in ext4_put_super() before the final release of the journal.
jbd2_journal_unregister_shrinker() and jbd2_journal_destroy() do not have
the strong dependence.
And one more thing we to could do is rename the 'j_jh_shrink_count' to something
like 'j_checkpoint_jh_count' because we always init it no matter we register the
shrinker or not later.
> So it probably makes more sense to keep jbd2_journal_unregister_shrinker()
> in jbd2_destroy_journal(), since arguably the fact that we are using a
> shrinker is an internal implementation detail, and the users of jbd2
> ideally shouldn't need to be expected to know they have unregister
> jbd2's shirnkers.
>
> Similarly, perhaps we should be moving jbd2_journal_register_shirnker()
> into jbd2_journal_init_common(). We can un-export the register and
> unshrink register functions, and declare them as static functions internal
> to fs/jbd2/journal.c.
>
Yeah, it's make sense and It's sound good to me if the shrinker doesn't have
side effects on osfs2.
Thanks,
Yi.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
@ 2021-07-03 3:05 ` Zhang Yi
0 siblings, 0 replies; 35+ messages in thread
From: Zhang Yi @ 2021-07-03 3:05 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Sachin Sant, Jan Kara, Guoqing Jiang, linux-fsdevel,
Ext4 Developers List, linuxppc-dev
On 2021/7/3 0:11, Theodore Ts'o wrote:
> On Fri, Jul 02, 2021 at 09:52:13PM +0800, Zhang Yi wrote:
>>
>> Sorry about not catching this problem, this fix is not format corrected,
>> if you think this fix is OK, I can send a patch after test.
>
> The issue I see with your approach, which removes the
> jbd2_journal_unregister_shrinker() call from jbd2_destsroy_journal(),
> is that means that *all* callers of jbd2_destroy_journal now have to
> be responsible for calling jbd2_journal_unregister_shrinker() --- and
> there a number of call sites to jbd2_journal_unregister_shrinker():
>
> fs/ext4/super.c: err = jbd2_journal_destroy(sbi->s_journal);
> fs/ext4/super.c: jbd2_journal_destroy(sbi->s_journal);
> fs/ext4/super.c: jbd2_journal_destroy(journal);
> fs/ext4/super.c: jbd2_journal_destroy(journal);
> fs/ext4/super.c: jbd2_journal_destroy(journal);
> fs/ocfs2/journal.c: if (!jbd2_journal_destroy(journal->j_journal) && !status) {
> fs/ocfs2/journal.c: jbd2_journal_destroy(journal);
> fs/ocfs2/journal.c: jbd2_journal_destroy(journal);
>
Originally, I want to add this shrinker as a optional feature for jbd2 because
only ext4 use it now and I'm not sure does ocfs2 needs this feature. So I export
jbd2_journal_[un]register_shrinker(), ext4 could invoke them individually.
If with my fix, there is no responsible for calling
jbd2_journal_unregister_shrinker() before every jbd2_journal_destroy(). There
are only two places that need to do this, one is the error path after
ext4_load_journal() because we have already register the shrinker, other one
is in ext4_put_super() before the final release of the journal.
jbd2_journal_unregister_shrinker() and jbd2_journal_destroy() do not have
the strong dependence.
And one more thing we to could do is rename the 'j_jh_shrink_count' to something
like 'j_checkpoint_jh_count' because we always init it no matter we register the
shrinker or not later.
> So it probably makes more sense to keep jbd2_journal_unregister_shrinker()
> in jbd2_destroy_journal(), since arguably the fact that we are using a
> shrinker is an internal implementation detail, and the users of jbd2
> ideally shouldn't need to be expected to know they have unregister
> jbd2's shirnkers.
>
> Similarly, perhaps we should be moving jbd2_journal_register_shirnker()
> into jbd2_journal_init_common(). We can un-export the register and
> unshrink register functions, and declare them as static functions internal
> to fs/jbd2/journal.c.
>
Yeah, it's make sense and It's sound good to me if the shrinker doesn't have
side effects on osfs2.
Thanks,
Yi.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
2021-07-03 3:05 ` Zhang Yi
@ 2021-07-03 3:35 ` Theodore Ts'o
-1 siblings, 0 replies; 35+ messages in thread
From: Theodore Ts'o @ 2021-07-03 3:35 UTC (permalink / raw)
To: Zhang Yi
Cc: Jan Kara, linuxppc-dev, Guoqing Jiang, Sachin Sant,
Ext4 Developers List, linux-fsdevel
On Sat, Jul 03, 2021 at 11:05:07AM +0800, Zhang Yi wrote:
>
> Originally, I want to add this shrinker as a optional feature for jbd2 because
> only ext4 use it now and I'm not sure does ocfs2 needs this feature. So I export
> jbd2_journal_[un]register_shrinker(), ext4 could invoke them individually.
The reason why bdev_try_to_free_page() callback was needed for ext4
--- namely so there was a way to release checkpointed buffers under
memory pressure --- also exists for ocfs2. It was probably true that
in most deployments of ocfs2, they weren't running with super-tight
memory availability, so it may not have been necessary the same way
that it might be necessary, say, if ext4 was being used on a Rasberry
Pi. :-)
> And one more thing we to could do is rename the 'j_jh_shrink_count' to something
> like 'j_checkpoint_jh_count' because we always init it no matter we register the
> shrinker or not later.
That makes sense.
In fact, unless I'm mistaken, I don't think it's legal to call
percpu_counter_{inc,dec} if the shrinker isn't initialized. So for
ocfs2, if we didn't initialize percpu_counter, when
__jbd2_journal_insert_checkpoint() tries to call percpu_counter_inc(),
I believe things would potentially go *boom* on some implementations
of the percpu counter (e.g., on Power and ARM). So not only would it
not hurt to register the shrinker for ocfs2, I think it's required.
So yeah, let's rename it to something like j_checkpoint_jh_count, and
then let's inline jbd2_journal_[un]register_shrinker() in
journal_init_common() and jbd2_journal_unregister_shrinker().
What do you think?
- Ted
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
@ 2021-07-03 3:35 ` Theodore Ts'o
0 siblings, 0 replies; 35+ messages in thread
From: Theodore Ts'o @ 2021-07-03 3:35 UTC (permalink / raw)
To: Zhang Yi
Cc: Sachin Sant, Jan Kara, Guoqing Jiang, linux-fsdevel,
Ext4 Developers List, linuxppc-dev
On Sat, Jul 03, 2021 at 11:05:07AM +0800, Zhang Yi wrote:
>
> Originally, I want to add this shrinker as a optional feature for jbd2 because
> only ext4 use it now and I'm not sure does ocfs2 needs this feature. So I export
> jbd2_journal_[un]register_shrinker(), ext4 could invoke them individually.
The reason why bdev_try_to_free_page() callback was needed for ext4
--- namely so there was a way to release checkpointed buffers under
memory pressure --- also exists for ocfs2. It was probably true that
in most deployments of ocfs2, they weren't running with super-tight
memory availability, so it may not have been necessary the same way
that it might be necessary, say, if ext4 was being used on a Rasberry
Pi. :-)
> And one more thing we to could do is rename the 'j_jh_shrink_count' to something
> like 'j_checkpoint_jh_count' because we always init it no matter we register the
> shrinker or not later.
That makes sense.
In fact, unless I'm mistaken, I don't think it's legal to call
percpu_counter_{inc,dec} if the shrinker isn't initialized. So for
ocfs2, if we didn't initialize percpu_counter, when
__jbd2_journal_insert_checkpoint() tries to call percpu_counter_inc(),
I believe things would potentially go *boom* on some implementations
of the percpu counter (e.g., on Power and ARM). So not only would it
not hurt to register the shrinker for ocfs2, I think it's required.
So yeah, let's rename it to something like j_checkpoint_jh_count, and
then let's inline jbd2_journal_[un]register_shrinker() in
journal_init_common() and jbd2_journal_unregister_shrinker().
What do you think?
- Ted
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
2021-07-02 22:11 ` Theodore Ts'o
@ 2021-07-03 3:37 ` Zhang Yi
-1 siblings, 0 replies; 35+ messages in thread
From: Zhang Yi @ 2021-07-03 3:37 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Jan Kara, linuxppc-dev, Guoqing Jiang, Sachin Sant,
Ext4 Developers List, linux-fsdevel
On 2021/7/3 6:11, Theodore Ts'o wrote:
> On Fri, Jul 02, 2021 at 12:11:54PM -0400, Theodore Ts'o wrote:
>> So it probably makes more sense to keep jbd2_journal_unregister_shrinker()
>> in jbd2_destroy_journal(), since arguably the fact that we are using a
>> shrinker is an internal implementation detail, and the users of jbd2
>> ideally shouldn't need to be expected to know they have unregister
>> jbd2's shirnkers.
>>
>> Similarly, perhaps we should be moving jbd2_journal_register_shirnker()
>> into jbd2_journal_init_common(). We can un-export the register and
>> unshrink register functions, and declare them as static functions internal
>> to fs/jbd2/journal.c.
>>
>> What do you think?
>
> Like this...
>
> commit 8f9e16badb8fda3391e03146a47c93e76680efaf
> Author: Theodore Ts'o <tytso@mit.edu>
> Date: Fri Jul 2 18:05:03 2021 -0400
>
> ext4: fix doubled call to jbd2_journal_unregister_shrinker()
>
> On Power and ARM platforms this was causing kernel crash when
> unmounting the file system, due to a percpu_counter getting destroyed
> twice.
>
> Fix this by cleaning how the jbd2 shrinker is initialized and
> uninitiazed by making it solely the responsibility of
> fs/jbd2/journal.c.
>
> Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
> Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index b8ff0399e171..dfa09a277b56 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1184,7 +1184,6 @@ static void ext4_put_super(struct super_block *sb)
> ext4_unregister_sysfs(sb);
>
> if (sbi->s_journal) {
> - jbd2_journal_unregister_shrinker(sbi->s_journal);
> aborted = is_journal_aborted(sbi->s_journal);
> err = jbd2_journal_destroy(sbi->s_journal);
> sbi->s_journal = NULL;
> @@ -5176,7 +5175,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> sbi->s_ea_block_cache = NULL;
>
> if (sbi->s_journal) {
> - jbd2_journal_unregister_shrinker(sbi->s_journal);
> jbd2_journal_destroy(sbi->s_journal);
> sbi->s_journal = NULL;
> }
> @@ -5502,12 +5500,6 @@ static int ext4_load_journal(struct super_block *sb,
> ext4_commit_super(sb);
> }
>
> - err = jbd2_journal_register_shrinker(journal);
> - if (err) {
> - EXT4_SB(sb)->s_journal = NULL;
> - goto err_out;
> - }
> -
> return 0;
>
> err_out:
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 152880c298ca..2595703aca51 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -99,6 +99,8 @@ EXPORT_SYMBOL(jbd2_journal_begin_ordered_truncate);
> EXPORT_SYMBOL(jbd2_inode_cache);
>
> static int jbd2_journal_create_slab(size_t slab_size);
> +static int jbd2_journal_register_shrinker(journal_t *journal);
> +static void jbd2_journal_unregister_shrinker(journal_t *journal);
>
> #ifdef CONFIG_JBD2_DEBUG
> void __jbd2_debug(int level, const char *file, const char *func,
> @@ -2043,7 +2045,8 @@ int jbd2_journal_load(journal_t *journal)
> goto recovery_error;
>
> journal->j_flags |= JBD2_LOADED;
> - return 0;
> +
> + return jbd2_journal_register_shrinker(journal);
>
I check the ocfs2 code, if we register shrinker here, __ocfs2_recovery_thread()->
ocfs2_recover_node() seems will register and unregister a lot of unnecessary
shrinkers. It depends on the lifetime of the shrinker and the journal, because of
the jbd2_journal_destroy() destroy everything, it not a simple undo of
jbd2_load_journal(), so it's not easy to add shrinker properly. But it doesn't
seems like a real problem, just curious.
Thanks,
Yi.
> recovery_error:
> printk(KERN_WARNING "JBD2: recovery failed\n");
> @@ -2099,7 +2102,7 @@ static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
> * Init a percpu counter to record the checkpointed buffers on the checkpoint
> * list and register a shrinker to release their journal_head.
> */
> -int jbd2_journal_register_shrinker(journal_t *journal)
> +static int jbd2_journal_register_shrinker(journal_t *journal)
> {
> int err;
>
> @@ -2122,7 +2125,6 @@ int jbd2_journal_register_shrinker(journal_t *journal)
>
> return 0;
> }
> -EXPORT_SYMBOL(jbd2_journal_register_shrinker);
>
> /**
> * jbd2_journal_unregister_shrinker()
> @@ -2130,12 +2132,13 @@ EXPORT_SYMBOL(jbd2_journal_register_shrinker);
> *
> * Unregister the checkpointed buffer shrinker and destroy the percpu counter.
> */
> -void jbd2_journal_unregister_shrinker(journal_t *journal)
> +static void jbd2_journal_unregister_shrinker(journal_t *journal)
> {
> - percpu_counter_destroy(&journal->j_jh_shrink_count);
> - unregister_shrinker(&journal->j_shrinker);
> + if (journal->j_shrinker.flags & SHRINKER_REGISTERED) {
> + percpu_counter_destroy(&journal->j_jh_shrink_count);
> + unregister_shrinker(&journal->j_shrinker);
> + }
> }
> -EXPORT_SYMBOL(jbd2_journal_unregister_shrinker);
>
> /**
> * jbd2_journal_destroy() - Release a journal_t structure.
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 6cc035321562..632afbe4b18f 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1556,8 +1556,6 @@ extern int jbd2_journal_set_features
> (journal_t *, unsigned long, unsigned long, unsigned long);
> extern void jbd2_journal_clear_features
> (journal_t *, unsigned long, unsigned long, unsigned long);
> -extern int jbd2_journal_register_shrinker(journal_t *journal);
> -extern void jbd2_journal_unregister_shrinker(journal_t *journal);
> extern int jbd2_journal_load (journal_t *journal);
> extern int jbd2_journal_destroy (journal_t *);
> extern int jbd2_journal_recover (journal_t *journal);
> .
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
@ 2021-07-03 3:37 ` Zhang Yi
0 siblings, 0 replies; 35+ messages in thread
From: Zhang Yi @ 2021-07-03 3:37 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Sachin Sant, Jan Kara, Guoqing Jiang, linux-fsdevel,
Ext4 Developers List, linuxppc-dev
On 2021/7/3 6:11, Theodore Ts'o wrote:
> On Fri, Jul 02, 2021 at 12:11:54PM -0400, Theodore Ts'o wrote:
>> So it probably makes more sense to keep jbd2_journal_unregister_shrinker()
>> in jbd2_destroy_journal(), since arguably the fact that we are using a
>> shrinker is an internal implementation detail, and the users of jbd2
>> ideally shouldn't need to be expected to know they have unregister
>> jbd2's shirnkers.
>>
>> Similarly, perhaps we should be moving jbd2_journal_register_shirnker()
>> into jbd2_journal_init_common(). We can un-export the register and
>> unshrink register functions, and declare them as static functions internal
>> to fs/jbd2/journal.c.
>>
>> What do you think?
>
> Like this...
>
> commit 8f9e16badb8fda3391e03146a47c93e76680efaf
> Author: Theodore Ts'o <tytso@mit.edu>
> Date: Fri Jul 2 18:05:03 2021 -0400
>
> ext4: fix doubled call to jbd2_journal_unregister_shrinker()
>
> On Power and ARM platforms this was causing kernel crash when
> unmounting the file system, due to a percpu_counter getting destroyed
> twice.
>
> Fix this by cleaning how the jbd2 shrinker is initialized and
> uninitiazed by making it solely the responsibility of
> fs/jbd2/journal.c.
>
> Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
> Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index b8ff0399e171..dfa09a277b56 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1184,7 +1184,6 @@ static void ext4_put_super(struct super_block *sb)
> ext4_unregister_sysfs(sb);
>
> if (sbi->s_journal) {
> - jbd2_journal_unregister_shrinker(sbi->s_journal);
> aborted = is_journal_aborted(sbi->s_journal);
> err = jbd2_journal_destroy(sbi->s_journal);
> sbi->s_journal = NULL;
> @@ -5176,7 +5175,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> sbi->s_ea_block_cache = NULL;
>
> if (sbi->s_journal) {
> - jbd2_journal_unregister_shrinker(sbi->s_journal);
> jbd2_journal_destroy(sbi->s_journal);
> sbi->s_journal = NULL;
> }
> @@ -5502,12 +5500,6 @@ static int ext4_load_journal(struct super_block *sb,
> ext4_commit_super(sb);
> }
>
> - err = jbd2_journal_register_shrinker(journal);
> - if (err) {
> - EXT4_SB(sb)->s_journal = NULL;
> - goto err_out;
> - }
> -
> return 0;
>
> err_out:
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 152880c298ca..2595703aca51 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -99,6 +99,8 @@ EXPORT_SYMBOL(jbd2_journal_begin_ordered_truncate);
> EXPORT_SYMBOL(jbd2_inode_cache);
>
> static int jbd2_journal_create_slab(size_t slab_size);
> +static int jbd2_journal_register_shrinker(journal_t *journal);
> +static void jbd2_journal_unregister_shrinker(journal_t *journal);
>
> #ifdef CONFIG_JBD2_DEBUG
> void __jbd2_debug(int level, const char *file, const char *func,
> @@ -2043,7 +2045,8 @@ int jbd2_journal_load(journal_t *journal)
> goto recovery_error;
>
> journal->j_flags |= JBD2_LOADED;
> - return 0;
> +
> + return jbd2_journal_register_shrinker(journal);
>
I check the ocfs2 code, if we register shrinker here, __ocfs2_recovery_thread()->
ocfs2_recover_node() seems will register and unregister a lot of unnecessary
shrinkers. It depends on the lifetime of the shrinker and the journal, because of
the jbd2_journal_destroy() destroy everything, it not a simple undo of
jbd2_load_journal(), so it's not easy to add shrinker properly. But it doesn't
seems like a real problem, just curious.
Thanks,
Yi.
> recovery_error:
> printk(KERN_WARNING "JBD2: recovery failed\n");
> @@ -2099,7 +2102,7 @@ static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
> * Init a percpu counter to record the checkpointed buffers on the checkpoint
> * list and register a shrinker to release their journal_head.
> */
> -int jbd2_journal_register_shrinker(journal_t *journal)
> +static int jbd2_journal_register_shrinker(journal_t *journal)
> {
> int err;
>
> @@ -2122,7 +2125,6 @@ int jbd2_journal_register_shrinker(journal_t *journal)
>
> return 0;
> }
> -EXPORT_SYMBOL(jbd2_journal_register_shrinker);
>
> /**
> * jbd2_journal_unregister_shrinker()
> @@ -2130,12 +2132,13 @@ EXPORT_SYMBOL(jbd2_journal_register_shrinker);
> *
> * Unregister the checkpointed buffer shrinker and destroy the percpu counter.
> */
> -void jbd2_journal_unregister_shrinker(journal_t *journal)
> +static void jbd2_journal_unregister_shrinker(journal_t *journal)
> {
> - percpu_counter_destroy(&journal->j_jh_shrink_count);
> - unregister_shrinker(&journal->j_shrinker);
> + if (journal->j_shrinker.flags & SHRINKER_REGISTERED) {
> + percpu_counter_destroy(&journal->j_jh_shrink_count);
> + unregister_shrinker(&journal->j_shrinker);
> + }
> }
> -EXPORT_SYMBOL(jbd2_journal_unregister_shrinker);
>
> /**
> * jbd2_journal_destroy() - Release a journal_t structure.
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 6cc035321562..632afbe4b18f 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1556,8 +1556,6 @@ extern int jbd2_journal_set_features
> (journal_t *, unsigned long, unsigned long, unsigned long);
> extern void jbd2_journal_clear_features
> (journal_t *, unsigned long, unsigned long, unsigned long);
> -extern int jbd2_journal_register_shrinker(journal_t *journal);
> -extern void jbd2_journal_unregister_shrinker(journal_t *journal);
> extern int jbd2_journal_load (journal_t *journal);
> extern int jbd2_journal_destroy (journal_t *);
> extern int jbd2_journal_recover (journal_t *journal);
> .
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
2021-07-03 3:37 ` Zhang Yi
@ 2021-07-03 3:52 ` Theodore Ts'o
-1 siblings, 0 replies; 35+ messages in thread
From: Theodore Ts'o @ 2021-07-03 3:52 UTC (permalink / raw)
To: Zhang Yi
Cc: Jan Kara, linuxppc-dev, Guoqing Jiang, Sachin Sant,
Ext4 Developers List, linux-fsdevel
On Sat, Jul 03, 2021 at 11:37:07AM +0800, Zhang Yi wrote:
> I check the ocfs2 code, if we register shrinker here, __ocfs2_recovery_thread()->
> ocfs2_recover_node() seems will register and unregister a lot of unnecessary
> shrinkers. It depends on the lifetime of the shrinker and the journal, because of
> the jbd2_journal_destroy() destroy everything, it not a simple undo of
> jbd2_load_journal(), so it's not easy to add shrinker properly. But it doesn't
> seems like a real problem, just curious.
ocfs2_recover_node() only gets called for nodes that need recovery ---
that is, when an ocfs2 server has crashed, then it becomes necessary
to replay that node's journal before that node's responsibilities can
be taken over by another server. So it doesn't get called that
frequently --- and when it is needed, the fact that we need to read
the journal, and replay its entries, the cost in comparison for
registering and unregistering the shrinker is going to be quite cheap.
Cheers,
- Ted
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
@ 2021-07-03 3:52 ` Theodore Ts'o
0 siblings, 0 replies; 35+ messages in thread
From: Theodore Ts'o @ 2021-07-03 3:52 UTC (permalink / raw)
To: Zhang Yi
Cc: Sachin Sant, Jan Kara, Guoqing Jiang, linux-fsdevel,
Ext4 Developers List, linuxppc-dev
On Sat, Jul 03, 2021 at 11:37:07AM +0800, Zhang Yi wrote:
> I check the ocfs2 code, if we register shrinker here, __ocfs2_recovery_thread()->
> ocfs2_recover_node() seems will register and unregister a lot of unnecessary
> shrinkers. It depends on the lifetime of the shrinker and the journal, because of
> the jbd2_journal_destroy() destroy everything, it not a simple undo of
> jbd2_load_journal(), so it's not easy to add shrinker properly. But it doesn't
> seems like a real problem, just curious.
ocfs2_recover_node() only gets called for nodes that need recovery ---
that is, when an ocfs2 server has crashed, then it becomes necessary
to replay that node's journal before that node's responsibilities can
be taken over by another server. So it doesn't get called that
frequently --- and when it is needed, the fact that we need to read
the journal, and replay its entries, the cost in comparison for
registering and unregistering the shrinker is going to be quite cheap.
Cheers,
- Ted
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
2021-07-03 3:35 ` Theodore Ts'o
@ 2021-07-03 4:55 ` Zhang Yi
-1 siblings, 0 replies; 35+ messages in thread
From: Zhang Yi @ 2021-07-03 4:55 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Jan Kara, linuxppc-dev, Guoqing Jiang, Sachin Sant,
Ext4 Developers List, linux-fsdevel
On 2021/7/3 11:35, Theodore Ts'o wrote:
> On Sat, Jul 03, 2021 at 11:05:07AM +0800, Zhang Yi wrote:
>>
>> Originally, I want to add this shrinker as a optional feature for jbd2 because
>> only ext4 use it now and I'm not sure does ocfs2 needs this feature. So I export
>> jbd2_journal_[un]register_shrinker(), ext4 could invoke them individually.
>
> The reason why bdev_try_to_free_page() callback was needed for ext4
> --- namely so there was a way to release checkpointed buffers under
> memory pressure --- also exists for ocfs2. It was probably true that
> in most deployments of ocfs2, they weren't running with super-tight
> memory availability, so it may not have been necessary the same way
> that it might be necessary, say, if ext4 was being used on a Rasberry
> Pi. :-)
>
>> And one more thing we to could do is rename the 'j_jh_shrink_count' to something
>> like 'j_checkpoint_jh_count' because we always init it no matter we register the
>> shrinker or not later.
>
> That makes sense.
>
> In fact, unless I'm mistaken, I don't think it's legal to call
> percpu_counter_{inc,dec} if the shrinker isn't initialized. So for
> ocfs2, if we didn't initialize percpu_counter, when
> __jbd2_journal_insert_checkpoint() tries to call percpu_counter_inc(),
> I believe things would potentially go *boom* on some implementations
> of the percpu counter (e.g., on Power and ARM). So not only would it
> not hurt to register the shrinker for ocfs2, I think it's required.
>
> So yeah, let's rename it to something like j_checkpoint_jh_count, and
> then let's inline jbd2_journal_[un]register_shrinker() in
> journal_init_common() and jbd2_journal_unregister_shrinker().
>
> What do you think?
>
Yeah, it sounds good to me. Do you want me to send the fix patch, or you
modify your commit 8f9e16badb8fd in another email directly?
Thanks,
Yi.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
@ 2021-07-03 4:55 ` Zhang Yi
0 siblings, 0 replies; 35+ messages in thread
From: Zhang Yi @ 2021-07-03 4:55 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Sachin Sant, Jan Kara, Guoqing Jiang, linux-fsdevel,
Ext4 Developers List, linuxppc-dev
On 2021/7/3 11:35, Theodore Ts'o wrote:
> On Sat, Jul 03, 2021 at 11:05:07AM +0800, Zhang Yi wrote:
>>
>> Originally, I want to add this shrinker as a optional feature for jbd2 because
>> only ext4 use it now and I'm not sure does ocfs2 needs this feature. So I export
>> jbd2_journal_[un]register_shrinker(), ext4 could invoke them individually.
>
> The reason why bdev_try_to_free_page() callback was needed for ext4
> --- namely so there was a way to release checkpointed buffers under
> memory pressure --- also exists for ocfs2. It was probably true that
> in most deployments of ocfs2, they weren't running with super-tight
> memory availability, so it may not have been necessary the same way
> that it might be necessary, say, if ext4 was being used on a Rasberry
> Pi. :-)
>
>> And one more thing we to could do is rename the 'j_jh_shrink_count' to something
>> like 'j_checkpoint_jh_count' because we always init it no matter we register the
>> shrinker or not later.
>
> That makes sense.
>
> In fact, unless I'm mistaken, I don't think it's legal to call
> percpu_counter_{inc,dec} if the shrinker isn't initialized. So for
> ocfs2, if we didn't initialize percpu_counter, when
> __jbd2_journal_insert_checkpoint() tries to call percpu_counter_inc(),
> I believe things would potentially go *boom* on some implementations
> of the percpu counter (e.g., on Power and ARM). So not only would it
> not hurt to register the shrinker for ocfs2, I think it's required.
>
> So yeah, let's rename it to something like j_checkpoint_jh_count, and
> then let's inline jbd2_journal_[un]register_shrinker() in
> journal_init_common() and jbd2_journal_unregister_shrinker().
>
> What do you think?
>
Yeah, it sounds good to me. Do you want me to send the fix patch, or you
modify your commit 8f9e16badb8fd in another email directly?
Thanks,
Yi.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
2021-07-03 4:55 ` Zhang Yi
@ 2021-07-04 14:04 ` Theodore Ts'o
-1 siblings, 0 replies; 35+ messages in thread
From: Theodore Ts'o @ 2021-07-04 14:04 UTC (permalink / raw)
To: Zhang Yi
Cc: Jan Kara, linuxppc-dev, Guoqing Jiang, Sachin Sant,
Ext4 Developers List, linux-fsdevel
On Sat, Jul 03, 2021 at 12:55:09PM +0800, Zhang Yi wrote:
> Yeah, it sounds good to me. Do you want me to send the fix patch, or you
> modify your commit 8f9e16badb8fd in another email directly?
I've gone ahead and made the changes; what do you think?
I like how it also removes 40 lines of code. :-)
- Ted
From ef3130d1b0b8ca769252d6a722a2e59a00141383 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Fri, 2 Jul 2021 18:05:03 -0400
Subject: [PATCH] ext4: inline jbd2_journal_[un]register_shrinker()
The function jbd2_journal_unregister_shrinker() was getting called
twice when the file system was getting unmounted. On Power and ARM
platforms this was causing kernel crash when unmounting the file
system, when a percpu_counter was destroyed twice.
Fix this by removing jbd2_journal_[un]register_shrinker() functions,
and inlining the shrinker setup and teardown into
journal_init_common() and jbd2_journal_destroy(). This means that
ext4 and ocfs2 now no longer need to know about registering and
unregistering jbd2's shrinker.
Also, while we're at it, rename the percpu counter from
j_jh_shrink_count to j_checkpoint_jh_count, since this makes it
clearer what this counter is intended to track.
Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
Reported-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
fs/ext4/super.c | 8 ---
fs/jbd2/checkpoint.c | 4 +-
fs/jbd2/journal.c | 148 +++++++++++++++++--------------------------
include/linux/jbd2.h | 6 +-
4 files changed, 63 insertions(+), 103 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index b8ff0399e171..dfa09a277b56 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1184,7 +1184,6 @@ static void ext4_put_super(struct super_block *sb)
ext4_unregister_sysfs(sb);
if (sbi->s_journal) {
- jbd2_journal_unregister_shrinker(sbi->s_journal);
aborted = is_journal_aborted(sbi->s_journal);
err = jbd2_journal_destroy(sbi->s_journal);
sbi->s_journal = NULL;
@@ -5176,7 +5175,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
sbi->s_ea_block_cache = NULL;
if (sbi->s_journal) {
- jbd2_journal_unregister_shrinker(sbi->s_journal);
jbd2_journal_destroy(sbi->s_journal);
sbi->s_journal = NULL;
}
@@ -5502,12 +5500,6 @@ static int ext4_load_journal(struct super_block *sb,
ext4_commit_super(sb);
}
- err = jbd2_journal_register_shrinker(journal);
- if (err) {
- EXT4_SB(sb)->s_journal = NULL;
- goto err_out;
- }
-
return 0;
err_out:
diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
index 51d1eb2ffeb9..746132998c57 100644
--- a/fs/jbd2/checkpoint.c
+++ b/fs/jbd2/checkpoint.c
@@ -701,7 +701,7 @@ int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
__buffer_unlink(jh);
jh->b_cp_transaction = NULL;
- percpu_counter_dec(&journal->j_jh_shrink_count);
+ percpu_counter_dec(&journal->j_checkpoint_jh_count);
jbd2_journal_put_journal_head(jh);
/* Is this transaction empty? */
@@ -764,7 +764,7 @@ void __jbd2_journal_insert_checkpoint(struct journal_head *jh,
jh->b_cpnext->b_cpprev = jh;
}
transaction->t_checkpoint_list = jh;
- percpu_counter_inc(&transaction->t_journal->j_jh_shrink_count);
+ percpu_counter_inc(&transaction->t_journal->j_checkpoint_jh_count);
}
/*
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 152880c298ca..8a9c94dd3599 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1283,6 +1283,48 @@ static int jbd2_min_tag_size(void)
return sizeof(journal_block_tag_t) - 4;
}
+/**
+ * jbd2_journal_shrink_scan()
+ *
+ * Scan the checkpointed buffer on the checkpoint list and release the
+ * journal_head.
+ */
+static unsigned long jbd2_journal_shrink_scan(struct shrinker *shrink,
+ struct shrink_control *sc)
+{
+ journal_t *journal = container_of(shrink, journal_t, j_shrinker);
+ unsigned long nr_to_scan = sc->nr_to_scan;
+ unsigned long nr_shrunk;
+ unsigned long count;
+
+ count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
+ trace_jbd2_shrink_scan_enter(journal, sc->nr_to_scan, count);
+
+ nr_shrunk = jbd2_journal_shrink_checkpoint_list(journal, &nr_to_scan);
+
+ count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
+ trace_jbd2_shrink_scan_exit(journal, nr_to_scan, nr_shrunk, count);
+
+ return nr_shrunk;
+}
+
+/**
+ * jbd2_journal_shrink_count()
+ *
+ * Count the number of checkpoint buffers on the checkpoint list.
+ */
+static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
+ struct shrink_control *sc)
+{
+ journal_t *journal = container_of(shrink, journal_t, j_shrinker);
+ unsigned long count;
+
+ count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
+ trace_jbd2_shrink_count(journal, sc->nr_to_scan, count);
+
+ return count;
+}
+
/*
* Management for journal control blocks: functions to create and
* destroy journal_t structures, and to initialise and read existing
@@ -1361,6 +1403,19 @@ static journal_t *journal_init_common(struct block_device *bdev,
journal->j_sb_buffer = bh;
journal->j_superblock = (journal_superblock_t *)bh->b_data;
+ journal->j_shrink_transaction = NULL;
+ journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
+ journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
+ journal->j_shrinker.seeks = DEFAULT_SEEKS;
+ journal->j_shrinker.batch = journal->j_max_transaction_buffers;
+
+ if (percpu_counter_init(&journal->j_checkpoint_jh_count, 0, GFP_KERNEL))
+ goto err_cleanup;
+
+ if (register_shrinker(&journal->j_shrinker)) {
+ percpu_counter_destroy(&journal->j_checkpoint_jh_count);
+ goto err_cleanup;
+ }
return journal;
err_cleanup:
@@ -2050,93 +2105,6 @@ int jbd2_journal_load(journal_t *journal)
return -EIO;
}
-/**
- * jbd2_journal_shrink_scan()
- *
- * Scan the checkpointed buffer on the checkpoint list and release the
- * journal_head.
- */
-static unsigned long jbd2_journal_shrink_scan(struct shrinker *shrink,
- struct shrink_control *sc)
-{
- journal_t *journal = container_of(shrink, journal_t, j_shrinker);
- unsigned long nr_to_scan = sc->nr_to_scan;
- unsigned long nr_shrunk;
- unsigned long count;
-
- count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
- trace_jbd2_shrink_scan_enter(journal, sc->nr_to_scan, count);
-
- nr_shrunk = jbd2_journal_shrink_checkpoint_list(journal, &nr_to_scan);
-
- count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
- trace_jbd2_shrink_scan_exit(journal, nr_to_scan, nr_shrunk, count);
-
- return nr_shrunk;
-}
-
-/**
- * jbd2_journal_shrink_count()
- *
- * Count the number of checkpoint buffers on the checkpoint list.
- */
-static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
- struct shrink_control *sc)
-{
- journal_t *journal = container_of(shrink, journal_t, j_shrinker);
- unsigned long count;
-
- count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
- trace_jbd2_shrink_count(journal, sc->nr_to_scan, count);
-
- return count;
-}
-
-/**
- * jbd2_journal_register_shrinker()
- * @journal: Journal to act on.
- *
- * Init a percpu counter to record the checkpointed buffers on the checkpoint
- * list and register a shrinker to release their journal_head.
- */
-int jbd2_journal_register_shrinker(journal_t *journal)
-{
- int err;
-
- journal->j_shrink_transaction = NULL;
-
- err = percpu_counter_init(&journal->j_jh_shrink_count, 0, GFP_KERNEL);
- if (err)
- return err;
-
- journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
- journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
- journal->j_shrinker.seeks = DEFAULT_SEEKS;
- journal->j_shrinker.batch = journal->j_max_transaction_buffers;
-
- err = register_shrinker(&journal->j_shrinker);
- if (err) {
- percpu_counter_destroy(&journal->j_jh_shrink_count);
- return err;
- }
-
- return 0;
-}
-EXPORT_SYMBOL(jbd2_journal_register_shrinker);
-
-/**
- * jbd2_journal_unregister_shrinker()
- * @journal: Journal to act on.
- *
- * Unregister the checkpointed buffer shrinker and destroy the percpu counter.
- */
-void jbd2_journal_unregister_shrinker(journal_t *journal)
-{
- percpu_counter_destroy(&journal->j_jh_shrink_count);
- unregister_shrinker(&journal->j_shrinker);
-}
-EXPORT_SYMBOL(jbd2_journal_unregister_shrinker);
-
/**
* jbd2_journal_destroy() - Release a journal_t structure.
* @journal: Journal to act on.
@@ -2209,8 +2177,10 @@ int jbd2_journal_destroy(journal_t *journal)
brelse(journal->j_sb_buffer);
}
- jbd2_journal_unregister_shrinker(journal);
-
+ if (journal->j_shrinker.flags & SHRINKER_REGISTERED) {
+ percpu_counter_destroy(&journal->j_checkpoint_jh_count);
+ unregister_shrinker(&journal->j_shrinker);
+ }
if (journal->j_proc_entry)
jbd2_stats_proc_exit(journal);
iput(journal->j_inode);
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 6cc035321562..fd933c45281a 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -918,11 +918,11 @@ struct journal_s
struct shrinker j_shrinker;
/**
- * @j_jh_shrink_count:
+ * @j_checkpoint_jh_count:
*
* Number of journal buffers on the checkpoint list. [j_list_lock]
*/
- struct percpu_counter j_jh_shrink_count;
+ struct percpu_counter j_checkpoint_jh_count;
/**
* @j_shrink_transaction:
@@ -1556,8 +1556,6 @@ extern int jbd2_journal_set_features
(journal_t *, unsigned long, unsigned long, unsigned long);
extern void jbd2_journal_clear_features
(journal_t *, unsigned long, unsigned long, unsigned long);
-extern int jbd2_journal_register_shrinker(journal_t *journal);
-extern void jbd2_journal_unregister_shrinker(journal_t *journal);
extern int jbd2_journal_load (journal_t *journal);
extern int jbd2_journal_destroy (journal_t *);
extern int jbd2_journal_recover (journal_t *journal);
--
2.31.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
@ 2021-07-04 14:04 ` Theodore Ts'o
0 siblings, 0 replies; 35+ messages in thread
From: Theodore Ts'o @ 2021-07-04 14:04 UTC (permalink / raw)
To: Zhang Yi
Cc: Sachin Sant, Jan Kara, Guoqing Jiang, linux-fsdevel,
Ext4 Developers List, linuxppc-dev
On Sat, Jul 03, 2021 at 12:55:09PM +0800, Zhang Yi wrote:
> Yeah, it sounds good to me. Do you want me to send the fix patch, or you
> modify your commit 8f9e16badb8fd in another email directly?
I've gone ahead and made the changes; what do you think?
I like how it also removes 40 lines of code. :-)
- Ted
From ef3130d1b0b8ca769252d6a722a2e59a00141383 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Fri, 2 Jul 2021 18:05:03 -0400
Subject: [PATCH] ext4: inline jbd2_journal_[un]register_shrinker()
The function jbd2_journal_unregister_shrinker() was getting called
twice when the file system was getting unmounted. On Power and ARM
platforms this was causing kernel crash when unmounting the file
system, when a percpu_counter was destroyed twice.
Fix this by removing jbd2_journal_[un]register_shrinker() functions,
and inlining the shrinker setup and teardown into
journal_init_common() and jbd2_journal_destroy(). This means that
ext4 and ocfs2 now no longer need to know about registering and
unregistering jbd2's shrinker.
Also, while we're at it, rename the percpu counter from
j_jh_shrink_count to j_checkpoint_jh_count, since this makes it
clearer what this counter is intended to track.
Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
Reported-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
fs/ext4/super.c | 8 ---
fs/jbd2/checkpoint.c | 4 +-
fs/jbd2/journal.c | 148 +++++++++++++++++--------------------------
include/linux/jbd2.h | 6 +-
4 files changed, 63 insertions(+), 103 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index b8ff0399e171..dfa09a277b56 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1184,7 +1184,6 @@ static void ext4_put_super(struct super_block *sb)
ext4_unregister_sysfs(sb);
if (sbi->s_journal) {
- jbd2_journal_unregister_shrinker(sbi->s_journal);
aborted = is_journal_aborted(sbi->s_journal);
err = jbd2_journal_destroy(sbi->s_journal);
sbi->s_journal = NULL;
@@ -5176,7 +5175,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
sbi->s_ea_block_cache = NULL;
if (sbi->s_journal) {
- jbd2_journal_unregister_shrinker(sbi->s_journal);
jbd2_journal_destroy(sbi->s_journal);
sbi->s_journal = NULL;
}
@@ -5502,12 +5500,6 @@ static int ext4_load_journal(struct super_block *sb,
ext4_commit_super(sb);
}
- err = jbd2_journal_register_shrinker(journal);
- if (err) {
- EXT4_SB(sb)->s_journal = NULL;
- goto err_out;
- }
-
return 0;
err_out:
diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
index 51d1eb2ffeb9..746132998c57 100644
--- a/fs/jbd2/checkpoint.c
+++ b/fs/jbd2/checkpoint.c
@@ -701,7 +701,7 @@ int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
__buffer_unlink(jh);
jh->b_cp_transaction = NULL;
- percpu_counter_dec(&journal->j_jh_shrink_count);
+ percpu_counter_dec(&journal->j_checkpoint_jh_count);
jbd2_journal_put_journal_head(jh);
/* Is this transaction empty? */
@@ -764,7 +764,7 @@ void __jbd2_journal_insert_checkpoint(struct journal_head *jh,
jh->b_cpnext->b_cpprev = jh;
}
transaction->t_checkpoint_list = jh;
- percpu_counter_inc(&transaction->t_journal->j_jh_shrink_count);
+ percpu_counter_inc(&transaction->t_journal->j_checkpoint_jh_count);
}
/*
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 152880c298ca..8a9c94dd3599 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1283,6 +1283,48 @@ static int jbd2_min_tag_size(void)
return sizeof(journal_block_tag_t) - 4;
}
+/**
+ * jbd2_journal_shrink_scan()
+ *
+ * Scan the checkpointed buffer on the checkpoint list and release the
+ * journal_head.
+ */
+static unsigned long jbd2_journal_shrink_scan(struct shrinker *shrink,
+ struct shrink_control *sc)
+{
+ journal_t *journal = container_of(shrink, journal_t, j_shrinker);
+ unsigned long nr_to_scan = sc->nr_to_scan;
+ unsigned long nr_shrunk;
+ unsigned long count;
+
+ count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
+ trace_jbd2_shrink_scan_enter(journal, sc->nr_to_scan, count);
+
+ nr_shrunk = jbd2_journal_shrink_checkpoint_list(journal, &nr_to_scan);
+
+ count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
+ trace_jbd2_shrink_scan_exit(journal, nr_to_scan, nr_shrunk, count);
+
+ return nr_shrunk;
+}
+
+/**
+ * jbd2_journal_shrink_count()
+ *
+ * Count the number of checkpoint buffers on the checkpoint list.
+ */
+static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
+ struct shrink_control *sc)
+{
+ journal_t *journal = container_of(shrink, journal_t, j_shrinker);
+ unsigned long count;
+
+ count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
+ trace_jbd2_shrink_count(journal, sc->nr_to_scan, count);
+
+ return count;
+}
+
/*
* Management for journal control blocks: functions to create and
* destroy journal_t structures, and to initialise and read existing
@@ -1361,6 +1403,19 @@ static journal_t *journal_init_common(struct block_device *bdev,
journal->j_sb_buffer = bh;
journal->j_superblock = (journal_superblock_t *)bh->b_data;
+ journal->j_shrink_transaction = NULL;
+ journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
+ journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
+ journal->j_shrinker.seeks = DEFAULT_SEEKS;
+ journal->j_shrinker.batch = journal->j_max_transaction_buffers;
+
+ if (percpu_counter_init(&journal->j_checkpoint_jh_count, 0, GFP_KERNEL))
+ goto err_cleanup;
+
+ if (register_shrinker(&journal->j_shrinker)) {
+ percpu_counter_destroy(&journal->j_checkpoint_jh_count);
+ goto err_cleanup;
+ }
return journal;
err_cleanup:
@@ -2050,93 +2105,6 @@ int jbd2_journal_load(journal_t *journal)
return -EIO;
}
-/**
- * jbd2_journal_shrink_scan()
- *
- * Scan the checkpointed buffer on the checkpoint list and release the
- * journal_head.
- */
-static unsigned long jbd2_journal_shrink_scan(struct shrinker *shrink,
- struct shrink_control *sc)
-{
- journal_t *journal = container_of(shrink, journal_t, j_shrinker);
- unsigned long nr_to_scan = sc->nr_to_scan;
- unsigned long nr_shrunk;
- unsigned long count;
-
- count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
- trace_jbd2_shrink_scan_enter(journal, sc->nr_to_scan, count);
-
- nr_shrunk = jbd2_journal_shrink_checkpoint_list(journal, &nr_to_scan);
-
- count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
- trace_jbd2_shrink_scan_exit(journal, nr_to_scan, nr_shrunk, count);
-
- return nr_shrunk;
-}
-
-/**
- * jbd2_journal_shrink_count()
- *
- * Count the number of checkpoint buffers on the checkpoint list.
- */
-static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
- struct shrink_control *sc)
-{
- journal_t *journal = container_of(shrink, journal_t, j_shrinker);
- unsigned long count;
-
- count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
- trace_jbd2_shrink_count(journal, sc->nr_to_scan, count);
-
- return count;
-}
-
-/**
- * jbd2_journal_register_shrinker()
- * @journal: Journal to act on.
- *
- * Init a percpu counter to record the checkpointed buffers on the checkpoint
- * list and register a shrinker to release their journal_head.
- */
-int jbd2_journal_register_shrinker(journal_t *journal)
-{
- int err;
-
- journal->j_shrink_transaction = NULL;
-
- err = percpu_counter_init(&journal->j_jh_shrink_count, 0, GFP_KERNEL);
- if (err)
- return err;
-
- journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
- journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
- journal->j_shrinker.seeks = DEFAULT_SEEKS;
- journal->j_shrinker.batch = journal->j_max_transaction_buffers;
-
- err = register_shrinker(&journal->j_shrinker);
- if (err) {
- percpu_counter_destroy(&journal->j_jh_shrink_count);
- return err;
- }
-
- return 0;
-}
-EXPORT_SYMBOL(jbd2_journal_register_shrinker);
-
-/**
- * jbd2_journal_unregister_shrinker()
- * @journal: Journal to act on.
- *
- * Unregister the checkpointed buffer shrinker and destroy the percpu counter.
- */
-void jbd2_journal_unregister_shrinker(journal_t *journal)
-{
- percpu_counter_destroy(&journal->j_jh_shrink_count);
- unregister_shrinker(&journal->j_shrinker);
-}
-EXPORT_SYMBOL(jbd2_journal_unregister_shrinker);
-
/**
* jbd2_journal_destroy() - Release a journal_t structure.
* @journal: Journal to act on.
@@ -2209,8 +2177,10 @@ int jbd2_journal_destroy(journal_t *journal)
brelse(journal->j_sb_buffer);
}
- jbd2_journal_unregister_shrinker(journal);
-
+ if (journal->j_shrinker.flags & SHRINKER_REGISTERED) {
+ percpu_counter_destroy(&journal->j_checkpoint_jh_count);
+ unregister_shrinker(&journal->j_shrinker);
+ }
if (journal->j_proc_entry)
jbd2_stats_proc_exit(journal);
iput(journal->j_inode);
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 6cc035321562..fd933c45281a 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -918,11 +918,11 @@ struct journal_s
struct shrinker j_shrinker;
/**
- * @j_jh_shrink_count:
+ * @j_checkpoint_jh_count:
*
* Number of journal buffers on the checkpoint list. [j_list_lock]
*/
- struct percpu_counter j_jh_shrink_count;
+ struct percpu_counter j_checkpoint_jh_count;
/**
* @j_shrink_transaction:
@@ -1556,8 +1556,6 @@ extern int jbd2_journal_set_features
(journal_t *, unsigned long, unsigned long, unsigned long);
extern void jbd2_journal_clear_features
(journal_t *, unsigned long, unsigned long, unsigned long);
-extern int jbd2_journal_register_shrinker(journal_t *journal);
-extern void jbd2_journal_unregister_shrinker(journal_t *journal);
extern int jbd2_journal_load (journal_t *journal);
extern int jbd2_journal_destroy (journal_t *);
extern int jbd2_journal_recover (journal_t *journal);
--
2.31.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
2021-07-04 14:04 ` Theodore Ts'o
@ 2021-07-05 2:17 ` Zhang Yi
-1 siblings, 0 replies; 35+ messages in thread
From: Zhang Yi @ 2021-07-05 2:17 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Jan Kara, linuxppc-dev, Guoqing Jiang, Sachin Sant,
Ext4 Developers List, linux-fsdevel
On 2021/7/4 22:04, Theodore Ts'o wrote:
> On Sat, Jul 03, 2021 at 12:55:09PM +0800, Zhang Yi wrote:
>> Yeah, it sounds good to me. Do you want me to send the fix patch, or you
>> modify your commit 8f9e16badb8fd in another email directly?
>
> I've gone ahead and made the changes; what do you think?
>
> I like how it also removes 40 lines of code. :-)
>
Thanks for the fix, this patch looks good to me besides one error
handling below.
>
>>From ef3130d1b0b8ca769252d6a722a2e59a00141383 Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o <tytso@mit.edu>
> Date: Fri, 2 Jul 2021 18:05:03 -0400
> Subject: [PATCH] ext4: inline jbd2_journal_[un]register_shrinker()
>
> The function jbd2_journal_unregister_shrinker() was getting called
> twice when the file system was getting unmounted. On Power and ARM
> platforms this was causing kernel crash when unmounting the file
> system, when a percpu_counter was destroyed twice.
>
> Fix this by removing jbd2_journal_[un]register_shrinker() functions,
> and inlining the shrinker setup and teardown into
> journal_init_common() and jbd2_journal_destroy(). This means that
> ext4 and ocfs2 now no longer need to know about registering and
> unregistering jbd2's shrinker.
>
> Also, while we're at it, rename the percpu counter from
> j_jh_shrink_count to j_checkpoint_jh_count, since this makes it
> clearer what this counter is intended to track.
>
> Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
> Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
> fs/ext4/super.c | 8 ---
> fs/jbd2/checkpoint.c | 4 +-
> fs/jbd2/journal.c | 148 +++++++++++++++++--------------------------
> include/linux/jbd2.h | 6 +-
> 4 files changed, 63 insertions(+), 103 deletions(-)
>
[..]
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 152880c298ca..8a9c94dd3599 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
[..]
> /*
> * Management for journal control blocks: functions to create and
> * destroy journal_t structures, and to initialise and read existing
> @@ -1361,6 +1403,19 @@ static journal_t *journal_init_common(struct block_device *bdev,
> journal->j_sb_buffer = bh;
> journal->j_superblock = (journal_superblock_t *)bh->b_data;
>
> + journal->j_shrink_transaction = NULL;
> + journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
> + journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
> + journal->j_shrinker.seeks = DEFAULT_SEEKS;
> + journal->j_shrinker.batch = journal->j_max_transaction_buffers;
> +
> + if (percpu_counter_init(&journal->j_checkpoint_jh_count, 0, GFP_KERNEL))
> + goto err_cleanup;
> +
> + if (register_shrinker(&journal->j_shrinker)) {
> + percpu_counter_destroy(&journal->j_checkpoint_jh_count);
> + goto err_cleanup;
> + }
Need to release j_sb_buffer in above two error path.
Thanks,
Yi.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
@ 2021-07-05 2:17 ` Zhang Yi
0 siblings, 0 replies; 35+ messages in thread
From: Zhang Yi @ 2021-07-05 2:17 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Sachin Sant, Jan Kara, Guoqing Jiang, linux-fsdevel,
Ext4 Developers List, linuxppc-dev
On 2021/7/4 22:04, Theodore Ts'o wrote:
> On Sat, Jul 03, 2021 at 12:55:09PM +0800, Zhang Yi wrote:
>> Yeah, it sounds good to me. Do you want me to send the fix patch, or you
>> modify your commit 8f9e16badb8fd in another email directly?
>
> I've gone ahead and made the changes; what do you think?
>
> I like how it also removes 40 lines of code. :-)
>
Thanks for the fix, this patch looks good to me besides one error
handling below.
>
>>From ef3130d1b0b8ca769252d6a722a2e59a00141383 Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o <tytso@mit.edu>
> Date: Fri, 2 Jul 2021 18:05:03 -0400
> Subject: [PATCH] ext4: inline jbd2_journal_[un]register_shrinker()
>
> The function jbd2_journal_unregister_shrinker() was getting called
> twice when the file system was getting unmounted. On Power and ARM
> platforms this was causing kernel crash when unmounting the file
> system, when a percpu_counter was destroyed twice.
>
> Fix this by removing jbd2_journal_[un]register_shrinker() functions,
> and inlining the shrinker setup and teardown into
> journal_init_common() and jbd2_journal_destroy(). This means that
> ext4 and ocfs2 now no longer need to know about registering and
> unregistering jbd2's shrinker.
>
> Also, while we're at it, rename the percpu counter from
> j_jh_shrink_count to j_checkpoint_jh_count, since this makes it
> clearer what this counter is intended to track.
>
> Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
> Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
> fs/ext4/super.c | 8 ---
> fs/jbd2/checkpoint.c | 4 +-
> fs/jbd2/journal.c | 148 +++++++++++++++++--------------------------
> include/linux/jbd2.h | 6 +-
> 4 files changed, 63 insertions(+), 103 deletions(-)
>
[..]
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 152880c298ca..8a9c94dd3599 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
[..]
> /*
> * Management for journal control blocks: functions to create and
> * destroy journal_t structures, and to initialise and read existing
> @@ -1361,6 +1403,19 @@ static journal_t *journal_init_common(struct block_device *bdev,
> journal->j_sb_buffer = bh;
> journal->j_superblock = (journal_superblock_t *)bh->b_data;
>
> + journal->j_shrink_transaction = NULL;
> + journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
> + journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
> + journal->j_shrinker.seeks = DEFAULT_SEEKS;
> + journal->j_shrinker.batch = journal->j_max_transaction_buffers;
> +
> + if (percpu_counter_init(&journal->j_checkpoint_jh_count, 0, GFP_KERNEL))
> + goto err_cleanup;
> +
> + if (register_shrinker(&journal->j_shrinker)) {
> + percpu_counter_destroy(&journal->j_checkpoint_jh_count);
> + goto err_cleanup;
> + }
Need to release j_sb_buffer in above two error path.
Thanks,
Yi.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
2021-07-04 14:04 ` Theodore Ts'o
@ 2021-07-05 9:58 ` Jan Kara
-1 siblings, 0 replies; 35+ messages in thread
From: Jan Kara @ 2021-07-05 9:58 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Zhang Yi, Jan Kara, linuxppc-dev, Guoqing Jiang, Sachin Sant,
Ext4 Developers List, linux-fsdevel
On Sun 04-07-21 10:04:21, Theodore Ts'o wrote:
> On Sat, Jul 03, 2021 at 12:55:09PM +0800, Zhang Yi wrote:
> > Yeah, it sounds good to me. Do you want me to send the fix patch, or you
> > modify your commit 8f9e16badb8fd in another email directly?
>
> I've gone ahead and made the changes; what do you think?
>
> I like how it also removes 40 lines of code. :-)
>
> - Ted
>
> From ef3130d1b0b8ca769252d6a722a2e59a00141383 Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o <tytso@mit.edu>
> Date: Fri, 2 Jul 2021 18:05:03 -0400
> Subject: [PATCH] ext4: inline jbd2_journal_[un]register_shrinker()
>
> The function jbd2_journal_unregister_shrinker() was getting called
> twice when the file system was getting unmounted. On Power and ARM
> platforms this was causing kernel crash when unmounting the file
> system, when a percpu_counter was destroyed twice.
>
> Fix this by removing jbd2_journal_[un]register_shrinker() functions,
> and inlining the shrinker setup and teardown into
> journal_init_common() and jbd2_journal_destroy(). This means that
> ext4 and ocfs2 now no longer need to know about registering and
> unregistering jbd2's shrinker.
>
> Also, while we're at it, rename the percpu counter from
> j_jh_shrink_count to j_checkpoint_jh_count, since this makes it
> clearer what this counter is intended to track.
>
> Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
> Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Except for the bug Zhang Yi noticed the patch looks good to me. Feel free
to add:
Reviewed-by: Jan Kara <jack@suse.cz>
after fixing that.
Honza
> ---
> fs/ext4/super.c | 8 ---
> fs/jbd2/checkpoint.c | 4 +-
> fs/jbd2/journal.c | 148 +++++++++++++++++--------------------------
> include/linux/jbd2.h | 6 +-
> 4 files changed, 63 insertions(+), 103 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index b8ff0399e171..dfa09a277b56 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1184,7 +1184,6 @@ static void ext4_put_super(struct super_block *sb)
> ext4_unregister_sysfs(sb);
>
> if (sbi->s_journal) {
> - jbd2_journal_unregister_shrinker(sbi->s_journal);
> aborted = is_journal_aborted(sbi->s_journal);
> err = jbd2_journal_destroy(sbi->s_journal);
> sbi->s_journal = NULL;
> @@ -5176,7 +5175,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> sbi->s_ea_block_cache = NULL;
>
> if (sbi->s_journal) {
> - jbd2_journal_unregister_shrinker(sbi->s_journal);
> jbd2_journal_destroy(sbi->s_journal);
> sbi->s_journal = NULL;
> }
> @@ -5502,12 +5500,6 @@ static int ext4_load_journal(struct super_block *sb,
> ext4_commit_super(sb);
> }
>
> - err = jbd2_journal_register_shrinker(journal);
> - if (err) {
> - EXT4_SB(sb)->s_journal = NULL;
> - goto err_out;
> - }
> -
> return 0;
>
> err_out:
> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> index 51d1eb2ffeb9..746132998c57 100644
> --- a/fs/jbd2/checkpoint.c
> +++ b/fs/jbd2/checkpoint.c
> @@ -701,7 +701,7 @@ int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
>
> __buffer_unlink(jh);
> jh->b_cp_transaction = NULL;
> - percpu_counter_dec(&journal->j_jh_shrink_count);
> + percpu_counter_dec(&journal->j_checkpoint_jh_count);
> jbd2_journal_put_journal_head(jh);
>
> /* Is this transaction empty? */
> @@ -764,7 +764,7 @@ void __jbd2_journal_insert_checkpoint(struct journal_head *jh,
> jh->b_cpnext->b_cpprev = jh;
> }
> transaction->t_checkpoint_list = jh;
> - percpu_counter_inc(&transaction->t_journal->j_jh_shrink_count);
> + percpu_counter_inc(&transaction->t_journal->j_checkpoint_jh_count);
> }
>
> /*
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 152880c298ca..8a9c94dd3599 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1283,6 +1283,48 @@ static int jbd2_min_tag_size(void)
> return sizeof(journal_block_tag_t) - 4;
> }
>
> +/**
> + * jbd2_journal_shrink_scan()
> + *
> + * Scan the checkpointed buffer on the checkpoint list and release the
> + * journal_head.
> + */
> +static unsigned long jbd2_journal_shrink_scan(struct shrinker *shrink,
> + struct shrink_control *sc)
> +{
> + journal_t *journal = container_of(shrink, journal_t, j_shrinker);
> + unsigned long nr_to_scan = sc->nr_to_scan;
> + unsigned long nr_shrunk;
> + unsigned long count;
> +
> + count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
> + trace_jbd2_shrink_scan_enter(journal, sc->nr_to_scan, count);
> +
> + nr_shrunk = jbd2_journal_shrink_checkpoint_list(journal, &nr_to_scan);
> +
> + count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
> + trace_jbd2_shrink_scan_exit(journal, nr_to_scan, nr_shrunk, count);
> +
> + return nr_shrunk;
> +}
> +
> +/**
> + * jbd2_journal_shrink_count()
> + *
> + * Count the number of checkpoint buffers on the checkpoint list.
> + */
> +static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
> + struct shrink_control *sc)
> +{
> + journal_t *journal = container_of(shrink, journal_t, j_shrinker);
> + unsigned long count;
> +
> + count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
> + trace_jbd2_shrink_count(journal, sc->nr_to_scan, count);
> +
> + return count;
> +}
> +
> /*
> * Management for journal control blocks: functions to create and
> * destroy journal_t structures, and to initialise and read existing
> @@ -1361,6 +1403,19 @@ static journal_t *journal_init_common(struct block_device *bdev,
> journal->j_sb_buffer = bh;
> journal->j_superblock = (journal_superblock_t *)bh->b_data;
>
> + journal->j_shrink_transaction = NULL;
> + journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
> + journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
> + journal->j_shrinker.seeks = DEFAULT_SEEKS;
> + journal->j_shrinker.batch = journal->j_max_transaction_buffers;
> +
> + if (percpu_counter_init(&journal->j_checkpoint_jh_count, 0, GFP_KERNEL))
> + goto err_cleanup;
> +
> + if (register_shrinker(&journal->j_shrinker)) {
> + percpu_counter_destroy(&journal->j_checkpoint_jh_count);
> + goto err_cleanup;
> + }
> return journal;
>
> err_cleanup:
> @@ -2050,93 +2105,6 @@ int jbd2_journal_load(journal_t *journal)
> return -EIO;
> }
>
> -/**
> - * jbd2_journal_shrink_scan()
> - *
> - * Scan the checkpointed buffer on the checkpoint list and release the
> - * journal_head.
> - */
> -static unsigned long jbd2_journal_shrink_scan(struct shrinker *shrink,
> - struct shrink_control *sc)
> -{
> - journal_t *journal = container_of(shrink, journal_t, j_shrinker);
> - unsigned long nr_to_scan = sc->nr_to_scan;
> - unsigned long nr_shrunk;
> - unsigned long count;
> -
> - count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
> - trace_jbd2_shrink_scan_enter(journal, sc->nr_to_scan, count);
> -
> - nr_shrunk = jbd2_journal_shrink_checkpoint_list(journal, &nr_to_scan);
> -
> - count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
> - trace_jbd2_shrink_scan_exit(journal, nr_to_scan, nr_shrunk, count);
> -
> - return nr_shrunk;
> -}
> -
> -/**
> - * jbd2_journal_shrink_count()
> - *
> - * Count the number of checkpoint buffers on the checkpoint list.
> - */
> -static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
> - struct shrink_control *sc)
> -{
> - journal_t *journal = container_of(shrink, journal_t, j_shrinker);
> - unsigned long count;
> -
> - count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
> - trace_jbd2_shrink_count(journal, sc->nr_to_scan, count);
> -
> - return count;
> -}
> -
> -/**
> - * jbd2_journal_register_shrinker()
> - * @journal: Journal to act on.
> - *
> - * Init a percpu counter to record the checkpointed buffers on the checkpoint
> - * list and register a shrinker to release their journal_head.
> - */
> -int jbd2_journal_register_shrinker(journal_t *journal)
> -{
> - int err;
> -
> - journal->j_shrink_transaction = NULL;
> -
> - err = percpu_counter_init(&journal->j_jh_shrink_count, 0, GFP_KERNEL);
> - if (err)
> - return err;
> -
> - journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
> - journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
> - journal->j_shrinker.seeks = DEFAULT_SEEKS;
> - journal->j_shrinker.batch = journal->j_max_transaction_buffers;
> -
> - err = register_shrinker(&journal->j_shrinker);
> - if (err) {
> - percpu_counter_destroy(&journal->j_jh_shrink_count);
> - return err;
> - }
> -
> - return 0;
> -}
> -EXPORT_SYMBOL(jbd2_journal_register_shrinker);
> -
> -/**
> - * jbd2_journal_unregister_shrinker()
> - * @journal: Journal to act on.
> - *
> - * Unregister the checkpointed buffer shrinker and destroy the percpu counter.
> - */
> -void jbd2_journal_unregister_shrinker(journal_t *journal)
> -{
> - percpu_counter_destroy(&journal->j_jh_shrink_count);
> - unregister_shrinker(&journal->j_shrinker);
> -}
> -EXPORT_SYMBOL(jbd2_journal_unregister_shrinker);
> -
> /**
> * jbd2_journal_destroy() - Release a journal_t structure.
> * @journal: Journal to act on.
> @@ -2209,8 +2177,10 @@ int jbd2_journal_destroy(journal_t *journal)
> brelse(journal->j_sb_buffer);
> }
>
> - jbd2_journal_unregister_shrinker(journal);
> -
> + if (journal->j_shrinker.flags & SHRINKER_REGISTERED) {
> + percpu_counter_destroy(&journal->j_checkpoint_jh_count);
> + unregister_shrinker(&journal->j_shrinker);
> + }
> if (journal->j_proc_entry)
> jbd2_stats_proc_exit(journal);
> iput(journal->j_inode);
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 6cc035321562..fd933c45281a 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -918,11 +918,11 @@ struct journal_s
> struct shrinker j_shrinker;
>
> /**
> - * @j_jh_shrink_count:
> + * @j_checkpoint_jh_count:
> *
> * Number of journal buffers on the checkpoint list. [j_list_lock]
> */
> - struct percpu_counter j_jh_shrink_count;
> + struct percpu_counter j_checkpoint_jh_count;
>
> /**
> * @j_shrink_transaction:
> @@ -1556,8 +1556,6 @@ extern int jbd2_journal_set_features
> (journal_t *, unsigned long, unsigned long, unsigned long);
> extern void jbd2_journal_clear_features
> (journal_t *, unsigned long, unsigned long, unsigned long);
> -extern int jbd2_journal_register_shrinker(journal_t *journal);
> -extern void jbd2_journal_unregister_shrinker(journal_t *journal);
> extern int jbd2_journal_load (journal_t *journal);
> extern int jbd2_journal_destroy (journal_t *);
> extern int jbd2_journal_recover (journal_t *journal);
> --
> 2.31.0
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
@ 2021-07-05 9:58 ` Jan Kara
0 siblings, 0 replies; 35+ messages in thread
From: Jan Kara @ 2021-07-05 9:58 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Sachin Sant, Jan Kara, Zhang Yi, Guoqing Jiang, linux-fsdevel,
Ext4 Developers List, linuxppc-dev
On Sun 04-07-21 10:04:21, Theodore Ts'o wrote:
> On Sat, Jul 03, 2021 at 12:55:09PM +0800, Zhang Yi wrote:
> > Yeah, it sounds good to me. Do you want me to send the fix patch, or you
> > modify your commit 8f9e16badb8fd in another email directly?
>
> I've gone ahead and made the changes; what do you think?
>
> I like how it also removes 40 lines of code. :-)
>
> - Ted
>
> From ef3130d1b0b8ca769252d6a722a2e59a00141383 Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o <tytso@mit.edu>
> Date: Fri, 2 Jul 2021 18:05:03 -0400
> Subject: [PATCH] ext4: inline jbd2_journal_[un]register_shrinker()
>
> The function jbd2_journal_unregister_shrinker() was getting called
> twice when the file system was getting unmounted. On Power and ARM
> platforms this was causing kernel crash when unmounting the file
> system, when a percpu_counter was destroyed twice.
>
> Fix this by removing jbd2_journal_[un]register_shrinker() functions,
> and inlining the shrinker setup and teardown into
> journal_init_common() and jbd2_journal_destroy(). This means that
> ext4 and ocfs2 now no longer need to know about registering and
> unregistering jbd2's shrinker.
>
> Also, while we're at it, rename the percpu counter from
> j_jh_shrink_count to j_checkpoint_jh_count, since this makes it
> clearer what this counter is intended to track.
>
> Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
> Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Except for the bug Zhang Yi noticed the patch looks good to me. Feel free
to add:
Reviewed-by: Jan Kara <jack@suse.cz>
after fixing that.
Honza
> ---
> fs/ext4/super.c | 8 ---
> fs/jbd2/checkpoint.c | 4 +-
> fs/jbd2/journal.c | 148 +++++++++++++++++--------------------------
> include/linux/jbd2.h | 6 +-
> 4 files changed, 63 insertions(+), 103 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index b8ff0399e171..dfa09a277b56 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1184,7 +1184,6 @@ static void ext4_put_super(struct super_block *sb)
> ext4_unregister_sysfs(sb);
>
> if (sbi->s_journal) {
> - jbd2_journal_unregister_shrinker(sbi->s_journal);
> aborted = is_journal_aborted(sbi->s_journal);
> err = jbd2_journal_destroy(sbi->s_journal);
> sbi->s_journal = NULL;
> @@ -5176,7 +5175,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> sbi->s_ea_block_cache = NULL;
>
> if (sbi->s_journal) {
> - jbd2_journal_unregister_shrinker(sbi->s_journal);
> jbd2_journal_destroy(sbi->s_journal);
> sbi->s_journal = NULL;
> }
> @@ -5502,12 +5500,6 @@ static int ext4_load_journal(struct super_block *sb,
> ext4_commit_super(sb);
> }
>
> - err = jbd2_journal_register_shrinker(journal);
> - if (err) {
> - EXT4_SB(sb)->s_journal = NULL;
> - goto err_out;
> - }
> -
> return 0;
>
> err_out:
> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> index 51d1eb2ffeb9..746132998c57 100644
> --- a/fs/jbd2/checkpoint.c
> +++ b/fs/jbd2/checkpoint.c
> @@ -701,7 +701,7 @@ int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
>
> __buffer_unlink(jh);
> jh->b_cp_transaction = NULL;
> - percpu_counter_dec(&journal->j_jh_shrink_count);
> + percpu_counter_dec(&journal->j_checkpoint_jh_count);
> jbd2_journal_put_journal_head(jh);
>
> /* Is this transaction empty? */
> @@ -764,7 +764,7 @@ void __jbd2_journal_insert_checkpoint(struct journal_head *jh,
> jh->b_cpnext->b_cpprev = jh;
> }
> transaction->t_checkpoint_list = jh;
> - percpu_counter_inc(&transaction->t_journal->j_jh_shrink_count);
> + percpu_counter_inc(&transaction->t_journal->j_checkpoint_jh_count);
> }
>
> /*
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 152880c298ca..8a9c94dd3599 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1283,6 +1283,48 @@ static int jbd2_min_tag_size(void)
> return sizeof(journal_block_tag_t) - 4;
> }
>
> +/**
> + * jbd2_journal_shrink_scan()
> + *
> + * Scan the checkpointed buffer on the checkpoint list and release the
> + * journal_head.
> + */
> +static unsigned long jbd2_journal_shrink_scan(struct shrinker *shrink,
> + struct shrink_control *sc)
> +{
> + journal_t *journal = container_of(shrink, journal_t, j_shrinker);
> + unsigned long nr_to_scan = sc->nr_to_scan;
> + unsigned long nr_shrunk;
> + unsigned long count;
> +
> + count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
> + trace_jbd2_shrink_scan_enter(journal, sc->nr_to_scan, count);
> +
> + nr_shrunk = jbd2_journal_shrink_checkpoint_list(journal, &nr_to_scan);
> +
> + count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
> + trace_jbd2_shrink_scan_exit(journal, nr_to_scan, nr_shrunk, count);
> +
> + return nr_shrunk;
> +}
> +
> +/**
> + * jbd2_journal_shrink_count()
> + *
> + * Count the number of checkpoint buffers on the checkpoint list.
> + */
> +static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
> + struct shrink_control *sc)
> +{
> + journal_t *journal = container_of(shrink, journal_t, j_shrinker);
> + unsigned long count;
> +
> + count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
> + trace_jbd2_shrink_count(journal, sc->nr_to_scan, count);
> +
> + return count;
> +}
> +
> /*
> * Management for journal control blocks: functions to create and
> * destroy journal_t structures, and to initialise and read existing
> @@ -1361,6 +1403,19 @@ static journal_t *journal_init_common(struct block_device *bdev,
> journal->j_sb_buffer = bh;
> journal->j_superblock = (journal_superblock_t *)bh->b_data;
>
> + journal->j_shrink_transaction = NULL;
> + journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
> + journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
> + journal->j_shrinker.seeks = DEFAULT_SEEKS;
> + journal->j_shrinker.batch = journal->j_max_transaction_buffers;
> +
> + if (percpu_counter_init(&journal->j_checkpoint_jh_count, 0, GFP_KERNEL))
> + goto err_cleanup;
> +
> + if (register_shrinker(&journal->j_shrinker)) {
> + percpu_counter_destroy(&journal->j_checkpoint_jh_count);
> + goto err_cleanup;
> + }
> return journal;
>
> err_cleanup:
> @@ -2050,93 +2105,6 @@ int jbd2_journal_load(journal_t *journal)
> return -EIO;
> }
>
> -/**
> - * jbd2_journal_shrink_scan()
> - *
> - * Scan the checkpointed buffer on the checkpoint list and release the
> - * journal_head.
> - */
> -static unsigned long jbd2_journal_shrink_scan(struct shrinker *shrink,
> - struct shrink_control *sc)
> -{
> - journal_t *journal = container_of(shrink, journal_t, j_shrinker);
> - unsigned long nr_to_scan = sc->nr_to_scan;
> - unsigned long nr_shrunk;
> - unsigned long count;
> -
> - count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
> - trace_jbd2_shrink_scan_enter(journal, sc->nr_to_scan, count);
> -
> - nr_shrunk = jbd2_journal_shrink_checkpoint_list(journal, &nr_to_scan);
> -
> - count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
> - trace_jbd2_shrink_scan_exit(journal, nr_to_scan, nr_shrunk, count);
> -
> - return nr_shrunk;
> -}
> -
> -/**
> - * jbd2_journal_shrink_count()
> - *
> - * Count the number of checkpoint buffers on the checkpoint list.
> - */
> -static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
> - struct shrink_control *sc)
> -{
> - journal_t *journal = container_of(shrink, journal_t, j_shrinker);
> - unsigned long count;
> -
> - count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
> - trace_jbd2_shrink_count(journal, sc->nr_to_scan, count);
> -
> - return count;
> -}
> -
> -/**
> - * jbd2_journal_register_shrinker()
> - * @journal: Journal to act on.
> - *
> - * Init a percpu counter to record the checkpointed buffers on the checkpoint
> - * list and register a shrinker to release their journal_head.
> - */
> -int jbd2_journal_register_shrinker(journal_t *journal)
> -{
> - int err;
> -
> - journal->j_shrink_transaction = NULL;
> -
> - err = percpu_counter_init(&journal->j_jh_shrink_count, 0, GFP_KERNEL);
> - if (err)
> - return err;
> -
> - journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
> - journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
> - journal->j_shrinker.seeks = DEFAULT_SEEKS;
> - journal->j_shrinker.batch = journal->j_max_transaction_buffers;
> -
> - err = register_shrinker(&journal->j_shrinker);
> - if (err) {
> - percpu_counter_destroy(&journal->j_jh_shrink_count);
> - return err;
> - }
> -
> - return 0;
> -}
> -EXPORT_SYMBOL(jbd2_journal_register_shrinker);
> -
> -/**
> - * jbd2_journal_unregister_shrinker()
> - * @journal: Journal to act on.
> - *
> - * Unregister the checkpointed buffer shrinker and destroy the percpu counter.
> - */
> -void jbd2_journal_unregister_shrinker(journal_t *journal)
> -{
> - percpu_counter_destroy(&journal->j_jh_shrink_count);
> - unregister_shrinker(&journal->j_shrinker);
> -}
> -EXPORT_SYMBOL(jbd2_journal_unregister_shrinker);
> -
> /**
> * jbd2_journal_destroy() - Release a journal_t structure.
> * @journal: Journal to act on.
> @@ -2209,8 +2177,10 @@ int jbd2_journal_destroy(journal_t *journal)
> brelse(journal->j_sb_buffer);
> }
>
> - jbd2_journal_unregister_shrinker(journal);
> -
> + if (journal->j_shrinker.flags & SHRINKER_REGISTERED) {
> + percpu_counter_destroy(&journal->j_checkpoint_jh_count);
> + unregister_shrinker(&journal->j_shrinker);
> + }
> if (journal->j_proc_entry)
> jbd2_stats_proc_exit(journal);
> iput(journal->j_inode);
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 6cc035321562..fd933c45281a 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -918,11 +918,11 @@ struct journal_s
> struct shrinker j_shrinker;
>
> /**
> - * @j_jh_shrink_count:
> + * @j_checkpoint_jh_count:
> *
> * Number of journal buffers on the checkpoint list. [j_list_lock]
> */
> - struct percpu_counter j_jh_shrink_count;
> + struct percpu_counter j_checkpoint_jh_count;
>
> /**
> * @j_shrink_transaction:
> @@ -1556,8 +1556,6 @@ extern int jbd2_journal_set_features
> (journal_t *, unsigned long, unsigned long, unsigned long);
> extern void jbd2_journal_clear_features
> (journal_t *, unsigned long, unsigned long, unsigned long);
> -extern int jbd2_journal_register_shrinker(journal_t *journal);
> -extern void jbd2_journal_unregister_shrinker(journal_t *journal);
> extern int jbd2_journal_load (journal_t *journal);
> extern int jbd2_journal_destroy (journal_t *);
> extern int jbd2_journal_recover (journal_t *journal);
> --
> 2.31.0
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
2021-07-04 14:04 ` Theodore Ts'o
@ 2021-07-05 11:27 ` Sachin Sant
-1 siblings, 0 replies; 35+ messages in thread
From: Sachin Sant @ 2021-07-05 11:27 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Zhang Yi, Jan Kara, linuxppc-dev, Guoqing Jiang,
Ext4 Developers List, linux-fsdevel
> On 04-Jul-2021, at 7:34 PM, Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Sat, Jul 03, 2021 at 12:55:09PM +0800, Zhang Yi wrote:
>> Yeah, it sounds good to me. Do you want me to send the fix patch, or you
>> modify your commit 8f9e16badb8fd in another email directly?
>
> I've gone ahead and made the changes; what do you think?
>
> I like how it also removes 40 lines of code. :-)
>
> - Ted
>
> From ef3130d1b0b8ca769252d6a722a2e59a00141383 Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o <tytso@mit.edu>
> Date: Fri, 2 Jul 2021 18:05:03 -0400
> Subject: [PATCH] ext4: inline jbd2_journal_[un]register_shrinker()
>
> The function jbd2_journal_unregister_shrinker() was getting called
> twice when the file system was getting unmounted. On Power and ARM
> platforms this was causing kernel crash when unmounting the file
> system, when a percpu_counter was destroyed twice.
>
> Fix this by removing jbd2_journal_[un]register_shrinker() functions,
> and inlining the shrinker setup and teardown into
> journal_init_common() and jbd2_journal_destroy(). This means that
> ext4 and ocfs2 now no longer need to know about registering and
> unregistering jbd2's shrinker.
>
> Also, while we're at it, rename the percpu counter from
> j_jh_shrink_count to j_checkpoint_jh_count, since this makes it
> clearer what this counter is intended to track.
>
> Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
> Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
This patch fixes the reported problem. Test ran to completion
without any crash.
Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
-Sachin
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
@ 2021-07-05 11:27 ` Sachin Sant
0 siblings, 0 replies; 35+ messages in thread
From: Sachin Sant @ 2021-07-05 11:27 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Jan Kara, Zhang Yi, Guoqing Jiang, linux-fsdevel,
Ext4 Developers List, linuxppc-dev
> On 04-Jul-2021, at 7:34 PM, Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Sat, Jul 03, 2021 at 12:55:09PM +0800, Zhang Yi wrote:
>> Yeah, it sounds good to me. Do you want me to send the fix patch, or you
>> modify your commit 8f9e16badb8fd in another email directly?
>
> I've gone ahead and made the changes; what do you think?
>
> I like how it also removes 40 lines of code. :-)
>
> - Ted
>
> From ef3130d1b0b8ca769252d6a722a2e59a00141383 Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o <tytso@mit.edu>
> Date: Fri, 2 Jul 2021 18:05:03 -0400
> Subject: [PATCH] ext4: inline jbd2_journal_[un]register_shrinker()
>
> The function jbd2_journal_unregister_shrinker() was getting called
> twice when the file system was getting unmounted. On Power and ARM
> platforms this was causing kernel crash when unmounting the file
> system, when a percpu_counter was destroyed twice.
>
> Fix this by removing jbd2_journal_[un]register_shrinker() functions,
> and inlining the shrinker setup and teardown into
> journal_init_common() and jbd2_journal_destroy(). This means that
> ext4 and ocfs2 now no longer need to know about registering and
> unregistering jbd2's shrinker.
>
> Also, while we're at it, rename the percpu counter from
> j_jh_shrink_count to j_checkpoint_jh_count, since this makes it
> clearer what this counter is intended to track.
>
> Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
> Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
This patch fixes the reported problem. Test ran to completion
without any crash.
Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
-Sachin
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH -v2] ext4: inline jbd2_journal_[un]register_shrinker()
2021-07-05 2:17 ` Zhang Yi
(?)
@ 2021-07-05 14:50 ` Theodore Ts'o
2021-07-05 18:29 ` Jon Hunter
2021-07-06 1:38 ` Zhang Yi
-1 siblings, 2 replies; 35+ messages in thread
From: Theodore Ts'o @ 2021-07-05 14:50 UTC (permalink / raw)
To: Ext4 Developers List
Cc: Jan Kara, Zhang Yi, Sachin Sant, Jon Hunter, Theodore Ts'o
The function jbd2_journal_unregister_shrinker() was getting called
twice when the file system was getting unmounted. On Power and ARM
platforms this was causing kernel crash when unmounting the file
system, when a percpu_counter was destroyed twice.
Fix this by removing jbd2_journal_[un]register_shrinker() functions,
and inlining the shrinker setup and teardown into
journal_init_common() and jbd2_journal_destroy(). This means that
ext4 and ocfs2 now no longer need to know about registering and
unregistering jbd2's shrinker.
Also, while we're at it, rename the percpu counter from
j_jh_shrink_count to j_checkpoint_jh_count, since this makes it
clearer what this counter is intended to track.
Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
Reported-by: Jon Hunter <jonathanh@nvidia.com>
Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
fs/ext4/super.c | 8 ---
fs/jbd2/checkpoint.c | 4 +-
fs/jbd2/journal.c | 149 +++++++++++++++++--------------------------
include/linux/jbd2.h | 6 +-
4 files changed, 64 insertions(+), 103 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index b8ff0399e171..dfa09a277b56 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1184,7 +1184,6 @@ static void ext4_put_super(struct super_block *sb)
ext4_unregister_sysfs(sb);
if (sbi->s_journal) {
- jbd2_journal_unregister_shrinker(sbi->s_journal);
aborted = is_journal_aborted(sbi->s_journal);
err = jbd2_journal_destroy(sbi->s_journal);
sbi->s_journal = NULL;
@@ -5176,7 +5175,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
sbi->s_ea_block_cache = NULL;
if (sbi->s_journal) {
- jbd2_journal_unregister_shrinker(sbi->s_journal);
jbd2_journal_destroy(sbi->s_journal);
sbi->s_journal = NULL;
}
@@ -5502,12 +5500,6 @@ static int ext4_load_journal(struct super_block *sb,
ext4_commit_super(sb);
}
- err = jbd2_journal_register_shrinker(journal);
- if (err) {
- EXT4_SB(sb)->s_journal = NULL;
- goto err_out;
- }
-
return 0;
err_out:
diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
index 51d1eb2ffeb9..746132998c57 100644
--- a/fs/jbd2/checkpoint.c
+++ b/fs/jbd2/checkpoint.c
@@ -701,7 +701,7 @@ int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
__buffer_unlink(jh);
jh->b_cp_transaction = NULL;
- percpu_counter_dec(&journal->j_jh_shrink_count);
+ percpu_counter_dec(&journal->j_checkpoint_jh_count);
jbd2_journal_put_journal_head(jh);
/* Is this transaction empty? */
@@ -764,7 +764,7 @@ void __jbd2_journal_insert_checkpoint(struct journal_head *jh,
jh->b_cpnext->b_cpprev = jh;
}
transaction->t_checkpoint_list = jh;
- percpu_counter_inc(&transaction->t_journal->j_jh_shrink_count);
+ percpu_counter_inc(&transaction->t_journal->j_checkpoint_jh_count);
}
/*
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 152880c298ca..35302bc192eb 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1283,6 +1283,48 @@ static int jbd2_min_tag_size(void)
return sizeof(journal_block_tag_t) - 4;
}
+/**
+ * jbd2_journal_shrink_scan()
+ *
+ * Scan the checkpointed buffer on the checkpoint list and release the
+ * journal_head.
+ */
+static unsigned long jbd2_journal_shrink_scan(struct shrinker *shrink,
+ struct shrink_control *sc)
+{
+ journal_t *journal = container_of(shrink, journal_t, j_shrinker);
+ unsigned long nr_to_scan = sc->nr_to_scan;
+ unsigned long nr_shrunk;
+ unsigned long count;
+
+ count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
+ trace_jbd2_shrink_scan_enter(journal, sc->nr_to_scan, count);
+
+ nr_shrunk = jbd2_journal_shrink_checkpoint_list(journal, &nr_to_scan);
+
+ count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
+ trace_jbd2_shrink_scan_exit(journal, nr_to_scan, nr_shrunk, count);
+
+ return nr_shrunk;
+}
+
+/**
+ * jbd2_journal_shrink_count()
+ *
+ * Count the number of checkpoint buffers on the checkpoint list.
+ */
+static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
+ struct shrink_control *sc)
+{
+ journal_t *journal = container_of(shrink, journal_t, j_shrinker);
+ unsigned long count;
+
+ count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
+ trace_jbd2_shrink_count(journal, sc->nr_to_scan, count);
+
+ return count;
+}
+
/*
* Management for journal control blocks: functions to create and
* destroy journal_t structures, and to initialise and read existing
@@ -1361,9 +1403,23 @@ static journal_t *journal_init_common(struct block_device *bdev,
journal->j_sb_buffer = bh;
journal->j_superblock = (journal_superblock_t *)bh->b_data;
+ journal->j_shrink_transaction = NULL;
+ journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
+ journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
+ journal->j_shrinker.seeks = DEFAULT_SEEKS;
+ journal->j_shrinker.batch = journal->j_max_transaction_buffers;
+
+ if (percpu_counter_init(&journal->j_checkpoint_jh_count, 0, GFP_KERNEL))
+ goto err_cleanup;
+
+ if (register_shrinker(&journal->j_shrinker)) {
+ percpu_counter_destroy(&journal->j_checkpoint_jh_count);
+ goto err_cleanup;
+ }
return journal;
err_cleanup:
+ brelse(journal->j_sb_buffer);
kfree(journal->j_wbuf);
jbd2_journal_destroy_revoke(journal);
kfree(journal);
@@ -2050,93 +2106,6 @@ int jbd2_journal_load(journal_t *journal)
return -EIO;
}
-/**
- * jbd2_journal_shrink_scan()
- *
- * Scan the checkpointed buffer on the checkpoint list and release the
- * journal_head.
- */
-static unsigned long jbd2_journal_shrink_scan(struct shrinker *shrink,
- struct shrink_control *sc)
-{
- journal_t *journal = container_of(shrink, journal_t, j_shrinker);
- unsigned long nr_to_scan = sc->nr_to_scan;
- unsigned long nr_shrunk;
- unsigned long count;
-
- count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
- trace_jbd2_shrink_scan_enter(journal, sc->nr_to_scan, count);
-
- nr_shrunk = jbd2_journal_shrink_checkpoint_list(journal, &nr_to_scan);
-
- count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
- trace_jbd2_shrink_scan_exit(journal, nr_to_scan, nr_shrunk, count);
-
- return nr_shrunk;
-}
-
-/**
- * jbd2_journal_shrink_count()
- *
- * Count the number of checkpoint buffers on the checkpoint list.
- */
-static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
- struct shrink_control *sc)
-{
- journal_t *journal = container_of(shrink, journal_t, j_shrinker);
- unsigned long count;
-
- count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
- trace_jbd2_shrink_count(journal, sc->nr_to_scan, count);
-
- return count;
-}
-
-/**
- * jbd2_journal_register_shrinker()
- * @journal: Journal to act on.
- *
- * Init a percpu counter to record the checkpointed buffers on the checkpoint
- * list and register a shrinker to release their journal_head.
- */
-int jbd2_journal_register_shrinker(journal_t *journal)
-{
- int err;
-
- journal->j_shrink_transaction = NULL;
-
- err = percpu_counter_init(&journal->j_jh_shrink_count, 0, GFP_KERNEL);
- if (err)
- return err;
-
- journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
- journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
- journal->j_shrinker.seeks = DEFAULT_SEEKS;
- journal->j_shrinker.batch = journal->j_max_transaction_buffers;
-
- err = register_shrinker(&journal->j_shrinker);
- if (err) {
- percpu_counter_destroy(&journal->j_jh_shrink_count);
- return err;
- }
-
- return 0;
-}
-EXPORT_SYMBOL(jbd2_journal_register_shrinker);
-
-/**
- * jbd2_journal_unregister_shrinker()
- * @journal: Journal to act on.
- *
- * Unregister the checkpointed buffer shrinker and destroy the percpu counter.
- */
-void jbd2_journal_unregister_shrinker(journal_t *journal)
-{
- percpu_counter_destroy(&journal->j_jh_shrink_count);
- unregister_shrinker(&journal->j_shrinker);
-}
-EXPORT_SYMBOL(jbd2_journal_unregister_shrinker);
-
/**
* jbd2_journal_destroy() - Release a journal_t structure.
* @journal: Journal to act on.
@@ -2209,8 +2178,10 @@ int jbd2_journal_destroy(journal_t *journal)
brelse(journal->j_sb_buffer);
}
- jbd2_journal_unregister_shrinker(journal);
-
+ if (journal->j_shrinker.flags & SHRINKER_REGISTERED) {
+ percpu_counter_destroy(&journal->j_checkpoint_jh_count);
+ unregister_shrinker(&journal->j_shrinker);
+ }
if (journal->j_proc_entry)
jbd2_stats_proc_exit(journal);
iput(journal->j_inode);
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 6cc035321562..fd933c45281a 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -918,11 +918,11 @@ struct journal_s
struct shrinker j_shrinker;
/**
- * @j_jh_shrink_count:
+ * @j_checkpoint_jh_count:
*
* Number of journal buffers on the checkpoint list. [j_list_lock]
*/
- struct percpu_counter j_jh_shrink_count;
+ struct percpu_counter j_checkpoint_jh_count;
/**
* @j_shrink_transaction:
@@ -1556,8 +1556,6 @@ extern int jbd2_journal_set_features
(journal_t *, unsigned long, unsigned long, unsigned long);
extern void jbd2_journal_clear_features
(journal_t *, unsigned long, unsigned long, unsigned long);
-extern int jbd2_journal_register_shrinker(journal_t *journal);
-extern void jbd2_journal_unregister_shrinker(journal_t *journal);
extern int jbd2_journal_load (journal_t *journal);
extern int jbd2_journal_destroy (journal_t *);
extern int jbd2_journal_recover (journal_t *journal);
--
2.31.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH -v2] ext4: inline jbd2_journal_[un]register_shrinker()
2021-07-05 14:50 ` [PATCH -v2] ext4: inline jbd2_journal_[un]register_shrinker() Theodore Ts'o
@ 2021-07-05 18:29 ` Jon Hunter
2021-07-06 1:38 ` Zhang Yi
1 sibling, 0 replies; 35+ messages in thread
From: Jon Hunter @ 2021-07-05 18:29 UTC (permalink / raw)
To: Theodore Ts'o, Ext4 Developers List
Cc: Jan Kara, Zhang Yi, Sachin Sant, linux-tegra
On 05/07/2021 15:50, Theodore Ts'o wrote:
> The function jbd2_journal_unregister_shrinker() was getting called
> twice when the file system was getting unmounted. On Power and ARM
> platforms this was causing kernel crash when unmounting the file
> system, when a percpu_counter was destroyed twice.
>
> Fix this by removing jbd2_journal_[un]register_shrinker() functions,
> and inlining the shrinker setup and teardown into
> journal_init_common() and jbd2_journal_destroy(). This means that
> ext4 and ocfs2 now no longer need to know about registering and
> unregistering jbd2's shrinker.
>
> Also, while we're at it, rename the percpu counter from
> j_jh_shrink_count to j_checkpoint_jh_count, since this makes it
> clearer what this counter is intended to track.
>
> Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Thanks, works for me.
Tested-by: Jon Hunter <jonathanh@nvidia.com>
Cheers
Jon
--
nvpublic
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH -v2] ext4: inline jbd2_journal_[un]register_shrinker()
2021-07-05 14:50 ` [PATCH -v2] ext4: inline jbd2_journal_[un]register_shrinker() Theodore Ts'o
2021-07-05 18:29 ` Jon Hunter
@ 2021-07-06 1:38 ` Zhang Yi
1 sibling, 0 replies; 35+ messages in thread
From: Zhang Yi @ 2021-07-06 1:38 UTC (permalink / raw)
To: Theodore Ts'o, Ext4 Developers List; +Cc: Jan Kara, Sachin Sant, Jon Hunter
On 2021/7/5 22:50, Theodore Ts'o wrote:
> The function jbd2_journal_unregister_shrinker() was getting called
> twice when the file system was getting unmounted. On Power and ARM
> platforms this was causing kernel crash when unmounting the file
> system, when a percpu_counter was destroyed twice.
>
> Fix this by removing jbd2_journal_[un]register_shrinker() functions,
> and inlining the shrinker setup and teardown into
> journal_init_common() and jbd2_journal_destroy(). This means that
> ext4 and ocfs2 now no longer need to know about registering and
> unregistering jbd2's shrinker.
>
> Also, while we're at it, rename the percpu counter from
> j_jh_shrink_count to j_checkpoint_jh_count, since this makes it
> clearer what this counter is intended to track.
>
> Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
This patch looks good to me.
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
Thanks,
Yi.
> ---
> fs/ext4/super.c | 8 ---
> fs/jbd2/checkpoint.c | 4 +-
> fs/jbd2/journal.c | 149 +++++++++++++++++--------------------------
> include/linux/jbd2.h | 6 +-
> 4 files changed, 64 insertions(+), 103 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index b8ff0399e171..dfa09a277b56 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1184,7 +1184,6 @@ static void ext4_put_super(struct super_block *sb)
> ext4_unregister_sysfs(sb);
>
> if (sbi->s_journal) {
> - jbd2_journal_unregister_shrinker(sbi->s_journal);
> aborted = is_journal_aborted(sbi->s_journal);
> err = jbd2_journal_destroy(sbi->s_journal);
> sbi->s_journal = NULL;
> @@ -5176,7 +5175,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> sbi->s_ea_block_cache = NULL;
>
> if (sbi->s_journal) {
> - jbd2_journal_unregister_shrinker(sbi->s_journal);
> jbd2_journal_destroy(sbi->s_journal);
> sbi->s_journal = NULL;
> }
> @@ -5502,12 +5500,6 @@ static int ext4_load_journal(struct super_block *sb,
> ext4_commit_super(sb);
> }
>
> - err = jbd2_journal_register_shrinker(journal);
> - if (err) {
> - EXT4_SB(sb)->s_journal = NULL;
> - goto err_out;
> - }
> -
> return 0;
>
> err_out:
> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> index 51d1eb2ffeb9..746132998c57 100644
> --- a/fs/jbd2/checkpoint.c
> +++ b/fs/jbd2/checkpoint.c
> @@ -701,7 +701,7 @@ int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
>
> __buffer_unlink(jh);
> jh->b_cp_transaction = NULL;
> - percpu_counter_dec(&journal->j_jh_shrink_count);
> + percpu_counter_dec(&journal->j_checkpoint_jh_count);
> jbd2_journal_put_journal_head(jh);
>
> /* Is this transaction empty? */
> @@ -764,7 +764,7 @@ void __jbd2_journal_insert_checkpoint(struct journal_head *jh,
> jh->b_cpnext->b_cpprev = jh;
> }
> transaction->t_checkpoint_list = jh;
> - percpu_counter_inc(&transaction->t_journal->j_jh_shrink_count);
> + percpu_counter_inc(&transaction->t_journal->j_checkpoint_jh_count);
> }
>
> /*
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 152880c298ca..35302bc192eb 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1283,6 +1283,48 @@ static int jbd2_min_tag_size(void)
> return sizeof(journal_block_tag_t) - 4;
> }
>
> +/**
> + * jbd2_journal_shrink_scan()
> + *
> + * Scan the checkpointed buffer on the checkpoint list and release the
> + * journal_head.
> + */
> +static unsigned long jbd2_journal_shrink_scan(struct shrinker *shrink,
> + struct shrink_control *sc)
> +{
> + journal_t *journal = container_of(shrink, journal_t, j_shrinker);
> + unsigned long nr_to_scan = sc->nr_to_scan;
> + unsigned long nr_shrunk;
> + unsigned long count;
> +
> + count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
> + trace_jbd2_shrink_scan_enter(journal, sc->nr_to_scan, count);
> +
> + nr_shrunk = jbd2_journal_shrink_checkpoint_list(journal, &nr_to_scan);
> +
> + count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
> + trace_jbd2_shrink_scan_exit(journal, nr_to_scan, nr_shrunk, count);
> +
> + return nr_shrunk;
> +}
> +
> +/**
> + * jbd2_journal_shrink_count()
> + *
> + * Count the number of checkpoint buffers on the checkpoint list.
> + */
> +static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
> + struct shrink_control *sc)
> +{
> + journal_t *journal = container_of(shrink, journal_t, j_shrinker);
> + unsigned long count;
> +
> + count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
> + trace_jbd2_shrink_count(journal, sc->nr_to_scan, count);
> +
> + return count;
> +}
> +
> /*
> * Management for journal control blocks: functions to create and
> * destroy journal_t structures, and to initialise and read existing
> @@ -1361,9 +1403,23 @@ static journal_t *journal_init_common(struct block_device *bdev,
> journal->j_sb_buffer = bh;
> journal->j_superblock = (journal_superblock_t *)bh->b_data;
>
> + journal->j_shrink_transaction = NULL;
> + journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
> + journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
> + journal->j_shrinker.seeks = DEFAULT_SEEKS;
> + journal->j_shrinker.batch = journal->j_max_transaction_buffers;
> +
> + if (percpu_counter_init(&journal->j_checkpoint_jh_count, 0, GFP_KERNEL))
> + goto err_cleanup;
> +
> + if (register_shrinker(&journal->j_shrinker)) {
> + percpu_counter_destroy(&journal->j_checkpoint_jh_count);
> + goto err_cleanup;
> + }
> return journal;
>
> err_cleanup:
> + brelse(journal->j_sb_buffer);
> kfree(journal->j_wbuf);
> jbd2_journal_destroy_revoke(journal);
> kfree(journal);
> @@ -2050,93 +2106,6 @@ int jbd2_journal_load(journal_t *journal)
> return -EIO;
> }
>
> -/**
> - * jbd2_journal_shrink_scan()
> - *
> - * Scan the checkpointed buffer on the checkpoint list and release the
> - * journal_head.
> - */
> -static unsigned long jbd2_journal_shrink_scan(struct shrinker *shrink,
> - struct shrink_control *sc)
> -{
> - journal_t *journal = container_of(shrink, journal_t, j_shrinker);
> - unsigned long nr_to_scan = sc->nr_to_scan;
> - unsigned long nr_shrunk;
> - unsigned long count;
> -
> - count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
> - trace_jbd2_shrink_scan_enter(journal, sc->nr_to_scan, count);
> -
> - nr_shrunk = jbd2_journal_shrink_checkpoint_list(journal, &nr_to_scan);
> -
> - count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
> - trace_jbd2_shrink_scan_exit(journal, nr_to_scan, nr_shrunk, count);
> -
> - return nr_shrunk;
> -}
> -
> -/**
> - * jbd2_journal_shrink_count()
> - *
> - * Count the number of checkpoint buffers on the checkpoint list.
> - */
> -static unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
> - struct shrink_control *sc)
> -{
> - journal_t *journal = container_of(shrink, journal_t, j_shrinker);
> - unsigned long count;
> -
> - count = percpu_counter_read_positive(&journal->j_jh_shrink_count);
> - trace_jbd2_shrink_count(journal, sc->nr_to_scan, count);
> -
> - return count;
> -}
> -
> -/**
> - * jbd2_journal_register_shrinker()
> - * @journal: Journal to act on.
> - *
> - * Init a percpu counter to record the checkpointed buffers on the checkpoint
> - * list and register a shrinker to release their journal_head.
> - */
> -int jbd2_journal_register_shrinker(journal_t *journal)
> -{
> - int err;
> -
> - journal->j_shrink_transaction = NULL;
> -
> - err = percpu_counter_init(&journal->j_jh_shrink_count, 0, GFP_KERNEL);
> - if (err)
> - return err;
> -
> - journal->j_shrinker.scan_objects = jbd2_journal_shrink_scan;
> - journal->j_shrinker.count_objects = jbd2_journal_shrink_count;
> - journal->j_shrinker.seeks = DEFAULT_SEEKS;
> - journal->j_shrinker.batch = journal->j_max_transaction_buffers;
> -
> - err = register_shrinker(&journal->j_shrinker);
> - if (err) {
> - percpu_counter_destroy(&journal->j_jh_shrink_count);
> - return err;
> - }
> -
> - return 0;
> -}
> -EXPORT_SYMBOL(jbd2_journal_register_shrinker);
> -
> -/**
> - * jbd2_journal_unregister_shrinker()
> - * @journal: Journal to act on.
> - *
> - * Unregister the checkpointed buffer shrinker and destroy the percpu counter.
> - */
> -void jbd2_journal_unregister_shrinker(journal_t *journal)
> -{
> - percpu_counter_destroy(&journal->j_jh_shrink_count);
> - unregister_shrinker(&journal->j_shrinker);
> -}
> -EXPORT_SYMBOL(jbd2_journal_unregister_shrinker);
> -
> /**
> * jbd2_journal_destroy() - Release a journal_t structure.
> * @journal: Journal to act on.
> @@ -2209,8 +2178,10 @@ int jbd2_journal_destroy(journal_t *journal)
> brelse(journal->j_sb_buffer);
> }
>
> - jbd2_journal_unregister_shrinker(journal);
> -
> + if (journal->j_shrinker.flags & SHRINKER_REGISTERED) {
> + percpu_counter_destroy(&journal->j_checkpoint_jh_count);
> + unregister_shrinker(&journal->j_shrinker);
> + }
> if (journal->j_proc_entry)
> jbd2_stats_proc_exit(journal);
> iput(journal->j_inode);
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 6cc035321562..fd933c45281a 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -918,11 +918,11 @@ struct journal_s
> struct shrinker j_shrinker;
>
> /**
> - * @j_jh_shrink_count:
> + * @j_checkpoint_jh_count:
> *
> * Number of journal buffers on the checkpoint list. [j_list_lock]
> */
> - struct percpu_counter j_jh_shrink_count;
> + struct percpu_counter j_checkpoint_jh_count;
>
> /**
> * @j_shrink_transaction:
> @@ -1556,8 +1556,6 @@ extern int jbd2_journal_set_features
> (journal_t *, unsigned long, unsigned long, unsigned long);
> extern void jbd2_journal_clear_features
> (journal_t *, unsigned long, unsigned long, unsigned long);
> -extern int jbd2_journal_register_shrinker(journal_t *journal);
> -extern void jbd2_journal_unregister_shrinker(journal_t *journal);
> extern int jbd2_journal_load (journal_t *journal);
> extern int jbd2_journal_destroy (journal_t *);
> extern int jbd2_journal_recover (journal_t *journal);
>
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2021-07-06 1:38 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-02 8:51 [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests Sachin Sant
2021-07-02 8:51 ` Sachin Sant
2021-07-02 9:38 ` Guoqing Jiang
2021-07-02 9:38 ` Guoqing Jiang
2021-07-02 13:13 ` Theodore Ts'o
2021-07-02 13:13 ` Theodore Ts'o
2021-07-02 13:23 ` Zhang Yi
2021-07-02 13:23 ` Zhang Yi
2021-07-02 13:52 ` Zhang Yi
2021-07-02 13:52 ` Zhang Yi
2021-07-02 16:11 ` Theodore Ts'o
2021-07-02 16:11 ` Theodore Ts'o
2021-07-02 22:11 ` Theodore Ts'o
2021-07-02 22:11 ` Theodore Ts'o
2021-07-03 3:37 ` Zhang Yi
2021-07-03 3:37 ` Zhang Yi
2021-07-03 3:52 ` Theodore Ts'o
2021-07-03 3:52 ` Theodore Ts'o
2021-07-03 3:05 ` Zhang Yi
2021-07-03 3:05 ` Zhang Yi
2021-07-03 3:35 ` Theodore Ts'o
2021-07-03 3:35 ` Theodore Ts'o
2021-07-03 4:55 ` Zhang Yi
2021-07-03 4:55 ` Zhang Yi
2021-07-04 14:04 ` Theodore Ts'o
2021-07-04 14:04 ` Theodore Ts'o
2021-07-05 2:17 ` Zhang Yi
2021-07-05 2:17 ` Zhang Yi
2021-07-05 14:50 ` [PATCH -v2] ext4: inline jbd2_journal_[un]register_shrinker() Theodore Ts'o
2021-07-05 18:29 ` Jon Hunter
2021-07-06 1:38 ` Zhang Yi
2021-07-05 9:58 ` [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests Jan Kara
2021-07-05 9:58 ` Jan Kara
2021-07-05 11:27 ` Sachin Sant
2021-07-05 11:27 ` Sachin Sant
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.