ext4/jbd2: inline_data fixes and minor cleanups
Subsystems (3)
v2 cover ext4/jbd2: inline_data fixes and minor cleanups
Hello,
Please find v2 of the inline_data fixes and some minor cleanups found during code review.
I have dropped patch-6 in v2 which was removing use of t_handle_lock (spinlock) from within jbd2_journal_wait_updates(). Based on Jan comments, I feel we can push that as killing of t_handle_lock into a separate series (which will be on top of this).
v1 -> v2
========
1. Added Jan's Reviewed-by tag & addressed one of his comment on no need to make jbd2_journal_wait_updates() function inline.
2. Dropped patch-6 as described above.
Description
============
Patch[1]: fixes BUG_ON with inline_data which was reported [1] with generic/475.
Patch[2]: is mostly cleanup found during code review of inline_data code.
Patch[3]: is a possible memory corruption fix in case of krealloc failure.
Patch[4-5]: Cleanups.
[v1]: https://lore.kernel.org/linux-ext4/[email protected]/T/
Ritesh Harjani (5):
ext4: Fix error handling in ext4_restore_inline_data()
ext4: Remove redundant max inline_size check in ext4_da_write_inline_data_begin()
ext4: Fix error handling in ext4_fc_record_modified_inode()
jbd2: Cleanup unused functions declarations from jbd2.h
jbd2: Refactor wait logic for transaction updates into a common function
fs/ext4/fast_commit.c | 64 ++++++++++++++++++++----------------------- fs/ext4/inline.c | 23 +++++++++------- fs/jbd2/commit.c | 19 ++----------- fs/jbd2/transaction.c | 53 +++++++++++++++++++++-------------- include/linux/jbd2.h | 11 ++------ 5 files changed, 80 insertions(+), 90 deletions(-)
--
2.31.1
PATCH v2 1/5 ext4: Fix error handling in ext4_restore_inline_data()
While running "./check -I 200 generic/475" it sometimes gives below kernel BUG(). Ideally we should not call ext4_write_inline_data() if ext4_create_inline_data() has failed.
<log snip>
[73131.453234] kernel BUG at fs/ext4/inline.c:223!
<code snip>
212 static void ext4_write_inline_data(struct inode *inode, struct ext4_iloc *iloc,
213 void *buffer, loff_t pos, unsigned int len)
214 {
<...>
223 BUG_ON(!EXT4_I(inode)->i_inline_off);
224 BUG_ON(pos + len > EXT4_I(inode)->i_inline_size);
This patch handles the error and prints out a emergency msg saying potential data loss for the given inode (since we couldn't restore the original inline_data due to some previous error).
[ 9571.070313] EXT4-fs (dm-0): error restoring inline_data for inode -- potential data loss! (inode 1703982, error -30)
--- fs/ext4/inline.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
fs/ext4/inline.c +9 -1
@@ -1133,7 +1133,15 @@ static void ext4_restore_inline_data(handle_t *handle, struct inode *inode,struct ext4_iloc *iloc, void *buf, int inline_size) { -ext4_create_inline_data (handle ,inode ,inline_size ); + int ret; + + ret =ext4_create_inline_data (handle, inode, inline_size); +if (ret) { +ext4_msg (inode->i_sb,KERN_EMERG , +"error restoring inline_data for inode -- potential data loss! (inode %lu, error %d)" , + inode->i_ino, ret); +return ; + }ext4_write_inline_data (inode, iloc, buf,0 , inline_size);ext4_set_inode_state (inode,EXT4_STATE_MAY_INLINE_DATA ); }
Signature · 1 line
2.31.1
PATCH v2 2/5 ext4: Remove redundant max inline_size check in ext4_da_write_inline_data_begin()
In reply to Ritesh Harjaniext4_prepare_inline_data() already checks for ext4_get_max_inline_size() and returns -ENOSPC. So there is no need to check it twice within ext4_da_write_inline_data_begin(). This patch removes the extra check.
It also makes it more clean.
No functionality change in this patch.
--- fs/ext4/inline.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
fs/ext4/inline.c +4 -9
@@ -911,7 +911,7 @@ int ext4_da_write_inline_data_begin(struct address_space *mapping,struct page **pagep, void **fsdata) { - int ret, inline_size; + int ret;handle_t *handle;struct page *page;struct ext4_iloc iloc;
@@ -928,14 +928,9 @@ int ext4_da_write_inline_data_begin(struct address_space *mapping, goto out; } - inline_size =ext4_get_max_inline_size (inode); - - ret = -ENOSPC ; -if (inline_size >= pos + len) { - ret =ext4_prepare_inline_data (handle, inode, pos + len); -if (ret && ret != -ENOSPC ) - goto out_journal; - } + ret =ext4_prepare_inline_data (handle, inode, pos + len); +if (ret && ret != -ENOSPC ) + goto out_journal;/* * We cannot recurse into the filesystem as the transaction
Signature · 1 line
2.31.1
PATCH v2 3/5 ext4: Fix error handling in ext4_fc_record_modified_inode()
In reply to Ritesh HarjaniCurrent code does not fully takes care of krealloc() error case, which could lead to silent memory corruption or a kernel bug. This patch fixes that.
Also it cleans up some duplicated error handling logic from various functions in fast_commit.c file.
--- fs/ext4/fast_commit.c | 64 ++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 35 deletions(-)
fs/ext4/fast_commit.c +29 -35
@@ -1392,14 +1392,15 @@ static int ext4_fc_record_modified_inode(struct super_block *sb, int ino)if (state->fc_modified_inodes[i] == ino)return 0 ;if (state->fc_modified_inodes_used == state->fc_modified_inodes_size) { - state->fc_modified_inodes_size += -EXT4_FC_REPLAY_REALLOC_INCREMENT ; state->fc_modified_inodes =krealloc ( - state->fc_modified_inodes,sizeof (int ) * - state->fc_modified_inodes_size, -GFP_KERNEL ); + state->fc_modified_inodes, +sizeof (int ) * (state->fc_modified_inodes_size + +EXT4_FC_REPLAY_REALLOC_INCREMENT ), +GFP_KERNEL );if (!state->fc_modified_inodes)return -ENOMEM ; + state->fc_modified_inodes_size += +EXT4_FC_REPLAY_REALLOC_INCREMENT ; } state->fc_modified_inodes[state->fc_modified_inodes_used++] = ino;return 0 ;
@@ -1431,7 +1432,9 @@ static int ext4_fc_replay_inode(struct super_block *sb, struct ext4_fc_tl *tl, } inode =NULL ; -ext4_fc_record_modified_inode (sb, ino); + ret =ext4_fc_record_modified_inode (sb, ino); +if (ret) + goto out; raw_fc_inode = (struct ext4_inode *) (val + offsetof(struct ext4_fc_inode , fc_raw_inode));
@@ -1621,6 +1624,8 @@ static int ext4_fc_replay_add_range(struct super_block *sb, } ret =ext4_fc_record_modified_inode (sb, inode->i_ino); +if (ret) + goto out; start =le32_to_cpu (ex->ee_block); start_pblk =ext4_ext_pblock (ex);
@@ -1638,18 +1643,14 @@ static int ext4_fc_replay_add_range(struct super_block *sb, map.m_pblk =0 ; ret =ext4_map_blocks (NULL , inode, &map,0 ); -if (ret <0 ) { -iput (inode); -return 0 ; - } +if (ret <0 ) + goto out;if (ret ==0 ) {/* Range is not mapped */ path =ext4_find_extent (inode, cur,NULL ,0 ); -if (IS_ERR (path)) { -iput (inode); -return 0 ; - } +if (IS_ERR (path)) + goto out;memset (&newex,0 ,sizeof (newex)); newex.ee_block =cpu_to_le32 (cur); ext4_ext_store_pblock(
@@ -1663,10 +1664,8 @@ static int ext4_fc_replay_add_range(struct super_block *sb,up_write ((&EXT4_I (inode)->i_data_sem));ext4_ext_drop_refs (path);kfree (path); -if (ret) { -iput (inode); -return 0 ; - } +if (ret) + goto out; goto next; }
@@ -1679,10 +1678,8 @@ static int ext4_fc_replay_add_range(struct super_block *sb, ret =ext4_ext_replay_update_ex (inode, cur, map.m_len,ext4_ext_is_unwritten (ex), start_pblk + cur - start); -if (ret) { -iput (inode); -return 0 ; - } +if (ret) + goto out;/* * Mark the old blocks as free since they aren't used * anymore. We maintain an array of all the modified
@@ -1702,10 +1699,8 @@ static int ext4_fc_replay_add_range(struct super_block *sb, ext4_ext_is_unwritten(ex), map.m_pblk); ret =ext4_ext_replay_update_ex (inode, cur, map.m_len,ext4_ext_is_unwritten (ex), map.m_pblk); -if (ret) { -iput (inode); -return 0 ; - } +if (ret) + goto out;/* * We may have split the extent tree while toggling the state. * Try to shrink the extent tree now.
@@ -1717,6 +1712,7 @@ static int ext4_fc_replay_add_range(struct super_block *sb, }ext4_ext_replay_shrink_inode (inode,i_size_read (inode) >> sb->s_blocksize_bits); +out:iput (inode);return 0 ; }
@@ -1746,6 +1742,8 @@ ext4_fc_replay_del_range(struct super_block *sb, struct ext4_fc_tl *tl, } ret =ext4_fc_record_modified_inode (sb, inode->i_ino); +if (ret) + goto out; jbd_debug(1, "DEL_RANGE, inode %ld, lblk %d, len %d\n", inode->i_ino, le32_to_cpu(lrange.fc_lblk),
@@ -1755,10 +1753,8 @@ ext4_fc_replay_del_range(struct super_block *sb, struct ext4_fc_tl *tl, map.m_len = remaining; ret =ext4_map_blocks (NULL , inode, &map,0 ); -if (ret <0 ) { -iput (inode); -return 0 ; - } +if (ret <0 ) + goto out;if (ret >0 ) { remaining -= ret; cur += ret;
@@ -1773,15 +1769,13 @@ ext4_fc_replay_del_range(struct super_block *sb, struct ext4_fc_tl *tl, ret =ext4_ext_remove_space (inode, lrange.fc_lblk, lrange.fc_lblk + lrange.fc_len -1 );up_write (&EXT4_I (inode)->i_data_sem); -if (ret) { -iput (inode); -return 0 ; - } +if (ret) + goto out;ext4_ext_replay_shrink_inode (inode,i_size_read (inode) >> sb->s_blocksize_bits);ext4_mark_inode_dirty (NULL , inode); +out:iput (inode); -return 0 ; }
Signature · 1 line
2.31.1
PATCH v2 4/5 jbd2: Cleanup unused functions declarations from jbd2.h
In reply to Ritesh HarjaniDuring code review found no references of few of these below function declarations. This patch cleans those up from jbd2.h
--- include/linux/jbd2.h | 7 ------- 1 file changed, 7 deletions(-)
include/linux/jbd2.h +0 -7
@@ -1419,9 +1419,7 @@ extern void jbd2_journal_unfile_buffer(journal_t *, struct journal_head *);extern bool __jbd2_journal_refile_buffer (struct journal_head *);extern void jbd2_journal_refile_buffer (journal_t *,struct journal_head *);extern void __jbd2_journal_file_buffer (struct journal_head *,transaction_t *,int ); -extern void __journal_free_buffer (struct journal_head *bh);extern void jbd2_journal_file_buffer (struct journal_head *,transaction_t *,int ); -extern void __journal_clean_data_list (transaction_t *transaction);static inline void jbd2_file_log_bh (struct list_head *head,struct buffer_head *bh) {list_add_tail (&bh->b_assoc_buffers, head);
@@ -1486,9 +1484,6 @@ extern int jbd2_journal_write_metadata_buffer(transaction_t *transaction,struct buffer_head **bh_out, sector_t blocknr); -/* Transaction locking */ -extern void __wait_on_journal (journal_t *); -/* Transaction cache support */ extern void jbd2_journal_destroy_transaction_cache (void );extern int __init jbd2_journal_init_transaction_cache (void );
@@ -1774,8 +1769,6 @@ static inline unsigned long jbd2_log_space_left(journal_t *journal)#define BJ_Reserved 4/* Buffer is reserved for access by journal */ #define BJ_Types 5 -extern int jbd_blocks_per_page (struct inode *inode); -/* JBD uses a CRC32 checksum */ #define JBD_MAX_CHECKSUM_SIZE 4
Signature · 1 line
2.31.1
PATCH v2 5/5 jbd2: Refactor wait logic for transaction updates into a common function
In reply to Ritesh HarjaniNo functionality change as such in this patch. This only refactors the common piece of code which waits for t_updates to finish into a common function named as jbd2_journal_wait_updates(journal_t *)
--- fs/jbd2/commit.c | 19 +++------------- fs/jbd2/transaction.c | 53 ++++++++++++++++++++++++++----------------- include/linux/jbd2.h | 4 +++- 3 files changed, 38 insertions(+), 38 deletions(-)
fs/jbd2/commit.c +3 -16
@@ -484,22 +484,9 @@ void jbd2_journal_commit_transaction(journal_t *journal) stats.run.rs_running =jbd2_time_diff (commit_transaction->t_start, stats.run.rs_locked); -spin_lock (&commit_transaction->t_handle_lock); -while (atomic_read (&commit_transaction->t_updates)) { -DEFINE_WAIT (wait); +// waits for any t_updates to finish +jbd2_journal_wait_updates (journal); -prepare_to_wait (&journal->j_wait_updates, &wait, -TASK_UNINTERRUPTIBLE ); -if (atomic_read (&commit_transaction->t_updates)) { -spin_unlock (&commit_transaction->t_handle_lock); -write_unlock (&journal->j_state_lock); -schedule (); -write_lock (&journal->j_state_lock); -spin_lock (&commit_transaction->t_handle_lock); - } -finish_wait (&journal->j_wait_updates, &wait); - } -spin_unlock (&commit_transaction->t_handle_lock); commit_transaction->t_state =T_SWITCH ;write_unlock (&journal->j_state_lock);
@@ -817,7 +804,7 @@ void jbd2_journal_commit_transaction(journal_t *journal) commit_transaction->t_state =T_COMMIT_DFLUSH ;write_unlock (&journal->j_state_lock); -/* +/* * If the journal is not located on the file system device, * then we must flush the file system device before we issue * the commit record
fs/jbd2/transaction.c +32 -21
72 lines not on this page · raw message
include/linux/jbd2.h +3 -1
@@ -594,7 +594,7 @@ struct transaction_s*/ unsigned long t_log_start; -/* +/* * Number of buffers on the t_buffers list [j_list_lock, no locks * needed for jbd2 thread] */
@@ -1538,6 +1538,8 @@ extern int jbd2_journal_flush(journal_t *journal, unsigned int flags);extern void jbd2_journal_lock_updates (journal_t *);extern void jbd2_journal_unlock_updates (journal_t *); +void jbd2_journal_wait_updates (journal_t *); +extern journal_t *jbd2_journal_init_dev (struct block_device *bdev,struct block_device *fs_dev,unsigned long long start,int len,int bsize);
Signature · 1 line
2.31.1
Quoted 9 lines · Ritesh Harjani
Please find v2 of the
inline_datafixes and some minor cleanups found during code review.I have dropped patch-6 in v2 which was removing use of
t_handle_lock(spinlock) from withinjbd2_journal_wait_updates(). Based on Jan comments, I feel we can push that as killing oft_handle_lockinto a separate series (which will be on top of this).[...]
Applied, thanks!
[1/5] ext4: Fix error handling in ext4_restore_inline_data()
commit: 2fdd85005f708691a64270ecb67d98191d668c4c
[2/5] ext4: Remove redundant max inline_size check in ext4_da_write_inline_data_begin()
commit: c7fc77e512a432bba754f969c4eb72b33cda3431
[3/5] ext4: Fix error handling in ext4_fc_record_modified_inode()
commit: 6dcee78ea266fb736a3357c2e04d81ee7ec7b6e4
[4/5] jbd2: Cleanup unused functions declarations from jbd2.h
commit: 16263b9820b0d40c778c8ee867f853d3fe638f37
[5/5] jbd2: Refactor wait logic for transaction updates into a common function
commit: b0544c1f23ddeabd89480d842867ca1c6894e021Best regards,
Signature · 1 line
Theodore Ts'o <[email protected]>