v1->v2:
-Collect RVB from Yi and Jan
-remove rename "flags" to "escape" in patch 2/9
-goto new copy_done tag in patch 4/9
-add more comment in patch 5/9
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: jump to new copy_done tag when b_frozen_data is created
concurrently
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/commit.c | 10 ++++-----
fs/jbd2/journal.c | 54 ++++++++++++++++++-----------------------------
2 files changed, 25 insertions(+), 39 deletions(-)
--
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/commit.c | 10 +++++-----
fs/jbd2/journal.c | 9 +++------
2 files changed, 8 insertions(+), 11 deletions(-)
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 207b24e12ce9..2dca2f613a8e 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -316,11 +316,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
- * Bit 1 set == buffer copy-out performed (kfree the data after IO)
+ * =0: Finished OK without escape
+ * =1: Finished OK with escape
*/
int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
@@ -455,7 +452,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
The new_bh is from alloc_buffer_head, we should call free_buffer_head to
free it in error case.
Signed-off-by: Kemeng Shi <[email protected]>
Reviewed-by: Zhang Yi <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
---
fs/jbd2/journal.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index b6c114c11b97..207b24e12ce9 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -399,6 +399,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
tmp = jbd2_alloc(bh_in->b_size, GFP_NOFS);
if (!tmp) {
brelse(new_bh);
+ free_buffer_head(new_bh);
return -ENOMEM;
}
spin_lock(&jh_in->b_state_lock);
--
2.30.0
We save jh2bh(jh_in) to bh_in, so use bh_in directly instead of
jh2bh(jh_in) to simplify the code.
Signed-off-by: Kemeng Shi <[email protected]>
Reviewed-by: Zhang Yi <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
---
fs/jbd2/journal.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index c98c06f3113a..71568cb82db7 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -360,8 +360,8 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
new_folio = virt_to_folio(jh_in->b_frozen_data);
new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
} else {
- new_folio = jh2bh(jh_in)->b_folio;
- new_offset = offset_in_folio(new_folio, jh2bh(jh_in)->b_data);
+ new_folio = bh_in->b_folio;
+ new_offset = offset_in_folio(new_folio, bh_in->b_data);
}
mapped_data = kmap_local_folio(new_folio, new_offset);
--
2.30.0
The j_commit_[sequence/request] are updated with j_state_lock held during
runtime. In kjournald2, two equality checks of j_commit_[sequence/request]
are under the same j_state_lock, then the second check is unnecessary.
Signed-off-by: Kemeng Shi <[email protected]>
Reviewed-by: Zhang Yi <[email protected]>
Reviewed-by: Jan Kara <[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 71568cb82db7..4f1ab68cdb6f 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
If b_frozen_data is created concurrently, we can update new_folio and
new_offset with b_frozen_data and then move forward
Signed-off-by: Kemeng Shi <[email protected]>
---
fs/jbd2/journal.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 714e2ef0115a..5fb5062cf7ae 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -351,7 +351,6 @@ 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.
@@ -399,22 +398,22 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
spin_lock(&jh_in->b_state_lock);
if (jh_in->b_frozen_data) {
jbd2_free(tmp, bh_in->b_size);
- goto repeat;
+ goto copy_done;
}
jh_in->b_frozen_data = tmp;
memcpy_from_folio(tmp, new_folio, new_offset, bh_in->b_size);
-
- new_folio = virt_to_folio(tmp);
- new_offset = offset_in_folio(new_folio, tmp);
- done_copy_out = 1;
-
/*
* This isn't strictly necessary, as we're using frozen
* data for the escaping, but it keeps consistency with
* b_frozen_data usage.
*/
jh_in->b_frozen_triggers = jh_in->b_triggers;
+
+copy_done:
+ new_folio = virt_to_folio(jh_in->b_frozen_data);
+ new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
+ done_copy_out = 1;
}
/*
--
2.30.0
We only need to sleep if no running transaction is expired. Simply remove
unnecessary "should_sleep".
Signed-off-by: Kemeng Shi <[email protected]>
Reviewed-by: Zhang Yi <[email protected]>
Reviewed-by: Jan Kara <[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 59bff0b75ce7..bf3a425dc057 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -220,15 +220,12 @@ static int kjournald2(void *arg)
* so we don't sleep
*/
DEFINE_WAIT(wait);
- int should_sleep = 1;
prepare_to_wait(&journal->j_wait_commit, &wait,
TASK_INTERRUPTIBLE);
transaction = journal->j_running_transaction;
- if (transaction && time_after_eq(jiffies,
- transaction->t_expires))
- should_sleep = 0;
- if (should_sleep) {
+ if (transaction == NULL ||
+ time_before(jiffies, transaction->t_expires)) {
write_unlock(&journal->j_state_lock);
schedule();
write_lock(&journal->j_state_lock);
--
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]>
Reviewed-by: Zhang Yi <[email protected]>
Reviewed-by: Jan Kara <[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 2dca2f613a8e..714e2ef0115a 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -325,7 +325,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;
@@ -380,16 +379,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
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.
Signed-off-by: Kemeng Shi <[email protected]>
Reviewed-by: Zhang Yi <[email protected]>
Reviewed-by: Jan Kara <[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 4f1ab68cdb6f..59bff0b75ce7 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
On 2024/5/14 19:24, 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]>
Thanks, looks good to me.
Reviewed-by: Zhang Yi <[email protected]>
> ---
> fs/jbd2/commit.c | 10 +++++-----
> fs/jbd2/journal.c | 9 +++------
> 2 files changed, 8 insertions(+), 11 deletions(-)
>
> 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 207b24e12ce9..2dca2f613a8e 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -316,11 +316,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
> - * Bit 1 set == buffer copy-out performed (kfree the data after IO)
> + * =0: Finished OK without escape
> + * =1: Finished OK with escape
> */
>
> int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> @@ -455,7 +452,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;
> }
>
> /*
>
On 2024/5/14 19:24, Kemeng Shi wrote:
> If b_frozen_data is created concurrently, we can update new_folio and
> new_offset with b_frozen_data and then move forward
>
> Signed-off-by: Kemeng Shi <[email protected]>
It's more straight forward, looks good to me.
Reviewed-by: Zhang Yi <[email protected]>
> ---
> fs/jbd2/journal.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 714e2ef0115a..5fb5062cf7ae 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -351,7 +351,6 @@ 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.
> @@ -399,22 +398,22 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> spin_lock(&jh_in->b_state_lock);
> if (jh_in->b_frozen_data) {
> jbd2_free(tmp, bh_in->b_size);
> - goto repeat;
> + goto copy_done;
> }
>
> jh_in->b_frozen_data = tmp;
> memcpy_from_folio(tmp, new_folio, new_offset, bh_in->b_size);
> -
> - new_folio = virt_to_folio(tmp);
> - new_offset = offset_in_folio(new_folio, tmp);
> - done_copy_out = 1;
> -
> /*
> * This isn't strictly necessary, as we're using frozen
> * data for the escaping, but it keeps consistency with
> * b_frozen_data usage.
> */
> jh_in->b_frozen_triggers = jh_in->b_triggers;
> +
> +copy_done:
> + new_folio = virt_to_folio(jh_in->b_frozen_data);
> + new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
> + done_copy_out = 1;
> }
>
> /*
>
On Tue 14-05-24 19:24:31, 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]>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/jbd2/commit.c | 10 +++++-----
> fs/jbd2/journal.c | 9 +++------
> 2 files changed, 8 insertions(+), 11 deletions(-)
>
> 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 207b24e12ce9..2dca2f613a8e 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -316,11 +316,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
> - * Bit 1 set == buffer copy-out performed (kfree the data after IO)
> + * =0: Finished OK without escape
> + * =1: Finished OK with escape
> */
>
> int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> @@ -455,7 +452,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
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Tue 14-05-24 19:24:33, Kemeng Shi wrote:
> If b_frozen_data is created concurrently, we can update new_folio and
> new_offset with b_frozen_data and then move forward
>
> Signed-off-by: Kemeng Shi <[email protected]>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/jbd2/journal.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 714e2ef0115a..5fb5062cf7ae 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -351,7 +351,6 @@ 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.
> @@ -399,22 +398,22 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> spin_lock(&jh_in->b_state_lock);
> if (jh_in->b_frozen_data) {
> jbd2_free(tmp, bh_in->b_size);
> - goto repeat;
> + goto copy_done;
> }
>
> jh_in->b_frozen_data = tmp;
> memcpy_from_folio(tmp, new_folio, new_offset, bh_in->b_size);
> -
> - new_folio = virt_to_folio(tmp);
> - new_offset = offset_in_folio(new_folio, tmp);
> - done_copy_out = 1;
> -
> /*
> * This isn't strictly necessary, as we're using frozen
> * data for the escaping, but it keeps consistency with
> * b_frozen_data usage.
> */
> jh_in->b_frozen_triggers = jh_in->b_triggers;
> +
> +copy_done:
> + new_folio = virt_to_folio(jh_in->b_frozen_data);
> + new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
> + done_copy_out = 1;
> }
>
> /*
> --
> 2.30.0
>
--
Jan Kara <[email protected]>
SUSE Labs, CR