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?
On Fri, Jul 02, 2021 at 05:38:10PM +0800, Guoqing Jiang wrote:
quoted hunk
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?
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.
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?
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:
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?
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:
@@ -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);returnerr;
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 [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]
From: Zhang Yi <yi.zhang@huawei.com> Date: 2021-07-03 03:37:15
On 2021/7/3 6:11, Theodore Ts'o wrote:
quoted 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]
@@ -2043,7 +2045,8 @@ int jbd2_journal_load(journal_t *journal)gotorecovery_error;journal->j_flags|=JBD2_LOADED;-return0;++returnjbd2_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.
@@ -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;
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
From: Zhang Yi <yi.zhang@huawei.com> Date: 2021-07-03 03:05:11
On 2021/7/3 0:11, Theodore Ts'o wrote:
On Fri, Jul 02, 2021 at 09:52:13PM +0800, Zhang Yi wrote:
quoted
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
From: Zhang Yi <yi.zhang@huawei.com> Date: 2021-07-03 04:55:15
On 2021/7/3 11:35, Theodore Ts'o wrote:
quoted hunk
On Sat, Jul 03, 2021 at 11:05:07AM +0800, Zhang Yi wrote:
quoted
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. :-)
quoted
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 <tytso@mit.edu>
Date: Fri, 2 Jul 2021 18:05:03 -0400
Subject: [PATCH] ext4: inline jbd2_journal_[un]register_shrinker()
The function jbd2_journal_unregister_shrinker() was getting called
twice when the file system was getting unmounted. On Power and ARM
platforms this was causing kernel crash when unmounting the file
system, when a percpu_counter was destroyed twice.
Fix this by removing jbd2_journal_[un]register_shrinker() functions,
and inlining the shrinker setup and teardown into
journal_init_common() and jbd2_journal_destroy(). This means that
ext4 and ocfs2 now no longer need to know about registering and
unregistering jbd2's shrinker.
Also, while we're at it, rename the percpu counter from
j_jh_shrink_count to j_checkpoint_jh_count, since this makes it
clearer what this counter is intended to track.
Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
Reported-by: Sachin Sant <redacted>
Reported-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
fs/ext4/super.c | 8 ---
fs/jbd2/checkpoint.c | 4 +-
fs/jbd2/journal.c | 148 +++++++++++++++++--------------------------
include/linux/jbd2.h | 6 +-
4 files changed, 63 insertions(+), 103 deletions(-)
@@ -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? */
From: Zhang Yi <yi.zhang@huawei.com> Date: 2021-07-05 02:17:34
On 2021/7/4 22:04, Theodore Ts'o wrote:
On Sat, Jul 03, 2021 at 12:55:09PM +0800, Zhang Yi wrote:
quoted
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.
quoted
From ef3130d1b0b8ca769252d6a722a2e59a00141383 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Fri, 2 Jul 2021 18:05:03 -0400
Subject: [PATCH] ext4: inline jbd2_journal_[un]register_shrinker()
The function jbd2_journal_unregister_shrinker() was getting called
twice when the file system was getting unmounted. On Power and ARM
platforms this was causing kernel crash when unmounting the file
system, when a percpu_counter was destroyed twice.
Fix this by removing jbd2_journal_[un]register_shrinker() functions,
and inlining the shrinker setup and teardown into
journal_init_common() and jbd2_journal_destroy(). This means that
ext4 and ocfs2 now no longer need to know about registering and
unregistering jbd2's shrinker.
Also, while we're at it, rename the percpu counter from
j_jh_shrink_count to j_checkpoint_jh_count, since this makes it
clearer what this counter is intended to track.
Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
Reported-by: Sachin Sant <redacted>
Reported-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
fs/ext4/super.c | 8 ---
fs/jbd2/checkpoint.c | 4 +-
fs/jbd2/journal.c | 148 +++++++++++++++++--------------------------
include/linux/jbd2.h | 6 +-
4 files changed, 63 insertions(+), 103 deletions(-)
From: Jan Kara <jack@suse.cz> Date: 2021-07-05 09:59:01
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:
quoted
Yeah, it sounds good to me. Do you want me to send the fix patch, or you
modify your commit 8f9e16badb8fd in another email directly?
I've gone ahead and made the changes; what do you think?
I like how it also removes 40 lines of code. :-)
- Ted
From ef3130d1b0b8ca769252d6a722a2e59a00141383 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Fri, 2 Jul 2021 18:05:03 -0400
Subject: [PATCH] ext4: inline jbd2_journal_[un]register_shrinker()
The function jbd2_journal_unregister_shrinker() was getting called
twice when the file system was getting unmounted. On Power and ARM
platforms this was causing kernel crash when unmounting the file
system, when a percpu_counter was destroyed twice.
Fix this by removing jbd2_journal_[un]register_shrinker() functions,
and inlining the shrinker setup and teardown into
journal_init_common() and jbd2_journal_destroy(). This means that
ext4 and ocfs2 now no longer need to know about registering and
unregistering jbd2's shrinker.
Also, while we're at it, rename the percpu counter from
j_jh_shrink_count to j_checkpoint_jh_count, since this makes it
clearer what this counter is intended to track.
Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
Reported-by: Sachin Sant <redacted>
Reported-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Except for the bug Zhang Yi noticed the patch looks good to me. Feel free
to add:
Reviewed-by: Jan Kara <jack@suse.cz>
after fixing that.
Honza
@@ -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? */
From: Sachin Sant <hidden> Date: 2021-07-05 11:27:51
On 04-Jul-2021, at 7:34 PM, Theodore Ts'o [off-list ref] wrote:
On Sat, Jul 03, 2021 at 12:55:09PM +0800, Zhang Yi wrote:
quoted
Yeah, it sounds good to me. Do you want me to send the fix patch, or you
modify your commit 8f9e16badb8fd in another email directly?
I've gone ahead and made the changes; what do you think?
I like how it also removes 40 lines of code. :-)
- Ted
From ef3130d1b0b8ca769252d6a722a2e59a00141383 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Fri, 2 Jul 2021 18:05:03 -0400
Subject: [PATCH] ext4: inline jbd2_journal_[un]register_shrinker()
The function jbd2_journal_unregister_shrinker() was getting called
twice when the file system was getting unmounted. On Power and ARM
platforms this was causing kernel crash when unmounting the file
system, when a percpu_counter was destroyed twice.
Fix this by removing jbd2_journal_[un]register_shrinker() functions,
and inlining the shrinker setup and teardown into
journal_init_common() and jbd2_journal_destroy(). This means that
ext4 and ocfs2 now no longer need to know about registering and
unregistering jbd2's shrinker.
Also, while we're at it, rename the percpu counter from
j_jh_shrink_count to j_checkpoint_jh_count, since this makes it
clearer what this counter is intended to track.
Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
Reported-by: Sachin Sant <redacted>
Reported-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
This patch fixes the reported problem. Test ran to completion
without any crash.
Tested-by: Sachin Sant <redacted>
-Sachin