[REPOST][RFC][PATCH] ext4: unify each meaning of the offset in ext4_check_dir_entry calling from some functions.

[REPOST][RFC][PATCH] ext4: unify each meaning of the offset in ext4_check_dir_entry calling from some functions.

[Toshiyuki Okajima]

From: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>

"offset" of the error message of ext4_check_dir_entry() might change the
meaning by the caller. There are 2 meanings:
- "File offset"
  called by:
   ext4_readdir, htree_dirblock_to_tree, search_dirblock, ext4_dx_find_entry,
   empty_dir
- "Buffer offset"
  called by:
   add_dirent_to_buf, ext4_delete_entry

The best way to solve this problem is to change the meaning of 
"Buffer offset" into "File offset" but it is not easy.
However, we can solve this problem easily if we unify the meanings into 
"Buffer offset". So, instead of "File Offset" meaning, we add the block number 
information to this message.

--- Examples ---
Original error message:
EXT4-fs error (device loop0): htree_dirblock_to_tree: bad entry in directory \
#12: rec_len is too small for name_len - offset=12288, inode=216, rec_len=12,\
 name_len=11

Error message which is changed by this patch:
EXT4-fs error (device loop0): htree_dirblock_to_tree: bad entry in directory \
#12: rec_len is too small for name_len - block_nr=78:offset=0, inode=216, \
rec_len=12, name_len=11

Signed-off-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
--
 fs/ext4/dir.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff -Nurp linux-2.6.33-rc3.org/fs/ext4/dir.c linux-2.6.33-rc3/fs/ext4/dir.c
--- linux-2.6.33-rc3.org/fs/ext4/dir.c	2010-01-06 09:02:46.000000000 +0900
+++ linux-2.6.33-rc3/fs/ext4/dir.c	2010-01-14 13:13:17.000000000 +0900
@@ -84,9 +84,10 @@ int ext4_check_dir_entry(const char *fun
 
 	if (error_msg != NULL)
 		ext4_error(dir->i_sb, function,
-			"bad entry in directory #%lu: %s - "
+			"bad entry in directory #%lu: %s - block_nr=%llu:"
 			"offset=%u, inode=%u, rec_len=%d, name_len=%d",
-			dir->i_ino, error_msg, offset,
+			dir->i_ino, error_msg, 
+			(u64)bh->b_blocknr, (u32)(offset%bh->b_size),
 			le32_to_cpu(de->inode),
 			rlen, de->name_len);
 	return error_msg == NULL ? 1 : 0;

Re: [REPOST][RFC][PATCH] ext4: unify each meaning of the offset in ext4_check_dir_entry calling from some functions.

[Andreas Dilger]

On 2010-01-14, at 01:57, Toshiyuki Okajima wrote:
> From: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
>
> "offset" of the error message of ext4_check_dir_entry() might change  
> the
> meaning by the caller. There are 2 meanings:
> - "File offset"
>  called by:
>   ext4_readdir, htree_dirblock_to_tree, search_dirblock,  
> ext4_dx_find_entry,
>   empty_dir
> - "Buffer offset"
>  called by:
>   add_dirent_to_buf, ext4_delete_entry

This makes sense to fix.

> The best way to solve this problem is to change the meaning of
> "Buffer offset" into "File offset" but it is not easy.
> However, we can solve this problem easily if we unify the meanings  
> into
> "Buffer offset". So, instead of "File Offset" meaning, we add the  
> block number information to this message.
>
> --- Examples ---
> Error message which is changed by this patch:
> EXT4-fs error (device loop0): htree_dirblock_to_tree: bad entry in  
> directory
> #12: rec_len is too small for name_len - block_nr=78:offset=0,  
> inode=216,
> rec_len=12, name_len=11

I personally would prefer to see "block=78, offset=0" instead of the  
above.

> @@ -84,9 +84,10 @@ int ext4_check_dir_entry(const char *fun
>
> 	if (error_msg != NULL)
> 		ext4_error(dir->i_sb, function,
> +			"bad entry in directory #%lu: %s - block_nr=%llu:"
> 			"offset=%u, inode=%u, rec_len=%d, name_len=%d",
> +			dir->i_ino, error_msg,
> +			(u64)bh->b_blocknr, (u32)(offset%bh->b_size),


One problem here is that "(u64)" is not necessarily a "long long".  On  
some 64-bit platforms it will be a "long" and this will generate a  
compiler warning.  Better to use "(unsigned long long)", and you may  
as well use "(unsigned)" for the offset value.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: [REPOST][RFC][PATCH] ext4: unify each meaning of the offset in ext4_check_dir_entry calling from some functions.

[Toshiyuki Okajima]

Andreas, thanks for your comment.

On Thu, 14 Jan 2010 10:18:53 -0500
Andreas Dilger <adilger@sun.com> wrote:

> > --- Examples ---
> > Error message which is changed by this patch:
> > EXT4-fs error (device loop0): htree_dirblock_to_tree: bad entry in  
> > directory
> > #12: rec_len is too small for name_len - block_nr=78:offset=0,  
> > inode=216,
> > rec_len=12, name_len=11
> 
> I personally would prefer to see "block=78, offset=0" instead of the  
> above.
OK. I change it.

> 
> > @@ -84,9 +84,10 @@ int ext4_check_dir_entry(const char *fun
> >
> > 	if (error_msg != NULL)
> > 		ext4_error(dir->i_sb, function,
> > +			"bad entry in directory #%lu: %s - block_nr=%llu:"
> > 			"offset=%u, inode=%u, rec_len=%d, name_len=%d",
> > +			dir->i_ino, error_msg,
> > +			(u64)bh->b_blocknr, (u32)(offset%bh->b_size),
> 
> 

> One problem here is that "(u64)" is not necessarily a "long long".  On  
> some 64-bit platforms it will be a "long" and this will generate a  
> compiler warning.  Better to use "(unsigned long long)", and you may  
> as well use "(unsigned)" for the offset value.
I understand. (u64 doesn't always mean "unsigned long long int")

I apply your all comment, and then I remake a patch.

Regards, 
Toshiyuki Okajima
---
Subject: [PATCH] ext4: unify each meaning of the offset in ext4_check_dir_entry calling from some functions.

From: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>

"offset" of the error message of ext4_check_dir_entry() might change the
meaning by the caller. There are 2 meanings:
- "File offset"
  called by:
   ext4_readdir, htree_dirblock_to_tree, search_dirblock, ext4_dx_find_entry,
   empty_dir
- "Buffer offset"
  called by:
   add_dirent_to_buf, ext4_delete_entry

The best way to solve this problem is to change the meaning of 
"Buffer offset" into "File offset" but it is not easy.
However, we can solve this problem easily if we unify the meanings into 
"Buffer offset". So, instead of "File Offset" meaning, we add the block number 
information to this message.

--- Examples ---
Original error message:
EXT4-fs error (device loop0): htree_dirblock_to_tree: bad entry in directory \
#12: rec_len is too small for name_len - offset=12288, inode=216, rec_len=12,\
 name_len=11

Error message which is changed by this patch:
EXT4-fs error (device loop0): htree_dirblock_to_tree: bad entry in directory \
#12: rec_len is too small for name_len - block=78, offset=0, inode=216, \
rec_len=12, name_len=11

Signed-off-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
---
 fs/ext4/dir.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index 9dc9316..65da7e5 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -84,9 +84,11 @@ int ext4_check_dir_entry(const char *function, struct inode *dir,
 
 	if (error_msg != NULL)
 		ext4_error(dir->i_sb, function,
-			"bad entry in directory #%lu: %s - "
+			"bad entry in directory #%lu: %s - block=%llu, "
 			"offset=%u, inode=%u, rec_len=%d, name_len=%d",
-			dir->i_ino, error_msg, offset,
+			dir->i_ino, error_msg, 
+			(unsigned long long)bh->b_blocknr, 
+			(unsigned)(offset%bh->b_size),
 			le32_to_cpu(de->inode),
 			rlen, de->name_len);
 	return error_msg == NULL ? 1 : 0;