[PATCH 1/4] ext4: release buffer when checksum failed

[PATCH 1/4] ext4: release buffer when checksum failed

[Guo Chao]

Commit b0336e8d (ext4: calculate and verify checksums of directory leaf
blocks) and commit dbe89444 (ext4: Calculate and verify checksums for
htree nodes) forget to release buffer when checksum failed, at some places.

Cc: D. J. Wong <darrick.wong@oracle.com>
Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>
---
 fs/ext4/dir.c   |    1 +
 fs/ext4/namei.c |   12 ++++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index 80a28b2..3882fbc 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -185,6 +185,7 @@ static int ext4_readdir(struct file *filp,
 					"at offset %llu",
 					(unsigned long long)filp->f_pos);
 			filp->f_pos += sb->s_blocksize - offset;
+			brelse(bh);
 			continue;
 		}
 		set_buffer_verified(bh);
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 8990165..a445247 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -837,6 +837,7 @@ static int ext4_htree_next_block(struct inode *dir, __u32 hash,
 		    !ext4_dx_csum_verify(dir,
 					 (struct ext4_dir_entry *)bh->b_data)) {
 			ext4_warning(dir->i_sb, "Node failed checksum");
+			brelse(bh);
 			return -EIO;
 		}
 		set_buffer_verified(bh);
@@ -877,8 +878,11 @@ static int htree_dirblock_to_tree(struct file *dir_file,
 	}
 
 	if (!buffer_verified(bh) &&
-	    !ext4_dirent_csum_verify(dir, (struct ext4_dir_entry *)bh->b_data))
+			!ext4_dirent_csum_verify(dir,
+				(struct ext4_dir_entry *)bh->b_data)) {
+		brelse(bh);
 		return -EIO;
+	}
 	set_buffer_verified(bh);
 
 	de = (struct ext4_dir_entry_2 *) bh->b_data;
@@ -1929,8 +1933,10 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
 		}
 		if (!buffer_verified(bh) &&
 		    !ext4_dirent_csum_verify(dir,
-				(struct ext4_dir_entry *)bh->b_data))
+				(struct ext4_dir_entry *)bh->b_data)) {
+			brelse(bh);
 			return -EIO;
+		}
 		set_buffer_verified(bh);
 		retval = add_dirent_to_buf(handle, dentry, inode, NULL, bh);
 		if (retval != -ENOSPC) {
@@ -2493,6 +2499,7 @@ static int empty_dir(struct inode *inode)
 			(struct ext4_dir_entry *)bh->b_data)) {
 		EXT4_ERROR_INODE(inode, "checksum error reading directory "
 				 "lblock 0");
+		brelse(bh);
 		return -EIO;
 	}
 	set_buffer_verified(bh);
@@ -2537,6 +2544,7 @@ static int empty_dir(struct inode *inode)
 					(struct ext4_dir_entry *)bh->b_data)) {
 				EXT4_ERROR_INODE(inode, "checksum error "
 						 "reading directory lblock 0");
+				brelse(bh);
 				return -EIO;
 			}
 			set_buffer_verified(bh);
-- 
1.7.9.5

[PATCH 2/4] ext4: remove unused variable in add_dirent_to_buf()

[Guo Chao]

After commit 978fef9 (create __ext4_insert_dentry for dir entry
insertion), 'reclen' is not used anymore.

Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>
---
 fs/ext4/namei.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index a445247..b3717a3 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1703,7 +1703,6 @@ static int add_dirent_to_buf(handle_t *handle, struct dentry *dentry,
 	const char	*name = dentry->d_name.name;
 	int		namelen = dentry->d_name.len;
 	unsigned int	blocksize = dir->i_sb->s_blocksize;
-	unsigned short	reclen;
 	int		csum_size = 0;
 	int		err;
 
@@ -1711,7 +1710,6 @@ static int add_dirent_to_buf(handle_t *handle, struct dentry *dentry,
 				       EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
 		csum_size = sizeof(struct ext4_dir_entry_tail);
 
-	reclen = EXT4_DIR_REC_LEN(namelen);
 	if (!de) {
 		err = ext4_find_dest_de(dir, inode,
 					bh, bh->b_data, blocksize - csum_size,
-- 
1.7.9.5

Re: [PATCH 2/4] ext4: remove unused variable in add_dirent_to_buf()

[Darrick J. Wong]

On Fri, Jan 18, 2013 at 04:01:12PM +0800, Guo Chao wrote:
> After commit 978fef9 (create __ext4_insert_dentry for dir entry
> insertion), 'reclen' is not used anymore.
> 
> Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>
This one looks ok too.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/ext4/namei.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index a445247..b3717a3 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1703,7 +1703,6 @@ static int add_dirent_to_buf(handle_t *handle, struct dentry *dentry,
>  	const char	*name = dentry->d_name.name;
>  	int		namelen = dentry->d_name.len;
>  	unsigned int	blocksize = dir->i_sb->s_blocksize;
> -	unsigned short	reclen;
>  	int		csum_size = 0;
>  	int		err;
>  
> @@ -1711,7 +1710,6 @@ static int add_dirent_to_buf(handle_t *handle, struct dentry *dentry,
>  				       EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
>  		csum_size = sizeof(struct ext4_dir_entry_tail);
>  
> -	reclen = EXT4_DIR_REC_LEN(namelen);
>  	if (!de) {
>  		err = ext4_find_dest_de(dir, inode,
>  					bh, bh->b_data, blocksize - csum_size,
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] ext4: remove unused variable in add_dirent_to_buf()

[Theodore Ts'o]

On Mon, Jan 28, 2013 at 05:12:18PM -0800, Darrick J. Wong wrote:
> On Fri, Jan 18, 2013 at 04:01:12PM +0800, Guo Chao wrote:
> > After commit 978fef9 (create __ext4_insert_dentry for dir entry
> > insertion), 'reclen' is not used anymore.
> > 
> > Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>
> This one looks ok too.
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks, applied.

					- Ted

[PATCH 3/4] ext4: remove useless assignment in dx_probe()

[Guo Chao]

No body care about the effect of this assignment.

Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>
---
 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 b3717a3..e35ea3d 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -714,7 +714,7 @@ dx_probe(const struct qstr *d_name, struct inode *dir,
 				*err = ERR_BAD_DX_DIR;
 			goto fail2;
 		}
-		at = entries = ((struct dx_node *) bh->b_data)->entries;
+		entries = ((struct dx_node *) bh->b_data)->entries;
 
 		if (!buffer_verified(bh) &&
 		    !ext4_dx_csum_verify(dir,
-- 
1.7.9.5

Re: [PATCH 3/4] ext4: remove useless assignment in dx_probe()

[Darrick J. Wong]

On Fri, Jan 18, 2013 at 04:01:13PM +0800, Guo Chao wrote:
> No body care about the effect of this assignment.
> 
> Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>
> ---
>  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 b3717a3..e35ea3d 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -714,7 +714,7 @@ dx_probe(const struct qstr *d_name, struct inode *dir,
>  				*err = ERR_BAD_DX_DIR;
>  			goto fail2;
>  		}
> -		at = entries = ((struct dx_node *) bh->b_data)->entries;
> +		entries = ((struct dx_node *) bh->b_data)->entries;

The 'at' variable seems to be used (in a if(0)'d code block) to check the
results of the binary search against a slow linear search.  Perhaps we should
get rid of the if(0) hunk about 30 lines up?  The 'at' variable itself could go
too, since it seems to be an alias of "p - 1" and frame->at.

--D
>  
>  		if (!buffer_verified(bh) &&
>  		    !ext4_dx_csum_verify(dir,
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] ext4: remove useless assignment in dx_probe()

[Theodore Ts'o]

On Mon, Jan 28, 2013 at 05:21:50PM -0800, Darrick J. Wong wrote:
> 
> The 'at' variable seems to be used (in a if(0)'d code block) to
> check the results of the binary search against a slow linear search.
> Perhaps we should get rid of the if(0) hunk about 30 lines up?  The
> 'at' variable itself could go too, since it seems to be an alias of
> "p - 1" and frame->at.

What I'd suggest doing (if someone is interested in doing the cleanup)
is moving the code into an inline function which is normally #ifdef'ed
to be an empty function, but which could be enabled if we want enable
the debugging cross check.  This is what we've done in other parts of
the ext4 code base, and by moving the debugging code so it's not
inline with the rest of the function, it should make it more readable.

						- Ted

Re: [PATCH 3/4] ext4: remove useless assignment in dx_probe()

[Theodore Ts'o]

On Fri, Jan 18, 2013 at 04:01:13PM +0800, Guo Chao wrote:
> No body care about the effect of this assignment.
> 
> Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>

Thanks, applied.

					- Ted

[PATCH 4/4] ext4: remove unnecessary NULL pointer check

[Guo Chao]

brelse() and ext4_journal_force_commit() are both inlined and able
to handle NULL.

Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>
---
 fs/ext4/namei.c |    3 +--
 fs/ext4/super.c |    6 +-----
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index e35ea3d..f0812c0 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2110,8 +2110,7 @@ static int ext4_dx_add_entry(handle_t *handle, struct dentry *dentry,
 journal_error:
 	ext4_std_error(dir->i_sb, err);
 cleanup:
-	if (bh)
-		brelse(bh);
+	brelse(bh);
 	dx_release(frames);
 	return err;
 }
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 3d4fb81..f3acd6f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4476,16 +4476,12 @@ static void ext4_clear_journal_err(struct super_block *sb,
 int ext4_force_commit(struct super_block *sb)
 {
 	journal_t *journal;
-	int ret = 0;
 
 	if (sb->s_flags & MS_RDONLY)
 		return 0;
 
 	journal = EXT4_SB(sb)->s_journal;
-	if (journal)
-		ret = ext4_journal_force_commit(journal);

Re: [PATCH 4/4] ext4: remove unnecessary NULL pointer check

[Darrick J. Wong]

On Fri, Jan 18, 2013 at 04:01:14PM +0800, Guo Chao wrote:
> brelse() and ext4_journal_force_commit() are both inlined and able
> to handle NULL.
> 
> Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>

This one looks ok too, so:
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

PS: Does the order of the patches matter?  Or are these just four patches that
are mostly independent of each other?

--D
> ---
>  fs/ext4/namei.c |    3 +--
>  fs/ext4/super.c |    6 +-----
>  2 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index e35ea3d..f0812c0 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -2110,8 +2110,7 @@ static int ext4_dx_add_entry(handle_t *handle, struct dentry *dentry,
>  journal_error:
>  	ext4_std_error(dir->i_sb, err);
>  cleanup:
> -	if (bh)
> -		brelse(bh);
> +	brelse(bh);
>  	dx_release(frames);
>  	return err;
>  }
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 3d4fb81..f3acd6f 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4476,16 +4476,12 @@ static void ext4_clear_journal_err(struct super_block *sb,
>  int ext4_force_commit(struct super_block *sb)
>  {
>  	journal_t *journal;
> -	int ret = 0;
>  
>  	if (sb->s_flags & MS_RDONLY)
>  		return 0;
>  
>  	journal = EXT4_SB(sb)->s_journal;
> -	if (journal)
> -		ret = ext4_journal_force_commit(journal);
> -
> -	return ret;
> +	return ext4_journal_force_commit(journal);
>  }
>  
>  static int ext4_sync_fs(struct super_block *sb, int wait)
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] ext4: remove unnecessary NULL pointer check

[Guo Chao]

On Mon, Jan 28, 2013 at 05:24:13PM -0800, Darrick J. Wong wrote:
> On Fri, Jan 18, 2013 at 04:01:14PM +0800, Guo Chao wrote:
> > brelse() and ext4_journal_force_commit() are both inlined and able
> > to handle NULL.
> > 
> > Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>
> 
> This one looks ok too, so:
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> PS: Does the order of the patches matter?  Or are these just four patches that
> are mostly independent of each other?
> 
> --D

They are random independent fixes.

Thanks

Re: [PATCH 4/4] ext4: remove unnecessary NULL pointer check

[Theodore Ts'o]

On Mon, Jan 28, 2013 at 05:24:13PM -0800, Darrick J. Wong wrote:
> On Fri, Jan 18, 2013 at 04:01:14PM +0800, Guo Chao wrote:
> > brelse() and ext4_journal_force_commit() are both inlined and able
> > to handle NULL.
> > 
> > Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>
> 
> This one looks ok too, so:
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks, applied.

					- Ted

Re: [PATCH 1/4] ext4: release buffer when checksum failed

[Darrick J. Wong]

On Fri, Jan 18, 2013 at 04:01:11PM +0800, Guo Chao wrote:
> Commit b0336e8d (ext4: calculate and verify checksums of directory leaf
> blocks) and commit dbe89444 (ext4: Calculate and verify checksums for
> htree nodes) forget to release buffer when checksum failed, at some places.
> 
> Cc: D. J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>

I think this looks ok...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/ext4/dir.c   |    1 +
>  fs/ext4/namei.c |   12 ++++++++++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> index 80a28b2..3882fbc 100644
> --- a/fs/ext4/dir.c
> +++ b/fs/ext4/dir.c
> @@ -185,6 +185,7 @@ static int ext4_readdir(struct file *filp,
>  					"at offset %llu",
>  					(unsigned long long)filp->f_pos);
>  			filp->f_pos += sb->s_blocksize - offset;
> +			brelse(bh);
>  			continue;
>  		}
>  		set_buffer_verified(bh);
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 8990165..a445247 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -837,6 +837,7 @@ static int ext4_htree_next_block(struct inode *dir, __u32 hash,
>  		    !ext4_dx_csum_verify(dir,
>  					 (struct ext4_dir_entry *)bh->b_data)) {
>  			ext4_warning(dir->i_sb, "Node failed checksum");
> +			brelse(bh);
>  			return -EIO;
>  		}
>  		set_buffer_verified(bh);
> @@ -877,8 +878,11 @@ static int htree_dirblock_to_tree(struct file *dir_file,
>  	}
>  
>  	if (!buffer_verified(bh) &&
> -	    !ext4_dirent_csum_verify(dir, (struct ext4_dir_entry *)bh->b_data))
> +			!ext4_dirent_csum_verify(dir,
> +				(struct ext4_dir_entry *)bh->b_data)) {
> +		brelse(bh);
>  		return -EIO;
> +	}
>  	set_buffer_verified(bh);
>  
>  	de = (struct ext4_dir_entry_2 *) bh->b_data;
> @@ -1929,8 +1933,10 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
>  		}
>  		if (!buffer_verified(bh) &&
>  		    !ext4_dirent_csum_verify(dir,
> -				(struct ext4_dir_entry *)bh->b_data))
> +				(struct ext4_dir_entry *)bh->b_data)) {
> +			brelse(bh);
>  			return -EIO;
> +		}
>  		set_buffer_verified(bh);
>  		retval = add_dirent_to_buf(handle, dentry, inode, NULL, bh);
>  		if (retval != -ENOSPC) {
> @@ -2493,6 +2499,7 @@ static int empty_dir(struct inode *inode)
>  			(struct ext4_dir_entry *)bh->b_data)) {
>  		EXT4_ERROR_INODE(inode, "checksum error reading directory "
>  				 "lblock 0");
> +		brelse(bh);
>  		return -EIO;
>  	}
>  	set_buffer_verified(bh);
> @@ -2537,6 +2544,7 @@ static int empty_dir(struct inode *inode)
>  					(struct ext4_dir_entry *)bh->b_data)) {
>  				EXT4_ERROR_INODE(inode, "checksum error "
>  						 "reading directory lblock 0");
> +				brelse(bh);
>  				return -EIO;
>  			}
>  			set_buffer_verified(bh);
> -- 
> 1.7.9.5
> 

Re: [PATCH 1/4] ext4: release buffer when checksum failed

[Theodore Ts'o]

On Fri, Jan 18, 2013 at 01:28:38PM -0800, Darrick J. Wong wrote:
> On Fri, Jan 18, 2013 at 04:01:11PM +0800, Guo Chao wrote:
> > Commit b0336e8d (ext4: calculate and verify checksums of directory leaf
> > blocks) and commit dbe89444 (ext4: Calculate and verify checksums for
> > htree nodes) forget to release buffer when checksum failed, at some places.
> > 
> > Cc: D. J. Wong <darrick.wong@oracle.com>
> > Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>
> 
> I think this looks ok...
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks, applied.

					- Ted