Re: [PATCH] clear extents flag on inodes created in ext4_mknod

2008-02-19 Thread Aneesh Kumar K.V 
On Tue, Feb 19, 2008 at 10:19:44PM +0530, Aneesh Kumar K.V wrote:
 On Tue, Feb 19, 2008 at 10:39:52AM -0600, Eric Sandeen wrote:
  Eric Sandeen wrote:
   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
   ===
   --- linux-2.6.24.orig/fs/ext4/namei.c
   +++ 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);
  
  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 ?
 

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:
[c0002c937320] [c01a0b84] .ext4_ext_find_extent+0x74/0x344 
(unreliable)
[c0002c9373e0] [c01a32c0] .ext4_ext_get_blocks+0x1e8/0xc3c
[c0002c937570] [c0191394] .ext4_get_blocks_wrap+0xa4/0x19c
[c0002c937640] [c0191588] .ext4_get_block+0xfc/0x16c
[c0002c937700] [c011490c] .__block_prepare_write+0x1f0/0x4f8
[c0002c937810] [c0114e5c] .block_write_begin+0xc4/0x160
[c0002c9378e0] [c018ec28] .ext4_write_begin+0x12c/0x24c
[c0002c9379e0] [c00a7ef4] .pagecache_write_begin+0x84/0x1e0
[c0002c937ac0] [c00f17f8] .__page_symlink+0x6c/0x154
[c0002c937b80] [c019784c] .ext4_symlink+0x1d8/0x2b8
[c0002c937c60] [c00f0508] .vfs_symlink+0x130/0x1c8
[c0002c937d00] [c00f0648] .sys_symlinkat+0xa8/0x110
[c0002c937e30] [c000872c] 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)




diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index d2c2e55..f430939 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -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);
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 23902ba..da942bc 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -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);
-
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

Re: [PATCH] clear extents flag on inodes created in ext4_mknod

2008-02-19 Thread Eric Sandeen 
Aneesh Kumar K.V wrote:
 On Tue, Feb 19, 2008 at 10:39:52AM -0600, Eric Sandeen wrote:
 Eric Sandeen wrote:
 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
 ===
 --- linux-2.6.24.orig/fs/ext4/namei.c
 +++ 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);
 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 ?

I think that makes sense.

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

Re: [PATCH] clear extents flag on inodes created in ext4_mknod

2008-02-19 Thread Eric Sandeen 
Aneesh Kumar K.V wrote:

 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

 
 diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
 index d2c2e55..f430939 100644
 --- a/fs/ext4/ialloc.c
 +++ b/fs/ext4/ialloc.c
 @@ -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

 +  */
 + 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);

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



 diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
 index 23902ba..da942bc 100644
 --- a/fs/ext4/namei.c
 +++ b/fs/ext4/namei.c
 @@ -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);

-
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

Re: [PATCH] clear extents flag on inodes created in ext4_mknod

2008-02-19 Thread Aneesh Kumar K.V 
On Tue, Feb 19, 2008 at 10:39:52AM -0600, Eric Sandeen wrote:
 Eric Sandeen wrote:
  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
  ===
  --- linux-2.6.24.orig/fs/ext4/namei.c
  +++ 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);
 
 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
-
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

Re: [PATCH] clear extents flag on inodes created in ext4_mknod

2008-02-19 Thread Eric Sandeen 
Eric Sandeen wrote:
 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
 ===
 --- linux-2.6.24.orig/fs/ext4/namei.c
 +++ 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);

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

-
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

Re: [PATCH] clear extents flag on inodes created in ext4_mknod

2008-02-18 Thread Mingming Cao 
Acked and added to the patch queue.
On Mon, 2008-02-18 at 15:00 -0600, Eric Sandeen wrote:
 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
 ===
 --- linux-2.6.24.orig/fs/ext4/namei.c
 +++ 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);
 
 
 -
 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

-
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