LinuxLists.cc - [PATCH 1/4] ext4: release buffer when checksum failed

[permalink] [raw]

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

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

Signed-off-by: Guo Chao <[email protected]>
---
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

2013-01-18 08:01:25

[permalink] [raw]

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

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

Signed-off-by: Guo Chao <[email protected]>
---
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);

2013-01-18 08:01:25

[permalink] [raw]

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

No body care about the effect of this assignment.

Signed-off-by: Guo Chao <[email protected]>
---
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

2013-01-18 21:33:02

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Guo Chao <[email protected]>

I think this looks ok...
Reviewed-by: Darrick J. Wong <[email protected]>
> ---
> 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
>

2013-01-29 01:14:03

[permalink] [raw]

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

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 <[email protected]>
This one looks ok too.
Reviewed-by: Darrick J. Wong <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2013-01-29 01:21:57

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2013-01-29 01:24:20

[permalink] [raw]

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

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 <[email protected]>

This one looks ok too, so:
Reviewed-by: Darrick J. Wong <[email protected]>

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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2013-01-29 02:26:02

[permalink] [raw]

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

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 <[email protected]>
> > Signed-off-by: Guo Chao <[email protected]>
>
> I think this looks ok...
> Reviewed-by: Darrick J. Wong <[email protected]>

Thanks, applied.

- Ted

2013-01-29 02:28:06

[permalink] [raw]

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

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 <[email protected]>
> This one looks ok too.
> Reviewed-by: Darrick J. Wong <[email protected]>

Thanks, applied.

- Ted

2013-01-29 02:35:19

[permalink] [raw]

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

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 <[email protected]>

Thanks, applied.

- Ted

2013-01-29 02:37:32

[permalink] [raw]

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

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

2013-01-29 02:40:11

[permalink] [raw]

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

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 <[email protected]>
>
> This one looks ok too, so:
> Reviewed-by: Darrick J. Wong <[email protected]>
>
> 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

2013-01-29 02:42:23