ext4: deleted unnecessary assignments and useless "if" statement

PATCH In review jon ernst 3 participants 6 messages Jun 20, 2013 – Jun 24, 2013 ext4 file system

lore

Share & tools

lore

jon ernst

author

2013-06-20 05:44 UTC #0 lore raw

PATCH 1/1 ext4: deleted unnecessary assignments and useless "if" statement

comparing unsigned variable with 0 always return false. err = 0 is duplicated and unnecessary.

Signed-off-by "Jon Ernst" <[email protected]>

fs/ext4/inode.c +0 -4

@@ -1098,8 +1098,6 @@ static int ext4_write_end(struct file *f
        if (i_size_changed)
                ext4_mark_inode_dirty(handle, inode);
 
-       if (copied < 0)
-               ret = copied;
        if (pos + len > inode->i_size && ext4_can_truncate(inode))
                /* if we have allocated more blocks and copied
                 * less. We will have blocks allocated outside

@@ -3365,7 +3363,6 @@ int ext4_block_zero_page_range(handle_t
                pos += blocksize;
        }
 
-       err = 0;
        if (buffer_freed(bh)) {
                BUFFER_TRACE(bh, "freed: skip");
                goto unlock;

@@ -3405,7 +3402,6 @@ int ext4_block_zero_page_range(handle_t
 
        BUFFER_TRACE(bh, "zeroed end of block");
 
-       err = 0;
        if (ext4_should_journal_data(inode)) {
                err = ext4_handle_dirty_metadata(handle, inode, bh);
        } else {

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

Theodore Ts'o

maintainer

2013-06-20 14:44 UTC #1 lore raw

Quoted 4 lines · jon ernst

comparing unsigned variable with 0 always return false. err = 0 is duplicated and unnecessary.
Signed-off-by "Jon Ernst" <[email protected]>

I've applied this with a few changes.

(a) your patch was white space corrupted
(b) one of the err = 0 which you removed was actually necessary.

I've reworked the error handling in ext4_block_zero_page_range() so it's a bit more obvious.

Also your change showed up a potential issue with ext4_write_end(), in that ext4_write_inline_data_end() returns an int, which can be negative in the case of an error, and it's assigning this to copied, which is an unsigned int.

- Ted

Quoted 1 line

From 6e4837a3e8bc04a1c49f8668c32ce3239968a8ce Mon Sep 17 00:00:00 2001

From: jon ernst <[email protected]>
Date: Thu, 20 Jun 2013 10:42:05 -0400
Subject: [PATCH] ext4: delete unnecessary C statements

Comparing unsigned variable with 0 always returns false. err = 0 is duplicated and unnecessary.

Signed-off-by "Jon Ernst" <[email protected]>

Signed-off-by "Theodore Ts'o" <[email protected]>

----

---
 fs/ext4/inode.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

fs/ext4/inode.c +1 -8

index 78cf398..baf5c2b 100644
@@ -1100,8 +1100,6 @@ static int ext4_write_end(struct file *file,
 	if (i_size_changed)
 		ext4_mark_inode_dirty(handle, inode);
 
-	if (copied < 0)
-		ret = copied;
 	if (pos + len > inode->i_size && ext4_can_truncate(inode))
 		/* if we have allocated more blocks and copied
 		 * less. We will have blocks allocated outside

@@ -3336,7 +3334,7 @@ int ext4_block_zero_page_range(handle_t *handle,
 	struct inode *inode = mapping->host;
 	struct buffer_head *bh;
 	struct page *page;
-	int err = 0;
+	int err;
 
 	page = find_or_create_page(mapping, from >> PAGE_CACHE_SHIFT,
 				   mapping_gfp_mask(mapping) & ~__GFP_FS);

@@ -3366,13 +3364,10 @@ int ext4_block_zero_page_range(handle_t *handle,
 		iblock++;
 		pos += blocksize;
 	}
-
-	err = 0;
 	if (buffer_freed(bh)) {
 		BUFFER_TRACE(bh, "freed: skip");
 		goto unlock;
 	}
-
 	if (!buffer_mapped(bh)) {
 		BUFFER_TRACE(bh, "unmapped");
 		ext4_get_block(inode, iblock, bh, 0);

@@ -3395,22 +3390,19 @@ int ext4_block_zero_page_range(handle_t *handle,
 		if (!buffer_uptodate(bh))
 			goto unlock;
 	}
-
 	if (ext4_should_journal_data(inode)) {
 		BUFFER_TRACE(bh, "get write access");
 		err = ext4_journal_get_write_access(handle, bh);
 		if (err)
 			goto unlock;
 	}
-
 	zero_user(page, offset, length);

Theodore Ts'o

maintainer

2013-06-20 14:52 UTC #2 lore raw

PATCH 1/1 ext4: check error return from ext4_write_inline_data_end()

The function ext4_write_inline_data_end() can return an error. So we need to assign it to a signed integer variable to check for an error return (since copied is an unsigned int).

Signed-off-by "Theodore Ts'o" <[email protected]>

Cc Zheng Liu <[email protected]>

Cc [email protected]

---
 fs/ext4/inode.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

fs/ext4/inode.c +7 -4

index baf5c2b..567a734 100644
@@ -1061,10 +1061,13 @@ static int ext4_write_end(struct file *file,
 		}
 	}
 
-	if (ext4_has_inline_data(inode))
-		copied = ext4_write_inline_data_end(inode, pos, len,
-						    copied, page);
-	else
+	if (ext4_has_inline_data(inode)) {
+		ret = ext4_write_inline_data_end(inode, pos, len,
+						 copied, page);
+		if (ret < 0)
+			goto errout;
+		copied = ret;
+	} else
 		copied = block_write_end(file, mapping, pos,
 					 len, copied, page, fsdata);

Signature · 1 line

1.7.12.rc0.22.gcdd159b

Theodore Ts'o

maintainer

2013-06-20 19:45 UTC #3 lore raw

PATCH 1/1 ext4: delete unnecessary C statements

From: jon ernst <[email protected]>

Comparing unsigned variable with 0 always returns false. err = 0 is duplicated and unnecessary.

[ tytso: Also cleaned up error handling in ext4_block_zero_page_range() ]

Signed-off-by "Jon Ernst" <[email protected]>

Signed-off-by "Theodore Ts'o" <[email protected]>

---
 fs/ext4/inode.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

fs/ext4/inode.c +0 -7

index 78cf398..67963cf 100644
@@ -1100,8 +1100,6 @@ static int ext4_write_end(struct file *file,
 	if (i_size_changed)
 		ext4_mark_inode_dirty(handle, inode);
 
-	if (copied < 0)
-		ret = copied;
 	if (pos + len > inode->i_size && ext4_can_truncate(inode))
 		/* if we have allocated more blocks and copied
 		 * less. We will have blocks allocated outside

@@ -3366,13 +3364,10 @@ int ext4_block_zero_page_range(handle_t *handle,
 		iblock++;
 		pos += blocksize;
 	}
-
-	err = 0;
 	if (buffer_freed(bh)) {
 		BUFFER_TRACE(bh, "freed: skip");
 		goto unlock;
 	}
-
 	if (!buffer_mapped(bh)) {
 		BUFFER_TRACE(bh, "unmapped");
 		ext4_get_block(inode, iblock, bh, 0);

@@ -3395,22 +3390,19 @@ int ext4_block_zero_page_range(handle_t *handle,
 		if (!buffer_uptodate(bh))
 			goto unlock;
 	}
-
 	if (ext4_should_journal_data(inode)) {
 		BUFFER_TRACE(bh, "get write access");
 		err = ext4_journal_get_write_access(handle, bh);
 		if (err)
 			goto unlock;
 	}
-
 	zero_user(page, offset, length);

Zheng Liu

Reviewed-by on v1 1/1

2013-06-24 13:33 UTC #4 lore raw

Re: [PATCH -v3] ext4: delete unnecessary C statements

Quoted 9 lines · Theodore Ts'o

From: jon ernst <[email protected]>
Comparing unsigned variable with 0 always returns false. err = 0 is duplicated and unnecessary.
[ tytso: Also cleaned up error handling in ext4_block_zero_page_range() ]
Signed-off-by "Jon Ernst" <[email protected]>
Signed-off-by "Theodore Ts'o" <[email protected]>

Reviewed-by Zheng Liu <[email protected]>

- Zheng

Quoted 62 lines

---
 fs/ext4/inode.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

fs/ext4/inode.c +1 -9

index 78cf398..67963cf 100644
@@ -1100,8 +1100,6 @@ static int ext4_write_end(struct file *file,
 	if (i_size_changed)
 		ext4_mark_inode_dirty(handle, inode);
 
-	if (copied < 0)
-		ret = copied;
 	if (pos + len > inode->i_size && ext4_can_truncate(inode))
 		/* if we have allocated more blocks and copied
 		 * less. We will have blocks allocated outside

@@ -3366,13 +3364,10 @@ int ext4_block_zero_page_range(handle_t *handle,
 		iblock++;
 		pos += blocksize;
 	}
-
-	err = 0;
 	if (buffer_freed(bh)) {
 		BUFFER_TRACE(bh, "freed: skip");
 		goto unlock;
 	}
-
 	if (!buffer_mapped(bh)) {
 		BUFFER_TRACE(bh, "unmapped");
 		ext4_get_block(inode, iblock, bh, 0);

@@ -3395,22 +3390,19 @@ int ext4_block_zero_page_range(handle_t *handle,
 		if (!buffer_uptodate(bh))
 			goto unlock;
 	}
-
 	if (ext4_should_journal_data(inode)) {
 		BUFFER_TRACE(bh, "get write access");
 		err = ext4_journal_get_write_access(handle, bh);
 		if (err)
 			goto unlock;
 	}
-
 	zero_user(page, offset, length);
-
 	BUFFER_TRACE(bh, "zeroed end of block");
 
-	err = 0;
 	if (ext4_should_journal_data(inode)) {
 		err = ext4_handle_dirty_metadata(handle, inode, bh);
 	} else {
+		err = 0;
 		mark_buffer_dirty(bh);
 		if (ext4_test_inode_state(inode, EXT4_STATE_ORDERED_MODE))
 			err = ext4_jbd2_file_inode(handle, inode);

Signature · 6 lines

1.7.12.rc0.22.gcdd159b

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

Zheng Liu

Reviewed-by on v1 1/1

2013-06-24 13:40 UTC #5 lore raw

Re: [PATCH] ext4: check error return from ext4_write_inline_data_end()

In reply to Theodore Ts'o

Quoted 7 lines · Theodore Ts'o

The function ext4_write_inline_data_end() can return an error. So we need to assign it to a signed integer variable to check for an error return (since copied is an unsigned int).
Signed-off-by "Theodore Ts'o" <[email protected]>
Cc Zheng Liu <[email protected]>
Cc [email protected]

Sorry for the late reply. Thanks for fixing this. The patch looks good to me.

Reviewed-by Zheng Liu <[email protected]>

- Zheng

Quoted 33 lines

---
 fs/ext4/inode.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

fs/ext4/inode.c +7 -4

index baf5c2b..567a734 100644
@@ -1061,10 +1061,13 @@ static int ext4_write_end(struct file *file,
 		}
 	}
 
-	if (ext4_has_inline_data(inode))
-		copied = ext4_write_inline_data_end(inode, pos, len,
-						    copied, page);
-	else
+	if (ext4_has_inline_data(inode)) {
+		ret = ext4_write_inline_data_end(inode, pos, len,
+						 copied, page);
+		if (ret < 0)
+			goto errout;
+		copied = ret;
+	} else
 		copied = block_write_end(file, mapping, pos,
 					 len, copied, page, fsdata);

Signature · 6 lines

1.7.12.rc0.22.gcdd159b

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