Re: [Devel] [PATCH] ext4: Discard preallocated block before swap_extents
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
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
---
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);
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] ext4: Discard preallocated block before swap_extents
Dmitry Monakhov 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
> ---
> 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
signature.asc
Description: PGP signature
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
