clear extents flag on inodes created in ext4_mknod

PATCH In review Eric Sandeen 3 participants 7 messages Feb 18, 2008 – Feb 19, 2008 ext4 file system

Share & tools

Eric Sandeen

author

2008-02-18 21:00 UTC #0 lore raw

PATCH 1/1 clear extents flag on inodes created in ext4_mknod

e2fsck doesn't expect to find char, block, fifo, or socket files with the extent flag set, so clear that in ext4_mknod.

Signed-off-by Eric Sandeen <[email protected]>

---

fs/ext4/namei.c +1 -0

Index: linux-2.6.24/fs/ext4/namei.c
===================================================================
@@ -1766,6 +1766,7 @@ retry:
 #ifdef CONFIG_EXT4DEV_FS_XATTR
 		inode->i_op = &ext4_special_inode_operations;
 #endif
+		EXT4_I(inode)->i_flags &= ~EXT4_EXTENTS_FL;
 		err = ext4_add_nondir(handle, dentry, inode);
 	}
 	ext4_journal_stop(handle);

Mingming Cao

2008-02-19 05:22 UTC #1 lore raw

Acked and added to the patch queue.

Quoted 24 lines · Eric Sandeen

e2fsck doesn't expect to find char, block, fifo, or socket files with the extent flag set, so clear that in ext4_mknod.
Signed-off-by Eric Sandeen <[email protected]>
---
Index: linux-2.6.24/fs/ext4/namei.c
===================================================================
fs/ext4/namei.c +1 -1
@@ -1766,6 +1766,7 @@ retry:
 #ifdef CONFIG_EXT4DEV_FS_XATTR
 		inode->i_op = &ext4_special_inode_operations;
 #endif
+		EXT4_I(inode)->i_flags &= ~EXT4_EXTENTS_FL;
 		err = ext4_add_nondir(handle, dentry, inode);
 	}
 	ext4_journal_stop(handle);
 
 
-
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Sandeen

author

2008-02-19 16:39 UTC #2 lore raw

In reply to Eric Sandeen

Quoted 11 lines · Eric Sandeen

e2fsck doesn't expect to find char, block, fifo, or socket files with the extent flag set, so clear that in ext4_mknod.
Signed-off-by Eric Sandeen <[email protected]>
---
Index: linux-2.6.24/fs/ext4/namei.c
===================================================================

fs/ext4/namei.c retry: · line 1772

Quoted 7 lines

fs/ext4/namei.c +1 -0

@@ -1766,6 +1766,7 @@ retry:
 #ifdef CONFIG_EXT4DEV_FS_XATTR
 		inode->i_op = &ext4_special_inode_operations;
 #endif
+		EXT4_I(inode)->i_flags &= ~EXT4_EXTENTS_FL;
 		err = ext4_add_nondir(handle, dentry, inode);
 	}
 	ext4_journal_stop(handle);

now that I think about it; perhaps it would be better to put this logic into ext4_new_inode, rather than setting it by default and clearing it here... that way new_inode() has all the logic about whether or not a particular type of file is in extents format.

Think it's worth changing?

-Eric

Aneesh Kumar K.V

2008-02-19 16:49 UTC #3 lore raw

Quoted 16 lines · Eric Sandeen

Quoted 15 lines · Eric Sandeen
e2fsck doesn't expect to find char, block, fifo, or socket files with the extent flag set, so clear that in ext4_mknod.
Signed-off-by Eric Sandeen <[email protected]>
---
Index: linux-2.6.24/fs/ext4/namei.c
===================================================================
fs/ext4/namei.c +1 -0
@@ -1766,6 +1766,7 @@ retry:
 #ifdef CONFIG_EXT4DEV_FS_XATTR
 		inode->i_op = &ext4_special_inode_operations;
 #endif
+		EXT4_I(inode)->i_flags &= ~EXT4_EXTENTS_FL;

Quoted 9 lines

fs/ext4/namei.c retry: · line 1772
Quoted 3 lines
fs/ext4/namei.c +0 -0
 		err = ext4_add_nondir(handle, dentry, inode);
 	}
 	ext4_journal_stop(handle);
now that I think about it; perhaps it would be better to put this logic into ext4_new_inode, rather than setting it by default and clearing it here... that way new_inode() has all the logic about whether or not a particular type of file is in extents format.

How about enabling it only for directory and regular files rather than enabling it globally and then disabling the flag for symlink and device files ?

-aneesh

Eric Sandeen

author

2008-02-19 16:58 UTC #4 lore raw

Quoted 26 lines · Aneesh Kumar K.V

Quoted 7 lines · Eric Sandeen
Quoted 6 lines · Eric Sandeen
e2fsck doesn't expect to find char, block, fifo, or socket files with the extent flag set, so clear that in ext4_mknod.
Signed-off-by Eric Sandeen <[email protected]>
---
Quoted 17 lines
fs/ext4/namei.c retry: · line 1772
Quoted 12 lines
Index: linux-2.6.24/fs/ext4/namei.c
===================================================================
fs/ext4/namei.c +1 -0
@@ -1766,6 +1766,7 @@ retry:
 #ifdef CONFIG_EXT4DEV_FS_XATTR
 		inode->i_op = &ext4_special_inode_operations;
 #endif
+		EXT4_I(inode)->i_flags &= ~EXT4_EXTENTS_FL;
 		err = ext4_add_nondir(handle, dentry, inode);
 	}
 	ext4_journal_stop(handle);
now that I think about it; perhaps it would be better to put this logic into ext4_new_inode, rather than setting it by default and clearing it here... that way new_inode() has all the logic about whether or not a particular type of file is in extents format.

Quoted 3 lines

How about enabling it only for directory and regular files rather than enabling it globally and then disabling the flag for symlink and device files ?

I think that makes sense.

-Eric

Aneesh Kumar K.V

2008-02-19 17:05 UTC #5 lore raw

In reply to Aneesh Kumar K.V

Quoted 27 lines · Aneesh Kumar K.V

Quoted 16 lines · Eric Sandeen
Quoted 15 lines · Eric Sandeen
e2fsck doesn't expect to find char, block, fifo, or socket files with the extent flag set, so clear that in ext4_mknod.
Signed-off-by Eric Sandeen <[email protected]>
---
Index: linux-2.6.24/fs/ext4/namei.c
===================================================================
fs/ext4/namei.c +1 -0
@@ -1766,6 +1766,7 @@ retry:
 #ifdef CONFIG_EXT4DEV_FS_XATTR
 		inode->i_op = &ext4_special_inode_operations;
 #endif
+		EXT4_I(inode)->i_flags &= ~EXT4_EXTENTS_FL;
Quoted 9 lines
fs/ext4/namei.c retry: · line 1772
Quoted 3 lines
fs/ext4/namei.c +0 -0
 		err = ext4_add_nondir(handle, dentry, inode);
 	}
 	ext4_journal_stop(handle);
now that I think about it; perhaps it would be better to put this logic into ext4_new_inode, rather than setting it by default and clearing it here... that way new_inode() has all the logic about whether or not a particular type of file is in extents format.

Quoted 4 lines

How about enabling it only for directory and regular files rather than enabling it globally and then disabling the flag for symlink and device files ?

how about something like below. There are two reason for not inheriting the i_flag from directory.

a) if the directory have extent flag set we end up with symlink which have extent flag set but on which ext4_ext_tree_init is not called.

b) if we create a directory with extent flag and later mount the file system with -o noextents, the files created in that directory will also have extent flag set but we would not have called ext4_ext_tree_init for them.

Both results in the stack below which cause ext4_error.

Call Trace:
[c00000002c937320] [c0000000001a0b84] .ext4_ext_find_extent+0x74/0x344 (unreliable)
[c00000002c9373e0] [c0000000001a32c0] .ext4_ext_get_blocks+0x1e8/0xc3c
[c00000002c937570] [c000000000191394] .ext4_get_blocks_wrap+0xa4/0x19c
[c00000002c937640] [c000000000191588] .ext4_get_block+0xfc/0x16c
[c00000002c937700] [c00000000011490c] .__block_prepare_write+0x1f0/0x4f8
[c00000002c937810] [c000000000114e5c] .block_write_begin+0xc4/0x160
[c00000002c9378e0] [c00000000018ec28] .ext4_write_begin+0x12c/0x24c
[c00000002c9379e0] [c0000000000a7ef4] .pagecache_write_begin+0x84/0x1e0
[c00000002c937ac0] [c0000000000f17f8] .__page_symlink+0x6c/0x154
[c00000002c937b80] [c00000000019784c] .ext4_symlink+0x1d8/0x2b8
[c00000002c937c60] [c0000000000f0508] .vfs_symlink+0x130/0x1c8
[c00000002c937d00] [c0000000000f0648] .sys_symlinkat+0xa8/0x110
[c00000002c937e30] [c00000000000872c] syscall_exit+0x0/0x40

and console message:

EXT4-fs error (device sda7): ext4_ext_find_extent: bad header in inode #204044: invalid magic - magic 0, entries 0, max 0(0), depth 0(0)

fs/ext4/ialloc.c +10 -3

index d2c2e55..f430939 100644
@@ -794,7 +794,12 @@ got:
 	ei->i_dir_start_lookup = 0;
 	ei->i_disksize = 0;
 
-	ei->i_flags = EXT4_I(dir)->i_flags & ~EXT4_INDEX_FL;
+	/*
+	 * Don't inherit extent flag from directory. We set extent flag on
+	 * newly created directory and file only if -o extent mount option is
+	 * specfied
+	 */
+	ei->i_flags = EXT4_I(dir)->i_flags & ~ (EXT4_INDEX_FL|EXT4_EXTENTS_FL);
 	if (S_ISLNK(mode))
 		ei->i_flags &= ~(EXT4_IMMUTABLE_FL|EXT4_APPEND_FL);
 	/* dirsync only applies to directories */

@@ -836,8 +841,10 @@ got:
 		ext4_std_error(sb, err);
 		goto fail_free_drop;
 	}
-	if (test_opt(sb, EXTENTS) && !S_ISLNK(mode)) {
-		EXT4_I(inode)->i_flags |= EXT4_EXTENTS_FL;
+	if (test_opt(sb, EXTENTS)) {
+		/* set extent flag only for diretory and file */
+		if (S_ISDIR(mode) || S_ISREG(mode))
+			EXT4_I(inode)->i_flags |= EXT4_EXTENTS_FL;
 		ext4_ext_tree_init(handle, inode);
 		err = ext4_update_incompat_feature(handle, sb,
 						EXT4_FEATURE_INCOMPAT_EXTENTS);

fs/ext4/namei.c +0 -1

index 23902ba..da942bc 100644
@@ -1771,7 +1771,6 @@ retry:
 #ifdef CONFIG_EXT4DEV_FS_XATTR
 		inode->i_op = &ext4_special_inode_operations;
 #endif
-		EXT4_I(inode)->i_flags &= ~EXT4_EXTENTS_FL;
 		err = ext4_add_nondir(handle, dentry, inode);
 	}
 	ext4_journal_stop(handle);

Eric Sandeen

author

2008-02-19 17:22 UTC #6 lore raw

Quoted 7 lines · Aneesh Kumar K.V

Quoted 4 lines
How about enabling it only for directory and regular files rather than enabling it globally and then disabling the flag for symlink and device files ?
how about something like below. There are two reason for not inheriting the i_flag from directory.

Looks about right to me... see below

Quoted 6 lines

fs/ext4/ialloc.c +0 -0
index d2c2e55..f430939 100644

fs/ext4/ialloc.c got: · line 800

Quoted 8 lines

fs/ext4/ialloc.c +4 -1

@@ -794,7 +794,12 @@ got:
 	ei->i_dir_start_lookup = 0;
 	ei->i_disksize = 0;
 
-	ei->i_flags = EXT4_I(dir)->i_flags & ~EXT4_INDEX_FL;
+	/*
+	 * Don't inherit extent flag from directory. We set extent flag on
+	 * newly created directory and file only if -o extent mount option is
+	 * specfied

nitpick: ^^^^ typo

Quoted 8 lines

fs/ext4/ialloc.c +2 -0

+	 */
+	ei->i_flags = EXT4_I(dir)->i_flags & ~ (EXT4_INDEX_FL|EXT4_EXTENTS_FL);
 	if (S_ISLNK(mode))
 		ei->i_flags &= ~(EXT4_IMMUTABLE_FL|EXT4_APPEND_FL);
 	/* dirsync only applies to directories */

@@ -836,8 +841,10 @@ got:
 		ext4_std_error(sb, err);
 		goto fail_free_drop;

fs/ext4/ialloc.c got: · line 850

Quoted 10 lines

fs/ext4/ialloc.c +4 -2

 	}
-	if (test_opt(sb, EXTENTS) && !S_ISLNK(mode)) {
-		EXT4_I(inode)->i_flags |= EXT4_EXTENTS_FL;
+	if (test_opt(sb, EXTENTS)) {
+		/* set extent flag only for diretory and file */
+		if (S_ISDIR(mode) || S_ISREG(mode))
+			EXT4_I(inode)->i_flags |= EXT4_EXTENTS_FL;
 		ext4_ext_tree_init(handle, inode);
 		err = ext4_update_incompat_feature(handle, sb,
 						EXT4_FEATURE_INCOMPAT_EXTENTS);

Hm you only set the extents flag for dir & reg, but you call the next two functions for all files? I think the next 2 lines need to be part of the conditional, no? (and then there can be just one big test, indentation can come out a bit...?)

But, the spirit of the patch looks right.

-Eric

Quoted 12 lines

fs/ext4/namei.c +0 -1

index 23902ba..da942bc 100644
@@ -1771,7 +1771,6 @@ retry:
 #ifdef CONFIG_EXT4DEV_FS_XATTR
 		inode->i_op = &ext4_special_inode_operations;
 #endif
-		EXT4_I(inode)->i_flags &= ~EXT4_EXTENTS_FL;
 		err = ext4_add_nondir(handle, dentry, inode);
 	}
 	ext4_journal_stop(handle);