[Devel] [PATCH] vz6 ext4: Discard preallocated block before swap_extents

[Vasily Averin]

Dima,
please take look at comment below.

On 2017-02-25 18:16, Dmitry Monakhov wrote:
> Inode preallocation consists of two parts (used and unused) fully controlled
> by inode, so it must be discarded before swap extents.
> Currently we may skip drop_preallocation if file is sparse.
> 
> This patch does:
> - Moves ext4_discard_preallocations to ext4_swap_extents.
>   This makes more readable and reliable for future changes.
> - Cleanup main move_extent loop
> 
> https://jira.sw.ru/browse/PSBM-57003
> xfstests:ext4/024 (pended: https://github.com/dmonakhov/xfstests/commit/7a4763963f73ea5d5bba45eefa484494aa3df7cf)
> Signed-off-by: Dmitry Monakhov <dmonakhov at openvz.org>
> ---
>  fs/ext4/extents.c     |  3 +++
>  fs/ext4/move_extent.c | 17 +++++++----------
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 85c4d4e..fd49ab0 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4371,6 +4371,9 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
>  	BUG_ON(!mutex_is_locked(&inode1->i_mutex));
>  	BUG_ON(!mutex_is_locked(&inode1->i_mutex));
>  
> +	ext4_discard_preallocations(inode1);
> +	ext4_discard_preallocations(inode2);
> +
>  	while (count) {
>  		struct ext4_extent *ex1, *ex2, tmp_ex;
>  		ext4_lblk_t e1_blk, e2_blk;
> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> index 39eaa8f..97a7db5 100644
> --- a/fs/ext4/move_extent.c
> +++ b/fs/ext4/move_extent.c
> @@ -628,6 +628,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
>  	ext4_lblk_t o_end, o_start = orig_blk;
>  	ext4_lblk_t d_start = donor_blk;
>  	int ret;
> +	__u64 m_len = *moved_len;
>  
>  	if (orig_inode->i_sb != donor_inode->i_sb) {
>  		ext4_debug("ext4 move extent: The argument files "
> @@ -696,7 +697,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
>  			if (next_blk == EXT_MAX_BLOCKS) {
>  				o_start = o_end;
>  				ret = -ENODATA;
> -				goto out;
> +				break;
>  			}
>  			d_start += next_blk - o_start;
>  			o_start = next_blk;
> @@ -708,7 +709,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
>  			o_start = cur_blk;
>  			/* Extent inside requested range ?*/
>  			if (cur_blk >= o_end)
> -				goto out;
> +				break;
>  		} else { /* in_range(o_start, o_blk, o_len) */
>  			cur_len += cur_blk - o_start;
>  		}
> @@ -743,6 +744,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
>  			break;
>  		o_start += cur_len;
>  		d_start += cur_len;
> +		m_len += cur_len;
>  	repeat:
>  		if (path) {
>  			ext4_ext_drop_refs(path);
> @@ -755,15 +757,10 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
>  		*moved_len = len;
>  
>  out:
> -	if (*moved_len) {
> -		ext4_discard_preallocations(orig_inode);
> -		ext4_discard_preallocations(donor_inode);
> -	}
> +	WARN_ON(m_len > len);
> +	if (ret == 0)
> +		*moved_len = m_len;
>  
> -	if (path) {
> -		ext4_ext_drop_refs(path);
> -		kfree(path);
> -	}

I do not understand why kfree for path is dropped here. 
Rest places looks reasonable for me,
but this one looks like some mistake.

Take a look -- path is still was freed inside cycle,
why it should not be freed at finish too?

>  	up_write(&EXT4_I(orig_inode)->i_data_sem);
>  	up_write(&EXT4_I(donor_inode)->i_data_sem);
>  	up_write(&orig_inode->i_alloc_sem);
>