From 57802c73bf1ba7adf17f3d39b68bab7f4d9a5635 Mon Sep 17 00:00:00 2001 From: Li zeming Date: Tue, 2 Apr 2024 10:23:00 +0800 Subject: ext4: block_validity: Remove unnecessary ‘NULL’ values from new_node MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit new_node is assigned first, so it does not need to initialize the assignment. Signed-off-by: Li zeming Reviewed-by: Andreas Dilger Link: https://patch.msgid.link/20240402022300.25858-1-zeming@nfschina.com Signed-off-by: Theodore Ts'o --- fs/ext4/block_validity.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c index 6fe3c941b565..87ee3a17bd29 100644 --- a/fs/ext4/block_validity.c +++ b/fs/ext4/block_validity.c @@ -72,7 +72,7 @@ static int add_system_zone(struct ext4_system_blocks *system_blks, { struct ext4_system_zone *new_entry, *entry; struct rb_node **n = &system_blks->root.rb_node, *node; - struct rb_node *parent = NULL, *new_node = NULL; + struct rb_node *parent = NULL, *new_node; while (*n) { parent = *n; -- cgit From be210737fe6cef2d0d578e23342261688c9317e1 Mon Sep 17 00:00:00 2001 From: Thorsten Blum Date: Tue, 2 Apr 2024 12:51:58 +0200 Subject: jbd2: use str_plural() to fix Coccinelle warning Fixes the following Coccinelle/coccicheck warning reported by string_choices.cocci: opportunity for str_plural(dropped) Signed-off-by: Thorsten Blum Link: https://patch.msgid.link/20240402105157.254389-2-thorsten.blum@toblux.com Signed-off-by: Theodore Ts'o --- fs/jbd2/recovery.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c index 1f7664984d6e..af930c3d0d97 100644 --- a/fs/jbd2/recovery.c +++ b/fs/jbd2/recovery.c @@ -19,6 +19,7 @@ #include #include #include +#include #endif /* @@ -374,7 +375,7 @@ int jbd2_journal_skip_recovery(journal_t *journal) be32_to_cpu(journal->j_superblock->s_sequence); jbd2_debug(1, "JBD2: ignoring %d transaction%s from the journal.\n", - dropped, (dropped == 1) ? "" : "s"); + dropped, str_plural(dropped)); #endif journal->j_transaction_sequence = ++info.end_transaction; journal->j_head = info.head_block; -- cgit From 8dc9c3da79c84b13fdb135e2fb0a149a8175bffe Mon Sep 17 00:00:00 2001 From: Xiaxi Shen Date: Tue, 30 Apr 2024 20:30:17 -0700 Subject: ext4: fix uninitialized variable in ext4_inlinedir_to_tree Syzbot has found an uninit-value bug in ext4_inlinedir_to_tree This error happens because ext4_inlinedir_to_tree does not handle the case when ext4fs_dirhash returns an error This can be avoided by checking the return value of ext4fs_dirhash and propagating the error, similar to how it's done with ext4_htree_store_dirent Signed-off-by: Xiaxi Shen Reported-and-tested-by: syzbot+eaba5abe296837a640c0@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=eaba5abe296837a640c0 Link: https://patch.msgid.link/20240501033017.220000-1-shenxiaxi26@gmail.com Signed-off-by: Theodore Ts'o --- fs/ext4/inline.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c index d5bd1e3a5d36..e7a09a99837b 100644 --- a/fs/ext4/inline.c +++ b/fs/ext4/inline.c @@ -1410,7 +1410,11 @@ int ext4_inlinedir_to_tree(struct file *dir_file, hinfo->hash = EXT4_DIRENT_HASH(de); hinfo->minor_hash = EXT4_DIRENT_MINOR_HASH(de); } else { - ext4fs_dirhash(dir, de->name, de->name_len, hinfo); + err = ext4fs_dirhash(dir, de->name, de->name_len, hinfo); + if (err) { + ret = err; + goto out; + } } if ((hinfo->hash < start_hash) || ((hinfo->hash == start_hash) && -- cgit From cc102aa24638b90e04364d64e4f58a1fa91a1976 Mon Sep 17 00:00:00 2001 From: Kemeng Shi Date: Tue, 14 May 2024 19:24:30 +0800 Subject: jbd2: avoid memleak in jbd2_journal_write_metadata_buffer 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 Reviewed-by: Zhang Yi Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240514112438.1269037-2-shikemeng@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/jbd2/journal.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index 03c4b9214f56..5ad255ffb289 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -399,6 +399,7 @@ repeat: 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); -- cgit From abe48a52250a91f0d1f9b5052a246c63cb845827 Mon Sep 17 00:00:00 2001 From: Kemeng Shi Date: Tue, 14 May 2024 19:24:31 +0800 Subject: jbd2: remove unused return info from jbd2_journal_write_metadata_buffer The done_copy_out info from jbd2_journal_write_metadata_buffer is not used. Simply remove it. Signed-off-by: Kemeng Shi Reviewed-by: Zhang Yi Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240514112438.1269037-3-shikemeng@huaweicloud.com Signed-off-by: Theodore Ts'o --- 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 75ea4e9a5cab..6d3fc992287d 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; @@ -660,10 +660,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]); @@ -672,7 +672,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 5ad255ffb289..667272239889 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 @@ repeat: set_buffer_shadow(bh_in); spin_unlock(&jh_in->b_state_lock); - return do_escape | (done_copy_out << 1); + return do_escape; } /* -- cgit From 5dd3e8c0758a7c7a8c8f3002b2632a3c9d31c808 Mon Sep 17 00:00:00 2001 From: Kemeng Shi Date: Tue, 14 May 2024 19:24:32 +0800 Subject: jbd2: remove unnedded "need_copy_out" in jbd2_journal_write_metadata_buffer 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 Reviewed-by: Zhang Yi Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240514112438.1269037-4-shikemeng@huaweicloud.com Signed-off-by: Theodore Ts'o --- 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 667272239889..3a7695bfb76f 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 @@ repeat: /* * 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); -- cgit From 4c15129aaad54af2df4665ddde9245788ee9fa9c Mon Sep 17 00:00:00 2001 From: Kemeng Shi Date: Tue, 14 May 2024 19:24:33 +0800 Subject: jbd2: jump to new copy_done tag when b_frozen_data is created concurrently 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 Reviewed-by: Zhang Yi Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240514112438.1269037-5-shikemeng@huaweicloud.com Signed-off-by: Theodore Ts'o --- 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 3a7695bfb76f..739802000739 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 @@ repeat: 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; } /* -- cgit From daabedd664021afa9e7e3225489888a4fee99bad Mon Sep 17 00:00:00 2001 From: Kemeng Shi Date: Tue, 14 May 2024 19:24:34 +0800 Subject: jbd2: remove unneeded kmap to do escape in jbd2_journal_write_metadata_buffer The data to do escape could be accessed directly from b_frozen_data, just remove unneeded kmap. Signed-off-by: Kemeng Shi Reviewed-by: Zhang Yi Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240514112438.1269037-6-shikemeng@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/jbd2/journal.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index 739802000739..d69f8e0a12e0 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -419,12 +419,11 @@ copy_done: /* * Did we need to do an escaping? Now we've done all the * copying, we can finally do so. + * b_frozen_data is from jbd2_alloc() which always provides an + * address from the direct kernels mapping. */ - if (do_escape) { - mapped_data = kmap_local_folio(new_folio, new_offset); - *((unsigned int *)mapped_data) = 0; - kunmap_local(mapped_data); - } + if (do_escape) + *((unsigned int *)jh_in->b_frozen_data) = 0; folio_set_bh(new_bh, new_folio, new_offset); new_bh->b_size = bh_in->b_size; -- cgit From 4eb9bd13eba3c168e91460972a76ddd53ac3b2b0 Mon Sep 17 00:00:00 2001 From: Kemeng Shi Date: Tue, 14 May 2024 19:24:35 +0800 Subject: jbd2: use bh_in instead of jh2bh(jh_in) to simplify code 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 Reviewed-by: Zhang Yi Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240514112438.1269037-7-shikemeng@huaweicloud.com Signed-off-by: Theodore Ts'o --- 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 d69f8e0a12e0..1ea96ab9374a 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); -- cgit From d5c545735aa0d6443460ac407d35f8fa235d5102 Mon Sep 17 00:00:00 2001 From: Kemeng Shi Date: Tue, 14 May 2024 19:24:36 +0800 Subject: jbd2: remove dead equality check of j_commit_[sequence/request] in kjournald2 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 Reviewed-by: Zhang Yi Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240514112438.1269037-8-shikemeng@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/jbd2/journal.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index 1ea96ab9374a..079b3eeaa4d5 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -224,8 +224,6 @@ loop: 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)) -- cgit From 136d3e0703e807b14ed6f9b8a5a544b6ba08d940 Mon Sep 17 00:00:00 2001 From: Kemeng Shi Date: Tue, 14 May 2024 19:24:37 +0800 Subject: jbd2: remove dead check of JBD2_UNMOUNT in kjournald2 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 Reviewed-by: Zhang Yi Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240514112438.1269037-9-shikemeng@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/jbd2/journal.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index 079b3eeaa4d5..c7869b187380 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -228,8 +228,6 @@ loop: 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(); -- cgit From b07855348b305c234d9fabb7ab9b50fa9b3a7759 Mon Sep 17 00:00:00 2001 From: Kemeng Shi Date: Tue, 14 May 2024 19:24:38 +0800 Subject: jbd2: remove unnecessary "should_sleep" in kjournald2 We only need to sleep if no running transaction is expired. Simply remove unnecessary "should_sleep". Signed-off-by: Kemeng Shi Reviewed-by: Zhang Yi Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240514112438.1269037-10-shikemeng@huaweicloud.com Signed-off-by: Theodore Ts'o --- 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 c7869b187380..8d782a88aee1 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -220,15 +220,12 @@ loop: * 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); -- cgit From 907c3fe532253a6ef4eb9c4d67efb71fab58c706 Mon Sep 17 00:00:00 2001 From: "Luis Henriques (SUSE)" Date: Wed, 15 May 2024 09:28:57 +0100 Subject: ext4: fix infinite loop when replaying fast_commit When doing fast_commit replay an infinite loop may occur due to an uninitialized extent_status struct. ext4_ext_determine_insert_hole() does not detect the replay and calls ext4_es_find_extent_range(), which will return immediately without initializing the 'es' variable. Because 'es' contains garbage, an integer overflow may happen causing an infinite loop in this function, easily reproducible using fstest generic/039. This commit fixes this issue by unconditionally initializing the structure in function ext4_es_find_extent_range(). Thanks to Zhang Yi, for figuring out the real problem! Fixes: 8016e29f4362 ("ext4: fast commit recovery path") Signed-off-by: Luis Henriques (SUSE) Reviewed-by: Zhang Yi Link: https://patch.msgid.link/20240515082857.32730-1-luis.henriques@linux.dev Signed-off-by: Theodore Ts'o --- fs/ext4/extents_status.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c index 4a00e2f019d9..3a53dbb85e15 100644 --- a/fs/ext4/extents_status.c +++ b/fs/ext4/extents_status.c @@ -310,6 +310,8 @@ void ext4_es_find_extent_range(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t end, struct extent_status *es) { + es->es_lblk = es->es_len = es->es_pblk = 0; + if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY) return; -- cgit From 8e4e5cdf2fdeb99445a468b6b6436ad79b9ecb30 Mon Sep 17 00:00:00 2001 From: Zhang Yi Date: Fri, 17 May 2024 20:39:56 +0800 Subject: ext4: factor out a common helper to query extent map Factor out a new common helper ext4_map_query_blocks() from the ext4_da_map_blocks(), it query and return the extent map status on the inode's extent path, no logic changes. Signed-off-by: Zhang Yi Reviewed-by: Jan Kara Reviewed-by: Ritesh Harjani (IBM) Link: https://patch.msgid.link/20240517124005.347221-2-yi.zhang@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/ext4/inode.c | 57 ++++++++++++++++++++++++++++++++------------------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 4bae9ccf5fe0..168819b4db01 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -453,6 +453,35 @@ static void ext4_map_blocks_es_recheck(handle_t *handle, } #endif /* ES_AGGRESSIVE_TEST */ +static int ext4_map_query_blocks(handle_t *handle, struct inode *inode, + struct ext4_map_blocks *map) +{ + unsigned int status; + int retval; + + if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) + retval = ext4_ext_map_blocks(handle, inode, map, 0); + else + retval = ext4_ind_map_blocks(handle, inode, map, 0); + + if (retval <= 0) + return retval; + + if (unlikely(retval != map->m_len)) { + ext4_warning(inode->i_sb, + "ES len assertion failed for inode " + "%lu: retval %d != map->m_len %d", + inode->i_ino, retval, map->m_len); + WARN_ON(1); + } + + status = map->m_flags & EXT4_MAP_UNWRITTEN ? + EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN; + ext4_es_insert_extent(inode, map->m_lblk, map->m_len, + map->m_pblk, status); + return retval; +} + /* * The ext4_map_blocks() function tries to look up the requested blocks, * and returns if the blocks are already mapped. @@ -1744,33 +1773,11 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock, down_read(&EXT4_I(inode)->i_data_sem); if (ext4_has_inline_data(inode)) retval = 0; - else if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) - retval = ext4_ext_map_blocks(NULL, inode, map, 0); else - retval = ext4_ind_map_blocks(NULL, inode, map, 0); - if (retval < 0) { - up_read(&EXT4_I(inode)->i_data_sem); - return retval; - } - if (retval > 0) { - unsigned int status; - - if (unlikely(retval != map->m_len)) { - ext4_warning(inode->i_sb, - "ES len assertion failed for inode " - "%lu: retval %d != map->m_len %d", - inode->i_ino, retval, map->m_len); - WARN_ON(1); - } - - status = map->m_flags & EXT4_MAP_UNWRITTEN ? - EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN; - ext4_es_insert_extent(inode, map->m_lblk, map->m_len, - map->m_pblk, status); - up_read(&EXT4_I(inode)->i_data_sem); - return retval; - } + retval = ext4_map_query_blocks(NULL, inode, map); up_read(&EXT4_I(inode)->i_data_sem); + if (retval) + return retval; add_delayed: down_write(&EXT4_I(inode)->i_data_sem); -- cgit From 0ea6560abb3bac1ffcfa4bf6b2c4d344fdc27b3c Mon Sep 17 00:00:00 2001 From: Zhang Yi Date: Fri, 17 May 2024 20:39:57 +0800 Subject: ext4: check the extent status again before inserting delalloc block ext4_da_map_blocks looks up for any extent entry in the extent status tree (w/o i_data_sem) and then the looks up for any ondisk extent mapping (with i_data_sem in read mode). If it finds a hole in the extent status tree or if it couldn't find any entry at all, it then takes the i_data_sem in write mode to add a da entry into the extent status tree. This can actually race with page mkwrite & fallocate path. Note that this is ok between 1. ext4 buffered-write path v/s ext4_page_mkwrite(), because of the folio lock 2. ext4 buffered write path v/s ext4 fallocate because of the inode lock. But this can race between ext4_page_mkwrite() & ext4 fallocate path ext4_page_mkwrite() ext4_fallocate() block_page_mkwrite() ext4_da_map_blocks() //find hole in extent status tree ext4_alloc_file_blocks() ext4_map_blocks() //allocate block and unwritten extent ext4_insert_delayed_block() ext4_da_reserve_space() //reserve one more block ext4_es_insert_delayed_block() //drop unwritten extent and add delayed extent by mistake Then, the delalloc extent is wrong until writeback and the extra reserved block can't be released any more and it triggers below warning: EXT4-fs (pmem2): Inode 13 (00000000bbbd4d23): i_reserved_data_blocks(1) not cleared! Fix the problem by looking up extent status tree again while the i_data_sem is held in write mode. If it still can't find any entry, then we insert a new da entry into the extent status tree. Cc: stable@vger.kernel.org Signed-off-by: Zhang Yi Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240517124005.347221-3-yi.zhang@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/ext4/inode.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 168819b4db01..4b0d64a76e88 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -1737,6 +1737,7 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock, if (ext4_es_is_hole(&es)) goto add_delayed; +found: /* * Delayed extent could be allocated by fallocate. * So we need to check it. @@ -1781,6 +1782,26 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock, add_delayed: down_write(&EXT4_I(inode)->i_data_sem); + /* + * Page fault path (ext4_page_mkwrite does not take i_rwsem) + * and fallocate path (no folio lock) can race. Make sure we + * lookup the extent status tree here again while i_data_sem + * is held in write mode, before inserting a new da entry in + * the extent status tree. + */ + if (ext4_es_lookup_extent(inode, iblock, NULL, &es)) { + if (!ext4_es_is_hole(&es)) { + up_write(&EXT4_I(inode)->i_data_sem); + goto found; + } + } else if (!ext4_has_inline_data(inode)) { + retval = ext4_map_query_blocks(NULL, inode, map); + if (retval) { + up_write(&EXT4_I(inode)->i_data_sem); + return retval; + } + } + retval = ext4_insert_delayed_block(inode, map->m_lblk); up_write(&EXT4_I(inode)->i_data_sem); if (retval) -- cgit From b37c907073e80016333b442c845c3acc198e570f Mon Sep 17 00:00:00 2001 From: Zhang Yi Date: Fri, 17 May 2024 20:39:58 +0800 Subject: ext4: warn if delalloc counters are not zero on inactive The per-inode i_reserved_data_blocks count the reserved delalloc blocks in a regular file, it should be zero when destroying the file. The per-fs s_dirtyclusters_counter count all reserved delalloc blocks in a filesystem, it also should be zero when umounting the filesystem. Now we have only an error message if the i_reserved_data_blocks is not zero, which is unable to be simply captured, so add WARN_ON_ONCE to make it more visable. Signed-off-by: Zhang Yi Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240517124005.347221-4-yi.zhang@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/ext4/super.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index c682fb927b64..da980ca9e962 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1327,6 +1327,9 @@ static void ext4_put_super(struct super_block *sb) ext4_group_desc_free(sbi); ext4_flex_groups_free(sbi); + + WARN_ON_ONCE(!(sbi->s_mount_state & EXT4_ERROR_FS) && + percpu_counter_sum(&sbi->s_dirtyclusters_counter)); ext4_percpu_param_destroy(sbi); #ifdef CONFIG_QUOTA for (int i = 0; i < EXT4_MAXQUOTAS; i++) @@ -1457,7 +1460,8 @@ static void ext4_destroy_inode(struct inode *inode) dump_stack(); } - if (EXT4_I(inode)->i_reserved_data_blocks) + if (!(EXT4_SB(inode->i_sb)->s_mount_state & EXT4_ERROR_FS) && + WARN_ON_ONCE(EXT4_I(inode)->i_reserved_data_blocks)) ext4_msg(inode->i_sb, KERN_ERR, "Inode %lu (%p): i_reserved_data_blocks (%u) not cleared!", inode->i_ino, EXT4_I(inode), -- cgit From 14a210c110d1ffbef4ed56e88e3c34de04790ff8 Mon Sep 17 00:00:00 2001 From: Zhang Yi Date: Fri, 17 May 2024 20:39:59 +0800 Subject: ext4: trim delalloc extent In ext4_da_map_blocks(), we could find four kind of extents in the extent status tree: hole, unwritten, written and delayed extent. Now we only trim the map len if we found an unwritten extent or a written extent. This is okay now since map->m_len is always set to one and we always insert one delayed block at a time. But this will become isn't okay for other two cases if ext4_insert_delayed_block() and ext4_da_map_blocks() support inserting multiple map->len blocks later. 1. If we found a hole in the extent status tree which es->es_len is shorter than the length we want to write, we should trim the map->m_len to prevent adding extra delay more blocks than we expected. For example, assume we write data [A, C) to a file that contains a hole extent [A, B) and a written extent [B, D) in the cache. A B C D before da write: ...hhhhhh|wwwwww.... Then we will get extent [A, B), we should trim map->m_len to B-A before inserting new delalloc blocks, if not, the range [B, C) will be duplicated. 2. If we found a delayed extent in the extent status tree which es->es_len is shorter than the length we want to write, we should trim the map->m_len to es->es_len and return directly since the front part of this map has been delayed, we can't insert the delalloc extent that contains the latter part in this round, we should return the delayed length and the caller should increase the position and call ext4_da_map_blocks() again. For example, assume we write data [A, C) to a file that contains a delayed extent [A, B) in the cache. A B C before da write: ...dddddd|hhh.... Then we will get delayed extent [A, B), we should also trim map->m_len to B-A and return, if not, we will incorrectly assume that the write is complete and won't insert [B, C). So we need to always trim the map->m_len if the found es->es_len in the extent status tree is shorter than the map->m_len, prearing for inserting a extent with multiple delalloc blocks. This patch only does a pre-fix, the handle is crude and ext4_da_map_blocks() deserve a cleanup, we will do that later. Signed-off-by: Zhang Yi Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240517124005.347221-5-yi.zhang@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/ext4/inode.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 4b0d64a76e88..200bbe89ded2 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -1734,6 +1734,11 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock, /* Lookup extent status tree firstly */ if (ext4_es_lookup_extent(inode, iblock, NULL, &es)) { + retval = es.es_len - (iblock - es.es_lblk); + if (retval > map->m_len) + retval = map->m_len; + map->m_len = retval; + if (ext4_es_is_hole(&es)) goto add_delayed; @@ -1750,10 +1755,6 @@ found: } map->m_pblk = ext4_es_pblock(&es) + iblock - es.es_lblk; - retval = es.es_len - (iblock - es.es_lblk); - if (retval > map->m_len) - retval = map->m_len; - map->m_len = retval; if (ext4_es_is_written(&es)) map->m_flags |= EXT4_MAP_MAPPED; else if (ext4_es_is_unwritten(&es)) @@ -1790,6 +1791,11 @@ add_delayed: * the extent status tree. */ if (ext4_es_lookup_extent(inode, iblock, NULL, &es)) { + retval = es.es_len - (iblock - es.es_lblk); + if (retval > map->m_len) + retval = map->m_len; + map->m_len = retval; + if (!ext4_es_is_hole(&es)) { up_write(&EXT4_I(inode)->i_data_sem); goto found; -- cgit From bb6b18057f18e9b7f53a524721060652de151e8a Mon Sep 17 00:00:00 2001 From: Zhang Yi Date: Fri, 17 May 2024 20:40:00 +0800 Subject: ext4: drop iblock parameter The start block of the delalloc extent to be inserted is equal to map->m_lblk, just drop the duplicate iblock input parameter. Signed-off-by: Zhang Yi Reviewed-by: Jan Kara Reviewed-by: Ritesh Harjani (IBM) Link: https://patch.msgid.link/20240517124005.347221-6-yi.zhang@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/ext4/inode.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 200bbe89ded2..20528a04903a 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -1712,8 +1712,7 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk) * time. This function looks up the requested blocks and sets the * buffer delay bit under the protection of i_data_sem. */ -static int ext4_da_map_blocks(struct inode *inode, sector_t iblock, - struct ext4_map_blocks *map, +static int ext4_da_map_blocks(struct inode *inode, struct ext4_map_blocks *map, struct buffer_head *bh) { struct extent_status es; @@ -1733,8 +1732,8 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock, (unsigned long) map->m_lblk); /* Lookup extent status tree firstly */ - if (ext4_es_lookup_extent(inode, iblock, NULL, &es)) { - retval = es.es_len - (iblock - es.es_lblk); + if (ext4_es_lookup_extent(inode, map->m_lblk, NULL, &es)) { + retval = es.es_len - (map->m_lblk - es.es_lblk); if (retval > map->m_len) retval = map->m_len; map->m_len = retval; @@ -1754,7 +1753,7 @@ found: return 0; } - map->m_pblk = ext4_es_pblock(&es) + iblock - es.es_lblk; + map->m_pblk = ext4_es_pblock(&es) + map->m_lblk - es.es_lblk; if (ext4_es_is_written(&es)) map->m_flags |= EXT4_MAP_MAPPED; else if (ext4_es_is_unwritten(&es)) @@ -1790,8 +1789,8 @@ add_delayed: * is held in write mode, before inserting a new da entry in * the extent status tree. */ - if (ext4_es_lookup_extent(inode, iblock, NULL, &es)) { - retval = es.es_len - (iblock - es.es_lblk); + if (ext4_es_lookup_extent(inode, map->m_lblk, NULL, &es)) { + retval = es.es_len - (map->m_lblk - es.es_lblk); if (retval > map->m_len) retval = map->m_len; map->m_len = retval; @@ -1848,7 +1847,7 @@ int ext4_da_get_block_prep(struct inode *inode, sector_t iblock, * preallocated blocks are unmapped but should treated * the same as allocated blocks. */ - ret = ext4_da_map_blocks(inode, iblock, &map, bh); + ret = ext4_da_map_blocks(inode, &map, bh); if (ret <= 0) return ret; -- cgit From 12eba993b94c9186e44a01f46e38b3ab3c635f2d Mon Sep 17 00:00:00 2001 From: Zhang Yi Date: Fri, 17 May 2024 20:40:01 +0800 Subject: ext4: make ext4_es_insert_delayed_block() insert multi-blocks Rename ext4_es_insert_delayed_block() to ext4_es_insert_delayed_extent() and pass length parameter to make it insert multiple delalloc blocks at a time. For the case of bigalloc, split the allocated parameter to lclu_allocated and end_allocated. lclu_allocated indicates the allocation state of the cluster which is containing the lblk, end_allocated indicates the allocation state of the extent end, clusters in the middle of delay allocated extent must be unallocated. Signed-off-by: Zhang Yi Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240517124005.347221-7-yi.zhang@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/ext4/extents_status.c | 70 +++++++++++++++++++++++++++++++-------------- fs/ext4/extents_status.h | 5 ++-- fs/ext4/inode.c | 2 +- include/trace/events/ext4.h | 16 ++++++----- 4 files changed, 62 insertions(+), 31 deletions(-) diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c index 3a53dbb85e15..17dcf13adde2 100644 --- a/fs/ext4/extents_status.c +++ b/fs/ext4/extents_status.c @@ -2054,34 +2054,49 @@ bool ext4_is_pending(struct inode *inode, ext4_lblk_t lblk) } /* - * ext4_es_insert_delayed_block - adds a delayed block to the extents status - * tree, adding a pending reservation where - * needed + * ext4_es_insert_delayed_extent - adds some delayed blocks to the extents + * status tree, adding a pending reservation + * where needed * * @inode - file containing the newly added block - * @lblk - logical block to be added - * @allocated - indicates whether a physical cluster has been allocated for - * the logical cluster that contains the block + * @lblk - start logical block to be added + * @len - length of blocks to be added + * @lclu_allocated/end_allocated - indicates whether a physical cluster has + * been allocated for the logical cluster + * that contains the start/end block. Note that + * end_allocated should always be set to false + * if the start and the end block are in the + * same cluster */ -void ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk, - bool allocated) +void ext4_es_insert_delayed_extent(struct inode *inode, ext4_lblk_t lblk, + ext4_lblk_t len, bool lclu_allocated, + bool end_allocated) { + struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb); struct extent_status newes; + ext4_lblk_t end = lblk + len - 1; int err1 = 0, err2 = 0, err3 = 0; struct extent_status *es1 = NULL; struct extent_status *es2 = NULL; - struct pending_reservation *pr = NULL; + struct pending_reservation *pr1 = NULL; + struct pending_reservation *pr2 = NULL; if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY) return; - es_debug("add [%u/1) delayed to extent status tree of inode %lu\n", - lblk, inode->i_ino); + es_debug("add [%u/%u) delayed to extent status tree of inode %lu\n", + lblk, len, inode->i_ino); + if (!len) + return; + + WARN_ON_ONCE((EXT4_B2C(sbi, lblk) == EXT4_B2C(sbi, end)) && + end_allocated); newes.es_lblk = lblk; - newes.es_len = 1; + newes.es_len = len; ext4_es_store_pblock_status(&newes, ~0, EXTENT_STATUS_DELAYED); - trace_ext4_es_insert_delayed_block(inode, &newes, allocated); + trace_ext4_es_insert_delayed_extent(inode, &newes, lclu_allocated, + end_allocated); ext4_es_insert_extent_check(inode, &newes); @@ -2090,11 +2105,15 @@ retry: es1 = __es_alloc_extent(true); if ((err1 || err2) && !es2) es2 = __es_alloc_extent(true); - if ((err1 || err2 || err3) && allocated && !pr) - pr = __alloc_pending(true); + if (err1 || err2 || err3) { + if (lclu_allocated && !pr1) + pr1 = __alloc_pending(true); + if (end_allocated && !pr2) + pr2 = __alloc_pending(true); + } write_lock(&EXT4_I(inode)->i_es_lock); - err1 = __es_remove_extent(inode, lblk, lblk, NULL, es1); + err1 = __es_remove_extent(inode, lblk, end, NULL, es1); if (err1 != 0) goto error; /* Free preallocated extent if it didn't get used. */ @@ -2114,13 +2133,22 @@ retry: es2 = NULL; } - if (allocated) { - err3 = __insert_pending(inode, lblk, &pr); + if (lclu_allocated) { + err3 = __insert_pending(inode, lblk, &pr1); if (err3 != 0) goto error; - if (pr) { - __free_pending(pr); - pr = NULL; + if (pr1) { + __free_pending(pr1); + pr1 = NULL; + } + } + if (end_allocated) { + err3 = __insert_pending(inode, end, &pr2); + if (err3 != 0) + goto error; + if (pr2) { + __free_pending(pr2); + pr2 = NULL; } } error: diff --git a/fs/ext4/extents_status.h b/fs/ext4/extents_status.h index d9847a4a25db..3c8e2edee5d5 100644 --- a/fs/ext4/extents_status.h +++ b/fs/ext4/extents_status.h @@ -249,8 +249,9 @@ extern void ext4_exit_pending(void); extern void ext4_init_pending_tree(struct ext4_pending_tree *tree); extern void ext4_remove_pending(struct inode *inode, ext4_lblk_t lblk); extern bool ext4_is_pending(struct inode *inode, ext4_lblk_t lblk); -extern void ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk, - bool allocated); +extern void ext4_es_insert_delayed_extent(struct inode *inode, ext4_lblk_t lblk, + ext4_lblk_t len, bool lclu_allocated, + bool end_allocated); extern unsigned int ext4_es_delayed_clu(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len); extern void ext4_clear_inode_es(struct inode *inode); diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 20528a04903a..c55f1607b7d8 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -1702,7 +1702,7 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk) } } - ext4_es_insert_delayed_block(inode, lblk, allocated); + ext4_es_insert_delayed_extent(inode, lblk, 1, allocated, false); return 0; } diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h index a697f4b77162..6b41ac61310f 100644 --- a/include/trace/events/ext4.h +++ b/include/trace/events/ext4.h @@ -2478,11 +2478,11 @@ TRACE_EVENT(ext4_es_shrink, __entry->scan_time, __entry->nr_skipped, __entry->retried) ); -TRACE_EVENT(ext4_es_insert_delayed_block, +TRACE_EVENT(ext4_es_insert_delayed_extent, TP_PROTO(struct inode *inode, struct extent_status *es, - bool allocated), + bool lclu_allocated, bool end_allocated), - TP_ARGS(inode, es, allocated), + TP_ARGS(inode, es, lclu_allocated, end_allocated), TP_STRUCT__entry( __field( dev_t, dev ) @@ -2491,7 +2491,8 @@ TRACE_EVENT(ext4_es_insert_delayed_block, __field( ext4_lblk_t, len ) __field( ext4_fsblk_t, pblk ) __field( char, status ) - __field( bool, allocated ) + __field( bool, lclu_allocated ) + __field( bool, end_allocated ) ), TP_fast_assign( @@ -2501,16 +2502,17 @@ TRACE_EVENT(ext4_es_insert_delayed_block, __entry->len = es->es_len; __entry->pblk = ext4_es_show_pblock(es); __entry->status = ext4_es_status(es); - __entry->allocated = allocated; + __entry->lclu_allocated = lclu_allocated; + __entry->end_allocated = end_allocated; ), TP_printk("dev %d,%d ino %lu es [%u/%u) mapped %llu status %s " - "allocated %d", + "allocated %d %d", MAJOR(__entry->dev), MINOR(__entry->dev), (unsigned long) __entry->ino, __entry->lblk, __entry->len, __entry->pblk, show_extent_status(__entry->status), - __entry->allocated) + __entry->lclu_allocated, __entry->end_allocated) ); /* fsmap traces */ -- cgit From 0d66b23d79c750276f791411d81a524549a64852 Mon Sep 17 00:00:00 2001 From: Zhang Yi Date: Fri, 17 May 2024 20:40:02 +0800 Subject: ext4: make ext4_da_reserve_space() reserve multi-clusters Add 'nr_resv' parameter to ext4_da_reserve_space(), which indicates the number of clusters wants to reserve, make it reserve multiple clusters at a time. Signed-off-by: Zhang Yi Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240517124005.347221-8-yi.zhang@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/ext4/inode.c | 18 +++++++++--------- include/trace/events/ext4.h | 10 ++++++---- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index c55f1607b7d8..8e5815b30b5f 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -1479,9 +1479,9 @@ static int ext4_journalled_write_end(struct file *file, } /* - * Reserve space for a single cluster + * Reserve space for 'nr_resv' clusters */ -static int ext4_da_reserve_space(struct inode *inode) +static int ext4_da_reserve_space(struct inode *inode, int nr_resv) { struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb); struct ext4_inode_info *ei = EXT4_I(inode); @@ -1492,18 +1492,18 @@ static int ext4_da_reserve_space(struct inode *inode) * us from metadata over-estimation, though we may go over by * a small amount in the end. Here we just reserve for data. */ - ret = dquot_reserve_block(inode, EXT4_C2B(sbi, 1)); + ret = dquot_reserve_block(inode, EXT4_C2B(sbi, nr_resv)); if (ret) return ret; spin_lock(&ei->i_block_reservation_lock); - if (ext4_claim_free_clusters(sbi, 1, 0)) { + if (ext4_claim_free_clusters(sbi, nr_resv, 0)) { spin_unlock(&ei->i_block_reservation_lock); - dquot_release_reservation_block(inode, EXT4_C2B(sbi, 1)); + dquot_release_reservation_block(inode, EXT4_C2B(sbi, nr_resv)); return -ENOSPC; } - ei->i_reserved_data_blocks++; - trace_ext4_da_reserve_space(inode); + ei->i_reserved_data_blocks += nr_resv; + trace_ext4_da_reserve_space(inode, nr_resv); spin_unlock(&ei->i_block_reservation_lock); return 0; /* success */ @@ -1678,7 +1678,7 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk) * extents status tree doesn't get a match. */ if (sbi->s_cluster_ratio == 1) { - ret = ext4_da_reserve_space(inode); + ret = ext4_da_reserve_space(inode, 1); if (ret != 0) /* ENOSPC */ return ret; } else { /* bigalloc */ @@ -1690,7 +1690,7 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk) if (ret < 0) return ret; if (ret == 0) { - ret = ext4_da_reserve_space(inode); + ret = ext4_da_reserve_space(inode, 1); if (ret != 0) /* ENOSPC */ return ret; } else { diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h index 6b41ac61310f..cc5e9b7b2b44 100644 --- a/include/trace/events/ext4.h +++ b/include/trace/events/ext4.h @@ -1246,14 +1246,15 @@ TRACE_EVENT(ext4_da_update_reserve_space, ); TRACE_EVENT(ext4_da_reserve_space, - TP_PROTO(struct inode *inode), + TP_PROTO(struct inode *inode, int nr_resv), - TP_ARGS(inode), + TP_ARGS(inode, nr_resv), TP_STRUCT__entry( __field( dev_t, dev ) __field( ino_t, ino ) __field( __u64, i_blocks ) + __field( int, reserve_blocks ) __field( int, reserved_data_blocks ) __field( __u16, mode ) ), @@ -1262,16 +1263,17 @@ TRACE_EVENT(ext4_da_reserve_space, __entry->dev = inode->i_sb->s_dev; __entry->ino = inode->i_ino; __entry->i_blocks = inode->i_blocks; + __entry->reserve_blocks = nr_resv; __entry->reserved_data_blocks = EXT4_I(inode)->i_reserved_data_blocks; __entry->mode = inode->i_mode; ), - TP_printk("dev %d,%d ino %lu mode 0%o i_blocks %llu " + TP_printk("dev %d,%d ino %lu mode 0%o i_blocks %llu reserve_blocks %d" "reserved_data_blocks %d", MAJOR(__entry->dev), MINOR(__entry->dev), (unsigned long) __entry->ino, __entry->mode, __entry->i_blocks, - __entry->reserved_data_blocks) + __entry->reserve_blocks, __entry->reserved_data_blocks) ); TRACE_EVENT(ext4_da_release_space, -- cgit From 49bf6ab4d30b7a39d86a585e0a58f6c449d2e009 Mon Sep 17 00:00:00 2001 From: Zhang Yi Date: Fri, 17 May 2024 20:40:03 +0800 Subject: ext4: factor out a helper to check the cluster allocation state Factor out a common helper ext4_clu_alloc_state(), check whether the cluster containing a delalloc block to be added has been allocated or has delalloc reservation, no logic changes. Signed-off-by: Zhang Yi Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240517124005.347221-9-yi.zhang@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/ext4/inode.c | 55 ++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 38 insertions(+), 17 deletions(-) diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 8e5815b30b5f..4d8c9ab1ddb1 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -1649,6 +1649,35 @@ static void ext4_print_free_blocks(struct inode *inode) return; } +/* + * Check whether the cluster containing lblk has been allocated or has + * delalloc reservation. + * + * Returns 0 if the cluster doesn't have either, 1 if it has delalloc + * reservation, 2 if it's already been allocated, negative error code on + * failure. + */ +static int ext4_clu_alloc_state(struct inode *inode, ext4_lblk_t lblk) +{ + struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb); + int ret; + + /* Has delalloc reservation? */ + if (ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)) + return 1; + + /* Already been allocated? */ + if (ext4_es_scan_clu(inode, &ext4_es_is_mapped, lblk)) + return 2; + ret = ext4_clu_mapped(inode, EXT4_B2C(sbi, lblk)); + if (ret < 0) + return ret; + if (ret > 0) + return 2; + + return 0; +} + /* * ext4_insert_delayed_block - adds a delayed block to the extents status * tree, incrementing the reserved cluster/block @@ -1682,23 +1711,15 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk) if (ret != 0) /* ENOSPC */ return ret; } else { /* bigalloc */ - if (!ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)) { - if (!ext4_es_scan_clu(inode, - &ext4_es_is_mapped, lblk)) { - ret = ext4_clu_mapped(inode, - EXT4_B2C(sbi, lblk)); - if (ret < 0) - return ret; - if (ret == 0) { - ret = ext4_da_reserve_space(inode, 1); - if (ret != 0) /* ENOSPC */ - return ret; - } else { - allocated = true; - } - } else { - allocated = true; - } + ret = ext4_clu_alloc_state(inode, lblk); + if (ret < 0) + return ret; + if (ret == 2) + allocated = true; + if (ret == 0) { + ret = ext4_da_reserve_space(inode, 1); + if (ret != 0) /* ENOSPC */ + return ret; } } -- cgit From 1850d76c1b781ad9c7dc3c4968fb40c1915231c0 Mon Sep 17 00:00:00 2001 From: Zhang Yi Date: Fri, 17 May 2024 20:40:04 +0800 Subject: ext4: make ext4_insert_delayed_block() insert multi-blocks Rename ext4_insert_delayed_block() to ext4_insert_delayed_blocks(), pass length parameter to make it insert multiple delalloc blocks at a time. For non-bigalloc case, just reserve len blocks and insert delalloc extent. For bigalloc case, we can ensure that the clusters in the middle of a extent must be unallocated, we only need to check whether the start and end clusters are delayed/allocated. We should subtract the space for the start and/or end block(s) if they are allocated. Signed-off-by: Zhang Yi Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240517124005.347221-10-yi.zhang@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/ext4/inode.c | 51 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 15 deletions(-) diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 4d8c9ab1ddb1..91eb3ba68970 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -1679,24 +1679,29 @@ static int ext4_clu_alloc_state(struct inode *inode, ext4_lblk_t lblk) } /* - * ext4_insert_delayed_block - adds a delayed block to the extents status - * tree, incrementing the reserved cluster/block - * count or making a pending reservation - * where needed + * ext4_insert_delayed_blocks - adds a multiple delayed blocks to the extents + * status tree, incrementing the reserved + * cluster/block count or making pending + * reservations where needed * * @inode - file containing the newly added block - * @lblk - logical block to be added + * @lblk - start logical block to be added + * @len - length of blocks to be added * * Returns 0 on success, negative error code on failure. */ -static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk) +static int ext4_insert_delayed_blocks(struct inode *inode, ext4_lblk_t lblk, + ext4_lblk_t len) { struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb); int ret; - bool allocated = false; + bool lclu_allocated = false; + bool end_allocated = false; + ext4_lblk_t resv_clu; + ext4_lblk_t end = lblk + len - 1; /* - * If the cluster containing lblk is shared with a delayed, + * If the cluster containing lblk or end is shared with a delayed, * written, or unwritten extent in a bigalloc file system, it's * already been accounted for and does not need to be reserved. * A pending reservation must be made for the cluster if it's @@ -1707,23 +1712,39 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk) * extents status tree doesn't get a match. */ if (sbi->s_cluster_ratio == 1) { - ret = ext4_da_reserve_space(inode, 1); + ret = ext4_da_reserve_space(inode, len); if (ret != 0) /* ENOSPC */ return ret; } else { /* bigalloc */ + resv_clu = EXT4_B2C(sbi, end) - EXT4_B2C(sbi, lblk) + 1; + ret = ext4_clu_alloc_state(inode, lblk); if (ret < 0) return ret; - if (ret == 2) - allocated = true; - if (ret == 0) { - ret = ext4_da_reserve_space(inode, 1); + if (ret > 0) { + resv_clu--; + lclu_allocated = (ret == 2); + } + + if (EXT4_B2C(sbi, lblk) != EXT4_B2C(sbi, end)) { + ret = ext4_clu_alloc_state(inode, end); + if (ret < 0) + return ret; + if (ret > 0) { + resv_clu--; + end_allocated = (ret == 2); + } + } + + if (resv_clu) { + ret = ext4_da_reserve_space(inode, resv_clu); if (ret != 0) /* ENOSPC */ return ret; } } - ext4_es_insert_delayed_extent(inode, lblk, 1, allocated, false); + ext4_es_insert_delayed_extent(inode, lblk, len, lclu_allocated, + end_allocated); return 0; } @@ -1828,7 +1849,7 @@ add_delayed: } } - retval = ext4_insert_delayed_block(inode, map->m_lblk); + retval = ext4_insert_delayed_blocks(inode, map->m_lblk, map->m_len); up_write(&EXT4_I(inode)->i_data_sem); if (retval) return retval; -- cgit From 8262fe9a902c8a7b68c8521ebe18360a9145bada Mon Sep 17 00:00:00 2001 From: Zhang Yi Date: Fri, 17 May 2024 20:40:05 +0800 Subject: ext4: make ext4_da_map_blocks() buffer_head unaware After calling the ext4_da_map_blocks(), a delalloc extent state could be identified through the EXT4_MAP_DELAYED flag in map. So factor out buffer_head related handles in ext4_da_map_blocks(), make this function buffer_head unaware and becomes a common helper, and also update the stale function commtents, preparing for the iomap da write path in the future. Signed-off-by: Zhang Yi Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240517124005.347221-11-yi.zhang@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/ext4/inode.c | 63 ++++++++++++++++++++++++++++----------------------------- 1 file changed, 31 insertions(+), 32 deletions(-) diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 91eb3ba68970..c81cf67d8e77 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -1749,36 +1749,32 @@ static int ext4_insert_delayed_blocks(struct inode *inode, ext4_lblk_t lblk, } /* - * This function is grabs code from the very beginning of - * ext4_map_blocks, but assumes that the caller is from delayed write - * time. This function looks up the requested blocks and sets the - * buffer delay bit under the protection of i_data_sem. + * Looks up the requested blocks and sets the delalloc extent map. + * First try to look up for the extent entry that contains the requested + * blocks in the extent status tree without i_data_sem, then try to look + * up for the ondisk extent mapping with i_data_sem in read mode, + * finally hold i_data_sem in write mode, looks up again and add a + * delalloc extent entry if it still couldn't find any extent. Pass out + * the mapped extent through @map and return 0 on success. */ -static int ext4_da_map_blocks(struct inode *inode, struct ext4_map_blocks *map, - struct buffer_head *bh) +static int ext4_da_map_blocks(struct inode *inode, struct ext4_map_blocks *map) { struct extent_status es; int retval; - sector_t invalid_block = ~((sector_t) 0xffff); #ifdef ES_AGGRESSIVE_TEST struct ext4_map_blocks orig_map; memcpy(&orig_map, map, sizeof(*map)); #endif - if (invalid_block < ext4_blocks_count(EXT4_SB(inode->i_sb)->s_es)) - invalid_block = ~0; - map->m_flags = 0; ext_debug(inode, "max_blocks %u, logical block %lu\n", map->m_len, (unsigned long) map->m_lblk); /* Lookup extent status tree firstly */ if (ext4_es_lookup_extent(inode, map->m_lblk, NULL, &es)) { - retval = es.es_len - (map->m_lblk - es.es_lblk); - if (retval > map->m_len) - retval = map->m_len; - map->m_len = retval; + map->m_len = min_t(unsigned int, map->m_len, + es.es_len - (map->m_lblk - es.es_lblk)); if (ext4_es_is_hole(&es)) goto add_delayed; @@ -1788,10 +1784,8 @@ found: * Delayed extent could be allocated by fallocate. * So we need to check it. */ - if (ext4_es_is_delayed(&es) && !ext4_es_is_unwritten(&es)) { - map_bh(bh, inode->i_sb, invalid_block); - set_buffer_new(bh); - set_buffer_delay(bh); + if (ext4_es_is_delonly(&es)) { + map->m_flags |= EXT4_MAP_DELAYED; return 0; } @@ -1806,7 +1800,7 @@ found: #ifdef ES_AGGRESSIVE_TEST ext4_map_blocks_es_recheck(NULL, inode, map, &orig_map, 0); #endif - return retval; + return 0; } /* @@ -1820,7 +1814,7 @@ found: retval = ext4_map_query_blocks(NULL, inode, map); up_read(&EXT4_I(inode)->i_data_sem); if (retval) - return retval; + return retval < 0 ? retval : 0; add_delayed: down_write(&EXT4_I(inode)->i_data_sem); @@ -1832,10 +1826,8 @@ add_delayed: * the extent status tree. */ if (ext4_es_lookup_extent(inode, map->m_lblk, NULL, &es)) { - retval = es.es_len - (map->m_lblk - es.es_lblk); - if (retval > map->m_len) - retval = map->m_len; - map->m_len = retval; + map->m_len = min_t(unsigned int, map->m_len, + es.es_len - (map->m_lblk - es.es_lblk)); if (!ext4_es_is_hole(&es)) { up_write(&EXT4_I(inode)->i_data_sem); @@ -1845,18 +1837,14 @@ add_delayed: retval = ext4_map_query_blocks(NULL, inode, map); if (retval) { up_write(&EXT4_I(inode)->i_data_sem); - return retval; + return retval < 0 ? retval : 0; } } + map->m_flags |= EXT4_MAP_DELAYED; retval = ext4_insert_delayed_blocks(inode, map->m_lblk, map->m_len); up_write(&EXT4_I(inode)->i_data_sem); - if (retval) - return retval; - map_bh(bh, inode->i_sb, invalid_block); - set_buffer_new(bh); - set_buffer_delay(bh); return retval; } @@ -1876,11 +1864,15 @@ int ext4_da_get_block_prep(struct inode *inode, sector_t iblock, struct buffer_head *bh, int create) { struct ext4_map_blocks map; + sector_t invalid_block = ~((sector_t) 0xffff); int ret = 0; BUG_ON(create == 0); BUG_ON(bh->b_size != inode->i_sb->s_blocksize); + if (invalid_block < ext4_blocks_count(EXT4_SB(inode->i_sb)->s_es)) + invalid_block = ~0; + map.m_lblk = iblock; map.m_len = 1; @@ -1889,10 +1881,17 @@ int ext4_da_get_block_prep(struct inode *inode, sector_t iblock, * preallocated blocks are unmapped but should treated * the same as allocated blocks. */ - ret = ext4_da_map_blocks(inode, &map, bh); - if (ret <= 0) + ret = ext4_da_map_blocks(inode, &map); + if (ret < 0) return ret; + if (map.m_flags & EXT4_MAP_DELAYED) { + map_bh(bh, inode->i_sb, invalid_block); + set_buffer_new(bh); + set_buffer_delay(bh); + return 0; + } + map_bh(bh, inode->i_sb, map.m_pblk); ext4_update_bh_state(bh, map.m_flags); -- cgit From 7c73ddb7589fb8ddb1136b6306dfb72089c81511 Mon Sep 17 00:00:00 2001 From: Zhang Yi Date: Mon, 20 May 2024 21:18:31 +0800 Subject: jbd2: speed up jbd2_transaction_committed() jbd2_transaction_committed() is used to check whether a transaction with the given tid has already committed, it holds j_state_lock in read mode and check the tid of current running transaction and committing transaction, but holding the j_state_lock is expensive. We have already stored the sequence number of the most recently committed transaction in journal t->j_commit_sequence, we could do this check by comparing it with the given tid instead. If the given tid isn't smaller than j_commit_sequence, we can ensure that the given transaction has been committed. That way we could drop the expensive lock and achieve about 10% ~ 20% performance gains in concurrent DIOs on may virtual machine with 100G ramdisk. fio -filename=/mnt/foo -direct=1 -iodepth=10 -rw=$rw -ioengine=libaio \ -bs=4k -size=10G -numjobs=10 -runtime=60 -overwrite=1 -name=test \ -group_reporting Before: overwrite IOPS=88.2k, BW=344MiB/s read IOPS=95.7k, BW=374MiB/s rand overwrite IOPS=98.7k, BW=386MiB/s randread IOPS=102k, BW=397MiB/s After: overwrite IOPS=105k, BW=410MiB/s read IOPS=112k, BW=436MiB/s rand overwrite IOPS=104k, BW=404MiB/s randread IOPS=111k, BW=432MiB/s CC: Dave Chinner Suggested-by: Dave Chinner Link: https://lore.kernel.org/linux-ext4/ZjILCPNZRHeazSqV@dread.disaster.area/ Signed-off-by: Zhang Yi Reviewed-by: Jan Kara Reviewed-by: Ritesh Harjani (IBM) Link: https://patch.msgid.link/20240520131831.2910790-1-yi.zhang@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/jbd2/commit.c | 2 +- fs/jbd2/journal.c | 12 +----------- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c index 6d3fc992287d..ec5ed7eb1938 100644 --- a/fs/jbd2/commit.c +++ b/fs/jbd2/commit.c @@ -1107,7 +1107,7 @@ restart_loop: commit_transaction->t_state = T_COMMIT_CALLBACK; J_ASSERT(commit_transaction == journal->j_committing_transaction); - journal->j_commit_sequence = commit_transaction->t_tid; + WRITE_ONCE(journal->j_commit_sequence, commit_transaction->t_tid); journal->j_committing_transaction = NULL; commit_time = ktime_to_ns(ktime_sub(ktime_get(), start_time)); diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index 8d782a88aee1..25ef4fc80667 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -775,17 +775,7 @@ EXPORT_SYMBOL(jbd2_fc_end_commit_fallback); /* Return 1 when transaction with given tid has already committed. */ int jbd2_transaction_committed(journal_t *journal, tid_t tid) { - int ret = 1; - - read_lock(&journal->j_state_lock); - if (journal->j_running_transaction && - journal->j_running_transaction->t_tid == tid) - ret = 0; - if (journal->j_committing_transaction && - journal->j_committing_transaction->t_tid == tid) - ret = 0; - read_unlock(&journal->j_state_lock); - return ret; + return tid_geq(READ_ONCE(journal->j_commit_sequence), tid); } EXPORT_SYMBOL(jbd2_transaction_committed); -- cgit From be27cd64461c45a6088a91a04eba5cd44e1767ef Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 23 May 2024 15:54:12 -0700 Subject: ext4: use memtostr_pad() for s_volume_name As with the other strings in struct ext4_super_block, s_volume_name is not NUL terminated. The other strings were marked in commit 072ebb3bffe6 ("ext4: add nonstring annotations to ext4.h"). Using strscpy() isn't the right replacement for strncpy(); it should use memtostr_pad() instead. Reported-by: syzbot+50835f73143cc2905b9e@syzkaller.appspotmail.com Closes: https://lore.kernel.org/all/00000000000019f4c00619192c05@google.com/ Fixes: 744a56389f73 ("ext4: replace deprecated strncpy with alternatives") Signed-off-by: Kees Cook Link: https://patch.msgid.link/20240523225408.work.904-kees@kernel.org Signed-off-by: Theodore Ts'o --- fs/ext4/ext4.h | 2 +- fs/ext4/ioctl.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 983dad8c07ec..efed7f09876d 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -1347,7 +1347,7 @@ struct ext4_super_block { /*60*/ __le32 s_feature_incompat; /* incompatible feature set */ __le32 s_feature_ro_compat; /* readonly-compatible feature set */ /*68*/ __u8 s_uuid[16]; /* 128-bit uuid for volume */ -/*78*/ char s_volume_name[EXT4_LABEL_MAX]; /* volume name */ +/*78*/ char s_volume_name[EXT4_LABEL_MAX] __nonstring; /* volume name */ /*88*/ char s_last_mounted[64] __nonstring; /* directory where last mounted */ /*C8*/ __le32 s_algorithm_usage_bitmap; /* For compression */ /* diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c index dab7acd49709..e8bf5972dd47 100644 --- a/fs/ext4/ioctl.c +++ b/fs/ext4/ioctl.c @@ -1151,7 +1151,7 @@ static int ext4_ioctl_getlabel(struct ext4_sb_info *sbi, char __user *user_label BUILD_BUG_ON(EXT4_LABEL_MAX >= FSLABEL_MAX); lock_buffer(sbi->s_sbh); - strscpy_pad(label, sbi->s_es->s_volume_name); + memtostr_pad(label, sbi->s_es->s_volume_name); unlock_buffer(sbi->s_sbh); if (copy_to_user(user_label, label, sizeof(label))) -- cgit From 89a8718cef859091239fa60b4b5749ecea93f55d Mon Sep 17 00:00:00 2001 From: Jeff Johnson Date: Sun, 26 May 2024 11:53:49 -0700 Subject: jbd2: add missing MODULE_DESCRIPTION() Fix the 'make W=1' warning: WARNING: modpost: missing MODULE_DESCRIPTION() in fs/jbd2/jbd2.o Signed-off-by: Jeff Johnson Link: https://patch.msgid.link/20240526-md-fs-jbd2-v1-1-7bba6665327d@quicinc.com Signed-off-by: Theodore Ts'o --- fs/jbd2/journal.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index 25ef4fc80667..3501f75f0fbf 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -3157,6 +3157,7 @@ static void __exit journal_exit(void) jbd2_journal_destroy_caches(); } +MODULE_DESCRIPTION("Generic filesystem journal-writing module"); MODULE_LICENSE("GPL"); module_init(journal_init); module_exit(journal_exit); -- cgit From 7378e8991a459f3e5672e4b06bf346adce062fd8 Mon Sep 17 00:00:00 2001 From: Jeff Johnson Date: Mon, 27 May 2024 11:02:29 -0700 Subject: ext4: add missing MODULE_DESCRIPTION() Fix the 'make W=1' warning: WARNING: modpost: missing MODULE_DESCRIPTION() in fs/ext4/ext4-inode-test.o Signed-off-by: Jeff Johnson Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240527-md-fs-ext4-v1-1-07aad5936bb1@quicinc.com Signed-off-by: Theodore Ts'o --- fs/ext4/inode-test.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/ext4/inode-test.c b/fs/ext4/inode-test.c index f0c0fd507fbc..749af7ad4e09 100644 --- a/fs/ext4/inode-test.c +++ b/fs/ext4/inode-test.c @@ -279,4 +279,5 @@ static struct kunit_suite ext4_inode_test_suite = { kunit_test_suites(&ext4_inode_test_suite); +MODULE_DESCRIPTION("KUnit test of ext4 inode timestamp decoding"); MODULE_LICENSE("GPL v2"); -- cgit From 2d4d6bda0f7ba0f188a22698855a1397c8999df3 Mon Sep 17 00:00:00 2001 From: "Luis Henriques (SUSE)" Date: Mon, 27 May 2024 17:14:47 +0100 Subject: ext4: use ext4_update_inode_fsync_trans() helper in inode creation Call helper function ext4_update_inode_fsync_trans() instead of open coding it in __ext4_new_inode(). This helper checks both that the handle is valid *and* that it hasn't been aborted due to some fatal error in the journalling layer, using is_handle_aborted(). Signed-off-by: Luis Henriques (SUSE) Link: https://patch.msgid.link/20240527161447.21434-1-luis.henriques@linux.dev Signed-off-by: Theodore Ts'o --- fs/ext4/ialloc.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c index e9bbb1da2d0a..9dfd768ed9f8 100644 --- a/fs/ext4/ialloc.c +++ b/fs/ext4/ialloc.c @@ -1336,10 +1336,7 @@ got: } } - if (ext4_handle_valid(handle)) { - ei->i_sync_tid = handle->h_transaction->t_tid; - ei->i_datasync_tid = handle->h_transaction->t_tid; - } + ext4_update_inode_fsync_trans(handle, inode, 1); err = ext4_mark_inode_dirty(handle, inode); if (err) { -- cgit From 63469662cc45d41705f14b4648481d5d29cf5999 Mon Sep 17 00:00:00 2001 From: "Luis Henriques (SUSE)" Date: Wed, 29 May 2024 10:20:30 +0100 Subject: ext4: fix possible tid_t sequence overflows In the fast commit code there are a few places where tid_t variables are being compared without taking into account the fact that these sequence numbers may wrap. Fix this issue by using the helper functions tid_gt() and tid_geq(). Signed-off-by: Luis Henriques (SUSE) Reviewed-by: Jan Kara Reviewed-by: Harshad Shirwadkar Link: https://patch.msgid.link/20240529092030.9557-3-luis.henriques@linux.dev Signed-off-by: Theodore Ts'o --- fs/ext4/fast_commit.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c index 87c009e0c59a..f53a02bc0c33 100644 --- a/fs/ext4/fast_commit.c +++ b/fs/ext4/fast_commit.c @@ -353,7 +353,7 @@ void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handl read_unlock(&sbi->s_journal->j_state_lock); } spin_lock(&sbi->s_fc_lock); - if (sbi->s_fc_ineligible_tid < tid) + if (tid_gt(tid, sbi->s_fc_ineligible_tid)) sbi->s_fc_ineligible_tid = tid; spin_unlock(&sbi->s_fc_lock); WARN_ON(reason >= EXT4_FC_REASON_MAX); @@ -1207,7 +1207,7 @@ restart_fc: if (ret == -EALREADY) { /* There was an ongoing commit, check if we need to restart */ if (atomic_read(&sbi->s_fc_subtid) <= subtid && - commit_tid > journal->j_commit_sequence) + tid_gt(commit_tid, journal->j_commit_sequence)) goto restart_fc; ext4_fc_update_stats(sb, EXT4_FC_STATUS_SKIPPED, 0, 0, commit_tid); @@ -1282,7 +1282,7 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) list_del_init(&iter->i_fc_list); ext4_clear_inode_state(&iter->vfs_inode, EXT4_STATE_FC_COMMITTING); - if (iter->i_sync_tid <= tid) + if (tid_geq(tid, iter->i_sync_tid)) ext4_fc_reset_inode(&iter->vfs_inode); /* Make sure EXT4_STATE_FC_COMMITTING bit is clear */ smp_mb(); @@ -1313,7 +1313,7 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) list_splice_init(&sbi->s_fc_q[FC_Q_STAGING], &sbi->s_fc_q[FC_Q_MAIN]); - if (tid >= sbi->s_fc_ineligible_tid) { + if (tid_geq(tid, sbi->s_fc_ineligible_tid)) { sbi->s_fc_ineligible_tid = 0; ext4_clear_mount_flag(sb, EXT4_MF_FC_INELIGIBLE); } -- cgit From 7882b0187bbeb647967a7b5998ce4ad26ef68a9a Mon Sep 17 00:00:00 2001 From: "Luis Henriques (SUSE)" Date: Tue, 18 Jun 2024 15:43:12 +0100 Subject: ext4: don't track ranges in fast_commit if inode has inlined data When fast-commit needs to track ranges, it has to handle inodes that have inlined data in a different way because ext4_fc_write_inode_data(), in the actual commit path, will attempt to map the required blocks for the range. However, inodes that have inlined data will have it's data stored in inode->i_block and, eventually, in the extended attribute space. Unfortunately, because fast commit doesn't currently support extended attributes, the solution is to mark this commit as ineligible. Link: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1039883 Signed-off-by: Luis Henriques (SUSE) Tested-by: Ben Hutchings Fixes: 9725958bb75c ("ext4: fast commit may miss tracking unwritten range during ftruncate") Link: https://patch.msgid.link/20240618144312.17786-1-luis.henriques@linux.dev Signed-off-by: Theodore Ts'o --- fs/ext4/fast_commit.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c index f53a02bc0c33..3926a05eceee 100644 --- a/fs/ext4/fast_commit.c +++ b/fs/ext4/fast_commit.c @@ -649,6 +649,12 @@ void ext4_fc_track_range(handle_t *handle, struct inode *inode, ext4_lblk_t star if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE)) return; + if (ext4_has_inline_data(inode)) { + ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_XATTR, + handle); + return; + } + args.start = start; args.end = end; -- cgit From 65121eff3e4c8c90f8126debf3c369228691c591 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Thu, 13 Jun 2024 17:02:34 +0200 Subject: ext4: avoid writing unitialized memory to disk in EA inodes If the extended attribute size is not a multiple of block size, the last block in the EA inode will have uninitialized tail which will get written to disk. We will never expose the data to userspace but still this is not a good practice so just zero out the tail of the block as it isn't going to cause a noticeable performance overhead. Fixes: e50e5129f384 ("ext4: xattr-in-inode support") Reported-by: syzbot+9c1fe13fcb51574b249b@syzkaller.appspotmail.com Reported-by: Hugh Dickins Signed-off-by: Jan Kara Link: https://patch.msgid.link/20240613150234.25176-1-jack@suse.cz Signed-off-by: Theodore Ts'o --- fs/ext4/xattr.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index 6460879b9fcb..46ce2f21fef9 100644 --- a/fs/ext4/xattr.c +++ b/fs/ext4/xattr.c @@ -1433,6 +1433,12 @@ retry: goto out; memcpy(bh->b_data, buf, csize); + /* + * Zero out block tail to avoid writing uninitialized memory + * to disk. + */ + if (csize < blocksize) + memset(bh->b_data + csize, 0, blocksize - csize); set_buffer_uptodate(bh); ext4_handle_dirty_metadata(handle, ea_inode, bh); -- cgit From 0bab8db4152c4a2185a1367db09cc402bdc62d5e Mon Sep 17 00:00:00 2001 From: Ye Bin Date: Thu, 20 Jun 2024 15:24:05 +0800 Subject: jbd2: avoid mount failed when commit block is partial submitted We encountered a problem that the file system could not be mounted in the power-off scenario. The analysis of the file system mirror shows that only part of the data is written to the last commit block. The valid data of the commit block is concentrated in the first sector. However, the data of the entire block is involved in the checksum calculation. For different hardware, the minimum atomic unit may be different. If the checksum of a committed block is incorrect, clear the data except the 'commit_header' and then calculate the checksum. If the checkusm is correct, it is considered that the block is partially committed, Then continue to replay journal. Signed-off-by: Ye Bin Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240620072405.3533701-1-yebin@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/jbd2/recovery.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c index af930c3d0d97..667f67342c52 100644 --- a/fs/jbd2/recovery.c +++ b/fs/jbd2/recovery.c @@ -444,6 +444,27 @@ static int jbd2_commit_block_csum_verify(journal_t *j, void *buf) return provided == cpu_to_be32(calculated); } +static bool jbd2_commit_block_csum_verify_partial(journal_t *j, void *buf) +{ + struct commit_header *h; + __be32 provided; + __u32 calculated; + void *tmpbuf; + + tmpbuf = kzalloc(j->j_blocksize, GFP_KERNEL); + if (!tmpbuf) + return false; + + memcpy(tmpbuf, buf, sizeof(struct commit_header)); + h = tmpbuf; + provided = h->h_chksum[0]; + h->h_chksum[0] = 0; + calculated = jbd2_chksum(j, j->j_csum_seed, tmpbuf, j->j_blocksize); + kfree(tmpbuf); + + return provided == cpu_to_be32(calculated); +} + static int jbd2_block_tag_csum_verify(journal_t *j, journal_block_tag_t *tag, journal_block_tag3_t *tag3, void *buf, __u32 sequence) @@ -811,6 +832,13 @@ static int do_one_pass(journal_t *journal, if (pass == PASS_SCAN && !jbd2_commit_block_csum_verify(journal, bh->b_data)) { + if (jbd2_commit_block_csum_verify_partial( + journal, + bh->b_data)) { + pr_notice("JBD2: Find incomplete commit block in transaction %u block %lu\n", + next_commit_ID, next_log_block); + goto chksum_ok; + } chksum_error: if (commit_time < last_trans_commit_time) goto ignore_crc_mismatch; @@ -825,6 +853,7 @@ static int do_one_pass(journal_t *journal, } } if (pass == PASS_SCAN) { + chksum_ok: last_trans_commit_time = commit_time; head_block = next_log_block; } @@ -844,6 +873,7 @@ static int do_one_pass(journal_t *journal, next_log_block); need_check_commit_time = true; } + /* If we aren't in the REVOKE pass, then we can * just skip over this block. */ if (pass != PASS_REVOKE) { -- cgit From 4aa99c71e42ad60178c1154ec24e3df9c684fb67 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 24 Jun 2024 19:01:17 +0200 Subject: jbd2: make jbd2_journal_get_max_txn_bufs() internal There's no reason to have jbd2_journal_get_max_txn_bufs() public function. Currently all users are internal and can use journal->j_max_transaction_buffers instead. This saves some unnecessary recomputations of the limit as a bonus which becomes important as this function gets more complex in the following patch. CC: stable@vger.kernel.org Signed-off-by: Jan Kara Reviewed-by: Zhang Yi Link: https://patch.msgid.link/20240624170127.3253-1-jack@suse.cz Signed-off-by: Theodore Ts'o --- fs/jbd2/commit.c | 2 +- fs/jbd2/journal.c | 5 +++++ include/linux/jbd2.h | 5 ----- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c index ec5ed7eb1938..4305a1ac808a 100644 --- a/fs/jbd2/commit.c +++ b/fs/jbd2/commit.c @@ -766,7 +766,7 @@ start_journal_io: if (first_block < journal->j_tail) freed += journal->j_last - journal->j_first; /* Update tail only if we free significant amount of space */ - if (freed < jbd2_journal_get_max_txn_bufs(journal)) + if (freed < journal->j_max_transaction_buffers) update_tail = 0; } J_ASSERT(commit_transaction->t_state == T_COMMIT); diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index 3501f75f0fbf..7dea8cfe7162 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -1674,6 +1674,11 @@ journal_t *jbd2_journal_init_inode(struct inode *inode) return journal; } +static int jbd2_journal_get_max_txn_bufs(journal_t *journal) +{ + return (journal->j_total_len - journal->j_fc_wbufsize) / 4; +} + /* * Given a journal_t structure, initialise the various fields for * startup of a new journaling session. We use this both when creating diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index ab04c1c27fae..f91b930abe20 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -1660,11 +1660,6 @@ int jbd2_wait_inode_data(journal_t *journal, struct jbd2_inode *jinode); int jbd2_fc_wait_bufs(journal_t *journal, int num_blks); int jbd2_fc_release_bufs(journal_t *journal); -static inline int jbd2_journal_get_max_txn_bufs(journal_t *journal) -{ - return (journal->j_total_len - journal->j_fc_wbufsize) / 4; -} - /* * is_journal_abort * -- cgit From e3a00a23781c1f2fcda98a7aecaac515558e7a35 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 24 Jun 2024 19:01:18 +0200 Subject: jbd2: precompute number of transaction descriptor blocks Instead of computing the number of descriptor blocks a transaction can have each time we need it (which is currently when starting each transaction but will become more frequent later) precompute the number once during journal initialization together with maximum transaction size. We perform the precomputation whenever journal feature set is updated similarly as for computation of journal->j_revoke_records_per_block. CC: stable@vger.kernel.org Signed-off-by: Jan Kara Reviewed-by: Zhang Yi Link: https://patch.msgid.link/20240624170127.3253-2-jack@suse.cz Signed-off-by: Theodore Ts'o --- fs/jbd2/journal.c | 61 ++++++++++++++++++++++++++++++++++++++------------- fs/jbd2/transaction.c | 24 +------------------- include/linux/jbd2.h | 7 ++++++ 3 files changed, 54 insertions(+), 38 deletions(-) diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index 7dea8cfe7162..2ae861abf770 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -1427,6 +1427,48 @@ static int journal_revoke_records_per_block(journal_t *journal) return space / record_size; } +static int jbd2_journal_get_max_txn_bufs(journal_t *journal) +{ + return (journal->j_total_len - journal->j_fc_wbufsize) / 4; +} + +/* + * Base amount of descriptor blocks we reserve for each transaction. + */ +static int jbd2_descriptor_blocks_per_trans(journal_t *journal) +{ + int tag_space = journal->j_blocksize - sizeof(journal_header_t); + int tags_per_block; + + /* Subtract UUID */ + tag_space -= 16; + if (jbd2_journal_has_csum_v2or3(journal)) + tag_space -= sizeof(struct jbd2_journal_block_tail); + /* Commit code leaves a slack space of 16 bytes at the end of block */ + tags_per_block = (tag_space - 16) / journal_tag_bytes(journal); + /* + * Revoke descriptors are accounted separately so we need to reserve + * space for commit block and normal transaction descriptor blocks. + */ + return 1 + DIV_ROUND_UP(jbd2_journal_get_max_txn_bufs(journal), + tags_per_block); +} + +/* + * Initialize number of blocks each transaction reserves for its bookkeeping + * and maximum number of blocks a transaction can use. This needs to be called + * after the journal size and the fastcommit area size are initialized. + */ +static void jbd2_journal_init_transaction_limits(journal_t *journal) +{ + journal->j_revoke_records_per_block = + journal_revoke_records_per_block(journal); + journal->j_transaction_overhead_buffers = + jbd2_descriptor_blocks_per_trans(journal); + journal->j_max_transaction_buffers = + jbd2_journal_get_max_txn_bufs(journal); +} + /* * Load the on-disk journal superblock and read the key fields into the * journal_t. @@ -1468,8 +1510,8 @@ static int journal_load_superblock(journal_t *journal) if (jbd2_journal_has_csum_v2or3(journal)) journal->j_csum_seed = jbd2_chksum(journal, ~0, sb->s_uuid, sizeof(sb->s_uuid)); - journal->j_revoke_records_per_block = - journal_revoke_records_per_block(journal); + /* After journal features are set, we can compute transaction limits */ + jbd2_journal_init_transaction_limits(journal); if (jbd2_has_feature_fast_commit(journal)) { journal->j_fc_last = be32_to_cpu(sb->s_maxlen); @@ -1674,11 +1716,6 @@ journal_t *jbd2_journal_init_inode(struct inode *inode) return journal; } -static int jbd2_journal_get_max_txn_bufs(journal_t *journal) -{ - return (journal->j_total_len - journal->j_fc_wbufsize) / 4; -} - /* * Given a journal_t structure, initialise the various fields for * startup of a new journaling session. We use this both when creating @@ -1724,8 +1761,6 @@ static int journal_reset(journal_t *journal) journal->j_commit_sequence = journal->j_transaction_sequence - 1; journal->j_commit_request = journal->j_commit_sequence; - journal->j_max_transaction_buffers = jbd2_journal_get_max_txn_bufs(journal); - /* * Now that journal recovery is done, turn fast commits off here. This * way, if fast commit was enabled before the crash but if now FS has @@ -2266,8 +2301,6 @@ jbd2_journal_initialize_fast_commit(journal_t *journal) journal->j_fc_first = journal->j_last + 1; journal->j_fc_off = 0; journal->j_free = journal->j_last - journal->j_first; - journal->j_max_transaction_buffers = - jbd2_journal_get_max_txn_bufs(journal); return 0; } @@ -2355,8 +2388,7 @@ int jbd2_journal_set_features(journal_t *journal, unsigned long compat, sb->s_feature_ro_compat |= cpu_to_be32(ro); sb->s_feature_incompat |= cpu_to_be32(incompat); unlock_buffer(journal->j_sb_buffer); - journal->j_revoke_records_per_block = - journal_revoke_records_per_block(journal); + jbd2_journal_init_transaction_limits(journal); return 1; #undef COMPAT_FEATURE_ON @@ -2387,8 +2419,7 @@ void jbd2_journal_clear_features(journal_t *journal, unsigned long compat, sb->s_feature_compat &= ~cpu_to_be32(compat); sb->s_feature_ro_compat &= ~cpu_to_be32(ro); sb->s_feature_incompat &= ~cpu_to_be32(incompat); - journal->j_revoke_records_per_block = - journal_revoke_records_per_block(journal); + jbd2_journal_init_transaction_limits(journal); } EXPORT_SYMBOL(jbd2_journal_clear_features); diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index cb0b8d6fc0c6..a095f1a3114b 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c @@ -62,28 +62,6 @@ void jbd2_journal_free_transaction(transaction_t *transaction) kmem_cache_free(transaction_cache, transaction); } -/* - * Base amount of descriptor blocks we reserve for each transaction. - */ -static int jbd2_descriptor_blocks_per_trans(journal_t *journal) -{ - int tag_space = journal->j_blocksize - sizeof(journal_header_t); - int tags_per_block; - - /* Subtract UUID */ - tag_space -= 16; - if (jbd2_journal_has_csum_v2or3(journal)) - tag_space -= sizeof(struct jbd2_journal_block_tail); - /* Commit code leaves a slack space of 16 bytes at the end of block */ - tags_per_block = (tag_space - 16) / journal_tag_bytes(journal); - /* - * Revoke descriptors are accounted separately so we need to reserve - * space for commit block and normal transaction descriptor blocks. - */ - return 1 + DIV_ROUND_UP(journal->j_max_transaction_buffers, - tags_per_block); -} - /* * jbd2_get_transaction: obtain a new transaction_t object. * @@ -109,7 +87,7 @@ static void jbd2_get_transaction(journal_t *journal, transaction->t_expires = jiffies + journal->j_commit_interval; atomic_set(&transaction->t_updates, 0); atomic_set(&transaction->t_outstanding_credits, - jbd2_descriptor_blocks_per_trans(journal) + + journal->j_transaction_overhead_buffers + atomic_read(&journal->j_reserved_credits)); atomic_set(&transaction->t_outstanding_revokes, 0); atomic_set(&transaction->t_handle_count, 0); diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index f91b930abe20..b900c642210c 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -1085,6 +1085,13 @@ struct journal_s */ int j_revoke_records_per_block; + /** + * @j_transaction_overhead: + * + * Number of blocks each transaction needs for its own bookkeeping + */ + int j_transaction_overhead_buffers; + /** * @j_commit_interval: * -- cgit From 27ba5b67312a944576addc4df44ac3b709aabede Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 24 Jun 2024 19:01:19 +0200 Subject: jbd2: avoid infinite transaction commit loop Commit 9f356e5a4f12 ("jbd2: Account descriptor blocks into t_outstanding_credits") started to account descriptor blocks into transactions outstanding credits. However it didn't appropriately decrease the maximum amount of credits available to userspace. Thus if the filesystem requests a transaction smaller than j_max_transaction_buffers but large enough that when descriptor blocks are added the size exceeds j_max_transaction_buffers, we confuse add_transaction_credits() into thinking previous handles have grown the transaction too much and enter infinite journal commit loop in start_this_handle() -> add_transaction_credits() trying to create transaction with enough credits available. Fix the problem by properly accounting for transaction space reserved for descriptor blocks when verifying requested transaction handle size. CC: stable@vger.kernel.org Fixes: 9f356e5a4f12 ("jbd2: Account descriptor blocks into t_outstanding_credits") Reported-by: Alexander Coffin Link: https://lore.kernel.org/all/CA+hUFcuGs04JHZ_WzA1zGN57+ehL2qmHOt5a7RMpo+rv6Vyxtw@mail.gmail.com Signed-off-by: Jan Kara Reviewed-by: Zhang Yi Link: https://patch.msgid.link/20240624170127.3253-3-jack@suse.cz Signed-off-by: Theodore Ts'o --- fs/jbd2/transaction.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index a095f1a3114b..66513c18ca29 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c @@ -191,6 +191,13 @@ static void sub_reserved_credits(journal_t *journal, int blocks) wake_up(&journal->j_wait_reserved); } +/* Maximum number of blocks for user transaction payload */ +static int jbd2_max_user_trans_buffers(journal_t *journal) +{ + return journal->j_max_transaction_buffers - + journal->j_transaction_overhead_buffers; +} + /* * Wait until we can add credits for handle to the running transaction. Called * with j_state_lock held for reading. Returns 0 if handle joined the running @@ -240,12 +247,12 @@ __must_hold(&journal->j_state_lock) * big to fit this handle? Wait until reserved credits are freed. */ if (atomic_read(&journal->j_reserved_credits) + total > - journal->j_max_transaction_buffers) { + jbd2_max_user_trans_buffers(journal)) { read_unlock(&journal->j_state_lock); jbd2_might_wait_for_commit(journal); wait_event(journal->j_wait_reserved, atomic_read(&journal->j_reserved_credits) + total <= - journal->j_max_transaction_buffers); + jbd2_max_user_trans_buffers(journal)); __acquire(&journal->j_state_lock); /* fake out sparse */ return 1; } @@ -285,14 +292,14 @@ __must_hold(&journal->j_state_lock) needed = atomic_add_return(rsv_blocks, &journal->j_reserved_credits); /* We allow at most half of a transaction to be reserved */ - if (needed > journal->j_max_transaction_buffers / 2) { + if (needed > jbd2_max_user_trans_buffers(journal) / 2) { sub_reserved_credits(journal, rsv_blocks); atomic_sub(total, &t->t_outstanding_credits); read_unlock(&journal->j_state_lock); jbd2_might_wait_for_commit(journal); wait_event(journal->j_wait_reserved, atomic_read(&journal->j_reserved_credits) + rsv_blocks - <= journal->j_max_transaction_buffers / 2); + <= jbd2_max_user_trans_buffers(journal) / 2); __acquire(&journal->j_state_lock); /* fake out sparse */ return 1; } @@ -322,12 +329,12 @@ static int start_this_handle(journal_t *journal, handle_t *handle, * size and limit the number of total credits to not exceed maximum * transaction size per operation. */ - if ((rsv_blocks > journal->j_max_transaction_buffers / 2) || - (rsv_blocks + blocks > journal->j_max_transaction_buffers)) { + if (rsv_blocks > jbd2_max_user_trans_buffers(journal) / 2 || + rsv_blocks + blocks > jbd2_max_user_trans_buffers(journal)) { printk(KERN_ERR "JBD2: %s wants too many credits " "credits:%d rsv_credits:%d max:%d\n", current->comm, blocks, rsv_blocks, - journal->j_max_transaction_buffers); + jbd2_max_user_trans_buffers(journal)); WARN_ON(1); return -ENOSPC; } -- cgit From 1cf5b024a3ffa479ed14e520f81549fce037f322 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 24 Jun 2024 19:01:20 +0200 Subject: jbd2: drop pointless shrinker batch initialization In jbd2_journal_init_common() we set batch size of a shrinker shrinking checkpointed buffers to journal->j_max_transaction_buffers. But that is guaranteed to be 0 at that point so we effectively stay with the default shrinker batch size of 128. It has been like this since introduction of jbd2 shrinkers so just drop the pointless initialization. Signed-off-by: Jan Kara Reviewed-by: Zhang Yi Link: https://patch.msgid.link/20240624170127.3253-4-jack@suse.cz Signed-off-by: Theodore Ts'o --- fs/jbd2/journal.c | 1 - 1 file changed, 1 deletion(-) diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index 2ae861abf770..25d373fac2f2 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -1617,7 +1617,6 @@ static journal_t *journal_init_common(struct block_device *bdev, journal->j_shrinker->scan_objects = jbd2_journal_shrink_scan; journal->j_shrinker->count_objects = jbd2_journal_shrink_count; - journal->j_shrinker->batch = journal->j_max_transaction_buffers; journal->j_shrinker->private_data = journal; shrinker_register(journal->j_shrinker); -- cgit From a794c9ad026f0a28044347f31929fcdb0270eadc Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 1 Jul 2024 15:28:00 +0200 Subject: jbd2: increase maximum transaction size Originally, we were quite conservative in limiting maximum transaction size to a quarter of the journal because we were not accounting transaction descriptor and revoke blocks. These days we do properly account them and reserve space for them from the total transaction credits. Thus there's no need to be so conservative and we can increase the maximum transaction size to one third of the journal (even half should work fine in principle but the performance will likely suffer in that case). This also fixes failures to grow filesystems with tiny journals. Link: CA+hUFcuGs04JHZ_WzA1zGN57+ehL2qmHOt5a7RMpo+rv6Vyxtw@mail.gmail.com Signed-off-by: Jan Kara Reviewed-by: Zhang Yi Link: https://patch.msgid.link/20240701132800.7158-1-jack@suse.cz Signed-off-by: Theodore Ts'o --- fs/jbd2/journal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index 25d373fac2f2..1ebf2393bfb7 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -1429,7 +1429,7 @@ static int journal_revoke_records_per_block(journal_t *journal) static int jbd2_journal_get_max_txn_bufs(journal_t *journal) { - return (journal->j_total_len - journal->j_fc_wbufsize) / 4; + return (journal->j_total_len - journal->j_fc_wbufsize) / 3; } /* -- cgit From 83f4414b8f84249d538905825b088ff3ae555652 Mon Sep 17 00:00:00 2001 From: Wojciech Gładysz Date: Wed, 3 Jul 2024 09:01:12 +0200 Subject: ext4: sanity check for NULL pointer after ext4_force_shutdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test case: 2 threads write short inline data to a file. In ext4_page_mkwrite the resulting inline data is converted. Handling ext4_grp_locked_error with description "block bitmap and bg descriptor inconsistent: X vs Y free clusters" calls ext4_force_shutdown. The conversion clears EXT4_STATE_MAY_INLINE_DATA but fails for ext4_destroy_inline_data_nolock and ext4_mark_iloc_dirty due to ext4_forced_shutdown. The restoration of inline data fails for the same reason not setting EXT4_STATE_MAY_INLINE_DATA. Without the flag set a regular process path in ext4_da_write_end follows trying to dereference page folio private pointer that has not been set. The fix calls early return with -EIO error shall the pointer to private be NULL. Sample crash report: Unable to handle kernel paging request at virtual address dfff800000000004 KASAN: null-ptr-deref in range [0x0000000000000020-0x0000000000000027] Mem abort info: ESR = 0x0000000096000005 EC = 0x25: DABT (current EL), IL = 32 bits SET = 0, FnV = 0 EA = 0, S1PTW = 0 FSC = 0x05: level 1 translation fault Data abort info: ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000 CM = 0, WnR = 0, TnD = 0, TagAccess = 0 GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [dfff800000000004] address between user and kernel address ranges Internal error: Oops: 0000000096000005 [#1] PREEMPT SMP Modules linked in: CPU: 1 PID: 20274 Comm: syz-executor185 Not tainted 6.9.0-rc7-syzkaller-gfda5695d692c #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/27/2024 pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : __block_commit_write+0x64/0x2b0 fs/buffer.c:2167 lr : __block_commit_write+0x3c/0x2b0 fs/buffer.c:2160 sp : ffff8000a1957600 x29: ffff8000a1957610 x28: dfff800000000000 x27: ffff0000e30e34b0 x26: 0000000000000000 x25: dfff800000000000 x24: dfff800000000000 x23: fffffdffc397c9e0 x22: 0000000000000020 x21: 0000000000000020 x20: 0000000000000040 x19: fffffdffc397c9c0 x18: 1fffe000367bd196 x17: ffff80008eead000 x16: ffff80008ae89e3c x15: 00000000200000c0 x14: 1fffe0001cbe4e04 x13: 0000000000000000 x12: 0000000000000000 x11: 0000000000000001 x10: 0000000000ff0100 x9 : 0000000000000000 x8 : 0000000000000004 x7 : 0000000000000000 x6 : 0000000000000000 x5 : fffffdffc397c9c0 x4 : 0000000000000020 x3 : 0000000000000020 x2 : 0000000000000040 x1 : 0000000000000020 x0 : fffffdffc397c9c0 Call trace: __block_commit_write+0x64/0x2b0 fs/buffer.c:2167 block_write_end+0xb4/0x104 fs/buffer.c:2253 ext4_da_do_write_end fs/ext4/inode.c:2955 [inline] ext4_da_write_end+0x2c4/0xa40 fs/ext4/inode.c:3028 generic_perform_write+0x394/0x588 mm/filemap.c:3985 ext4_buffered_write_iter+0x2c0/0x4ec fs/ext4/file.c:299 ext4_file_write_iter+0x188/0x1780 call_write_iter include/linux/fs.h:2110 [inline] new_sync_write fs/read_write.c:497 [inline] vfs_write+0x968/0xc3c fs/read_write.c:590 ksys_write+0x15c/0x26c fs/read_write.c:643 __do_sys_write fs/read_write.c:655 [inline] __se_sys_write fs/read_write.c:652 [inline] __arm64_sys_write+0x7c/0x90 fs/read_write.c:652 __invoke_syscall arch/arm64/kernel/syscall.c:34 [inline] invoke_syscall+0x98/0x2b8 arch/arm64/kernel/syscall.c:48 el0_svc_common+0x130/0x23c arch/arm64/kernel/syscall.c:133 do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:152 el0_svc+0x54/0x168 arch/arm64/kernel/entry-common.c:712 el0t_64_sync_handler+0x84/0xfc arch/arm64/kernel/entry-common.c:730 el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:598 Code: 97f85911 f94002da 91008356 d343fec8 (38796908) ---[ end trace 0000000000000000 ]--- ---------------- Code disassembly (best guess): 0: 97f85911 bl 0xffffffffffe16444 4: f94002da ldr x26, [x22] 8: 91008356 add x22, x26, #0x20 c: d343fec8 lsr x8, x22, #3 * 10: 38796908 ldrb w8, [x8, x25] <-- trapping instruction Reported-by: syzbot+18df508cf00a0598d9a6@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=18df508cf00a0598d9a6 Link: https://lore.kernel.org/all/000000000000f19a1406109eb5c5@google.com/T/ Signed-off-by: Wojciech Gładysz Link: https://patch.msgid.link/20240703070112.10235-1-wojciech.gladysz@infogain.com Signed-off-by: Theodore Ts'o --- fs/buffer.c | 2 ++ fs/ext4/inode.c | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/fs/buffer.c b/fs/buffer.c index 8c19e705b9c3..645f0387dfe1 100644 --- a/fs/buffer.c +++ b/fs/buffer.c @@ -2187,6 +2187,8 @@ static void __block_commit_write(struct folio *folio, size_t from, size_t to) struct buffer_head *bh, *head; bh = head = folio_buffers(folio); + if (!bh) + return; blocksize = bh->b_size; block_start = 0; diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index c81cf67d8e77..941c1c0d5c6e 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -3019,6 +3019,11 @@ static int ext4_da_do_write_end(struct address_space *mapping, bool disksize_changed = false; loff_t new_i_size; + if (unlikely(!folio_buffers(folio))) { + folio_unlock(folio); + folio_put(folio); + return -EIO; + } /* * block_write_end() will mark the inode as dirty with I_DIRTY_PAGES * flag, which all that's needed to trigger page writeback. -- cgit From 50ea741def587a64e08879ce6c6a30131f7111e7 Mon Sep 17 00:00:00 2001 From: Baokun Li Date: Tue, 2 Jul 2024 21:23:48 +0800 Subject: ext4: check dot and dotdot of dx_root before making dir indexed Syzbot reports a issue as follows: ============================================ BUG: unable to handle page fault for address: ffffed11022e24fe PGD 23ffee067 P4D 23ffee067 PUD 0 Oops: Oops: 0000 [#1] PREEMPT SMP KASAN PTI CPU: 0 PID: 5079 Comm: syz-executor306 Not tainted 6.10.0-rc5-g55027e689933 #0 Call Trace: make_indexed_dir+0xdaf/0x13c0 fs/ext4/namei.c:2341 ext4_add_entry+0x222a/0x25d0 fs/ext4/namei.c:2451 ext4_rename fs/ext4/namei.c:3936 [inline] ext4_rename2+0x26e5/0x4370 fs/ext4/namei.c:4214 [...] ============================================ The immediate cause of this problem is that there is only one valid dentry for the block to be split during do_split, so split==0 results in out of bounds accesses to the map triggering the issue. do_split unsigned split dx_make_map count = 1 split = count/2 = 0; continued = hash2 == map[split - 1].hash; ---> map[4294967295] The maximum length of a filename is 255 and the minimum block size is 1024, so it is always guaranteed that the number of entries is greater than or equal to 2 when do_split() is called. But syzbot's crafted image has no dot and dotdot in dir, and the dentry distribution in dirblock is as follows: bus dentry1 hole dentry2 free |xx--|xx-------------|...............|xx-------------|...............| 0 12 (8+248)=256 268 256 524 (8+256)=264 788 236 1024 So when renaming dentry1 increases its name_len length by 1, neither hole nor free is sufficient to hold the new dentry, and make_indexed_dir() is called. In make_indexed_dir() it is assumed that the first two entries of the dirblock must be dot and dotdot, so bus and dentry1 are left in dx_root because they are treated as dot and dotdot, and only dentry2 is moved to the new leaf block. That's why count is equal to 1. Therefore add the ext4_check_dx_root() helper function to add more sanity checks to dot and dotdot before starting the conversion to avoid the above issue. Reported-by: syzbot+ae688d469e36fb5138d0@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=ae688d469e36fb5138d0 Fixes: ac27a0ec112a ("[PATCH] ext4: initial copy of files from ext3") Cc: stable@kernel.org Signed-off-by: Baokun Li Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240702132349.2600605-2-libaokun@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/ext4/namei.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 5 deletions(-) diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index a630b27a4cc6..6917c1de8f41 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -2217,6 +2217,52 @@ static int add_dirent_to_buf(handle_t *handle, struct ext4_filename *fname, return err ? err : err2; } +static bool ext4_check_dx_root(struct inode *dir, struct dx_root *root) +{ + struct fake_dirent *fde; + const char *error_msg; + unsigned int rlen; + unsigned int blocksize = dir->i_sb->s_blocksize; + char *blockend = (char *)root + dir->i_sb->s_blocksize; + + fde = &root->dot; + if (unlikely(fde->name_len != 1)) { + error_msg = "invalid name_len for '.'"; + goto corrupted; + } + if (unlikely(strncmp(root->dot_name, ".", fde->name_len))) { + error_msg = "invalid name for '.'"; + goto corrupted; + } + rlen = ext4_rec_len_from_disk(fde->rec_len, blocksize); + if (unlikely((char *)fde + rlen >= blockend)) { + error_msg = "invalid rec_len for '.'"; + goto corrupted; + } + + fde = &root->dotdot; + if (unlikely(fde->name_len != 2)) { + error_msg = "invalid name_len for '..'"; + goto corrupted; + } + if (unlikely(strncmp(root->dotdot_name, "..", fde->name_len))) { + error_msg = "invalid name for '..'"; + goto corrupted; + } + rlen = ext4_rec_len_from_disk(fde->rec_len, blocksize); + if (unlikely((char *)fde + rlen >= blockend)) { + error_msg = "invalid rec_len for '..'"; + goto corrupted; + } + + return true; + +corrupted: + EXT4_ERROR_INODE(dir, "Corrupt dir, %s, running e2fsck is recommended", + error_msg); + return false; +} + /* * This converts a one block unindexed directory to a 3 block indexed * directory, and adds the dentry to the indexed directory. @@ -2251,17 +2297,17 @@ static int make_indexed_dir(handle_t *handle, struct ext4_filename *fname, brelse(bh); return retval; } + root = (struct dx_root *) bh->b_data; + if (!ext4_check_dx_root(dir, root)) { + brelse(bh); + return -EFSCORRUPTED; + } /* The 0th block becomes the root, move the dirents out */ fde = &root->dotdot; de = (struct ext4_dir_entry_2 *)((char *)fde + ext4_rec_len_from_disk(fde->rec_len, blocksize)); - if ((char *) de >= (((char *) root) + blocksize)) { - EXT4_ERROR_INODE(dir, "invalid rec_len for '..'"); - brelse(bh); - return -EFSCORRUPTED; - } len = ((char *) root) + (blocksize - csum_size) - (char *) de; /* Allocate new block for the 0th block's dirents */ -- cgit From f9ca51596bbfd0f9c386dd1c613c394c78d9e5e6 Mon Sep 17 00:00:00 2001 From: Baokun Li Date: Tue, 2 Jul 2024 21:23:49 +0800 Subject: ext4: make sure the first directory block is not a hole The syzbot constructs a directory that has no dirblock but is non-inline, i.e. the first directory block is a hole. And no errors are reported when creating files in this directory in the following flow. ext4_mknod ... ext4_add_entry // Read block 0 ext4_read_dirblock(dir, block, DIRENT) bh = ext4_bread(NULL, inode, block, 0) if (!bh && (type == INDEX || type == DIRENT_HTREE)) // The first directory block is a hole // But type == DIRENT, so no error is reported. After that, we get a directory block without '.' and '..' but with a valid dentry. This may cause some code that relies on dot or dotdot (such as make_indexed_dir()) to crash. Therefore when ext4_read_dirblock() finds that the first directory block is a hole report that the filesystem is corrupted and return an error to avoid loading corrupted data from disk causing something bad. Reported-by: syzbot+ae688d469e36fb5138d0@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=ae688d469e36fb5138d0 Fixes: 4e19d6b65fb4 ("ext4: allow directory holes") Cc: stable@kernel.org Signed-off-by: Baokun Li Reviewed-by: Jan Kara Link: https://patch.msgid.link/20240702132349.2600605-3-libaokun@huaweicloud.com Signed-off-by: Theodore Ts'o --- fs/ext4/namei.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 6917c1de8f41..1311ad0464b2 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -151,10 +151,11 @@ static struct buffer_head *__ext4_read_dirblock(struct inode *inode, return bh; } - if (!bh && (type == INDEX || type == DIRENT_HTREE)) { + /* The first directory block must not be a hole. */ + if (!bh && (type == INDEX || type == DIRENT_HTREE || block == 0)) { ext4_error_inode(inode, func, line, block, - "Directory hole found for htree %s block", - (type == INDEX) ? "index" : "leaf"); + "Directory hole found for htree %s block %u", + (type == INDEX) ? "index" : "leaf", block); return ERR_PTR(-EFSCORRUPTED); } if (!bh) @@ -3129,10 +3130,7 @@ bool ext4_empty_dir(struct inode *inode) EXT4_ERROR_INODE(inode, "invalid size"); return false; } - /* The first directory block must not be a hole, - * so treat it as DIRENT_HTREE - */ - bh = ext4_read_dirblock(inode, 0, DIRENT_HTREE); + bh = ext4_read_dirblock(inode, 0, EITHER); if (IS_ERR(bh)) return false; @@ -3577,10 +3575,7 @@ static struct buffer_head *ext4_get_first_dir_block(handle_t *handle, struct ext4_dir_entry_2 *de; unsigned int offset; - /* The first directory block must not be a hole, so - * treat it as DIRENT_HTREE - */ - bh = ext4_read_dirblock(inode, 0, DIRENT_HTREE); + bh = ext4_read_dirblock(inode, 0, EITHER); if (IS_ERR(bh)) { *retval = PTR_ERR(bh); return NULL; -- cgit