Received-SPF: pass (google.com: best guess record for domain of linux-ext4-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2] ext4: remove code duplication in free_ind_block()
To:     Jan Kara <jack@suse.cz>
Cc:     Theodore Ts'o <tytso@mit.edu>,
        Andreas Dilger <adilger.kernel@dilger.ca>,
        linux-ext4@vger.kernel.org
References: <7751907d-738f-a533-8be9-78c6aff5c8be@virtuozzo.com>
 <20190529150142.GA2081@quack2.suse.cz>
From:   Vasily Averin <vvs@virtuozzo.com>
Message-ID: <9f64b443-3344-1fd0-ac3b-75887eb1e629@virtuozzo.com>
Date:   Thu, 30 May 2019 10:13:26 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.0
MIME-Version: 1.0
In-Reply-To: <20190529150142.GA2081@quack2.suse.cz>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

On 5/29/19 6:01 PM, Jan Kara wrote:
> On Tue 12-03-19 16:09:12, Vasily Averin wrote:
>> free_ind_block(), free_dind_blocks() and free_tind_blocks() are replaced
>> by a single recursive function.
>> v2: rebase to v5.0
>>
>> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> 
> Thanks for the patch! Nice cleanup. The patch looks good to me. Feel free
> to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> Just one question. How did you test this? And if you have a testcase for
> this code, can you please add it to fstests so that it gets excercised?

Frankly speaking I've very carefully reviewed these changes,
complied and booted, but not tested it under any real load.

Thank you,
	Vasily Averin

>> ---
>>  fs/ext4/migrate.c | 115 +++++++++++++---------------------------------
>>  1 file changed, 32 insertions(+), 83 deletions(-)
>>
>> diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
>> index fde2f1bc96b0..6b811b7d110c 100644
>> --- a/fs/ext4/migrate.c
>> +++ b/fs/ext4/migrate.c
>> @@ -157,100 +157,43 @@ static int extend_credit_for_blkdel(handle_t *handle, struct inode *inode)
>>  	return retval;
>>  }
>>  
>> -static int free_dind_blocks(handle_t *handle,
>> -				struct inode *inode, __le32 i_data)
>> +static int free_ind_blocks(handle_t *handle,
>> +				struct inode *inode, __le32 i_data, int ind)
>>  {
>> -	int i;
>> -	__le32 *tmp_idata;
>> -	struct buffer_head *bh;
>> -	unsigned long max_entries = inode->i_sb->s_blocksize >> 2;
>> -
>> -	bh = ext4_sb_bread(inode->i_sb, le32_to_cpu(i_data), 0);
>> -	if (IS_ERR(bh))
>> -		return PTR_ERR(bh);
>> -
>> -	tmp_idata = (__le32 *)bh->b_data;
>> -	for (i = 0; i < max_entries; i++) {
>> -		if (tmp_idata[i]) {
>> -			extend_credit_for_blkdel(handle, inode);
>> -			ext4_free_blocks(handle, inode, NULL,
>> -					 le32_to_cpu(tmp_idata[i]), 1,
>> -					 EXT4_FREE_BLOCKS_METADATA |
>> -					 EXT4_FREE_BLOCKS_FORGET);
>> -		}
>> -	}
>> -	put_bh(bh);
>> -	extend_credit_for_blkdel(handle, inode);
>> -	ext4_free_blocks(handle, inode, NULL, le32_to_cpu(i_data), 1,
>> -			 EXT4_FREE_BLOCKS_METADATA |
>> -			 EXT4_FREE_BLOCKS_FORGET);
>> -	return 0;
>> -}
>> -
>> -static int free_tind_blocks(handle_t *handle,
>> -				struct inode *inode, __le32 i_data)
>> -{
>> -	int i, retval = 0;
>> -	__le32 *tmp_idata;
>> -	struct buffer_head *bh;
>> -	unsigned long max_entries = inode->i_sb->s_blocksize >> 2;
>> -
>> -	bh = ext4_sb_bread(inode->i_sb, le32_to_cpu(i_data), 0);
>> -	if (IS_ERR(bh))
>> -		return PTR_ERR(bh);
>> -
>> -	tmp_idata = (__le32 *)bh->b_data;
>> -	for (i = 0; i < max_entries; i++) {
>> -		if (tmp_idata[i]) {
>> -			retval = free_dind_blocks(handle,
>> -					inode, tmp_idata[i]);
>> -			if (retval) {
>> -				put_bh(bh);
>> -				return retval;
>> +	if (ind > 0) {
>> +		int retval = 0;
>> +		__le32 *tmp_idata;
>> +		ext4_lblk_t i, max_entries;
>> +		struct buffer_head *bh;
>> +
>> +		bh = ext4_sb_bread(inode->i_sb, le32_to_cpu(i_data), 0);
>> +		if (IS_ERR(bh))
>> +			return PTR_ERR(bh);
>> +
>> +		tmp_idata = (__le32 *)bh->b_data;
>> +		max_entries = inode->i_sb->s_blocksize >> 2;
>> +		for (i = 0; i < max_entries; i++) {
>> +			if (tmp_idata[i]) {
>> +				retval = free_ind_blocks(handle,
>> +						inode, tmp_idata[i], ind - 1);
>> +				if (retval) {
>> +					put_bh(bh);
>> +					return retval;
>> +				}
>>  			}
>>  		}
>> +		put_bh(bh);
>>  	}
>> -	put_bh(bh);
>>  	extend_credit_for_blkdel(handle, inode);
>>  	ext4_free_blocks(handle, inode, NULL, le32_to_cpu(i_data), 1,
>> -			 EXT4_FREE_BLOCKS_METADATA |
>> -			 EXT4_FREE_BLOCKS_FORGET);
>> -	return 0;
>> -}
>> -
>> -static int free_ind_block(handle_t *handle, struct inode *inode, __le32 *i_data)
>> -{
>> -	int retval;
>> -
>> -	/* ei->i_data[EXT4_IND_BLOCK] */
>> -	if (i_data[0]) {
>> -		extend_credit_for_blkdel(handle, inode);
>> -		ext4_free_blocks(handle, inode, NULL,
>> -				le32_to_cpu(i_data[0]), 1,
>> -				 EXT4_FREE_BLOCKS_METADATA |
>> -				 EXT4_FREE_BLOCKS_FORGET);
>> -	}
>> -
>> -	/* ei->i_data[EXT4_DIND_BLOCK] */
>> -	if (i_data[1]) {
>> -		retval = free_dind_blocks(handle, inode, i_data[1]);
>> -		if (retval)
>> -			return retval;
>> -	}
>> -
>> -	/* ei->i_data[EXT4_TIND_BLOCK] */
>> -	if (i_data[2]) {
>> -		retval = free_tind_blocks(handle, inode, i_data[2]);
>> -		if (retval)
>> -			return retval;
>> -	}
>> +			 EXT4_FREE_BLOCKS_METADATA | EXT4_FREE_BLOCKS_FORGET);
>>  	return 0;
>>  }
>>  
>>  static int ext4_ext_swap_inode_data(handle_t *handle, struct inode *inode,
>>  						struct inode *tmp_inode)
>>  {
>> -	int retval;
>> +	int i, retval;
>>  	__le32	i_data[3];
>>  	struct ext4_inode_info *ei = EXT4_I(inode);
>>  	struct ext4_inode_info *tmp_ei = EXT4_I(tmp_inode);
>> @@ -307,7 +250,13 @@ static int ext4_ext_swap_inode_data(handle_t *handle, struct inode *inode,
>>  	 * We mark the inode dirty after, because we decrement the
>>  	 * i_blocks when freeing the indirect meta-data blocks
>>  	 */
>> -	retval = free_ind_block(handle, inode, i_data);
>> +	for (i = 0; i < ARRAY_SIZE(i_data); i++) {
>> +		if (i_data[i]) {
>> +			retval = free_ind_blocks(handle, inode, i_data[i], i);
>> +			if (retval)
>> +				break;
>> +		}
>> +	}
>>  	ext4_mark_inode_dirty(handle, inode);
>>  
>>  err_out:
>> -- 
>> 2.17.1
>>