[Devel] vz6 ext4: Discard preallocated block before swap_extents

Submitted by Dmitry Monakhov on Feb. 25, 2017, 3:16 p.m.

Details

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

Commit Message

Dmitry Monakhov Feb. 25, 2017, 3:16 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

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

Patch hide | download patch | download mbox

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);
-	}
 	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);

Comments

Dmitry Monakhov Feb. 27, 2017, 11:34 a.m.
Vasily Averin <vvs@virtuozzo.com> 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 <dmonakhov@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.
Yes, this is copy-paste mistake. Please see updated verstion

From: Dmitry Monakhov <dmonakhov@openvz.org>
To: devel@openvz.org
Cc: dmonakhov@openvz.org,
	vvs@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-dmonakhov@openvz.org>

>
> 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);
>>