Dear Ted,
could you please comment the state of this patch?
Should I re-new or resend it?
Do you probably have some objections or may be expect some troubles related to this patch?
On 5/30/19 10:13 AM, Vasily Averin wrote:
> 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@xxxxxxxxxxxxx>
>>
>> Thanks for the patch! Nice cleanup. The patch looks good to me. Feel free
>> to add:
>>
>> Reviewed-by: Jan Kara <jack@xxxxxxx>
>>
>> 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
>>>
