From 59ebc7fd74506367b109497eeef36034e648c943 Mon Sep 17 00:00:00 2001 From: Zheng Yongjun Date: Thu, 24 Dec 2020 21:22:44 +0800 Subject: [PATCH 1/8] ext4: use DEFINE_MUTEX() for mutex lock mutex lock can be initialized automatically with DEFINE_MUTEX() rather than explicitly calling mutex_init(). Signed-off-by: Zheng Yongjun Link: https://lore.kernel.org/r/20201224132244.30907-1-zhengyongjun3@huawei.com Signed-off-by: Theodore Ts'o --- fs/ext4/super.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 9a6f9875aa34..f361f9fb40d1 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -59,7 +59,7 @@ #include static struct ext4_lazy_init *ext4_li_info; -static struct mutex ext4_li_mtx; +static DEFINE_MUTEX(ext4_li_mtx); static struct ratelimit_state ext4_mount_msg_ratelimit; static int ext4_load_journal(struct super_block *, struct ext4_super_block *, @@ -6667,7 +6667,6 @@ static int __init ext4_init_fs(void) ratelimit_state_init(&ext4_mount_msg_ratelimit, 30 * HZ, 64); ext4_li_info = NULL; - mutex_init(&ext4_li_mtx); /* Build-time check for flags consistency */ ext4_check_flag_values(); From 027f14f5357279655c3ebc6d14daff8368d4f53f Mon Sep 17 00:00:00 2001 From: Theodore Ts'o Date: Thu, 21 Jan 2021 12:33:20 -0500 Subject: [PATCH 2/8] ext4: don't try to processed freed blocks until mballoc is initialized If we try to make any changes via the journal between when the journal is initialized, but before the multi-block allocated is initialized, we will end up deferencing a NULL pointer when the journal commit callback function calls ext4_process_freed_data(). The proximate cause of this failure was commit 2d01ddc86606 ("ext4: save error info to sb through journal if available") since file system corruption problems detected before the call to ext4_mb_init() would result in a journal commit before we aborted the mount of the file system.... and we would then trigger the NULL pointer deref. Link: https://lore.kernel.org/r/YAm8qH/0oo2ofSMR@mit.edu Reported-by: Murphy Zhou Reviewed-by: Jan Kara Signed-off-by: Theodore Ts'o --- fs/ext4/super.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index f361f9fb40d1..071d131fadd8 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -4875,7 +4875,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) set_task_ioprio(sbi->s_journal->j_task, journal_ioprio); - sbi->s_journal->j_commit_callback = ext4_journal_commit_callback; sbi->s_journal->j_submit_inode_data_buffers = ext4_journal_submit_inode_data_buffers; sbi->s_journal->j_finish_inode_data_buffers = @@ -4987,6 +4986,14 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) goto failed_mount5; } + /* + * We can only set up the journal commit callback once + * mballoc is initialized + */ + if (sbi->s_journal) + sbi->s_journal->j_commit_callback = + ext4_journal_commit_callback; + block = ext4_count_free_clusters(sb); ext4_free_blocks_count_set(sbi->s_es, EXT4_C2B(sbi, block)); From 96e7c02d0bbca9f33fb4175e5f302dab1de6f985 Mon Sep 17 00:00:00 2001 From: Daejun Park Date: Mon, 11 Jan 2021 10:37:26 +0900 Subject: [PATCH 3/8] ext4: Change list_for_each* to list_for_each_entry* In the fast_commit.c, list_for_each* + list_entry can be changed to list_for_each_entry*. It reduces number of variables and lines. Signed-off-by: Daejun Park Link: https://lore.kernel.org/r/20210111013726epcms2p4579ae56040d7043db785bf0d0a785dc7@epcms2p4 Signed-off-by: Theodore Ts'o --- fs/ext4/fast_commit.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c index 0a14a7c87bf8..619412134bbf 100644 --- a/fs/ext4/fast_commit.c +++ b/fs/ext4/fast_commit.c @@ -915,13 +915,11 @@ static int ext4_fc_submit_inode_data_all(journal_t *journal) struct super_block *sb = (struct super_block *)(journal->j_private); struct ext4_sb_info *sbi = EXT4_SB(sb); struct ext4_inode_info *ei; - struct list_head *pos; int ret = 0; spin_lock(&sbi->s_fc_lock); ext4_set_mount_flag(sb, EXT4_MF_FC_COMMITTING); - list_for_each(pos, &sbi->s_fc_q[FC_Q_MAIN]) { - ei = list_entry(pos, struct ext4_inode_info, i_fc_list); + list_for_each_entry(ei, &sbi->s_fc_q[FC_Q_MAIN], i_fc_list) { ext4_set_inode_state(&ei->vfs_inode, EXT4_STATE_FC_COMMITTING); while (atomic_read(&ei->i_fc_updates)) { DEFINE_WAIT(wait); @@ -978,17 +976,15 @@ __releases(&sbi->s_fc_lock) { struct super_block *sb = (struct super_block *)(journal->j_private); struct ext4_sb_info *sbi = EXT4_SB(sb); - struct ext4_fc_dentry_update *fc_dentry; + struct ext4_fc_dentry_update *fc_dentry, *fc_dentry_n; struct inode *inode; - struct list_head *pos, *n, *fcd_pos, *fcd_n; - struct ext4_inode_info *ei; + struct ext4_inode_info *ei, *ei_n; int ret; if (list_empty(&sbi->s_fc_dentry_q[FC_Q_MAIN])) return 0; - list_for_each_safe(fcd_pos, fcd_n, &sbi->s_fc_dentry_q[FC_Q_MAIN]) { - fc_dentry = list_entry(fcd_pos, struct ext4_fc_dentry_update, - fcd_list); + list_for_each_entry_safe(fc_dentry, fc_dentry_n, + &sbi->s_fc_dentry_q[FC_Q_MAIN], fcd_list) { if (fc_dentry->fcd_op != EXT4_FC_TAG_CREAT) { spin_unlock(&sbi->s_fc_lock); if (!ext4_fc_add_dentry_tlv( @@ -1004,8 +1000,8 @@ __releases(&sbi->s_fc_lock) } inode = NULL; - list_for_each_safe(pos, n, &sbi->s_fc_q[FC_Q_MAIN]) { - ei = list_entry(pos, struct ext4_inode_info, i_fc_list); + list_for_each_entry_safe(ei, ei_n, &sbi->s_fc_q[FC_Q_MAIN], + i_fc_list) { if (ei->vfs_inode.i_ino == fc_dentry->fcd_ino) { inode = &ei->vfs_inode; break; @@ -1057,7 +1053,6 @@ static int ext4_fc_perform_commit(journal_t *journal) struct ext4_sb_info *sbi = EXT4_SB(sb); struct ext4_inode_info *iter; struct ext4_fc_head head; - struct list_head *pos; struct inode *inode; struct blk_plug plug; int ret = 0; @@ -1099,8 +1094,7 @@ static int ext4_fc_perform_commit(journal_t *journal) goto out; } - list_for_each(pos, &sbi->s_fc_q[FC_Q_MAIN]) { - iter = list_entry(pos, struct ext4_inode_info, i_fc_list); + list_for_each_entry(iter, &sbi->s_fc_q[FC_Q_MAIN], i_fc_list) { inode = &iter->vfs_inode; if (!ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING)) continue; @@ -1226,9 +1220,8 @@ static void ext4_fc_cleanup(journal_t *journal, int full) { struct super_block *sb = journal->j_private; struct ext4_sb_info *sbi = EXT4_SB(sb); - struct ext4_inode_info *iter; + struct ext4_inode_info *iter, *iter_n; struct ext4_fc_dentry_update *fc_dentry; - struct list_head *pos, *n; if (full && sbi->s_fc_bh) sbi->s_fc_bh = NULL; @@ -1236,8 +1229,8 @@ static void ext4_fc_cleanup(journal_t *journal, int full) jbd2_fc_release_bufs(journal); spin_lock(&sbi->s_fc_lock); - list_for_each_safe(pos, n, &sbi->s_fc_q[FC_Q_MAIN]) { - iter = list_entry(pos, struct ext4_inode_info, i_fc_list); + list_for_each_entry_safe(iter, iter_n, &sbi->s_fc_q[FC_Q_MAIN], + i_fc_list) { list_del_init(&iter->i_fc_list); ext4_clear_inode_state(&iter->vfs_inode, EXT4_STATE_FC_COMMITTING); From c6c818e50d1982ebac4dcd3ae18c1b49a66ebacb Mon Sep 17 00:00:00 2001 From: Vinicius Tinti Date: Tue, 2 Feb 2021 16:28:37 +0000 Subject: [PATCH 4/8] ext4: factor out htree rep invariant check This patch moves some debugging code which is used to validate the hash tree node when doing a binary search of an htree node into a separate function, which is disabled by default (since it is only used by developers when they are modifying the htree code paths). In addition to cleaning up the code to make it more maintainable, it silences a Clang compiler warning when -Wunreachable-code-aggressive is enabled. (There is no plan to enable this warning by default, since there it has far too many false positives; nevertheless, this commit reduces one of the many false positives by one.) Signed-off-by: Vinicius Tinti Link: https://lore.kernel.org/r/20210202162837.129631-1-viniciustinti@gmail.com Signed-off-by: Theodore Ts'o --- fs/ext4/namei.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index cf652ba3e74d..a6e28b4b5a95 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -731,6 +731,29 @@ struct stats dx_show_entries(struct dx_hash_info *hinfo, struct inode *dir, (space/bcount)*100/blocksize); return (struct stats) { names, space, bcount}; } + +/* + * Linear search cross check + */ +static inline void htree_rep_invariant_check(struct dx_entry *at, + struct dx_entry *target, + u32 hash, unsigned int n) +{ + while (n--) { + dxtrace(printk(KERN_CONT ",")); + if (dx_get_hash(++at) > hash) { + at--; + break; + } + } + ASSERT(at == target - 1); +} +#else /* DX_DEBUG */ +static inline void htree_rep_invariant_check(struct dx_entry *at, + struct dx_entry *target, + u32 hash, unsigned int n) +{ +} #endif /* DX_DEBUG */ /* @@ -827,20 +850,7 @@ dx_probe(struct ext4_filename *fname, struct inode *dir, p = m + 1; } - if (0) { // linear search cross check - unsigned n = count - 1; - at = entries; - while (n--) - { - dxtrace(printk(KERN_CONT ",")); - if (dx_get_hash(++at) > hash) - { - at--; - break; - } - } - ASSERT(at == p - 1); - } + htree_rep_invariant_check(entries, p, hash, count - 1); at = p - 1; dxtrace(printk(KERN_CONT " %x->%u\n", From b5776e7524afbd4569978ff790864755c438bba7 Mon Sep 17 00:00:00 2001 From: Theodore Ts'o Date: Thu, 4 Feb 2021 00:05:20 -0500 Subject: [PATCH 5/8] ext4: fix potential htree index checksum corruption In the case where we need to do an interior node split, and immediately afterwards, we are unable to allocate a new directory leaf block due to ENOSPC, the directory index checksum's will not be filled in correctly (and indeed, will not be correctly journalled). This looks like a bug that was introduced when we added largedir support. The original code doesn't make any sense (and should have been caught in code review), but it was hidden because most of the time, the index node checksum will be set by do_split(). But if do_split bails out due to ENOSPC, then ext4_handle_dirty_dx_node() won't get called, and so the directory index checksum field will not get set, leading to: EXT4-fs error (device sdb): dx_probe:858: inode #6635543: block 4022: comm nfsd: Directory index failed checksum Google-Bug-Id: 176345532 Fixes: e08ac99fa2a2 ("ext4: add largedir feature") Cc: Artem Blagodarenko Signed-off-by: Theodore Ts'o --- fs/ext4/namei.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index a6e28b4b5a95..115762180801 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -2411,11 +2411,10 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname, (frame - 1)->bh); if (err) goto journal_error; - if (restart) { - err = ext4_handle_dirty_dx_node(handle, dir, - frame->bh); + err = ext4_handle_dirty_dx_node(handle, dir, + frame->bh); + if (err) goto journal_error; - } } else { struct dx_root *dxroot; memcpy((char *) entries2, (char *) entries, From 3258386aba670e3406a499d2d0b7395e14c8d097 Mon Sep 17 00:00:00 2001 From: Eric Whitney Date: Wed, 13 Jan 2021 17:14:03 -0500 Subject: [PATCH 6/8] ext4: reset retry counter when ext4_alloc_file_blocks() makes progress Change the retry policy in ext4_alloc_file_blocks() to allow for a full retry cycle whenever a portion of an allocation request has been fulfilled. A large allocation request often results in multiple calls to ext4_map_blocks(), each of which is potentially subject to a temporary ENOSPC condition and retry cycle. The current code only allows for a single retry cycle. This patch does not address a known bug or reported complaint. However, it should make block allocation for fallocate and zero range more robust. In addition, simplify the conditional controlling the allocation while loop, where testing len alone is sufficient. Remove the assignment to ret2 in the error path after the call to ext4_map_blocks() since its value isn't subsequently used. Signed-off-by: Eric Whitney Link: https://lore.kernel.org/r/20210113221403.18258-1-enwlinux@gmail.com Signed-off-by: Theodore Ts'o --- fs/ext4/extents.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index 3960b7ec3ab7..77c7c8a54da7 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -4382,8 +4382,7 @@ static int ext4_alloc_file_blocks(struct file *file, ext4_lblk_t offset, { struct inode *inode = file_inode(file); handle_t *handle; - int ret = 0; - int ret2 = 0, ret3 = 0; + int ret, ret2 = 0, ret3 = 0; int retries = 0; int depth = 0; struct ext4_map_blocks map; @@ -4408,7 +4407,7 @@ static int ext4_alloc_file_blocks(struct file *file, ext4_lblk_t offset, depth = ext_depth(inode); retry: - while (ret >= 0 && len) { + while (len) { /* * Recalculate credits when extent tree depth changes. */ @@ -4430,9 +4429,13 @@ static int ext4_alloc_file_blocks(struct file *file, ext4_lblk_t offset, inode->i_ino, map.m_lblk, map.m_len, ret); ext4_mark_inode_dirty(handle, inode); - ret2 = ext4_journal_stop(handle); + ext4_journal_stop(handle); break; } + /* + * allow a full retry cycle for any remaining allocations + */ + retries = 0; map.m_lblk += ret; map.m_len = len = len - ret; epos = (loff_t)map.m_lblk << inode->i_blkbits; @@ -4450,11 +4453,8 @@ static int ext4_alloc_file_blocks(struct file *file, ext4_lblk_t offset, if (unlikely(ret2)) break; } - if (ret == -ENOSPC && - ext4_should_retry_alloc(inode->i_sb, &retries)) { - ret = 0; + if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries)) goto retry; - } return ret > 0 ? ret2 : ret; } From 302fdadeafe4be539f247abf25f61822e4a5a577 Mon Sep 17 00:00:00 2001 From: Geert Uytterhoeven Date: Fri, 22 Jan 2021 12:02:34 +0100 Subject: [PATCH 7/8] ext: EXT4_KUNIT_TESTS should depend on EXT4_FS instead of selecting it EXT4_KUNIT_TESTS selects EXT4_FS, thus enabling an optional feature the user may not want to enable. Fix this by making the test depend on EXT4_FS instead. Fixes: 1cbeab1b242d16fd ("ext4: add kunit test for decoding extended timestamps") Reviewed-by: Randy Dunlap Signed-off-by: Geert Uytterhoeven Link: https://lore.kernel.org/r/20210122110234.2825685-1-geert@linux-m68k.org Signed-off-by: Theodore Ts'o --- fs/ext4/Kconfig | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/fs/ext4/Kconfig b/fs/ext4/Kconfig index 619dd35ddd48..86699c8cab28 100644 --- a/fs/ext4/Kconfig +++ b/fs/ext4/Kconfig @@ -103,8 +103,7 @@ config EXT4_DEBUG config EXT4_KUNIT_TESTS tristate "KUnit tests for ext4" if !KUNIT_ALL_TESTS - select EXT4_FS - depends on KUNIT + depends on EXT4_FS && KUNIT default KUNIT_ALL_TESTS help This builds the ext4 KUnit tests. From 0a76945fd1ba2ab44da7b578b311efdfedf92e6c Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Tue, 9 Feb 2021 17:32:06 -0800 Subject: [PATCH 8/8] ext4: add .kunitconfig fragment to enable ext4-specific tests As of [1], we no longer want EXT4_KUNIT_TESTS and others to `select` their deps. This means it can get harder to get all the right things selected as we gain more tests w/ more deps over time. This patch (and [2]) proposes we store kunitconfig fragments in-tree to represent sets of tests. (N.B. right now we only have one ext4 test). There's still a discussion to be had about how to have a hierarchy of these files (e.g. if one wanted to test all of fs/, not just fs/ext4). But this fragment would likely be a leaf node and isn't blocked on deciding if we want `import` statements and the like. Usage ===== Before [2] (on its way to being merged): $ cp fs/ext4/.kunitconfig .kunit/ $ ./tools/testing/kunit/kunit.py run After [2]: $ ./tools/testing/kunit/kunit.py run --kunitconfig=fs/ext4/.kunitconfig ".kunitconfig" vs "kunitconfig" =============================== See also: commit 14ee5cfd4512 ("kunit: Rename 'kunitconfig' to '.kunitconfig'"). * The bit about .gitignore exluding it by default is now a con, however. * But there are a lot of directories with files that begin with "k" and so this could cause some annoyance w/ tab completion* * This is the name kunit.py expects right now, so some people are used to .kunitconfig over "kunitconfig" [1] https://lore.kernel.org/linux-ext4/20210122110234.2825685-1-geert@linux-m68k.org/ [2] https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=kunit&id=243180f5924ed27ea417db39feb7f9691777688e * 372/5556 directories isn't too much, but still not a small number: $ find -type f -name 'k*' | xargs dirname | sort -u | wc -l 372 Signed-off-by: Daniel Latypov Link: https://lore.kernel.org/r/20210210013206.136227-1-dlatypov@google.com Signed-off-by: Theodore Ts'o --- fs/ext4/.kunitconfig | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 fs/ext4/.kunitconfig diff --git a/fs/ext4/.kunitconfig b/fs/ext4/.kunitconfig new file mode 100644 index 000000000000..bf51da7cd9fc --- /dev/null +++ b/fs/ext4/.kunitconfig @@ -0,0 +1,3 @@ +CONFIG_KUNIT=y +CONFIG_EXT4_FS=y +CONFIG_EXT4_KUNIT_TESTS=y