| Message ID | 20240506141801.1165315-3-shikemeng@huaweicloud.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | A fix and some cleanups to jbd2 | expand |
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 <shikemeng@huaweicloud.com> > --- > 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 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 <shikemeng@huaweicloud.com> >> --- >> 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 Wed 08-05-24 09:42:30, Kemeng Shi wrote: > > > 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 <shikemeng@huaweicloud.com> > >> --- > >> 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. Yep, looks like a fine solution to me. Honza
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; } /*
The done_copy_out info from jbd2_journal_write_metadata_buffer is not used. Simply remove it. Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> --- fs/jbd2/journal.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)