From: Zhang Yi <[email protected]>
The write_flags print in the trace of jbd2_write_superblock() is not
real, so move the modification before the trace.
Signed-off-by: Zhang Yi <[email protected]>
---
fs/jbd2/journal.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 30dec2bd2ecc..e7aa47a02d4d 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1791,9 +1791,11 @@ static int jbd2_write_superblock(journal_t *journal, blk_opf_t write_flags)
return -EIO;
}
- trace_jbd2_write_superblock(journal, write_flags);
if (!(journal->j_flags & JBD2_BARRIER))
write_flags &= ~(REQ_FUA | REQ_PREFLUSH);
+
+ trace_jbd2_write_superblock(journal, write_flags);
+
if (buffer_write_io_error(bh)) {
/*
* Oh, dear. A previous attempt to write the journal
--
2.39.2
From: Zhang Yi <[email protected]>
Current jbd2 only add REQ_SYNC for descriptor block, metadata log
buffer, commit buffer and superblock buffer, the submitted IO could be
throttled by writeback throttle in block layer, that could lead to
priority inversion in some cases. The log IO looks like a kind of high
priority metadata IO, so it should not be throttled by WBT like QOS
policies in block layer, let's add REQ_SYNC | REQ_IDLE to exempt from
writeback throttle, and also add REQ_META together indicates it's a
metadata IO.
Signed-off-by: Zhang Yi <[email protected]>
---
fs/jbd2/commit.c | 8 ++++----
fs/jbd2/journal.c | 20 +++++++++++---------
include/linux/jbd2.h | 3 +++
3 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 8d6f934c3d95..58e4d4cbdf88 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -119,7 +119,7 @@ static int journal_submit_commit_record(journal_t *journal,
struct commit_header *tmp;
struct buffer_head *bh;
struct timespec64 now;
- blk_opf_t write_flags = REQ_OP_WRITE | REQ_SYNC;
+ blk_opf_t write_flags = REQ_OP_WRITE | JBD2_REQ_HIPRIO;
*cbh = NULL;
@@ -395,8 +395,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
*/
jbd2_journal_update_sb_log_tail(journal,
journal->j_tail_sequence,
- journal->j_tail,
- REQ_SYNC);
+ journal->j_tail, 0);
mutex_unlock(&journal->j_checkpoint_mutex);
} else {
jbd2_debug(3, "superblock not updated\n");
@@ -715,6 +714,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
for (i = 0; i < bufs; i++) {
struct buffer_head *bh = wbuf[i];
+
/*
* Compute checksum.
*/
@@ -727,7 +727,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
clear_buffer_dirty(bh);
set_buffer_uptodate(bh);
bh->b_end_io = journal_end_buffer_io_sync;
- submit_bh(REQ_OP_WRITE | REQ_SYNC, bh);
+ submit_bh(REQ_OP_WRITE | JBD2_REQ_HIPRIO, bh);
}
cond_resched();
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index e7aa47a02d4d..f2921d728dcc 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1100,8 +1100,7 @@ int __jbd2_update_log_tail(journal_t *journal, tid_t tid, unsigned long block)
* space and if we lose sb update during power failure we'd replay
* old transaction with possibly newly overwritten data.
*/
- ret = jbd2_journal_update_sb_log_tail(journal, tid, block,
- REQ_SYNC | REQ_FUA);
+ ret = jbd2_journal_update_sb_log_tail(journal, tid, block, REQ_FUA);
if (ret)
goto out;
@@ -1768,8 +1767,7 @@ static int journal_reset(journal_t *journal)
*/
jbd2_journal_update_sb_log_tail(journal,
journal->j_tail_sequence,
- journal->j_tail,
- REQ_SYNC | REQ_FUA);
+ journal->j_tail, REQ_FUA);
mutex_unlock(&journal->j_checkpoint_mutex);
}
return jbd2_journal_start_thread(journal);
@@ -1791,6 +1789,11 @@ static int jbd2_write_superblock(journal_t *journal, blk_opf_t write_flags)
return -EIO;
}
+ /*
+ * Always set high priority flags to exempt from block layer's
+ * QOS policies, e.g. writeback throttle.
+ */
+ write_flags |= JBD2_REQ_HIPRIO;
if (!(journal->j_flags & JBD2_BARRIER))
write_flags &= ~(REQ_FUA | REQ_PREFLUSH);
@@ -2045,7 +2048,7 @@ void jbd2_journal_update_sb_errno(journal_t *journal)
jbd2_debug(1, "JBD2: updating superblock error (errno %d)\n", errcode);
sb->s_errno = cpu_to_be32(errcode);
- jbd2_write_superblock(journal, REQ_SYNC | REQ_FUA);
+ jbd2_write_superblock(journal, REQ_FUA);
}
EXPORT_SYMBOL(jbd2_journal_update_sb_errno);
@@ -2166,8 +2169,7 @@ int jbd2_journal_destroy(journal_t *journal)
++journal->j_transaction_sequence;
write_unlock(&journal->j_state_lock);
- jbd2_mark_journal_empty(journal,
- REQ_SYNC | REQ_PREFLUSH | REQ_FUA);
+ jbd2_mark_journal_empty(journal, REQ_PREFLUSH | REQ_FUA);
mutex_unlock(&journal->j_checkpoint_mutex);
} else
err = -EIO;
@@ -2468,7 +2470,7 @@ int jbd2_journal_flush(journal_t *journal, unsigned int flags)
* the magic code for a fully-recovered superblock. Any future
* commits of data to the journal will restore the current
* s_start value. */
- jbd2_mark_journal_empty(journal, REQ_SYNC | REQ_FUA);
+ jbd2_mark_journal_empty(journal, REQ_FUA);
if (flags)
err = __jbd2_journal_erase(journal, flags);
@@ -2514,7 +2516,7 @@ int jbd2_journal_wipe(journal_t *journal, int write)
if (write) {
/* Lock to make assertions happy... */
mutex_lock_io(&journal->j_checkpoint_mutex);
- jbd2_mark_journal_empty(journal, REQ_SYNC | REQ_FUA);
+ jbd2_mark_journal_empty(journal, REQ_FUA);
mutex_unlock(&journal->j_checkpoint_mutex);
}
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 52772c826c86..f7e8274b46ae 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1374,6 +1374,9 @@ JBD2_FEATURE_INCOMPAT_FUNCS(csum2, CSUM_V2)
JBD2_FEATURE_INCOMPAT_FUNCS(csum3, CSUM_V3)
JBD2_FEATURE_INCOMPAT_FUNCS(fast_commit, FAST_COMMIT)
+/* Journal high priority write IO operation flags */
+#define JBD2_REQ_HIPRIO (REQ_META | REQ_SYNC | REQ_IDLE)
+
/*
* Journal flag definitions
*/
--
2.39.2
On Sat 25-11-23 20:17:38, Zhang Yi wrote:
> From: Zhang Yi <[email protected]>
>
> The write_flags print in the trace of jbd2_write_superblock() is not
> real, so move the modification before the trace.
>
> Signed-off-by: Zhang Yi <[email protected]>
Good catch. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/jbd2/journal.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 30dec2bd2ecc..e7aa47a02d4d 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1791,9 +1791,11 @@ static int jbd2_write_superblock(journal_t *journal, blk_opf_t write_flags)
> return -EIO;
> }
>
> - trace_jbd2_write_superblock(journal, write_flags);
> if (!(journal->j_flags & JBD2_BARRIER))
> write_flags &= ~(REQ_FUA | REQ_PREFLUSH);
> +
> + trace_jbd2_write_superblock(journal, write_flags);
> +
> if (buffer_write_io_error(bh)) {
> /*
> * Oh, dear. A previous attempt to write the journal
> --
> 2.39.2
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
Hello!
On Sat 25-11-23 20:17:39, Zhang Yi wrote:
> From: Zhang Yi <[email protected]>
>
> Current jbd2 only add REQ_SYNC for descriptor block, metadata log
> buffer, commit buffer and superblock buffer, the submitted IO could be
> throttled by writeback throttle in block layer, that could lead to
> priority inversion in some cases. The log IO looks like a kind of high
> priority metadata IO, so it should not be throttled by WBT like QOS
> policies in block layer, let's add REQ_SYNC | REQ_IDLE to exempt from
> writeback throttle, and also add REQ_META together indicates it's a
> metadata IO.
So I agree about REQ_META but with REQ_IDLE I'm not so sure. __REQ_IDLE is
documented as "anticipate more IO after this one" so that is a good fit for
normal transaction writes but not so much for journal superblock writes.
OTOH as far as I'm checking XFS uses REQ_IDLE as well for its log IO and
the only places where REQ_IDLE is really checked is in blk-wbt... So I
guess this makes sense.
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 52772c826c86..f7e8274b46ae 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1374,6 +1374,9 @@ JBD2_FEATURE_INCOMPAT_FUNCS(csum2, CSUM_V2)
> JBD2_FEATURE_INCOMPAT_FUNCS(csum3, CSUM_V3)
> JBD2_FEATURE_INCOMPAT_FUNCS(fast_commit, FAST_COMMIT)
>
> +/* Journal high priority write IO operation flags */
> +#define JBD2_REQ_HIPRIO (REQ_META | REQ_SYNC | REQ_IDLE)
Since all journal IO is submitted with these flags I'd maybe name this
JBD2_JOURNAL_REQ_FLAGS? Otherwise the patch looks good to me so feel free
to add:
Reviewed-by: Jan Kara <[email protected]>
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
Hello!
On 2023/11/28 0:11, Jan Kara wrote:
> Hello!
>
> On Sat 25-11-23 20:17:39, Zhang Yi wrote:
>> From: Zhang Yi <[email protected]>
>>
>> Current jbd2 only add REQ_SYNC for descriptor block, metadata log
>> buffer, commit buffer and superblock buffer, the submitted IO could be
>> throttled by writeback throttle in block layer, that could lead to
>> priority inversion in some cases. The log IO looks like a kind of high
>> priority metadata IO, so it should not be throttled by WBT like QOS
>> policies in block layer, let's add REQ_SYNC | REQ_IDLE to exempt from
>> writeback throttle, and also add REQ_META together indicates it's a
>> metadata IO.
>
> So I agree about REQ_META but with REQ_IDLE I'm not so sure. __REQ_IDLE is
> documented as "anticipate more IO after this one" so that is a good fit for
> normal transaction writes but not so much for journal superblock writes.
> OTOH as far as I'm checking XFS uses REQ_IDLE as well for its log IO and
> the only places where REQ_IDLE is really checked is in blk-wbt... So I
> guess this makes sense.
>
Thanks a lot for your review. Yeah, We've checked the block cgroup related
qos policies and blk-wbt, block cgroup based policies does not throttle IO
with REQ_META, and blk-wbt exempt IO with both REQ_IDLE and REQ_SYNC. But
submit_bh() can make sure the journal IO is always bind to the root cgroup,
so only blk-wbt is left for us to deal with, so I add REQ_IDLE like xfs does.
>> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
>> index 52772c826c86..f7e8274b46ae 100644
>> --- a/include/linux/jbd2.h
>> +++ b/include/linux/jbd2.h
>> @@ -1374,6 +1374,9 @@ JBD2_FEATURE_INCOMPAT_FUNCS(csum2, CSUM_V2)
>> JBD2_FEATURE_INCOMPAT_FUNCS(csum3, CSUM_V3)
>> JBD2_FEATURE_INCOMPAT_FUNCS(fast_commit, FAST_COMMIT)
>>
>> +/* Journal high priority write IO operation flags */
>> +#define JBD2_REQ_HIPRIO (REQ_META | REQ_SYNC | REQ_IDLE)
>
> Since all journal IO is submitted with these flags I'd maybe name this
> JBD2_JOURNAL_REQ_FLAGS? Otherwise the patch looks good to me so feel free
> to add:
>
Sure, JBD2_JOURNAL_REQ_FLAGS looks fine, I will use it in v2.
Thanks,
Yi.