ext4: Only call update_backups if necessary in online resize.

From: Tao Ma <boyu.mt@taobao.com>

Now in online resize, we will add a bunch of groups at one time in
ext4_flex_group_add, so in most cases a lot of group descriptors
will be in the same group block. But in the end of this function,
update_backups will be called for every group descriptor and the
same block will be copied and journalled again and again.
It is really a waste.

So add a old_gdb to store the group block we have already done
the backup and skip the backup until we meet with a new group block.

Signed-off-by: Tao Ma <boyu.mt@taobao.com>
---
 fs/ext4/resize.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

On Mon, Sep 24, 2012 at 11:00:17PM +0800, Tao Ma wrote:
> @@ -1355,8 +1356,16 @@ exit_journal:
>  			int gdb_num;
>  			gdb_num = group / EXT4_BLOCKS_PER_GROUP(sb);
>  			gdb_bh = sbi->s_group_desc[gdb_num];
> -			update_backups(sb, gdb_bh->b_blocknr, gdb_bh->b_data,
> -				       gdb_bh->b_size);
> +			/*
> +			 * only backup the group descriptor block
> +			 * which hasn't been updated before.
> +			 */
> +			if (old_gdb != gdb_bh->b_blocknr) {
> +				update_backups(sb, gdb_bh->b_blocknr,
> +					       gdb_bh->b_data,
> +					       gdb_bh->b_size);
> +				old_gdb = gdb_bh->b_blocknr;
> +			}
>  		}
>  	}

Don't we also need to add a call to update_backups() at the end of the
loop so we update the backup block descriptors for the very last set
of block groups?

Thanks,

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

On 09/26/2012 11:39 AM, Theodore Ts'o wrote:
> On Mon, Sep 24, 2012 at 11:00:17PM +0800, Tao Ma wrote:
>> @@ -1355,8 +1356,16 @@ exit_journal:
>>  			int gdb_num;
>>  			gdb_num = group / EXT4_BLOCKS_PER_GROUP(sb);
>>  			gdb_bh = sbi->s_group_desc[gdb_num];
>> -			update_backups(sb, gdb_bh->b_blocknr, gdb_bh->b_data,
>> -				       gdb_bh->b_size);
>> +			/*
>> +			 * only backup the group descriptor block
>> +			 * which hasn't been updated before.
>> +			 */
>> +			if (old_gdb != gdb_bh->b_blocknr) {
>> +				update_backups(sb, gdb_bh->b_blocknr,
>> +					       gdb_bh->b_data,
>> +					       gdb_bh->b_size);
>> +				old_gdb = gdb_bh->b_blocknr;
>> +			}
>>  		}
>>  	}
> 
> Don't we also need to add a call to update_backups() at the end of the
> loop so we update the backup block descriptors for the very last set
> of block groups?
Why?  If it isn't the same as the previous one, it will be updated. If
it is the same, it don't need to be updated. I don't see what your mean
here.

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

On Wed, Sep 26, 2012 at 11:51:01AM +0800, Tao Ma wrote:
> Why?  If it isn't the same as the previous one, it will be updated. If
> it is the same, it don't need to be updated. I don't see what your mean
> here.

Oh, I see; sorry, I was just looking at the patch and not the file
with the changes in context.  I thought you were updating the backup
descriptors after modifying the last block group in the meta_bg group,
but that was my confusion.

Yes, your patch looks good and I will apply it, thanks!!

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

2012/9/26 Theodore Ts'o <tytso@mit.edu>
>
> On Wed, Sep 26, 2012 at 11:51:01AM +0800, Tao Ma wrote:
> > Why?  If it isn't the same as the previous one, it will be updated. If
> > it is the same, it don't need to be updated. I don't see what your mean
> > here.
>
> Oh, I see; sorry, I was just looking at the patch and not the file
> with the changes in context.  I thought you were updating the backup
> descriptors after modifying the last block group in the meta_bg group,
> but that was my confusion.
>
> Yes, your patch looks good and I will apply it, thanks!!
>
>                                       - Ted

Hi Ted,

Is this patch should be merged with your previous patches about meta_bg
online resize on 9/14?

[PATCH 2/9] avoid duplicate writes of the backup bg descriptor blocks
[PATCH 5/9] add online resizing support for meta_bg and 64-bit file systems

Regards,
Kevin Liao
--
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

On Wed, Sep 26, 2012 at 01:26:14PM +0800, Kevin Liao wrote:
> 
> Is this patch should be merged with your previous patches about meta_bg
> online resize on 9/14?
> 
> [PATCH 2/9] avoid duplicate writes of the backup bg descriptor blocks
> [PATCH 5/9] add online resizing support for meta_bg and 64-bit file systems

It would be, except that those patches are now in the "master" branch
of the ext4 git tree, which is an implicit promise on my part not to
rebase or change the commit.  This allows people to build git trees
based on the "master" branch.  Commits between "master" and "dev" are
still subject to change, although I am pretty happen with them at that
point, so it will general be only to fix bugs in the patches, or to
adjust the commit descrpition, etc.

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