2013-04-09 15:16:06

by Dmitry Monakhov

Subject: [PATCH] ext4: defragmentation code cleanup

- grab_cache_page_write_begin() may not wait on page's writeback since
(1d1d1a767206). But it is still reasonable to wait on page's writeback
here in order to be on the safe side.

- Fix miss typo: pass 'length' instead of 'end' to __block_write_begin()
https://bugzilla.kernel.org/show_bug.cgi?id=56241

TESTCASE: git://oss.sgi.com/xfs/cmds/xfstests.git
MKFS_OPTIONS="-b1024" ; ./check ext4/304

Signed-off-by: Dmitry Monakhov <[email protected]>
---
fs/ext4/move_extent.c | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
index 33e1c08..58b33e9 100644
--- a/fs/ext4/move_extent.c
+++ b/fs/ext4/move_extent.c
@@ -737,6 +737,7 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode,
donor_off += dext_alen;
orig_off += dext_alen;

+ BUG_ON(replaced_count > count);
/* Already moved the expected blocks */
if (replaced_count >= count)
break;
@@ -814,7 +815,13 @@ mext_page_double_lock(struct inode *inode1, struct inode *inode2,
page_cache_release(page[0]);
return -ENOMEM;
}

2013-04-10 06:49:34

by Akira Fujita

Subject: Re: [PATCH] ext4: defragmentation code cleanup

Hi,

(2013/04/10 0:15), Dmitry Monakhov wrote:
> - grab_cache_page_write_begin() may not wait on page's writeback since
> (1d1d1a767206). But it is still reasonable to wait on page's writeback
> here in order to be on the safe side.
>
> - Fix miss typo: pass 'length' instead of 'end' to __block_write_begin()
> https://bugzilla.kernel.org/show_bug.cgi?id=56241

Looks good to me, thanks.

Reviewed-by: Akira Fujita <a-fujita.rs.jp.nec.com>

> TESTCASE: git://oss.sgi.com/xfs/cmds/xfstests.git
> MKFS_OPTIONS="-b1024" ; ./check ext4/304
>
> Signed-off-by: Dmitry Monakhov <[email protected]>
> ---
> fs/ext4/move_extent.c | 11 +++++++++--
> 1 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> index 33e1c08..58b33e9 100644
> --- a/fs/ext4/move_extent.c
> +++ b/fs/ext4/move_extent.c
> @@ -737,6 +737,7 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode,
> donor_off += dext_alen;
> orig_off += dext_alen;
>
> + BUG_ON(replaced_count > count);
> /* Already moved the expected blocks */
> if (replaced_count >= count)
> break;
> @@ -814,7 +815,13 @@ mext_page_double_lock(struct inode *inode1, struct inode *inode2,
> page_cache_release(page[0]);
> return -ENOMEM;
> }
> -
> + /*
> + * grab_cache_page_write_begin() may not wait on page's writeback if
> + * BDI not demand that. But it is reasonable to be very conservative
> + * here and explicitly wait on page's writeback
> + */
> + wait_on_page_writeback(page[0]);
> + wait_on_page_writeback(page[1]);
> if (inode1 > inode2) {
> struct page *tmp;
> tmp = page[0];
> @@ -1033,7 +1040,7 @@ data_copy:
> }
> /* Perform all necessary steps similar write_begin()/write_end()
> * but keeping in mind that i_size will not change */
> - *err = __block_write_begin(pagep[0], from, from + replaced_size,
> + *err = __block_write_begin(pagep[0], from, replaced_size,
> ext4_get_block);
> if (!*err)
> *err = block_commit_write(pagep[0], from, from + replaced_size);
>

2013-04-12 03:33:18

by Theodore Ts'o

Subject: Re: [PATCH] ext4: defragmentation code cleanup

On Wed, Apr 10, 2013 at 03:43:09PM +0900, Akira Fujita wrote:
> Hi,
>
> (2013/04/10 0:15), Dmitry Monakhov wrote:
> > - grab_cache_page_write_begin() may not wait on page's writeback since
> > (1d1d1a767206). But it is still reasonable to wait on page's writeback
> > here in order to be on the safe side.
> >
> > - Fix miss typo: pass 'length' instead of 'end' to __block_write_begin()
> > https://bugzilla.kernel.org/show_bug.cgi?id=56241
>
> Looks good to me, thanks.
>
> Reviewed-by: Akira Fujita <a-fujita.rs.jp.nec.com>

Applied, thanks.

- Ted