Re: [powerpc][5.13.0-next-20210701] Kernel crash while running ltp(chdir01) tests
From: Zhang Yi <yi.zhang@huawei.com>
Date: 2021-07-03 03:37:15
Also in:
linux-fsdevel, linuxppc-dev
On 2021/7/3 6:11, Theodore Ts'o wrote:
quoted hunk ↗ jump to hunk
On Fri, Jul 02, 2021 at 12:11:54PM -0400, Theodore Ts'o wrote:quoted
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 [off-list ref] 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 [off-list ref] Signed-off-by: Theodore Ts'o [off-list ref]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.
quoted hunk ↗ jump to hunk
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);.