From add3efdd78b8a0478ce423bb9d4df6bd95e8b335 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Tue, 5 Nov 2019 17:44:07 +0100 Subject: jbd2: Fix possible overflow in jbd2_log_space_left() When number of free space in the journal is very low, the arithmetic in jbd2_log_space_left() could underflow resulting in very high number of free blocks and thus triggering assertion failure in transaction commit code complaining there's not enough space in the journal: J_ASSERT(journal->j_free > 1); Properly check for the low number of free blocks. CC: stable@vger.kernel.org Reviewed-by: Theodore Ts'o Signed-off-by: Jan Kara Link: https://lore.kernel.org/r/20191105164437.32602-1-jack@suse.cz Signed-off-by: Theodore Ts'o --- include/linux/jbd2.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'include') diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index 603fbc4e2f70..10e6049c0ba9 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -1582,7 +1582,7 @@ static inline int jbd2_space_needed(journal_t *journal) static inline unsigned long jbd2_log_space_left(journal_t *journal) { /* Allow for rounding errors */ - unsigned long free = journal->j_free - 32; + long free = journal->j_free - 32; if (journal->j_committing_transaction) { unsigned long committing = atomic_read(&journal-> @@ -1591,7 +1591,7 @@ static inline unsigned long jbd2_log_space_left(journal_t *journal) /* Transaction + control blocks */ free -= committing + (committing >> JBD2_CONTROL_BLOCKS_SHIFT); } - return free; + return max_t(long, free, 0); } /* -- cgit v1.2.3 From a9a8344ee1714f835ba394077e8c13d751e2f148 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Tue, 5 Nov 2019 17:44:17 +0100 Subject: ext4, jbd2: Provide accessor function for handle credits Provide accessor function to get number of credits available in a handle and use it from ext4. Later, computation of available credits won't be so straightforward. Reviewed-by: Theodore Ts'o Signed-off-by: Jan Kara Link: https://lore.kernel.org/r/20191105164437.32602-11-jack@suse.cz Signed-off-by: Theodore Ts'o --- include/linux/jbd2.h | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'include') diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index 10e6049c0ba9..727ff91d7f3e 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -1645,6 +1645,12 @@ static inline tid_t jbd2_get_latest_transaction(journal_t *journal) return tid; } + +static inline int jbd2_handle_buffer_credits(handle_t *handle) +{ + return handle->h_buffer_credits; +} + #ifdef __KERNEL__ #define buffer_trace_init(bh) do {} while (0) -- cgit v1.2.3 From 9f356e5a4f12008fa0df8b6385fc0ab830416e72 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Tue, 5 Nov 2019 17:44:24 +0100 Subject: jbd2: Account descriptor blocks into t_outstanding_credits Currently, journal descriptor blocks were not accounted in transaction->t_outstanding_credits and we were just leaving some slack space in the journal for them (in jbd2_log_space_left() and jbd2_space_needed()). This is making proper accounting (and reservation we want to add) of descriptor blocks difficult so switch to accounting descriptor blocks in transaction->t_outstanding_credits and just reserve the same amount of credits in t_outstanding credits for journal descriptor blocks when creating transaction. Signed-off-by: Jan Kara Link: https://lore.kernel.org/r/20191105164437.32602-18-jack@suse.cz Signed-off-by: Theodore Ts'o --- include/linux/jbd2.h | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) (limited to 'include') diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index 727ff91d7f3e..bef4f74b1ea0 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -681,8 +681,10 @@ struct transaction_s atomic_t t_updates; /* - * Number of buffers reserved for use by all handles in this transaction - * handle but not yet modified. [none] + * Number of blocks reserved for this transaction in the journal. + * This is including all credits reserved when starting transaction + * handles as well as all journal descriptor blocks needed for this + * transaction. [none] */ atomic_t t_outstanding_credits; @@ -1560,20 +1562,13 @@ static inline int jbd2_journal_has_csum_v2or3(journal_t *journal) return journal->j_chksum_driver != NULL; } -/* - * We reserve t_outstanding_credits >> JBD2_CONTROL_BLOCKS_SHIFT for - * transaction control blocks. - */ -#define JBD2_CONTROL_BLOCKS_SHIFT 5 - /* * Return the minimum number of blocks which must be free in the journal * before a new transaction may be started. Must be called under j_state_lock. */ static inline int jbd2_space_needed(journal_t *journal) { - int nblocks = journal->j_max_transaction_buffers; - return nblocks + (nblocks >> JBD2_CONTROL_BLOCKS_SHIFT); + return journal->j_max_transaction_buffers; } /* @@ -1585,11 +1580,8 @@ static inline unsigned long jbd2_log_space_left(journal_t *journal) long free = journal->j_free - 32; if (journal->j_committing_transaction) { - unsigned long committing = atomic_read(&journal-> - j_committing_transaction->t_outstanding_credits); - - /* Transaction + control blocks */ - free -= committing + (committing >> JBD2_CONTROL_BLOCKS_SHIFT); + free -= atomic_read(&journal-> + j_committing_transaction->t_outstanding_credits); } return max_t(long, free, 0); } -- cgit v1.2.3 From 77444ac4f9537bc4211f928959d5231445e30c6e Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Tue, 5 Nov 2019 17:44:25 +0100 Subject: jbd2: Drop jbd2_space_needed() The function is now just a trivial wrapper returning journal->j_max_transaction_buffers. Drop it. Reviewed-by: Theodore Ts'o Signed-off-by: Jan Kara Link: https://lore.kernel.org/r/20191105164437.32602-19-jack@suse.cz Signed-off-by: Theodore Ts'o --- include/linux/jbd2.h | 9 --------- 1 file changed, 9 deletions(-) (limited to 'include') diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index bef4f74b1ea0..1dd2703a8e26 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -1562,15 +1562,6 @@ static inline int jbd2_journal_has_csum_v2or3(journal_t *journal) return journal->j_chksum_driver != NULL; } -/* - * Return the minimum number of blocks which must be free in the journal - * before a new transaction may be started. Must be called under j_state_lock. - */ -static inline int jbd2_space_needed(journal_t *journal) -{ - return journal->j_max_transaction_buffers; -} - /* * Return number of free blocks in the log. Must be called under j_state_lock. */ -- cgit v1.2.3 From fdc3ef882a5d59c1709a13b5486ae2b1632e12b6 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Tue, 5 Nov 2019 17:44:26 +0100 Subject: jbd2: Reserve space for revoke descriptor blocks Extend functions for starting, extending, and restarting transaction handles to take number of revoke records handle must be able to accommodate. These functions then make sure transaction has enough credits to be able to store resulting revoke descriptor blocks. Also revoke code tracks number of revoke records created by a handle to catch situation where some place didn't reserve enough space for revoke records. Similarly to standard transaction credits, space for unused reserved revoke records is released when the handle is stopped. On the ext4 side we currently take a simplistic approach of reserving space for 1024 revoke records for any transaction. This grows amount of credits reserved for each handle only by a few and is enough for any normal workload so that we don't hit warnings in jbd2. We will refine the logic in following commits. Signed-off-by: Jan Kara Link: https://lore.kernel.org/r/20191105164437.32602-20-jack@suse.cz Signed-off-by: Theodore Ts'o --- include/linux/jbd2.h | 43 +++++++++++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 10 deletions(-) (limited to 'include') diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index 1dd2703a8e26..2a3d5f50e7a1 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -478,6 +478,7 @@ struct jbd2_revoke_table_s; * @h_journal: Which journal handle belongs to - used iff h_reserved set. * @h_rsv_handle: Handle reserved for finishing the logical operation. * @h_buffer_credits: Number of remaining buffers we are allowed to dirty. + * @h_revoke_credits: Number of remaining revoke records available for handle * @h_ref: Reference count on this handle. * @h_err: Field for caller's use to track errors through large fs operations. * @h_sync: Flag for sync-on-close. @@ -488,6 +489,7 @@ struct jbd2_revoke_table_s; * @h_line_no: For handle statistics. * @h_start_jiffies: Handle Start time. * @h_requested_credits: Holds @h_buffer_credits after handle is started. + * @h_revoke_credits_requested: Holds @h_revoke_credits after handle is started. * @saved_alloc_context: Saved context while transaction is open. **/ @@ -505,6 +507,8 @@ struct jbd2_journal_handle handle_t *h_rsv_handle; int h_buffer_credits; + int h_revoke_credits; + int h_revoke_credits_requested; int h_ref; int h_err; @@ -688,6 +692,17 @@ struct transaction_s */ atomic_t t_outstanding_credits; + /* + * Number of revoke records for this transaction added by already + * stopped handles. [none] + */ + atomic_t t_outstanding_revokes; + + /* + * How many handles used this transaction? [none] + */ + atomic_t t_handle_count; + /* * Forward and backward links for the circular list of all transactions * awaiting checkpoint. [j_list_lock] @@ -705,11 +720,6 @@ struct transaction_s */ ktime_t t_start_time; - /* - * How many handles used this transaction? [none] - */ - atomic_t t_handle_count; - /* * This transaction is being forced and some process is * waiting for it to finish. @@ -1026,6 +1036,13 @@ struct journal_s */ int j_max_transaction_buffers; + /** + * @j_revoke_records_per_block: + * + * Number of revoke records that fit in one descriptor block. + */ + int j_revoke_records_per_block; + /** * @j_commit_interval: * @@ -1360,14 +1377,16 @@ static inline handle_t *journal_current_handle(void) extern handle_t *jbd2_journal_start(journal_t *, int nblocks); extern handle_t *jbd2__journal_start(journal_t *, int blocks, int rsv_blocks, - gfp_t gfp_mask, unsigned int type, - unsigned int line_no); + int revoke_records, gfp_t gfp_mask, + unsigned int type, unsigned int line_no); extern int jbd2_journal_restart(handle_t *, int nblocks); -extern int jbd2__journal_restart(handle_t *, int nblocks, gfp_t gfp_mask); +extern int jbd2__journal_restart(handle_t *, int nblocks, + int revoke_records, gfp_t gfp_mask); extern int jbd2_journal_start_reserved(handle_t *handle, unsigned int type, unsigned int line_no); extern void jbd2_journal_free_reserved(handle_t *handle); -extern int jbd2_journal_extend (handle_t *, int nblocks); +extern int jbd2_journal_extend(handle_t *handle, int nblocks, + int revoke_records); extern int jbd2_journal_get_write_access(handle_t *, struct buffer_head *); extern int jbd2_journal_get_create_access (handle_t *, struct buffer_head *); extern int jbd2_journal_get_undo_access(handle_t *, struct buffer_head *); @@ -1631,7 +1650,11 @@ static inline tid_t jbd2_get_latest_transaction(journal_t *journal) static inline int jbd2_handle_buffer_credits(handle_t *handle) { - return handle->h_buffer_credits; + journal_t *journal = handle->h_transaction->t_journal; + + return handle->h_buffer_credits - + DIV_ROUND_UP(handle->h_revoke_credits_requested, + journal->j_revoke_records_per_block); } #ifdef __KERNEL__ -- cgit v1.2.3 From 933f1c1e0b75bbc29730eef07c9e196c6dfd37e5 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Tue, 5 Nov 2019 17:44:27 +0100 Subject: jbd2: Rename h_buffer_credits to h_total_credits The credit counter now contains both buffer and revoke descriptor block credits. Rename to counter to h_total_credits to reflect that. No functional change. Reviewed-by: Theodore Ts'o Signed-off-by: Jan Kara Link: https://lore.kernel.org/r/20191105164437.32602-21-jack@suse.cz Signed-off-by: Theodore Ts'o --- include/linux/jbd2.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'include') diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index 2a3d5f50e7a1..3115eeb44039 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -477,7 +477,8 @@ struct jbd2_revoke_table_s; * @h_transaction: Which compound transaction is this update a part of? * @h_journal: Which journal handle belongs to - used iff h_reserved set. * @h_rsv_handle: Handle reserved for finishing the logical operation. - * @h_buffer_credits: Number of remaining buffers we are allowed to dirty. + * @h_total_credits: Number of remaining buffers we are allowed to add to + journal. These are dirty buffers and revoke descriptor blocks. * @h_revoke_credits: Number of remaining revoke records available for handle * @h_ref: Reference count on this handle. * @h_err: Field for caller's use to track errors through large fs operations. @@ -488,7 +489,7 @@ struct jbd2_revoke_table_s; * @h_type: For handle statistics. * @h_line_no: For handle statistics. * @h_start_jiffies: Handle Start time. - * @h_requested_credits: Holds @h_buffer_credits after handle is started. + * @h_requested_credits: Holds @h_total_credits after handle is started. * @h_revoke_credits_requested: Holds @h_revoke_credits after handle is started. * @saved_alloc_context: Saved context while transaction is open. **/ @@ -506,7 +507,7 @@ struct jbd2_journal_handle }; handle_t *h_rsv_handle; - int h_buffer_credits; + int h_total_credits; int h_revoke_credits; int h_revoke_credits_requested; int h_ref; @@ -1652,7 +1653,7 @@ static inline int jbd2_handle_buffer_credits(handle_t *handle) { journal_t *journal = handle->h_transaction->t_journal; - return handle->h_buffer_credits - + return handle->h_total_credits - DIV_ROUND_UP(handle->h_revoke_credits_requested, journal->j_revoke_records_per_block); } -- cgit v1.2.3 From 83448bdfb59731c2f54784ed3f4a93ff95be6e7e Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Tue, 5 Nov 2019 17:44:29 +0100 Subject: ext4: Reserve revoke credits for freed blocks So far we have reserved only relatively high fixed amount of revoke credits for each transaction. We over-reserved by large amount for most cases but when freeing large directories or files with data journalling, the fixed amount is not enough. In fact the worst case estimate is inconveniently large (maximum extent size) for freeing of one extent. We fix this by doing proper estimate of the amount of blocks that need to be revoked when removing blocks from the inode due to truncate or hole punching and otherwise reserve just a small amount of revoke credits for each transaction to accommodate freeing of xattrs block or so. Signed-off-by: Jan Kara Link: https://lore.kernel.org/r/20191105164437.32602-23-jack@suse.cz Signed-off-by: Theodore Ts'o --- include/trace/events/ext4.h | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) (limited to 'include') diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h index d68e9e536814..182c9fe9c0e9 100644 --- a/include/trace/events/ext4.h +++ b/include/trace/events/ext4.h @@ -1746,15 +1746,16 @@ TRACE_EVENT(ext4_load_inode, TRACE_EVENT(ext4_journal_start, TP_PROTO(struct super_block *sb, int blocks, int rsv_blocks, - unsigned long IP), + int revoke_creds, unsigned long IP), - TP_ARGS(sb, blocks, rsv_blocks, IP), + TP_ARGS(sb, blocks, rsv_blocks, revoke_creds, IP), TP_STRUCT__entry( __field( dev_t, dev ) __field(unsigned long, ip ) __field( int, blocks ) __field( int, rsv_blocks ) + __field( int, revoke_creds ) ), TP_fast_assign( @@ -1762,11 +1763,13 @@ TRACE_EVENT(ext4_journal_start, __entry->ip = IP; __entry->blocks = blocks; __entry->rsv_blocks = rsv_blocks; + __entry->revoke_creds = revoke_creds; ), - TP_printk("dev %d,%d blocks, %d rsv_blocks, %d caller %pS", - MAJOR(__entry->dev), MINOR(__entry->dev), - __entry->blocks, __entry->rsv_blocks, (void *)__entry->ip) + TP_printk("dev %d,%d blocks %d, rsv_blocks %d, revoke_creds %d, " + "caller %pS", MAJOR(__entry->dev), MINOR(__entry->dev), + __entry->blocks, __entry->rsv_blocks, __entry->revoke_creds, + (void *)__entry->ip) ); TRACE_EVENT(ext4_journal_start_reserved, -- cgit v1.2.3 From 0094f981bbaca3ae707c95c5e5977429d29c2dd0 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Tue, 5 Nov 2019 17:44:30 +0100 Subject: jbd2: Provide trace event for handle restarts Provide trace event for handle restarts to ease debugging. Reviewed-by: Theodore Ts'o Signed-off-by: Jan Kara Link: https://lore.kernel.org/r/20191105164437.32602-24-jack@suse.cz Signed-off-by: Theodore Ts'o --- include/trace/events/jbd2.h | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) (limited to 'include') diff --git a/include/trace/events/jbd2.h b/include/trace/events/jbd2.h index 2310b259329f..d16a32867f3a 100644 --- a/include/trace/events/jbd2.h +++ b/include/trace/events/jbd2.h @@ -133,7 +133,7 @@ TRACE_EVENT(jbd2_submit_inode_data, (unsigned long) __entry->ino) ); -TRACE_EVENT(jbd2_handle_start, +DECLARE_EVENT_CLASS(jbd2_handle_start_class, TP_PROTO(dev_t dev, unsigned long tid, unsigned int type, unsigned int line_no, int requested_blocks), @@ -161,6 +161,20 @@ TRACE_EVENT(jbd2_handle_start, __entry->type, __entry->line_no, __entry->requested_blocks) ); +DEFINE_EVENT(jbd2_handle_start_class, jbd2_handle_start, + TP_PROTO(dev_t dev, unsigned long tid, unsigned int type, + unsigned int line_no, int requested_blocks), + + TP_ARGS(dev, tid, type, line_no, requested_blocks) +); + +DEFINE_EVENT(jbd2_handle_start_class, jbd2_handle_restart, + TP_PROTO(dev_t dev, unsigned long tid, unsigned int type, + unsigned int line_no, int requested_blocks), + + TP_ARGS(dev, tid, type, line_no, requested_blocks) +); + TRACE_EVENT(jbd2_handle_extend, TP_PROTO(dev_t dev, unsigned long tid, unsigned int type, unsigned int line_no, int buffer_credits, -- cgit v1.2.3