All of lore.kernel.org
 help / color / mirror / Atom feed
* bug with large_dir in 5.12.17
@ 2021-07-22 14:23 Carlos Carvalho
  2021-07-27  6:22 ` Andreas Dilger
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Carlos Carvalho @ 2021-07-22 14:23 UTC (permalink / raw)
  To: linux-ext4

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: bug with large_dir in 5.12.17
  2021-07-22 14:23 bug with large_dir in 5.12.17 Carlos Carvalho
@ 2021-07-27  6:22 ` Andreas Dilger
  2021-07-27 19:07   ` Carlos Carvalho
  2021-07-28 16:07   ` Благодаренко Артём
  2021-07-28 13:56 ` Благодаренко Артём
  2021-07-29 19:23 ` Благодаренко Артём
  2 siblings, 2 replies; 11+ messages in thread
From: Andreas Dilger @ 2021-07-27  6:22 UTC (permalink / raw)
  To: Carlos Carvalho; +Cc: linux-ext4

[-- Attachment #1: Type: text/plain, Size: 979 bytes --]

On Jul 22, 2021, at 8:23 AM, Carlos Carvalho <carlos@fisica.ufpr.br> wrote:
> 
> 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






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: bug with large_dir in 5.12.17
  2021-07-27  6:22 ` Andreas Dilger
@ 2021-07-27 19:07   ` Carlos Carvalho
  2021-07-28  0:38     ` Carlos Carvalho
  2021-07-28 16:07   ` Благодаренко Артём
  1 sibling, 1 reply; 11+ messages in thread
From: Carlos Carvalho @ 2021-07-27 19:07 UTC (permalink / raw)
  To: Andreas Dilger; +Cc: linux-ext4

Andreas Dilger (adilger@dilger.ca) wrote on Tue, Jul 27, 2021 at 03:22:10AM -03:
> On Jul 22, 2021, at 8:23 AM, Carlos Carvalho <carlos@fisica.ufpr.br> wrote:
> > 
> > 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?

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.

> Did you test on any newer kernels than 5.2.17?

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: bug with large_dir in 5.12.17
  2021-07-27 19:07   ` Carlos Carvalho
@ 2021-07-28  0:38     ` Carlos Carvalho
  0 siblings, 0 replies; 11+ messages in thread
From: Carlos Carvalho @ 2021-07-28  0:38 UTC (permalink / raw)
  To: Andreas Dilger; +Cc: linux-ext4

To Andreas Dilger (carlos@fisica.ufpr.br) wrote on Tue, Jul 27, 2021 at 04:07:06PM -03:
> > Did you test on any newer kernels than 5.2.17?
> 
> 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.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: bug with large_dir in 5.12.17
  2021-07-22 14:23 bug with large_dir in 5.12.17 Carlos Carvalho
  2021-07-27  6:22 ` Andreas Dilger
@ 2021-07-28 13:56 ` Благодаренко Артём
  2021-07-29 19:23 ` Благодаренко Артём
  2 siblings, 0 replies; 11+ messages in thread
From: Благодаренко Артём @ 2021-07-28 13:56 UTC (permalink / raw)
  To: Carlos Carvalho; +Cc: linux-ext4

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

>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?

I am executing this test now. Will report shortly.

Best regards,
Artem Blagodarenko
> On 22 Jul 2021, at 17:23, Carlos Carvalho <carlos@fisica.ufpr.br> wrote:
> 
> 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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: bug with large_dir in 5.12.17
  2021-07-27  6:22 ` Andreas Dilger
  2021-07-27 19:07   ` Carlos Carvalho
@ 2021-07-28 16:07   ` Благодаренко Артём
  1 sibling, 0 replies; 11+ messages in thread
From: Благодаренко Артём @ 2021-07-28 16:07 UTC (permalink / raw)
  To: Andreas Dilger; +Cc: Carlos Carvalho, linux-ext4

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


> On 27 Jul 2021, at 09:22, Andreas Dilger <adilger@dilger.ca> wrote:
> 
> On Jul 22, 2021, at 8:23 AM, Carlos Carvalho <carlos@fisica.ufpr.br> wrote:
>> 
>> 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
> 
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: bug with large_dir in 5.12.17
  2021-07-22 14:23 bug with large_dir in 5.12.17 Carlos Carvalho
  2021-07-27  6:22 ` Andreas Dilger
  2021-07-28 13:56 ` Благодаренко Артём
@ 2021-07-29 19:23 ` Благодаренко Артём
       [not found]   ` <7f781a3cd7114db0842dc3f291cd3f6cd826917f.camel@voxelsoft.com>
  2021-08-04 19:25   ` Theodore Ts'o
  2 siblings, 2 replies; 11+ messages in thread
From: Благодаренко Артём @ 2021-07-29 19:23 UTC (permalink / raw)
  To: Carlos Carvalho; +Cc: linux-ext4, Theodore Tso, Andreas Dilger

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.

> On 22 Jul 2021, at 17:23, Carlos Carvalho <carlos@fisica.ufpr.br> wrote:
> 
> 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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: bug with large_dir in 5.12.17
       [not found]   ` <7f781a3cd7114db0842dc3f291cd3f6cd826917f.camel@voxelsoft.com>
@ 2021-07-31  5:13     ` Theodore Ts'o
  0 siblings, 0 replies; 11+ messages in thread
From: Theodore Ts'o @ 2021-07-31  5:13 UTC (permalink / raw)
  To: Denis
  Cc: Благодаренко
	Артём,
	linux-ext4

On Fri, Jul 30, 2021 at 02:15:12AM +0100, Denis wrote:
> 
> My emails to the list have been silently dropped and postmaster does
> not respond.

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

> http://voxelsoft.com/2021/ext4_large_dir_corruption.html

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:

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 5fd56f616cf0..f3bbcd4efb56 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -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(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 5fd56f616cf0..58fca88d7459 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -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
					

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: bug with large_dir in 5.12.17
  2021-07-29 19:23 ` Благодаренко Артём
       [not found]   ` <7f781a3cd7114db0842dc3f291cd3f6cd826917f.camel@voxelsoft.com>
@ 2021-08-04 19:25   ` Theodore Ts'o
  2021-08-04 21:15     ` Благодаренко Артём
  1 sibling, 1 reply; 11+ messages in thread
From: Theodore Ts'o @ 2021-08-04 19:25 UTC (permalink / raw)
  To: Благодаренко
	Артём
  Cc: Carlos Carvalho, linux-ext4, Theodore Tso, Andreas Dilger

On Thu, Jul 29, 2021 at 10:23:35PM +0300, Благодаренко Артём wrote:
> 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.

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 <tytso@mit.edu>
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.

Cc: stable@kernel.org # v5.11
Cc: Благодаренко Артём <artem.blagodarenko@gmail.com>
Fixes: b5776e7524af ("ext4: fix potential htree index checksum corruption)
Reported-by: Denis <denis@voxelsoft.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/ext4/namei.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 5fd56f616cf0..f3bbcd4efb56 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: bug with large_dir in 5.12.17
  2021-08-04 19:25   ` Theodore Ts'o
@ 2021-08-04 21:15     ` Благодаренко Артём
  2021-08-05 14:39       ` Theodore Ts'o
  0 siblings, 1 reply; 11+ messages in thread
From: Благодаренко Артём @ 2021-08-04 21:15 UTC (permalink / raw)
  To: Theodore Ts'o
  Cc: Carlos Carvalho, linux-ext4, Theodore Tso, Andreas Dilger

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



> On 4 Aug 2021, at 22:25, Theodore Ts'o <tytso@mit.edu> wrote:
> 
> On Thu, Jul 29, 2021 at 10:23:35PM +0300, Благодаренко Артём wrote:
>> 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.
> 
> 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 <tytso@mit.edu>
> 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.
> 
> Cc: stable@kernel.org # v5.11
> Cc: Благодаренко Артём <artem.blagodarenko@gmail.com>
> Fixes: b5776e7524af ("ext4: fix potential htree index checksum corruption)
> Reported-by: Denis <denis@voxelsoft.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
> fs/ext4/namei.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 5fd56f616cf0..f3bbcd4efb56 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -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
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: bug with large_dir in 5.12.17
  2021-08-04 21:15     ` Благодаренко Артём
@ 2021-08-05 14:39       ` Theodore Ts'o
  0 siblings, 0 replies; 11+ messages in thread
From: Theodore Ts'o @ 2021-08-05 14:39 UTC (permalink / raw)
  To: Благодаренко
	Артём
  Cc: Carlos Carvalho, linux-ext4, Theodore Tso, Andreas Dilger

On Thu, Aug 05, 2021 at 12:15:41AM +0300, Благодаренко Артём wrote:
> Hello Teodore,
> 
> Your one-line fix looks good.
> 
> I have tested it. 1560000 names created successfully.
> 
> But the patch with refactoring doesn’t work.

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 <tytso@mit.edu>
Date:   Thu Aug 5 09:49:40 2021 -0400

    ext4: add test to validate the large_dir feature
    
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/src/dirstress.c b/src/dirstress.c
index 615cb6e3..ec28d643 100644
--- a/src/dirstress.c
+++ b/src/dirstress.c
@@ -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);
diff --git a/tests/ext4/051 b/tests/ext4/051
new file mode 100755
index 00000000..387e2518
--- /dev/null
+++ b/tests/ext4/051
@@ -0,0 +1,62 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# FS QA Test 051
+#
+# Test ext4's large_dir feature
+#
+. ./common/preamble
+_begin_fstest auto quick dir
+
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	rm -r -f $tmp.*
+	if [ ! -z "$loop_mnt" ]; then
+		$UMOUNT_PROG $loop_mnt
+		rm -rf $loop_mnt
+	fi
+	[ ! -z "$fs_img" ] && rm -rf $fs_img
+}
+
+# Import common functions.
+# . ./common/filter
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs ext4
+_require_test
+_require_loop
+_require_scratch_ext4_feature "large_dir"
+
+echo "Silence is golden"
+
+loop_mnt=$TEST_DIR/$seq.mnt
+fs_img=$TEST_DIR/$seq.img
+status=0
+
+cp /dev/null $fs_img
+${MKFS_PROG} -t ${FSTYP} -b 1024 -N 600020 -O large_dir,^has_journal \
+	     $fs_img 40G >> $seqres.full 2>&1 || _fail "mkfs failed"
+
+mkdir -p $loop_mnt
+_mount -o loop $fs_img $loop_mnt > /dev/null  2>&1 || \
+	_fail "Couldn't do initial mount"
+
+/root/xfstests/src/dirstress -c -d /tmpmnt -p 1 -f 400000 -C \
+	>> $seqres.full 2>&1
+
+if ! $here/src/dirstress -c -d $loop_mnt -p 1 -f 400000 -C >$tmp.out 2>&1
+then
+    echo "    dirstress failed"
+    cat $tmp.out >> $seqres.full
+    echo "    dirstress failed" >> $seqres.full
+    status=1
+fi
+
+$UMOUNT_PROG $loop_mnt || _fail "umount failed"
+loop_mnt=
+
+$E2FSCK_PROG -fn $fs_img >> $seqres.full 2>&1 || _fail "file system corrupted"
diff --git a/tests/ext4/051.out b/tests/ext4/051.out
new file mode 100644
index 00000000..32f74d89
--- /dev/null
+++ b/tests/ext4/051.out
@@ -0,0 +1,2 @@
+QA output created by 051
+Silence is golden

^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-08-05 14:40 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-22 14:23 bug with large_dir in 5.12.17 Carlos Carvalho
2021-07-27  6:22 ` Andreas Dilger
2021-07-27 19:07   ` Carlos Carvalho
2021-07-28  0:38     ` Carlos Carvalho
2021-07-28 16:07   ` Благодаренко Артём
2021-07-28 13:56 ` Благодаренко Артём
2021-07-29 19:23 ` Благодаренко Артём
     [not found]   ` <7f781a3cd7114db0842dc3f291cd3f6cd826917f.camel@voxelsoft.com>
2021-07-31  5:13     ` Theodore Ts'o
2021-08-04 19:25   ` Theodore Ts'o
2021-08-04 21:15     ` Благодаренко Артём
2021-08-05 14:39       ` Theodore Ts'o

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.