[Devel] ext4: Discard preallocated block before swap_extents

Submitted by Dmitry Monakhov on Sept. 20, 2016, 5:41 p.m.

Details

Message ID 1474393312-16037-1-git-send-email-dmonakhov@openvz.org
State New
Series "ext4: Discard preallocated block before swap_extents"
Headers show

Commit Message

Dmitry Monakhov Sept. 20, 2016, 5:41 p.m.
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

xfstests:ext4/024 (pended: https://github.com/dmonakhov/xfstests/commit/7a4763963f73ea5d5bba45eefa484494aa3df7cf)
Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/extents.c     |  2 ++
 fs/ext4/move_extent.c | 17 +++++------------
 2 files changed, 7 insertions(+), 12 deletions(-)

Patch hide | download patch | download mbox

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index d7ccb7f..757ffb8 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5799,9 +5799,11 @@  ext4_swap_extents(handle_t *handle, struct inode *inode1,
 	BUG_ON(!inode_is_locked(inode1));
 	BUG_ON(!inode_is_locked(inode2));
 
+	ext4_discard_preallocations(inode1);
 	*erp = ext4_es_remove_extent(inode1, lblk1, count);
 	if (unlikely(*erp))
 		return 0;
+	ext4_discard_preallocations(inode2);
 	*erp = ext4_es_remove_extent(inode2, lblk2, count);
 	if (unlikely(*erp))
 		return 0;
diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
index 6fc14de..24a9586 100644
--- a/fs/ext4/move_extent.c
+++ b/fs/ext4/move_extent.c
@@ -632,7 +632,7 @@  ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
 
 		ret = get_ext_path(orig_inode, o_start, &path);
 		if (ret)
-			goto out;
+			break;
 		ex = path[path->p_depth].p_ext;
 		next_blk = ext4_ext_next_allocated_block(path);
 		cur_blk = le32_to_cpu(ex->ee_block);
@@ -642,7 +642,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;
@@ -654,7 +654,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;
 		}
@@ -687,17 +687,10 @@  ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
 			break;
 		o_start += cur_len;
 		d_start += cur_len;
+		*moved_len += cur_len;
 	}
-	*moved_len = o_start - orig_blk;
-	if (*moved_len > len)
-		*moved_len = len;
-
 out:
-	if (*moved_len) {
-		ext4_discard_preallocations(orig_inode);
-		ext4_discard_preallocations(donor_inode);
-	}
-
+	WARN_ON(*moved_len > len);
 	ext4_ext_drop_refs(path);
 	kfree(path);
 	ext4_double_up_write_data_sem(orig_inode, donor_inode);

Comments

Dmitry Monakhov Sept. 20, 2016, 5:44 p.m.
Dmitry Monakhov <dmonakhov@openvz.org> writes:

TEST_LOG: http://autotest.qa.sw.ru/avocado/bob.qa.sw.ru/job-results/job-2016-09-20T20.37-9107ed4/html/results.html

> 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
>
> xfstests:ext4/024 (pended: https://github.com/dmonakhov/xfstests/commit/7a4763963f73ea5d5bba45eefa484494aa3df7cf)
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  fs/ext4/extents.c     |  2 ++
>  fs/ext4/move_extent.c | 17 +++++------------
>  2 files changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index d7ccb7f..757ffb8 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -5799,9 +5799,11 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
>  	BUG_ON(!inode_is_locked(inode1));
>  	BUG_ON(!inode_is_locked(inode2));
>  
> +	ext4_discard_preallocations(inode1);
>  	*erp = ext4_es_remove_extent(inode1, lblk1, count);
>  	if (unlikely(*erp))
>  		return 0;
> +	ext4_discard_preallocations(inode2);
>  	*erp = ext4_es_remove_extent(inode2, lblk2, count);
>  	if (unlikely(*erp))
>  		return 0;
> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> index 6fc14de..24a9586 100644
> --- a/fs/ext4/move_extent.c
> +++ b/fs/ext4/move_extent.c
> @@ -632,7 +632,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
>  
>  		ret = get_ext_path(orig_inode, o_start, &path);
>  		if (ret)
> -			goto out;
> +			break;
>  		ex = path[path->p_depth].p_ext;
>  		next_blk = ext4_ext_next_allocated_block(path);
>  		cur_blk = le32_to_cpu(ex->ee_block);
> @@ -642,7 +642,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;
> @@ -654,7 +654,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;
>  		}
> @@ -687,17 +687,10 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
>  			break;
>  		o_start += cur_len;
>  		d_start += cur_len;
> +		*moved_len += cur_len;
>  	}
> -	*moved_len = o_start - orig_blk;
> -	if (*moved_len > len)
> -		*moved_len = len;
> -
>  out:
> -	if (*moved_len) {
> -		ext4_discard_preallocations(orig_inode);
> -		ext4_discard_preallocations(donor_inode);
> -	}
> -
> +	WARN_ON(*moved_len > len);
>  	ext4_ext_drop_refs(path);
>  	kfree(path);
>  	ext4_double_up_write_data_sem(orig_inode, donor_inode);
> -- 
> 2.7.4
Maxim Patlasov Sept. 24, 2016, 12:27 a.m.
Dima,


The patch looks fine and it works in my tests, but it slightly changes 
user-visible behavior: before patch, if ioctl(MOVE) failed, the user 
always saw moved_len=0. Now, with the patch applied, it can be !=0. 
(because ext4_ioctl() copies "me" back to user even if err != 0)


I understand that it matter of taste, but the code is more readable 
(imho) if all functions keep output args intact on failure. Hence, it 
would be nice to accumulate "cur_len" in some local var and then, in the 
end of ext4_move_extents, assign *moved_len only if ret == 0.


The above is pretty minor, so:

Reviewed-by: Maxim Patlasov <mpatlasov@virtuozzo.com>

Thanks,
Maxim


On 09/20/2016 10:41 AM, 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
>
> xfstests:ext4/024 (pended: https://github.com/dmonakhov/xfstests/commit/7a4763963f73ea5d5bba45eefa484494aa3df7cf)
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>   fs/ext4/extents.c     |  2 ++
>   fs/ext4/move_extent.c | 17 +++++------------
>   2 files changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index d7ccb7f..757ffb8 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -5799,9 +5799,11 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
>   	BUG_ON(!inode_is_locked(inode1));
>   	BUG_ON(!inode_is_locked(inode2));
>   
> +	ext4_discard_preallocations(inode1);
>   	*erp = ext4_es_remove_extent(inode1, lblk1, count);
>   	if (unlikely(*erp))
>   		return 0;
> +	ext4_discard_preallocations(inode2);
>   	*erp = ext4_es_remove_extent(inode2, lblk2, count);
>   	if (unlikely(*erp))
>   		return 0;
> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> index 6fc14de..24a9586 100644
> --- a/fs/ext4/move_extent.c
> +++ b/fs/ext4/move_extent.c
> @@ -632,7 +632,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
>   
>   		ret = get_ext_path(orig_inode, o_start, &path);
>   		if (ret)
> -			goto out;
> +			break;
>   		ex = path[path->p_depth].p_ext;
>   		next_blk = ext4_ext_next_allocated_block(path);
>   		cur_blk = le32_to_cpu(ex->ee_block);
> @@ -642,7 +642,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;
> @@ -654,7 +654,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;
>   		}
> @@ -687,17 +687,10 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
>   			break;
>   		o_start += cur_len;
>   		d_start += cur_len;
> +		*moved_len += cur_len;
>   	}
> -	*moved_len = o_start - orig_blk;
> -	if (*moved_len > len)
> -		*moved_len = len;
> -
>   out:
> -	if (*moved_len) {
> -		ext4_discard_preallocations(orig_inode);
> -		ext4_discard_preallocations(donor_inode);
> -	}
> -
> +	WARN_ON(*moved_len > len);
>   	ext4_ext_drop_refs(path);
>   	kfree(path);
>   	ext4_double_up_write_data_sem(orig_inode, donor_inode);