[PATCH 2/2] Ext3: remove a useless check for the function ext3_free_blocks_sb

[PATCH 2/2] Ext3: remove a useless check for the function ext3_free_blocks_sb

[Wang Shilong]

From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>

Because 'block + count < block' always comes to false, it is useless
to have this check, just remove it.

Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
---
 fs/ext3/balloc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/ext3/balloc.c b/fs/ext3/balloc.c
index 22548f5..c557f22 100644
--- a/fs/ext3/balloc.c
+++ b/fs/ext3/balloc.c
@@ -507,7 +507,6 @@ void ext3_free_blocks_sb(handle_t *handle, struct super_block *sb,
 	sbi = EXT3_SB(sb);
 	es = sbi->s_es;
 	if (block < le32_to_cpu(es->s_first_data_block) ||
-	    block + count < block ||
 	    block + count > le32_to_cpu(es->s_blocks_count)) {
 		ext3_error (sb, "ext3_free_blocks",
 			    "Freeing blocks not in datazone - "
-- 1.7.11.7

Re: [PATCH 2/2] Ext3: remove a useless check for the function ext3_free_blocks_sb

[Andreas Dilger]

On 2011-01-26, at 15:48, Wang Shilong <wangshilong1991@gmail.com> wrote:

> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
> 
> Because 'block + count < block' always comes to false, it is useless
> to have this check, just remove it.

This check can be true if the sum overflows the 32-bit variable it is stored in, so it is not useless...

Cheers, Andreas

> Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
> ---
> fs/ext3/balloc.c | 1 -
> 1 file changed, 1 deletion(-)
> 
> diff --git a/fs/ext3/balloc.c b/fs/ext3/balloc.c
> index 22548f5..c557f22 100644
> --- a/fs/ext3/balloc.c
> +++ b/fs/ext3/balloc.c
> @@ -507,7 +507,6 @@ void ext3_free_blocks_sb(handle_t *handle, struct super_block *sb,
>    sbi = EXT3_SB(sb);
>    es = sbi->s_es;
>    if (block < le32_to_cpu(es->s_first_data_block) ||
> -        block + count < block ||
>        block + count > le32_to_cpu(es->s_blocks_count)) {
>        ext3_error (sb, "ext3_free_blocks",
>                "Freeing blocks not in datazone - "
> -- 1.7.11.7
> 
> --
> 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

[PATCH 2/2] Ext3: remove a useless check for the function ext3_free_blocks_sb

[Wang Shilong]

From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>

Because 'block + count < block' always comes to false, it is useless
to have this check, just remove it.

Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
---
 fs/ext3/balloc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/ext3/balloc.c b/fs/ext3/balloc.c
index 22548f5..c557f22 100644
--- a/fs/ext3/balloc.c
+++ b/fs/ext3/balloc.c
@@ -507,7 +507,6 @@ void ext3_free_blocks_sb(handle_t *handle, struct super_block *sb,
 	sbi = EXT3_SB(sb);
 	es = sbi->s_es;
 	if (block < le32_to_cpu(es->s_first_data_block) ||
-	    block + count < block ||
 	    block + count > le32_to_cpu(es->s_blocks_count)) {
 		ext3_error (sb, "ext3_free_blocks",
 			    "Freeing blocks not in datazone - "
-- 1.7.11.7

[PATCH 2/2] Ext3: remove a useless check for the function ext3_free_blocks_sb

[Wang Shilong]

From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>

Because 'block + count < block' always comes to false, it is useless
to have this check, just remove it.

Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
---
 fs/ext3/balloc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/ext3/balloc.c b/fs/ext3/balloc.c
index 22548f5..c557f22 100644
--- a/fs/ext3/balloc.c
+++ b/fs/ext3/balloc.c
@@ -507,7 +507,6 @@ void ext3_free_blocks_sb(handle_t *handle, struct super_block *sb,
 	sbi = EXT3_SB(sb);
 	es = sbi->s_es;
 	if (block < le32_to_cpu(es->s_first_data_block) ||
-	    block + count < block ||
 	    block + count > le32_to_cpu(es->s_blocks_count)) {
 		ext3_error (sb, "ext3_free_blocks",
 			    "Freeing blocks not in datazone - "
-- 1.7.11.7

Re: [PATCH 2/2] Ext3: remove a useless check for the function ext3_free_blocks_sb

[Jan Kara]

On Sat 26-01-13 14:58:31, Wang Shilong wrote:
> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
> 
> Because 'block + count < block' always comes to false, it is useless
> to have this check, just remove it.
  As Andreas commented, the test is actually correct. BTW any reason why
you sent the patch three times?

								Honza

> Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
> ---
>  fs/ext3/balloc.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/ext3/balloc.c b/fs/ext3/balloc.c
> index 22548f5..c557f22 100644
> --- a/fs/ext3/balloc.c
> +++ b/fs/ext3/balloc.c
> @@ -507,7 +507,6 @@ void ext3_free_blocks_sb(handle_t *handle, struct super_block *sb,
>  	sbi = EXT3_SB(sb);
>  	es = sbi->s_es;
>  	if (block < le32_to_cpu(es->s_first_data_block) ||
> -	    block + count < block ||
>  	    block + count > le32_to_cpu(es->s_blocks_count)) {
>  		ext3_error (sb, "ext3_free_blocks",
>  			    "Freeing blocks not in datazone - "
> -- 1.7.11.7
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 2/2] Ext3: remove a useless check for the function ext3_free_blocks_sb

[Wang Shilong]

于 2013-1-28 6:07, Jan Kara 写道:
> On Sat 26-01-13 14:58:31, Wang Shilong wrote:
>> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>>
>> Because 'block + count < block' always comes to false, it is useless
>> to have this check, just remove it.
>   As Andreas commented, the test is actually correct. BTW any reason why
> you sent the patch three times?
My linux run in the virtual machine..and the clock is wrong...so i send the patch not clarified correctly
in marc.info..when i find it..I correct it, but first time i correct the time as 2012....how stupid it was..:-[
so i send the patch three times...sorry to bother...

BTW ， may i have a question....
As we know the block to be freed can not be superblock and GDT..
I don't see any check about it in ext2/ext3/ext4....

Thanks,
Wang
>
> 								Honza
>
>> Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>> ---
>>  fs/ext3/balloc.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/fs/ext3/balloc.c b/fs/ext3/balloc.c
>> index 22548f5..c557f22 100644
>> --- a/fs/ext3/balloc.c
>> +++ b/fs/ext3/balloc.c
>> @@ -507,7 +507,6 @@ void ext3_free_blocks_sb(handle_t *handle, struct super_block *sb,
>>  	sbi = EXT3_SB(sb);
>>  	es = sbi->s_es;
>>  	if (block < le32_to_cpu(es->s_first_data_block) ||
>> -	    block + count < block ||
>>  	    block + count > le32_to_cpu(es->s_blocks_count)) {
>>  		ext3_error (sb, "ext3_free_blocks",
>>  			    "Freeing blocks not in datazone - "
>> -- 1.7.11.7
>>

--
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/2] Ext3: remove a useless check for the function ext3_free_blocks_sb

[Jan Kara]

On Mon 28-01-13 22:01:21, Wang Shilong wrote:
> 于 2013-1-28 6:07, Jan Kara 写道:
> > On Sat 26-01-13 14:58:31, Wang Shilong wrote:
> >> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
> >>
> >> Because 'block + count < block' always comes to false, it is useless
> >> to have this check, just remove it.
> >   As Andreas commented, the test is actually correct. BTW any reason why
> > you sent the patch three times?
> My linux run in the virtual machine..and the clock is wrong...so i send the patch not clarified correctly
> in marc.info..when i find it..I correct it, but first time i correct the time as 2012....how stupid it was..:-[
> so i send the patch three times...sorry to bother...
> 
> BTW ， may i have a question....
> As we know the block to be freed can not be superblock and GDT..
> I don't see any check about it in ext2/ext3/ext4....
  Yes, you are right we apparently don't check for superblock or GDT
blocks. But checking for those will be a bit more complex and they are
really scarce so I don't think it's worth the overhead.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--
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/2] Ext3: remove a useless check for the function ext3_free_blocks_sb

[Theodore Ts'o]

On Tue, Jan 29, 2013 at 03:40:14PM +0100, Jan Kara wrote:
> > As we know the block to be freed can not be superblock and GDT..
> > I don't see any check about it in ext2/ext3/ext4....
>   Yes, you are right we apparently don't check for superblock or GDT
> blocks. But checking for those will be a bit more complex and they are
> really scarce so I don't think it's worth the overhead.

We actually do have code to test for that in ext4.  Take a look at
fs/ext4/block_validity.c.  We normally only check to make sure the
data block is within the file system bounds, but if you mount the file
system with the block_validity, it will actually check to make sure
the block being allocated or freed is not part of the superblock, GDT,
allocation bitmaps, or inode table.

I use this mount option when running my xfstests, just to add an
additional level of checking.  It's not enabled by default, since it
does increase CPU usage.  I suspect it would only be visible for
benchmarks such as TPC-C, but it's not something we've actually
measured to see if we could afford to enable by default.

The other major short coming is that we don't update the system zone
after an online resize, which means we don't protect the newly
metadata blocks until the next time the file system is mounted.  If we
added updating after a online resize, we'd also have to add some kind
of locking to protect the rbtree while it is being changed, and this
would increase its overhead if people wanted to use it in production.
We might be able to use RCU to avoid doing a real hard lock, but it's
not something that I've considered high priority.

If someone wanted to look at this, it would actually be a pretty good
starter project for someone who wanted to get started with doing ext4
hacking.

Cheers,

						- Ted