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
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
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.
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.
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.
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
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
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);
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.
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
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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Theodore Ts'o <[email protected]>
>
> 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);
> .
>
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
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.
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 <[email protected]>
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 <[email protected]>
Reported-by: Jon Hunter <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
---
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
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 <[email protected]>
> 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 <[email protected]>
> Reported-by: Jon Hunter <[email protected]>
> Signed-off-by: Theodore Ts'o <[email protected]>
> ---
> 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.
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 <[email protected]>
> 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 <[email protected]>
> Reported-by: Jon Hunter <[email protected]>
> Signed-off-by: Theodore Ts'o <[email protected]>
Except for the bug Zhang Yi noticed the patch looks good to me. Feel free
to add:
Reviewed-by: Jan Kara <[email protected]>
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 <[email protected]>
SUSE Labs, CR
> On 04-Jul-2021, at 7:34 PM, Theodore Ts'o <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> Reported-by: Jon Hunter <[email protected]>
> Signed-off-by: Theodore Ts'o <[email protected]>
> ---
This patch fixes the reported problem. Test ran to completion
without any crash.
Tested-by: Sachin Sant <[email protected]>
-Sachin
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 <[email protected]>
Reported-by: Sachin Sant <[email protected]>
Tested-by: Sachin Sant <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
---
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
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 <[email protected]>
> Reported-by: Sachin Sant <[email protected]>
> Tested-by: Sachin Sant <[email protected]>
> Reviewed-by: Jan Kara <[email protected]>
> Signed-off-by: Theodore Ts'o <[email protected]>
Thanks, works for me.
Tested-by: Jon Hunter <[email protected]>
Cheers
Jon
--
nvpublic
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 <[email protected]>
> Reported-by: Sachin Sant <[email protected]>
> Tested-by: Sachin Sant <[email protected]>
> Reviewed-by: Jan Kara <[email protected]>
> Signed-off-by: Theodore Ts'o <[email protected]>
This patch looks good to me.
Reviewed-by: Zhang Yi <[email protected]>
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);
>