Re: [Devel] [PATCH] vz6 ext4: Discard preallocated block before swap_extents
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(&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 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(&EXT4_I(orig_inode)->i_data_sem); >> up_write(&EXT4_I(donor_inode)->i_data_sem); >> up_write(&orig_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
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(&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); -- 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
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(&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..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
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(&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); > ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel