ext3: dirindex error pointer issues

PATCH In review Dmitriy Monakhov 2 participants 4 messages Mar 4, 2007 – Mar 12, 2007 ext4 file system

Share & tools

Dmitriy Monakhov

author

2007-03-04 14:17 UTC #0 lore raw

PATCH 1/1 ext3: dirindex error pointer issues

- ext3_dx_find_entry() exit with out setting proper error pointer
- do_split() exit with out setting proper error pointer it is realy painful because many callers contain folowing code:

           de = do_split(handle,dir, &bh, frame, &hinfo, &retval);
           if (!(de))
                        return retval;
           <<< WOW retval wasn't changed by do_split(), so caller failed
           <<< but return SUCCESS :)

- Rearrange do_split() error path. Current error path is realy ugly, all this up and down jump stuff doesn't make code easy to understand.

Signed-off-by Monakhov Dmitriy <[email protected]>

---
 fs/ext3/namei.c |   26 +++++++++++++++-----------
 fs/ext4/namei.c |   26 +++++++++++++++-----------
 2 files changed, 30 insertions(+), 22 deletions(-)

fs/ext3/namei.c +15 ~~-11~~

index 49159f1..1a52586 100644
@@ -969,6 +969,7 @@ static struct buffer_head * ext3_dx_find_entry(struct dentry *dentry,
 				  (block<<EXT3_BLOCK_SIZE_BITS(sb))
 					  +((char *)de - bh->b_data))) {
 				brelse (bh);
+				*err = ERR_BAD_DX_DIR;
 				goto errout;
 			}
 			*res_dir = de;

@@ -1134,9 +1135,9 @@ static struct ext3_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
 	char *data1 = (*bh)->b_data, *data2;
 	unsigned split;
 	struct ext3_dir_entry_2 *de = NULL, *de2;
-	int	err;
+	int	err = 0;
 
-	bh2 = ext3_append (handle, dir, &newblock, error);
+	bh2 = ext3_append (handle, dir, &newblock, &err);
 	if (!(bh2)) {
 		brelse(*bh);
 		*bh = NULL;

@@ -1145,14 +1146,9 @@ static struct ext3_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
 
 	BUFFER_TRACE(*bh, "get_write_access");
 	err = ext3_journal_get_write_access(handle, *bh);
-	if (err) {
-	journal_error:
-		brelse(*bh);
-		brelse(bh2);
-		*bh = NULL;
-		ext3_std_error(dir->i_sb, err);
-		goto errout;
-	}
+	if (err)
+		goto journal_error;
+
 	BUFFER_TRACE(frame->bh, "get_write_access");
 	err = ext3_journal_get_write_access(handle, frame->bh);
 	if (err)

@@ -1195,8 +1191,16 @@ static struct ext3_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
 		goto journal_error;
 	brelse (bh2);
 	dxtrace(dx_show_index ("frame", frame->entries));
-errout:
 	return de;
+
+journal_error:
+	brelse(*bh);
+	brelse(bh2);
+	*bh = NULL;
+errout:
+	ext3_std_error(dir->i_sb, err);
+	*error = err;
+	return NULL;
 }
 #endif

fs/ext4/namei.c +15 ~~-11~~

index e7e1d79..682a3d7 100644
@@ -967,6 +967,7 @@ static struct buffer_head * ext4_dx_find_entry(struct dentry *dentry,
 				  (block<<EXT4_BLOCK_SIZE_BITS(sb))
 					  +((char *)de - bh->b_data))) {
 				brelse (bh);
+				*err = ERR_BAD_DX_DIR;
 				goto errout;
 			}
 			*res_dir = de;

@@ -1132,9 +1133,9 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
 	char *data1 = (*bh)->b_data, *data2;
 	unsigned split;
 	struct ext4_dir_entry_2 *de = NULL, *de2;
-	int	err;
+	int	err = 0;
 
-	bh2 = ext4_append (handle, dir, &newblock, error);
+	bh2 = ext4_append (handle, dir, &newblock, &err);
 	if (!(bh2)) {
 		brelse(*bh);
 		*bh = NULL;

@@ -1143,14 +1144,9 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
 
 	BUFFER_TRACE(*bh, "get_write_access");
 	err = ext4_journal_get_write_access(handle, *bh);
-	if (err) {
-	journal_error:
-		brelse(*bh);
-		brelse(bh2);
-		*bh = NULL;
-		ext4_std_error(dir->i_sb, err);
-		goto errout;
-	}
+	if (err)
+		goto journal_error;
+
 	BUFFER_TRACE(frame->bh, "get_write_access");
 	err = ext4_journal_get_write_access(handle, frame->bh);
 	if (err)

@@ -1193,8 +1189,16 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
 		goto journal_error;
 	brelse (bh2);
 	dxtrace(dx_show_index ("frame", frame->entries));
-errout:
 	return de;
+
+journal_error:
+	brelse(*bh);
+	brelse(bh2);
+	*bh = NULL;
+errout:
+	ext4_std_error(dir->i_sb, err);
+	*error = err;
+	return NULL;
 }
 #endif

Signature · 1 line

1.5.0.1

Andreas Dilger

2007-03-05 02:13 UTC #1 lore raw

Quoted 22 lines · Dmitriy Monakhov

- ext3_dx_find_entry() exit with out setting proper error pointer
- do_split() exit with out setting proper error pointer it is realy painful because many callers contain folowing code:
           de = do_split(handle,dir, &bh, frame, &hinfo, &retval);
           if (!(de))
                        return retval;
           <<< WOW retval wasn't changed by do_split(), so caller failed
           <<< but return SUCCESS :)
- Rearrange do_split() error path. Current error path is realy ugly, all this up and down jump stuff doesn't make code easy to understand.
Signed-off-by Monakhov Dmitriy <[email protected]>
---
 fs/ext3/namei.c |   26 +++++++++++++++-----------
 fs/ext4/namei.c |   26 +++++++++++++++-----------
 2 files changed, 30 insertions(+), 22 deletions(-)
fs/ext3/namei.c +0 -0
index 49159f1..1a52586 100644

fs/ext3/namei.c static struct buffer_head * ext3_dx_find_entry(struct dentry *dentry, · line 975

Quoted 7 lines

fs/ext3/namei.c +1 -0

@@ -969,6 +969,7 @@ static struct buffer_head * ext3_dx_find_entry(struct dentry *dentry,
 				  (block<<EXT3_BLOCK_SIZE_BITS(sb))
 					  +((char *)de - bh->b_data))) {
 				brelse (bh);
+				*err = ERR_BAD_DX_DIR;
 				goto errout;
 			}
 			*res_dir = de;

I have no objection to this change (by principle of least surprise) but I don't know if it fixes a real problem. The one caller of this function treats ERR_BAD_DX_DIR the same as bh == NULL.

fs/ext3/namei.c static struct ext3_dir_entry_2 *do_split(handle_t *handle, struct inode *dir, · line 1140

Quoted 9 lines

fs/ext3/namei.c +2 -2

@@ -1134,9 +1135,9 @@ static struct ext3_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
 	char *data1 = (*bh)->b_data, *data2;
 	unsigned split;
 	struct ext3_dir_entry_2 *de = NULL, *de2;
-	int	err;
+	int	err = 0;
 
-	bh2 = ext3_append (handle, dir, &newblock, error);
+	bh2 = ext3_append (handle, dir, &newblock, &err);

Why don't we just remove "err" entirely and use *error to avoid any risk of setting err and not returning it in error? Also reduces stack usage that tiny little bit.

In ext3_dx_add_entry() it appears like we should "goto journal_error" to report an error after the failed call to do_split(), instead of just "goto cleanup" so that we report an error in the filesystem. Not 100% sure of this.

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

Dmitriy Monakhov

author

2007-03-05 07:33 UTC #2 lore raw

Quoted 18 lines · Andreas Dilger

Quoted 17 lines · Dmitriy Monakhov
- ext3_dx_find_entry() exit with out setting proper error pointer
- do_split() exit with out setting proper error pointer it is realy painful because many callers contain folowing code:
           de = do_split(handle,dir, &bh, frame, &hinfo, &retval);
           if (!(de))
                        return retval;
           <<< WOW retval wasn't changed by do_split(), so caller failed
           <<< but return SUCCESS :)
- Rearrange do_split() error path. Current error path is realy ugly, all this up and down jump stuff doesn't make code easy to understand.
Signed-off-by Monakhov Dmitriy <[email protected]>
---
 fs/ext3/namei.c |   26 +++++++++++++++-----------
 fs/ext4/namei.c |   26 +++++++++++++++-----------
 2 files changed, 30 insertions(+), 22 deletions(-)

Quoted 16 lines

fs/ext3/namei.c static struct buffer_head * ext3_dx_find_entry(struct dentry *dentry, · line 975
Quoted 12 lines
fs/ext3/namei.c +1 -0
index 49159f1..1a52586 100644
@@ -969,6 +969,7 @@ static struct buffer_head * ext3_dx_find_entry(struct dentry *dentry,
 				  (block<<EXT3_BLOCK_SIZE_BITS(sb))
 					  +((char *)de - bh->b_data))) {
 				brelse (bh);
+				*err = ERR_BAD_DX_DIR;
 				goto errout;
 			}
 			*res_dir = de;
I have no objection to this change (by principle of least surprise) but I don't know if it fixes a real problem. The one caller of this function treats ERR_BAD_DX_DIR the same as bh == NULL.

Execly ERR_BAD_DX_DIR treats the same as bh == NULL and let's look at callers code:
struct buffer_head * ext3_find_entry(......)
{
.......

              bh = ext3_dx_find_entry(dentry, res_dir, &err);
                /*
                 * On success, or if the error was file not found,
                 * return.  Otherwise, fall back to doing a search the
                 * old fashioned way.
                 */
                if (bh || (err != ERR_BAD_DX_DIR))
  <<<<< after ext3_dx_find_entry() has failed , but don't set err pointer 
  <<<<< we get  (bh == NULL, err == 0) , and then just return NULL.
                        return bh;

.......
}

Quoted 8 lines

fs/ext3/namei.c +1 -1

@@ -1134,9 +1135,9 @@ static struct ext3_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
 	char *data1 = (*bh)->b_data, *data2;
 	unsigned split;
 	struct ext3_dir_entry_2 *de = NULL, *de2;
-	int	err;
+	int	err = 0;

Quoted 6 lines

fs/ext3/namei.c static struct ext3_dir_entry_2 *do_split(handle_t *handle, struct inode *dir, · line 1140
Quoted 2 lines
fs/ext3/namei.c +1 -1
-	bh2 = ext3_append (handle, dir, &newblock, error);
+	bh2 = ext3_append (handle, dir, &newblock, &err);
Why don't we just remove "err" entirely and use *error to avoid any risk of setting err and not returning it in error? Also reduces stack usage that tiny little bit.

I've chosen this approuch becouse of:
1) this approuch was used in block allocation code.
2) this approuch prevent folowing errors:
*error = do_some_function(.....)
........ /* some code*/
if(error)
we do this checking as we do it thousands times before, but forget what error it pointer here. And compiler can't warn us here because it is absolutely legal operation. At least it is better to rename error to errorp.

Btw: I've thought about adding assertations to error path witch may check what errp pointer was properly initialized after error happends. folowing code is draft only:

fs/ext3/namei.c +2 -0

@@ -63,6 +63,7 @@ static struct buffer_head *ext3_append(handle_t *handle,
 		EXT3_I(inode)->i_disksize = inode->i_size;
 		ext3_journal_get_write_access(handle,bh);
 	}
+	assert(bh || *err);
 	return bh;
 }

@@ -433,6 +434,7 @@ fail2:
 		frame--;
 	}
 fail:
+	assert(*err != 0);
 	return NULL;
 }

Quoted 6 lines

In ext3_dx_add_entry() it appears like we should "goto journal_error" to report an error after the failed call to do_split(), instead of just "goto cleanup" so that we report an error in the filesystem. Not 100% sure of this.

do_split() already invoked ext3_std_error() on it's "journal_error" path.

Quoted 12 lines

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Dmitriy Monakhov

author

2007-03-12 07:20 UTC #3 lore raw

Re: [PATCH] ext3: dirindex error pointer issues (b)

In reply to Dmitriy Monakhov

Quoted 10 lines · Dmitriy Monakhov

- ext3_dx_find_entry() exit with out setting proper error pointer
- do_split() exit with out setting proper error pointer it is realy painful because many callers contain folowing code:
           de = do_split(handle,dir, &bh, frame, &hinfo, &retval);
           if (!(de))
                        return retval;
           <<< WOW retval wasn't changed by do_split(), so caller failed
           <<< but return SUCCESS :)
- Rearrange do_split() error path. Current error path is realy ugly, all this up and down jump stuff doesn't make code easy to understand.

Ohh my first patch change error message semantics in do_split(). Initially when ext3_append() failed we just exit without printing error. In fact ext3_append() may fail, it is legal and it's happens qite often (ENOSPC for example). This cause annoying fake error message. So restore this semantic as it was before patch.
Andrew please apply incremental patch what fix it:

Signed-off-by Monakhov Dmitriy <[email protected]>

fs/ext3/namei.c +1 -1

index 1a52586..7edb617 100644
@@ -1197,8 +1197,8 @@ journal_error:
 	brelse(*bh);
 	brelse(bh2);
 	*bh = NULL;
-errout:
 	ext3_std_error(dir->i_sb, err);
+errout:
 	*error = err;
 	return NULL;
 }

fs/ext4/namei.c +1 -1

index f0a6c26..02a75db 100644
@@ -1195,8 +1195,8 @@ journal_error:
 	brelse(*bh);
 	brelse(bh2);
 	*bh = NULL;
-errout:
 	ext4_std_error(dir->i_sb, err);
+errout:
 	*error = err;
 	return NULL;
 }