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

2017-02-27 Thread Dmitry Monakhov 
Vasily Averin  writes:

> 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 
>> ---
>>  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(>i_mutex));
>>  BUG_ON(!mutex_is_locked(>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.
Yes, this is copy-paste mistake. Please see updated verstion

From: Dmitry Monakhov 
To: devel@openvz.org
Cc: dmonak...@openvz.org,
v...@virtuozzo.com
Subject: [PATCH] vz6 ext4: Discard preallocated block before swap_extents v2
Date: Mon, 27 Feb 2017 15:33:07 +0400
Message-Id: <1488195187-26606-1-git-send-email-dmonak...@openvz.org>

>
> Take a look -- path is still was freed inside cycle,
> why it should not be freed at finish too?
>
>>  up_write(_I(orig_inode)->i_data_sem);
>>  up_write(_I(donor_inode)->i_data_sem);
>>  up_write(_inode->i_alloc_sem);
>> 
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

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

2017-02-27 Thread Dmitry Monakhov 
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 
---
 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(>i_mutex));
BUG_ON(!mutex_is_locked(>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);
-   }
up_write(_I(orig_inode)->i_data_sem);
up_write(_I(donor_inode)->i_data_sem);
up_write(_inode->i_alloc_sem);
-- 
2.9.3

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

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

2017-02-27 Thread Dmitry Monakhov 
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 
---
 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(>i_mutex));
BUG_ON(!mutex_is_locked(>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..df904aa 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);
@@ -750,15 +752,10 @@ ext4_move_extents(struct file *o_filp, struct file 
*d_filp, __u64 orig_blk,
path = NULL;
}
}
-   *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(m_len > len);
+   if (ret == 0)
+   *moved_len = m_len;
 
if (path) {
ext4_ext_drop_refs(path);
-- 
2.9.3

___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

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

2017-02-27 Thread 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 
> ---
>  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(>i_mutex));
>   BUG_ON(!mutex_is_locked(>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(_I(orig_inode)->i_data_sem);
>   up_write(_I(donor_inode)->i_data_sem);
>   up_write(_inode->i_alloc_sem);
> 
___
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel