bug with large_dir in 5.12.17

There is a bug when enabling large_dir in 5.12.17. I got this during a backup:

index full, reach max htree level :2
Large directory feature is not enabled on this filesystem

So I unmounted, ran tune2fs -O large_dir /dev/device and mounted again. However this error appeared:

dx_probe:864: inode #576594294: block 144245: comm rsync: directory leaf block found instead of index block

I unmounted, ran fsck and it "salvaged" a bunch of directories. However at the next backup run the same errors appeared again.

This is with vanilla 5.2.17.

Hi Carlos,
are you able to reproduce this error on a new directory that did not hit the 2-level htree limit before enabling large_dir, or did you only see this with directories that hit the 2-level htree limit before the update? Did you test on any newer kernels than 5.2.17?

Cheers, Andreas

Andreas Dilger ([email protected]) wrote on Tue, Jul 27, 2021 at 03:22:10AM -03:

I removed all directories where the error happens (several hours...) and ran the backup again, after fsck, and the error was the same, so it also happens in new ones.

Not yet. The machine is running 5.13.5 now but this particular backup hasn't run yet. I'm doing fsck now (takes 4h30) and will launch it again.

To Andreas Dilger ([email protected]) wrote on Tue, Jul 27, 2021 at 04:07:06PM -03:

The same happens with 5.13.5 after fsck. I didn't check with new directories, just ran rsync.

Hello Andreas,

This issue is reproducible in case the “large_dir” option is set initially. The partition is fresh, created just before the test started.

1560000 names created
ln: failed to create hard link 'n0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001571063': File exists
ln: failed to access 'n0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001571384': Structure needs cleaning
ln: failed to access 'n0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001572335': Structure needs cleaning
ln: failed to access 'n0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001573095': Structure needs cleaning
ln: failed to access 'n0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001574809': Structure needs cleaning
ln: failed to access 'n0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001576070': Structure needs cleaning

Best regards,
Artem Blagodarenko

Hello,

I have reproduced the issue on the master head

# git rev-parse HEAD
f158f8962ed7e884fa168f354c488f3afa3eb6db

Here is a reproducer. Not perfect bash script, but reproduce the problem well

#!/bin/bash
dir_path=/mnt/ext4/
cd $dir_path
i=0
target_file=$dir_path/foofile$i
touch $target_file
while test $i -lt 100000000 ; do

        if test $((i % 40000)) -eq 0; then
                echo "$i names created"
                target_file=$dir_path/foofile$i
                touch $target_file
        fi
    lncom=`printf "ln $target_file n%0253u\n" $i`
    `$lncom`
    i=$((i + 1))

done

Without large_dir option 1504565 files created and then I got no space

1480000 names created
ln: failed to create hard link 'n0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001504566': No space left on device
ln: failed to create hard link

[root@CO82 ~]# ls /mnt/ext4/ | wc -l
1510070
[root@CO82 ~]#

When I enabled large_dir and used the same script, but started from I=1600000

Some amount (36526) of names were created and finally this error happened.

sh ~/filldir.sh
1600000 names created
ln: failed to create hard link 'n0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001636527' => '/mnt/ext4//foofile1600000': Structure needs cleaning
ln: failed to access 'n0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001636797': Structure needs cleaning

From dimes

[27927.219844] EXT4-fs (loop1): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
[28058.315171] EXT4-fs error (device loop1): dx_probe:887: inode #2: block 147359: comm ln: directory leaf block found instead of index block
[28059.414053] EXT4-fs error (device loop1): dx_probe:887: inode #2: block 147359: comm ln: directory leaf block found instead of index block
[28059.627589] EXT4-fs error (device loop1): dx_probe:887: inode #2: block 147359: comm ln: directory leaf block found instead of index block

the 2-level htree limit before enabling large_dir, or did you only see this with directories that hit the 2-level htree limit before the update?

I am executing this test now. Will report shortly.

Best regards,
Artem Blagodarenko

Hello,

It looks like the fix b5776e7524afbd4569978ff790864755c438bba7 "ext4: fix potential htree index checksum corruption” introduced this regression. I reverted it and my test from previous message passed the dangerous level of 1570000 names count.
Now test is still in progress. 2520000 names are already created.

I am searching the way to fix this.

Best regards,
Artem Blagodarenko.

I'm not sure why your e-mails are getting dropped by vger.kernel.org. I will note that voxelsoft.com appears to be on the a barracuda spam blacklist:

https://mxtoolbox.com/SuperTool.aspx?action=blacklist%3avoxelsoft.com&run=toolpage

I've looked your analysis, and while I appreciate your goals:

* a naïve approach would be to reinstate the missing goto, but
1. such approach is likely to contribute to another mistake in the future
2. goto considered harmful
* instead refactor the outer scope to handle both restart conditions (goto agnostic)

I've looked at your proposed fix, and I think a simpler fix might be:

index 5fd56f616cf0..f3bbcd4efb56 100644
@@ -2517,7 +2517,7 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
 				goto journal_error;
 			err = ext4_handle_dirty_dx_node(handle, dir,
 							frame->bh);
-			if (err)
+			if (restart || err)
 				goto journal_error;
 		} else {
 			struct dx_root *dxroot;

It is similarly "goto agnostic" in that it doesn't actually add (or remove) any gotos.

I don't think your proposed fix improves the maintainability or understandability for the code, so my bias would be to go for the simpler fix.

If we were going to clean up the code, the main issues that I'd want to address are:

*) Goto statements aren't necessarilyy evil; using "goto errout" is
pretty common idiom, and I don't have a problem with that. Occasionally having a "goto again" when we need to retry some operation can actually be the cleanest way to code something. But the combination of the two --- when "goto cleanup" and "goto journal_error" is used for both purposes --- is super confusing, and that is caused the bug in the first place.

So for example, this diff increase the goto count by one, but the code is much more readable, because each goto and goto label do exactly one thing:

 fs/ext4/namei.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

index 5fd56f616cf0..58fca88d7459 100644
@@ -2398,14 +2398,15 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
 {
 	struct dx_frame frames[EXT4_HTREE_LEVEL], *frame;
 	struct dx_entry *entries, *at;
-	struct buffer_head *bh;
+	struct buffer_head *bh = NULL;
 	struct super_block *sb = dir->i_sb;
 	struct ext4_dir_entry_2 *de;
-	int restart;
 	int err;
 
-again:
-	restart = 0;
+	frames[0].bh = NULL;
+recalc_htree_path:
+	brelse(bh);
+	dx_release(frames);
 	frame = dx_probe(fname, dir, NULL, frames);
 	if (IS_ERR(frame))
 		return PTR_ERR(frame);

@@ -2436,7 +2437,7 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
 		ext4_lblk_t newblock;
 		int levels = frame - frames + 1;
 		unsigned int icount;
-		int add_level = 1;
+		int add_level = 1, restart = 0;
 		struct dx_entry *entries2;
 		struct dx_node *node2;
 		struct buffer_head *bh2;

@@ -2508,9 +2509,9 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
 			dxtrace(dx_show_index("node",
 			       ((struct dx_node *) bh2->b_data)->entries));
 			err = ext4_handle_dirty_dx_node(handle, dir, bh2);
+			brelse (bh2);
 			if (err)
 				goto journal_error;
-			brelse (bh2);
 			err = ext4_handle_dirty_dx_node(handle, dir,
 						   (frame - 1)->bh);
 			if (err)

@@ -2519,6 +2520,8 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
 							frame->bh);
 			if (err)
 				goto journal_error;
+			if (restart)
+				goto recalc_htree_path;
 		} else {
 			struct dx_root *dxroot;
 			memcpy((char *) entries2, (char *) entries,

@@ -2538,8 +2541,9 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
 				goto journal_error;
 			err = ext4_handle_dirty_dx_node(handle, dir, bh2);
 			brelse(bh2);
-			restart = 1;
-			goto journal_error;
+			if (err)
+				goto journal_error;
+			goto recalc_htree_path;
 		}
 	}
 	de = do_split(handle, dir, &bh, frame, &fname->hinfo);

@@ -2555,11 +2559,6 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
 cleanup:
 	brelse(bh);
 	dx_release(frames);
-	/* @restart is true means htree-path has been changed, we need to
-	 * repeat dx_probe() to find out valid htree-path
-	 */
-	if (restart && err == 0)
-		goto again;
 	return err;
 }
 

*) The ext4_dx_add_entry() function is just too big. It was
borderline unwieldy before we added the large_dir feature, and that large_dir feature just tip it over the line. We probably should to refactor separate functions for "do_split_index()" and "add_htree_level()".

*) The control avariables "add_level" and "restart" are an additional
cause of complexity. It may be that refactoring some of the separate functions, and simply forcing that the hree path get recalculated if an index node is split or the tree depth increases, would allow us to get rid of these two variables.

Denis, Artem, what do you think?

- Ted

Hi Artem, did you have a chance to take a look at some of the possible fixes which I floated on this thread?

Do you have any objections if I take this and send it to Linus?

Thanks,

- Ted

From fa8db30806b4e83981c65f18f98de33f804012d9 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <[email protected]>
Date: Wed, 4 Aug 2021 14:23:55 -0400
Subject: [PATCH] ext4: fix potential htree correuption when growing large_dir
directories
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Commit b5776e7524af ("ext4: fix potential htree index checksum corruption) removed a required restart when multiple levels of index nodes need to be split. Fix this to avoid directory htree corruptions when using the large_dir feature.

---
 fs/ext4/namei.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 5fd56f616cf0..f3bbcd4efb56 100644
@@ -2517,7 +2517,7 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
 				goto journal_error;
 			err = ext4_handle_dirty_dx_node(handle, dir,
 							frame->bh);
-			if (err)
+			if (restart || err)
 				goto journal_error;
 		} else {
 			struct dx_root *dxroot;

2.31.0

Hello Teodore,

Your one-line fix looks good.

I have tested it. 1560000 names created successfully.

But the patch with refactoring doesn’t work. I got this messages

1480000 names created
1520000 names created
ln: failed to access 'n0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001520519': Bad message
ln: failed to access 'n0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001520520': Bad message

[ 7699.212018] EXT4-fs error (device loop0): dx_probe:887: inode #2: block 144843: comm ln: Directory index failed checksum
[ 7699.216001] EXT4-fs error (device loop0): dx_probe:887: inode #2: block 144843: comm ln: Directory index failed checksum

I have no objections to send your one-line fix, but we need to double check refactoring.

Best regards,
Artem Blagodarenko

Thanks for testing both patches. I was more intersted in the one-line fix so I hadn't tried testing the refactoring patch. That's for a later cleanup.

In any case, I've come up with a test to automate testing and to make sure we don't regress in the future. It runs pretty quickly, for either failure or success, so it's good as a smoke test. We may want to consider a longer stress test as well. Artem, do you have any suggestions?

- Ted

commit 5c156367952b22431850dfad8b2b2c8b753a3e91

Author: Theodore Ts'o <[email protected]>
Date:   Thu Aug 5 09:49:40 2021 -0400

ext4: add test to validate the large_dir feature

index 615cb6e3..ec28d643 100644
@@ -16,6 +16,7 @@ int verbose;
 int pid;
 
 int checkflag=0;
+int create_only=0;
 
 #define MKNOD_DEV 0
 

@@ -51,7 +52,7 @@ main(
 	nprocs_per_dir = 1;
 	keep = 0;
         verbose = 0;
-	while ((c = getopt(argc, argv, "d:p:f:s:n:kvc")) != EOF) {
+	while ((c = getopt(argc, argv, "d:p:f:s:n:kvcC")) != EOF) {
 		switch(c) {
 			case 'p':
 				nprocs = atoi(optarg);

@@ -80,6 +81,9 @@ main(
 			case 'c':
                                 checkflag++;
                                 break;
+			case 'C':
+				create_only++;
+				break;
 		}
 	}
 	if (errflg || (dirname == NULL)) {

@@ -170,6 +174,7 @@ dirstress(
 	if (create_entries(nfiles)) {
             printf("!! [%d] create failed\n", pid);
         } else {
+	    if (create_only) return 0;
             if (verbose) fprintf(stderr,"** [%d] scramble entries\n", pid);
 	    if (scramble_entries(nfiles)) {
                 printf("!! [%d] scramble failed\n", pid);

new file mode 100644
index 00000000..32f74d89
@@ -0,0 +1,2 @@
+QA output created by 051
+Silence is golden