Patch 1 fixes memleak in jbd2_journal_write_metadata_buffer.
Patch 2-6 contain some cleanups to jbd2_journal_write_metadata_buffer().
Patch 7-9 contain some cleanups to kjournald2()
All tests in "kvm-xfstest smoke" survive. Please let me konw if more tests
should be ran. Thanks.
Kemeng Shi (9):
jbd2: avoid memleak in jbd2_journal_write_metadata_buffer
jbd2: remove unused return info from
jbd2_journal_write_metadata_buffer
jbd2: remove unnedded "need_copy_out" in
jbd2_journal_write_metadata_buffer
jbd2: move repeat tag around to remove a repeat check of b_frozen_data
jbd2: remove unneeded kmap to do escape in
jbd2_journal_write_metadata_buffer
jbd2: use bh_in instead of jh2bh(jh_in) to simplify code
jbd2: remove dead equality check of j_commit_[sequence/request] in
kjournald2
jbd2: remove dead check of JBD2_UNMOUNT in kjournald2
jbd2: remove unnecessary "should_sleep" in kjournald2
fs/jbd2/journal.c | 35 +++++++++++------------------------
1 file changed, 11 insertions(+), 24 deletions(-)
--
2.30.0
We make sure b_frozen_data is not NULL before jump to "repeat" tag, move
"repeat" tag around to remove repeat check of b_frozen_data.
Signed-off-by: Kemeng Shi <[email protected]>
---
fs/jbd2/journal.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 9a35d0c5b38c..77fcdc76fdfd 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -353,12 +353,12 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
atomic_set(&new_bh->b_count, 1);
spin_lock(&jh_in->b_state_lock);
-repeat:
/*
* If a new transaction has already done a buffer copy-out, then
* we use that version of the data for the commit.
*/
if (jh_in->b_frozen_data) {
+repeat:
done_copy_out = 1;
new_folio = virt_to_folio(jh_in->b_frozen_data);
new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
--
2.30.0
We always set JBD2_UNMOUNT with j_state_lock held in journal_kill_thread.
In kjournald2, we check JBD2_UNMOUNT flag two times under the same
j_state_lock. Then the second check is unnecessary.
Also see comment in struct journal_s about lock rule of j_flags.
Signed-off-by: Kemeng Shi <[email protected]>
---
fs/jbd2/journal.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index e8f592fbd6e1..ce9004f40ffb 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -228,8 +228,6 @@ static int kjournald2(void *arg)
if (transaction && time_after_eq(jiffies,
transaction->t_expires))
should_sleep = 0;
- if (journal->j_flags & JBD2_UNMOUNT)
- should_sleep = 0;
if (should_sleep) {
write_unlock(&journal->j_state_lock);
schedule();
--
2.30.0
The done_copy_out info from jbd2_journal_write_metadata_buffer is not
used. Simply remove it.
Signed-off-by: Kemeng Shi <[email protected]>
---
fs/jbd2/journal.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 207b24e12ce9..068031f35aea 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -320,7 +320,6 @@ static void journal_kill_thread(journal_t *journal)
*
* On success:
* Bit 0 set == escape performed on the data
- * Bit 1 set == buffer copy-out performed (kfree the data after IO)
*/
int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
@@ -455,7 +454,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
set_buffer_shadow(bh_in);
spin_unlock(&jh_in->b_state_lock);
- return do_escape | (done_copy_out << 1);
+ return do_escape;
}
/*
--
2.30.0
In kjournald2, two equality checks of j_commit_[sequence/request] are
under the same j_state_lock. As j_commit_[sequence/request] are updated
concurrently with j_state_lock held during runtime, the second check is
unnecessary.
The j_commit_sequence is only updated concurrently in
jbd2_journal_commit_transaction with j_state_lock held.
The j_commit_request is only updated concurrently in
__jbd2_log_start_commit with j_state_lock held.
Also see comment in struct journal_s about lock rule of j_commit_sequence
and j_commit_request.
Signed-off-by: Kemeng Shi <[email protected]>
---
fs/jbd2/journal.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 01e33b643e4d..e8f592fbd6e1 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -224,8 +224,6 @@ static int kjournald2(void *arg)
prepare_to_wait(&journal->j_wait_commit, &wait,
TASK_INTERRUPTIBLE);
- if (journal->j_commit_sequence != journal->j_commit_request)
- should_sleep = 0;
transaction = journal->j_running_transaction;
if (transaction && time_after_eq(jiffies,
transaction->t_expires))
--
2.30.0
As we only need to copy out when we should do escape, need_copy_out
could be simply replaced by "do_escape".
Signed-off-by: Kemeng Shi <[email protected]>
---
fs/jbd2/journal.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 068031f35aea..9a35d0c5b38c 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -327,7 +327,6 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
struct buffer_head **bh_out,
sector_t blocknr)
{
- int need_copy_out = 0;
int done_copy_out = 0;
int do_escape = 0;
char *mapped_data;
@@ -382,16 +381,14 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
/*
* Check for escaping
*/
- if (*((__be32 *)mapped_data) == cpu_to_be32(JBD2_MAGIC_NUMBER)) {
- need_copy_out = 1;
+ if (*((__be32 *)mapped_data) == cpu_to_be32(JBD2_MAGIC_NUMBER))
do_escape = 1;
- }
kunmap_local(mapped_data);
/*
* Do we need to do a data copy?
*/
- if (need_copy_out && !done_copy_out) {
+ if (do_escape && !done_copy_out) {
char *tmp;
spin_unlock(&jh_in->b_state_lock);
--
2.30.0
On 2024/5/6 22:17, Kemeng Shi wrote:
> The done_copy_out info from jbd2_journal_write_metadata_buffer is not
> used. Simply remove it.
>
> Signed-off-by: Kemeng Shi <[email protected]>
> ---
> fs/jbd2/journal.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 207b24e12ce9..068031f35aea 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -320,7 +320,6 @@ static void journal_kill_thread(journal_t *journal)
> *
> * On success:
> * Bit 0 set == escape performed on the data
> - * Bit 1 set == buffer copy-out performed (kfree the data after IO)
> */
>
> int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> @@ -455,7 +454,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> set_buffer_shadow(bh_in);
> spin_unlock(&jh_in->b_state_lock);
>
> - return do_escape | (done_copy_out << 1);
> + return do_escape;
> }
>
I checked the history, it seems that this bit has not been used since
the very beginning when the jbd code was merged in, I suppose we could
drop it. Since there is only one flag that is still in use, why not just
drop the flag and pass out do_escape through an output parameter, or
directly pass tag_flag, after that we could also drop the weird
"if (flags & 1)" check in jbd2_journal_commit_transaction()?
Thanks,
Yi.
On 2024/5/6 22:17, Kemeng Shi wrote:
> As we only need to copy out when we should do escape, need_copy_out
> could be simply replaced by "do_escape".
>
> Signed-off-by: Kemeng Shi <[email protected]>
Make sense, looks good to me.
Reviewed-by: Zhang Yi <[email protected]>
> ---
> fs/jbd2/journal.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 068031f35aea..9a35d0c5b38c 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -327,7 +327,6 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> struct buffer_head **bh_out,
> sector_t blocknr)
> {
> - int need_copy_out = 0;
> int done_copy_out = 0;
> int do_escape = 0;
> char *mapped_data;
> @@ -382,16 +381,14 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> /*
> * Check for escaping
> */
> - if (*((__be32 *)mapped_data) == cpu_to_be32(JBD2_MAGIC_NUMBER)) {
> - need_copy_out = 1;
> + if (*((__be32 *)mapped_data) == cpu_to_be32(JBD2_MAGIC_NUMBER))
> do_escape = 1;
> - }
> kunmap_local(mapped_data);
>
> /*
> * Do we need to do a data copy?
> */
> - if (need_copy_out && !done_copy_out) {
> + if (do_escape && !done_copy_out) {
> char *tmp;
>
> spin_unlock(&jh_in->b_state_lock);
>
on 5/7/2024 7:51 PM, Zhang Yi wrote:
> On 2024/5/6 22:17, Kemeng Shi wrote:
>> The done_copy_out info from jbd2_journal_write_metadata_buffer is not
>> used. Simply remove it.
>>
>> Signed-off-by: Kemeng Shi <[email protected]>
>> ---
>> fs/jbd2/journal.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
>> index 207b24e12ce9..068031f35aea 100644
>> --- a/fs/jbd2/journal.c
>> +++ b/fs/jbd2/journal.c
>> @@ -320,7 +320,6 @@ static void journal_kill_thread(journal_t *journal)
>> *
>> * On success:
>> * Bit 0 set == escape performed on the data
>> - * Bit 1 set == buffer copy-out performed (kfree the data after IO)
>> */
>>
>> int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>> @@ -455,7 +454,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>> set_buffer_shadow(bh_in);
>> spin_unlock(&jh_in->b_state_lock);
>>
>> - return do_escape | (done_copy_out << 1);
>> + return do_escape;
>> }
>>
>
> I checked the history, it seems that this bit has not been used since
> the very beginning when the jbd code was merged in, I suppose we could
> drop it. Since there is only one flag that is still in use, why not just
> drop the flag and pass out do_escape through an output parameter, or
> directly pass tag_flag, after that we could also drop the weird
> "if (flags & 1)" check in jbd2_journal_commit_transaction()?
Thanks for looking into this. I agree that the "flags & 1" is wired, I
wonder if we could further remove "flags & 1" with following change:
diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 5e122586e06e..67077308b56b 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -353,7 +353,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
struct buffer_head *descriptor;
struct buffer_head **wbuf = journal->j_wbuf;
int bufs;
- int flags;
+ int escape;
int err;
unsigned long long blocknr;
ktime_t start_time;
@@ -661,10 +661,10 @@ void jbd2_journal_commit_transaction(journal_t *journal)
*/
set_bit(BH_JWrite, &jh2bh(jh)->b_state);
JBUFFER_TRACE(jh, "ph3: write metadata");
- flags = jbd2_journal_write_metadata_buffer(commit_transaction,
+ escape = jbd2_journal_write_metadata_buffer(commit_transaction,
jh, &wbuf[bufs], blocknr);
- if (flags < 0) {
- jbd2_journal_abort(journal, flags);
+ if (escape < 0) {
+ jbd2_journal_abort(journal, escape);
continue;
}
jbd2_file_log_bh(&io_bufs, wbuf[bufs]);
@@ -673,7 +673,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
buffer */
tag_flag = 0;
- if (flags & 1)
+ if (escape)
tag_flag |= JBD2_FLAG_ESCAPE;
if (!first_tag)
tag_flag |= JBD2_FLAG_SAME_UUID;
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 08c59197c5bd..c8a1eca5caab 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -309,10 +309,8 @@ static void journal_kill_thread(journal_t *journal)
*
* Return value:
* <0: Error
- * >=0: Finished OK
- *
- * On success:
- * Bit 0 set == escape performed on the data
+ * =0: Finished OK without escape
+ * =1: Finished OK with escape
*/
Look forward to your reply.
Thanks.
Kemeng
>
> Thanks,
> Yi.
>
on 5/7/2024 8:41 PM, Zhang Yi wrote:
> On 2024/5/6 22:17, Kemeng Shi wrote:
>> We make sure b_frozen_data is not NULL before jump to "repeat" tag, move
>> "repeat" tag around to remove repeat check of b_frozen_data.
>>
>> Signed-off-by: Kemeng Shi <[email protected]>
>> ---
>> fs/jbd2/journal.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
>> index 9a35d0c5b38c..77fcdc76fdfd 100644
>> --- a/fs/jbd2/journal.c
>> +++ b/fs/jbd2/journal.c
>> @@ -353,12 +353,12 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>> atomic_set(&new_bh->b_count, 1);
>>
>> spin_lock(&jh_in->b_state_lock);
>> -repeat:
>> /*
>> * If a new transaction has already done a buffer copy-out, then
>> * we use that version of the data for the commit.
>> */
>> if (jh_in->b_frozen_data) {
>> +repeat:
>> done_copy_out = 1;
>> new_folio = virt_to_folio(jh_in->b_frozen_data);
>> new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
>>
>
> I suppose we could drop the repeat tag entirely, just set the new_folio and
> new_offset, and then goto handle do_escape. We don't need to call
> jbd2_buffer_frozen_trigger() and check for escaping again, is that right?
Sure, sounds reasonable to me. Will do it in next version. Thanks
>
> Thanks,
> Yi.
>
On 2024/5/6 22:18, Kemeng Shi wrote:
> We always set JBD2_UNMOUNT with j_state_lock held in journal_kill_thread.
> In kjournald2, we check JBD2_UNMOUNT flag two times under the same
> j_state_lock. Then the second check is unnecessary.
> Also see comment in struct journal_s about lock rule of j_flags.
>
> Signed-off-by: Kemeng Shi <[email protected]>
Looks good to me.
Reviewed-by: Zhang Yi <[email protected]>
> ---
> fs/jbd2/journal.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index e8f592fbd6e1..ce9004f40ffb 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -228,8 +228,6 @@ static int kjournald2(void *arg)
> if (transaction && time_after_eq(jiffies,
> transaction->t_expires))
> should_sleep = 0;
> - if (journal->j_flags & JBD2_UNMOUNT)
> - should_sleep = 0;
> if (should_sleep) {
> write_unlock(&journal->j_state_lock);
> schedule();
>
On Mon 06-05-24 22:17:59, Kemeng Shi wrote:
> In kjournald2, two equality checks of j_commit_[sequence/request] are
> under the same j_state_lock. As j_commit_[sequence/request] are updated
> concurrently with j_state_lock held during runtime, the second check is
> unnecessary.
> The j_commit_sequence is only updated concurrently in
> jbd2_journal_commit_transaction with j_state_lock held.
> The j_commit_request is only updated concurrently in
> __jbd2_log_start_commit with j_state_lock held.
> Also see comment in struct journal_s about lock rule of j_commit_sequence
> and j_commit_request.
>
> Signed-off-by: Kemeng Shi <[email protected]>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/jbd2/journal.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 01e33b643e4d..e8f592fbd6e1 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -224,8 +224,6 @@ static int kjournald2(void *arg)
>
> prepare_to_wait(&journal->j_wait_commit, &wait,
> TASK_INTERRUPTIBLE);
> - if (journal->j_commit_sequence != journal->j_commit_request)
> - should_sleep = 0;
> transaction = journal->j_running_transaction;
> if (transaction && time_after_eq(jiffies,
> transaction->t_expires))
> --
> 2.30.0
>
--
Jan Kara <[email protected]>
SUSE Labs, CR