From 91cc219ad963731191247c5f2db4118be2bc341a Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Mon, 10 Aug 2009 23:05:28 -0400 Subject: ext4: fix journal ref count in move_extent_par_page move_extent_par_page calls a_ops->write_begin() to increase journal handler's reference count. However, if either mext_replace_branches() or ext4_get_block fails, the increased reference count isn't decreased. This will cause a later attempt to umount of the fs to hang forever. The patch addresses the issue by calling ext4_journal_stop() if page is not NULL (which means a_ops->write_end() isn't invoked). Signed-off-by: Peng Tao Signed-off-by: "Theodore Ts'o" --- fs/ext4/move_extent.c | 1 + 1 file changed, 1 insertion(+) (limited to 'fs/ext4/move_extent.c') diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c index bbf2dd9404d..5821e0bee91 100644 --- a/fs/ext4/move_extent.c +++ b/fs/ext4/move_extent.c @@ -871,6 +871,7 @@ out: if (PageLocked(page)) unlock_page(page); page_cache_release(page); + ext4_journal_stop(handle); } out2: ext4_journal_stop(handle); -- cgit v1.2.3 From 70d5d3dcea47c16058d2b093c29e07fdf61b56ad Mon Sep 17 00:00:00 2001 From: Akira Fujita Date: Wed, 16 Sep 2009 14:28:22 -0400 Subject: ext4: Fix wrong comparisons in mext_check_arguments() The mext_check_arguments() function in move_extents.c has wrong comparisons. orig_start which is passed from user-space is block unit, but i_size of inode is byte unit, therefore the checks do not work fine. This mis-check leads to the overflow of 'len' and then hits BUG_ON() in ext4_move_extents(). The patch fixes this issue. Signed-off-by: Akira Fujita Reviewed-by: Greg Freemyer Signed-off-by: "Theodore Ts'o" --- fs/ext4/move_extent.c | 46 +++++++++++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 19 deletions(-) (limited to 'fs/ext4/move_extent.c') diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c index 5821e0bee91..c593eb2b193 100644 --- a/fs/ext4/move_extent.c +++ b/fs/ext4/move_extent.c @@ -898,6 +898,10 @@ mext_check_arguments(struct inode *orig_inode, struct inode *donor_inode, __u64 orig_start, __u64 donor_start, __u64 *len, __u64 moved_len) { + ext4_lblk_t orig_blocks, donor_blocks; + unsigned int blkbits = orig_inode->i_blkbits; + unsigned int blocksize = 1 << blkbits; + /* Regular file check */ if (!S_ISREG(orig_inode->i_mode) || !S_ISREG(donor_inode->i_mode)) { ext4_debug("ext4 move extent: The argument files should be " @@ -972,43 +976,47 @@ mext_check_arguments(struct inode *orig_inode, } if (orig_inode->i_size > donor_inode->i_size) { - if (orig_start >= donor_inode->i_size) { + donor_blocks = (donor_inode->i_size + blocksize - 1) >> blkbits; + /* TODO: eliminate this artificial restriction */ + if (orig_start >= donor_blocks) { ext4_debug("ext4 move extent: orig start offset " - "[%llu] should be less than donor file size " - "[%lld] [ino:orig %lu, donor_inode %lu]\n", - orig_start, donor_inode->i_size, + "[%llu] should be less than donor file blocks " + "[%u] [ino:orig %lu, donor %lu]\n", + orig_start, donor_blocks, orig_inode->i_ino, donor_inode->i_ino); return -EINVAL; } - if (orig_start + *len > donor_inode->i_size) { + /* TODO: eliminate this artificial restriction */ + if (orig_start + *len > donor_blocks) { ext4_debug("ext4 move extent: End offset [%llu] should " - "be less than donor file size [%lld]." - "So adjust length from %llu to %lld " + "be less than donor file blocks [%u]." + "So adjust length from %llu to %llu " "[ino:orig %lu, donor %lu]\n", - orig_start + *len, donor_inode->i_size, - *len, donor_inode->i_size - orig_start, + orig_start + *len, donor_blocks, + *len, donor_blocks - orig_start, orig_inode->i_ino, donor_inode->i_ino); - *len = donor_inode->i_size - orig_start; + *len = donor_blocks - orig_start; } } else { - if (orig_start >= orig_inode->i_size) { + orig_blocks = (orig_inode->i_size + blocksize - 1) >> blkbits; + if (orig_start >= orig_blocks) { ext4_debug("ext4 move extent: start offset [%llu] " - "should be less than original file size " - "[%lld] [inode:orig %lu, donor %lu]\n", - orig_start, orig_inode->i_size, + "should be less than original file blocks " + "[%u] [ino:orig %lu, donor %lu]\n", + orig_start, orig_blocks, orig_inode->i_ino, donor_inode->i_ino); return -EINVAL; } - if (orig_start + *len > orig_inode->i_size) { + if (orig_start + *len > orig_blocks) { ext4_debug("ext4 move extent: Adjust length " - "from %llu to %lld. Because it should be " - "less than original file size " + "from %llu to %llu. Because it should be " + "less than original file blocks " "[ino:orig %lu, donor %lu]\n", - *len, orig_inode->i_size - orig_start, + *len, orig_blocks - orig_start, orig_inode->i_ino, donor_inode->i_ino); - *len = orig_inode->i_size - orig_start; + *len = orig_blocks - orig_start; } } -- cgit v1.2.3 From daea696dbac0e33af3cfe304efbfb8d74e0effe6 Mon Sep 17 00:00:00 2001 From: Akira Fujita Date: Sat, 5 Sep 2009 22:11:55 -0400 Subject: ext4: Remove unneeded BUG_ON() in ext4_move_extents() The ext4_move_extents() functions checks with BUG_ON() whether the exchanged blocks count accords with request blocks count. But, if the target range (orig_start + len) includes sparse block(s), 'moved_len' (exchanged blocks count) does not agree with 'len' (request blocks count), since sparse block is not counted in 'moved_len'. This causes us to hit the BUG_ON(), even though the function succeeded. Signed-off-by: Akira Fujita Signed-off-by: "Theodore Ts'o" --- fs/ext4/move_extent.c | 3 --- 1 file changed, 3 deletions(-) (limited to 'fs/ext4/move_extent.c') diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c index c593eb2b193..c8c66b167cd 100644 --- a/fs/ext4/move_extent.c +++ b/fs/ext4/move_extent.c @@ -1322,8 +1322,5 @@ out2: if (ret) return ret; - /* All of the specified blocks must be exchanged in succeed */ - BUG_ON(*moved_len != len); - return 0; } -- cgit v1.2.3 From 44fc48f7048ab9657b524938a832fec4e0acea98 Mon Sep 17 00:00:00 2001 From: Akira Fujita Date: Sat, 5 Sep 2009 23:12:41 -0400 Subject: ext4: Fix small typo for move_extent_per_page() This function means moving extents every page, so change its name from move_exgtent_par_page(). Signed-off-by: Akira Fujita Signed-off-by: "Theodore Ts'o" --- fs/ext4/move_extent.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'fs/ext4/move_extent.c') diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c index c8c66b167cd..4c4491cef35 100644 --- a/fs/ext4/move_extent.c +++ b/fs/ext4/move_extent.c @@ -740,7 +740,7 @@ out: * on success, or a negative error value on failure. */ static int -move_extent_par_page(struct file *o_filp, struct inode *donor_inode, +move_extent_per_page(struct file *o_filp, struct inode *donor_inode, pgoff_t orig_page_offset, int data_offset_in_page, int block_len_in_page, int uninit) { @@ -1267,7 +1267,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, while (orig_page_offset <= seq_end_page) { /* Swap original branches with new branches */ - ret = move_extent_par_page(o_filp, donor_inode, + ret = move_extent_per_page(o_filp, donor_inode, orig_page_offset, data_offset_in_page, block_len_in_page, uninit); -- cgit v1.2.3 From e8505970af46658ece2545e9bc1fe594998fdcdf Mon Sep 17 00:00:00 2001 From: Akira Fujita Date: Wed, 16 Sep 2009 13:46:38 -0400 Subject: ext4: Replace get_ext_path macro with an inline funciton Replace get_ext_path macro with an inline function, since this macro looks like a function call but its arguments get modified. Ted pointed this out, thanks. Signed-off-by: Akira Fujita Signed-off-by: "Theodore Ts'o" --- fs/ext4/move_extent.c | 55 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 21 deletions(-) (limited to 'fs/ext4/move_extent.c') diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c index 4c4491cef35..e4bd8761498 100644 --- a/fs/ext4/move_extent.c +++ b/fs/ext4/move_extent.c @@ -19,14 +19,29 @@ #include "ext4_extents.h" #include "ext4.h" -#define get_ext_path(path, inode, block, ret) \ - do { \ - path = ext4_ext_find_extent(inode, block, path); \ - if (IS_ERR(path)) { \ - ret = PTR_ERR(path); \ - path = NULL; \ - } \ - } while (0) +/** + * get_ext_path - Find an extent path for designated logical block number. + * + * @inode: an inode which is searched + * @lblock: logical block number to find an extent path + * @path: pointer to an extent path pointer (for output) + * + * ext4_ext_find_extent wrapper. Return 0 on success, or a negative error value + * on failure. + */ +static inline int +get_ext_path(struct inode *inode, ext4_lblk_t lblock, + struct ext4_ext_path **path) +{ + int ret = 0; + + *path = ext4_ext_find_extent(inode, lblock, *path); + if (IS_ERR(*path)) { + ret = PTR_ERR(*path); + *path = NULL; + } + return ret; +} /** * copy_extent_status - Copy the extent's initialization status @@ -283,7 +298,7 @@ mext_insert_across_blocks(handle_t *handle, struct inode *orig_inode, } if (new_flag) { - get_ext_path(orig_path, orig_inode, eblock, err); + err = get_ext_path(orig_inode, eblock, &orig_path); if (orig_path == NULL) goto out; @@ -293,8 +308,8 @@ mext_insert_across_blocks(handle_t *handle, struct inode *orig_inode, } if (end_flag) { - get_ext_path(orig_path, orig_inode, - le32_to_cpu(end_ext->ee_block) - 1, err); + err = get_ext_path(orig_inode, + le32_to_cpu(end_ext->ee_block) - 1, &orig_path); if (orig_path == NULL) goto out; @@ -631,12 +646,12 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode, mext_double_down_write(orig_inode, donor_inode); /* Get the original extent for the block "orig_off" */ - get_ext_path(orig_path, orig_inode, orig_off, err); + err = get_ext_path(orig_inode, orig_off, &orig_path); if (orig_path == NULL) goto out; /* Get the donor extent for the head */ - get_ext_path(donor_path, donor_inode, donor_off, err); + err = get_ext_path(donor_inode, donor_off, &donor_path); if (donor_path == NULL) goto out; depth = ext_depth(orig_inode); @@ -678,7 +693,7 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode, if (orig_path) ext4_ext_drop_refs(orig_path); - get_ext_path(orig_path, orig_inode, orig_off, err); + err = get_ext_path(orig_inode, orig_off, &orig_path); if (orig_path == NULL) goto out; depth = ext_depth(orig_inode); @@ -692,8 +707,7 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode, if (donor_path) ext4_ext_drop_refs(donor_path); - get_ext_path(donor_path, donor_inode, - donor_off, err); + err = get_ext_path(donor_inode, donor_off, &donor_path); if (donor_path == NULL) goto out; depth = ext_depth(donor_inode); @@ -1154,12 +1168,12 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, if (file_end < block_end) len -= block_end - file_end; - get_ext_path(orig_path, orig_inode, block_start, ret); + ret = get_ext_path(orig_inode, block_start, &orig_path); if (orig_path == NULL) goto out2; /* Get path structure to check the hole */ - get_ext_path(holecheck_path, orig_inode, block_start, ret); + ret = get_ext_path(orig_inode, block_start, &holecheck_path); if (holecheck_path == NULL) goto out; @@ -1289,8 +1303,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, /* Decrease buffer counter */ if (holecheck_path) ext4_ext_drop_refs(holecheck_path); - get_ext_path(holecheck_path, orig_inode, - seq_start, ret); + ret = get_ext_path(orig_inode, seq_start, &holecheck_path); if (holecheck_path == NULL) break; depth = holecheck_path->p_depth; @@ -1298,7 +1311,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, /* Decrease buffer counter */ if (orig_path) ext4_ext_drop_refs(orig_path); - get_ext_path(orig_path, orig_inode, seq_start, ret); + ret = get_ext_path(orig_inode, seq_start, &orig_path); if (orig_path == NULL) break; -- cgit v1.2.3 From 2147b1a6a48e28399120ca51d4a91840a278611f Mon Sep 17 00:00:00 2001 From: Akira Fujita Date: Wed, 16 Sep 2009 13:46:35 -0400 Subject: ext4: Replace BUG_ON() with ext4_error() in move_extents.c Replace BUG_ON calls with a call to ext4_error() to print an error message if EXT4_IOC_MOVE_EXT failed with some kind of reasons. This will help to debug. Ted pointed this out, thanks. Signed-off-by: Akira Fujita Signed-off-by: "Theodore Ts'o" --- fs/ext4/move_extent.c | 149 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 109 insertions(+), 40 deletions(-) (limited to 'fs/ext4/move_extent.c') diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c index e4bd8761498..2258560e972 100644 --- a/fs/ext4/move_extent.c +++ b/fs/ext4/move_extent.c @@ -127,6 +127,31 @@ mext_next_extent(struct inode *inode, struct ext4_ext_path *path, return 1; } +/** + * mext_check_null_inode - NULL check for two inodes + * + * If inode1 or inode2 is NULL, return -EIO. Otherwise, return 0. + */ +static int +mext_check_null_inode(struct inode *inode1, struct inode *inode2, + const char *function) +{ + int ret = 0; + + if (inode1 == NULL) { + ext4_error(inode2->i_sb, function, + "Both inodes should not be NULL: " + "inode1 NULL inode2 %lu", inode2->i_ino); + ret = -EIO; + } else if (inode2 == NULL) { + ext4_error(inode1->i_sb, function, + "Both inodes should not be NULL: " + "inode1 %lu inode2 NULL", inode1->i_ino); + ret = -EIO; + } + return ret; +} + /** * mext_double_down_read - Acquire two inodes' read semaphore * @@ -139,8 +164,6 @@ mext_double_down_read(struct inode *orig_inode, struct inode *donor_inode) { struct inode *first = orig_inode, *second = donor_inode; - BUG_ON(orig_inode == NULL || donor_inode == NULL); - /* * Use the inode number to provide the stable locking order instead * of its address, because the C language doesn't guarantee you can @@ -167,8 +190,6 @@ mext_double_down_write(struct inode *orig_inode, struct inode *donor_inode) { struct inode *first = orig_inode, *second = donor_inode; - BUG_ON(orig_inode == NULL || donor_inode == NULL); - /* * Use the inode number to provide the stable locking order instead * of its address, because the C language doesn't guarantee you can @@ -193,8 +214,6 @@ mext_double_down_write(struct inode *orig_inode, struct inode *donor_inode) static void mext_double_up_read(struct inode *orig_inode, struct inode *donor_inode) { - BUG_ON(orig_inode == NULL || donor_inode == NULL); - up_read(&EXT4_I(orig_inode)->i_data_sem); up_read(&EXT4_I(donor_inode)->i_data_sem); } @@ -209,8 +228,6 @@ mext_double_up_read(struct inode *orig_inode, struct inode *donor_inode) static void mext_double_up_write(struct inode *orig_inode, struct inode *donor_inode) { - BUG_ON(orig_inode == NULL || donor_inode == NULL); - up_write(&EXT4_I(orig_inode)->i_data_sem); up_write(&EXT4_I(donor_inode)->i_data_sem); } @@ -534,7 +551,15 @@ mext_leaf_block(handle_t *handle, struct inode *orig_inode, * oext |-----------| * new_ext |-------| */ - BUG_ON(le32_to_cpu(oext->ee_block) + oext_alen - 1 < new_ext_end); + if (le32_to_cpu(oext->ee_block) + oext_alen - 1 < new_ext_end) { + ext4_error(orig_inode->i_sb, __func__, + "new_ext_end(%u) should be less than or equal to " + "oext->ee_block(%u) + oext_alen(%d) - 1", + new_ext_end, le32_to_cpu(oext->ee_block), + oext_alen); + ret = -EIO; + goto out; + } /* * Case: new_ext is smaller than original extent @@ -558,6 +583,7 @@ mext_leaf_block(handle_t *handle, struct inode *orig_inode, ret = mext_insert_extents(handle, orig_inode, orig_path, o_start, o_end, &start_ext, &new_ext, &end_ext); +out: return ret; } @@ -668,7 +694,20 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode, /* Loop for the donor extents */ while (1) { /* The extent for donor must be found. */ - BUG_ON(!dext || donor_off != le32_to_cpu(tmp_dext.ee_block)); + if (!dext) { + ext4_error(donor_inode->i_sb, __func__, + "The extent for donor must be found"); + err = -EIO; + goto out; + } else if (donor_off != le32_to_cpu(tmp_dext.ee_block)) { + ext4_error(donor_inode->i_sb, __func__, + "Donor offset(%u) and the first block of donor " + "extent(%u) should be equal", + donor_off, + le32_to_cpu(tmp_dext.ee_block)); + err = -EIO; + goto out; + } /* Set donor extent to orig extent */ err = mext_leaf_block(handle, orig_inode, @@ -1050,18 +1089,23 @@ mext_check_arguments(struct inode *orig_inode, * @inode1: the inode structure * @inode2: the inode structure * - * Lock two inodes' i_mutex by i_ino order. This function is moved from - * fs/inode.c. + * Lock two inodes' i_mutex by i_ino order. + * If inode1 or inode2 is NULL, return -EIO. Otherwise, return 0. */ -static void +static int mext_inode_double_lock(struct inode *inode1, struct inode *inode2) { - if (inode1 == NULL || inode2 == NULL || inode1 == inode2) { - if (inode1) - mutex_lock(&inode1->i_mutex); - else if (inode2) - mutex_lock(&inode2->i_mutex); - return; + int ret = 0; + + BUG_ON(inode1 == NULL && inode2 == NULL); + + ret = mext_check_null_inode(inode1, inode2, __func__); + if (ret < 0) + goto out; + + if (inode1 == inode2) { + mutex_lock(&inode1->i_mutex); + goto out; } if (inode1->i_ino < inode2->i_ino) { @@ -1071,6 +1115,9 @@ mext_inode_double_lock(struct inode *inode1, struct inode *inode2) mutex_lock_nested(&inode2->i_mutex, I_MUTEX_PARENT); mutex_lock_nested(&inode1->i_mutex, I_MUTEX_CHILD); } + +out: + return ret; } /** @@ -1079,17 +1126,28 @@ mext_inode_double_lock(struct inode *inode1, struct inode *inode2) * @inode1: the inode that is released first * @inode2: the inode that is released second * - * This function is moved from fs/inode.c. + * If inode1 or inode2 is NULL, return -EIO. Otherwise, return 0. */ -static void +static int mext_inode_double_unlock(struct inode *inode1, struct inode *inode2) { + int ret = 0; + + BUG_ON(inode1 == NULL && inode2 == NULL); + + ret = mext_check_null_inode(inode1, inode2, __func__); + if (ret < 0) + goto out; + if (inode1) mutex_unlock(&inode1->i_mutex); if (inode2 && inode2 != inode1) mutex_unlock(&inode2->i_mutex); + +out: + return ret; } /** @@ -1146,21 +1204,23 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, ext4_lblk_t block_end, seq_start, add_blocks, file_end, seq_blocks = 0; ext4_lblk_t rest_blocks; pgoff_t orig_page_offset = 0, seq_end_page; - int ret, depth, last_extent = 0; + int ret1, ret2, depth, last_extent = 0; int blocks_per_page = PAGE_CACHE_SIZE >> orig_inode->i_blkbits; int data_offset_in_page; int block_len_in_page; int uninit; /* protect orig and donor against a truncate */ - mext_inode_double_lock(orig_inode, donor_inode); + ret1 = mext_inode_double_lock(orig_inode, donor_inode); + if (ret1 < 0) + return ret1; mext_double_down_read(orig_inode, donor_inode); /* Check the filesystem environment whether move_extent can be done */ - ret = mext_check_arguments(orig_inode, donor_inode, orig_start, + ret1 = mext_check_arguments(orig_inode, donor_inode, orig_start, donor_start, &len, *moved_len); mext_double_up_read(orig_inode, donor_inode); - if (ret) + if (ret1) goto out2; file_end = (i_size_read(orig_inode) - 1) >> orig_inode->i_blkbits; @@ -1168,19 +1228,19 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, if (file_end < block_end) len -= block_end - file_end; - ret = get_ext_path(orig_inode, block_start, &orig_path); + ret1 = get_ext_path(orig_inode, block_start, &orig_path); if (orig_path == NULL) goto out2; /* Get path structure to check the hole */ - ret = get_ext_path(orig_inode, block_start, &holecheck_path); + ret1 = get_ext_path(orig_inode, block_start, &holecheck_path); if (holecheck_path == NULL) goto out; depth = ext_depth(orig_inode); ext_cur = holecheck_path[depth].p_ext; if (ext_cur == NULL) { - ret = -EINVAL; + ret1 = -EINVAL; goto out; } @@ -1193,13 +1253,13 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, last_extent = mext_next_extent(orig_inode, holecheck_path, &ext_cur); if (last_extent < 0) { - ret = last_extent; + ret1 = last_extent; goto out; } last_extent = mext_next_extent(orig_inode, orig_path, &ext_dummy); if (last_extent < 0) { - ret = last_extent; + ret1 = last_extent; goto out; } } @@ -1209,7 +1269,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, if (le32_to_cpu(ext_cur->ee_block) > block_end) { ext4_debug("ext4 move extent: The specified range of file " "may be the hole\n"); - ret = -EINVAL; + ret1 = -EINVAL; goto out; } @@ -1229,7 +1289,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, last_extent = mext_next_extent(orig_inode, holecheck_path, &ext_cur); if (last_extent < 0) { - ret = last_extent; + ret1 = last_extent; break; } add_blocks = ext4_ext_get_actual_len(ext_cur); @@ -1281,16 +1341,23 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, while (orig_page_offset <= seq_end_page) { /* Swap original branches with new branches */ - ret = move_extent_per_page(o_filp, donor_inode, + ret1 = move_extent_per_page(o_filp, donor_inode, orig_page_offset, data_offset_in_page, block_len_in_page, uninit); - if (ret < 0) + if (ret1 < 0) goto out; orig_page_offset++; /* Count how many blocks we have exchanged */ *moved_len += block_len_in_page; - BUG_ON(*moved_len > len); + if (*moved_len > len) { + ext4_error(orig_inode->i_sb, __func__, + "We replaced blocks too much! " + "sum of replaced: %llu requested: %llu", + *moved_len, len); + ret1 = -EIO; + goto out; + } data_offset_in_page = 0; rest_blocks -= block_len_in_page; @@ -1303,7 +1370,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, /* Decrease buffer counter */ if (holecheck_path) ext4_ext_drop_refs(holecheck_path); - ret = get_ext_path(orig_inode, seq_start, &holecheck_path); + ret1 = get_ext_path(orig_inode, seq_start, &holecheck_path); if (holecheck_path == NULL) break; depth = holecheck_path->p_depth; @@ -1311,7 +1378,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, /* Decrease buffer counter */ if (orig_path) ext4_ext_drop_refs(orig_path); - ret = get_ext_path(orig_inode, seq_start, &orig_path); + ret1 = get_ext_path(orig_inode, seq_start, &orig_path); if (orig_path == NULL) break; @@ -1330,10 +1397,12 @@ out: kfree(holecheck_path); } out2: - mext_inode_double_unlock(orig_inode, donor_inode); + ret2 = mext_inode_double_unlock(orig_inode, donor_inode); - if (ret) - return ret; + if (ret1) + return ret1; + else if (ret2) + return ret2; return 0; } -- cgit v1.2.3 From 347fa6f1c7cb5df2b38d3c9167cfe242ce0cd1da Mon Sep 17 00:00:00 2001 From: Akira Fujita Date: Wed, 16 Sep 2009 14:25:07 -0400 Subject: ext4: Add null extent check to ext_get_path There is the possibility that path structure which is taken by ext4_ext_find_extent() indicates null extents. Because during data block exchanging in ext4_move_extents(), constitution of an extent tree may be changed. As a solution, the patch adds null extent check to ext_get_path(). Reported-by: Peng Tao Signed-off-by: Akira Fujita Signed-off-by: "Theodore Ts'o" --- fs/ext4/move_extent.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) (limited to 'fs/ext4/move_extent.c') diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c index 2258560e972..1c509d54913 100644 --- a/fs/ext4/move_extent.c +++ b/fs/ext4/move_extent.c @@ -39,7 +39,9 @@ get_ext_path(struct inode *inode, ext4_lblk_t lblock, if (IS_ERR(*path)) { ret = PTR_ERR(*path); *path = NULL; - } + } else if ((*path)[ext_depth(inode)].p_ext == NULL) + ret = -ENODATA; + return ret; } @@ -316,7 +318,7 @@ mext_insert_across_blocks(handle_t *handle, struct inode *orig_inode, if (new_flag) { err = get_ext_path(orig_inode, eblock, &orig_path); - if (orig_path == NULL) + if (err) goto out; if (ext4_ext_insert_extent(handle, orig_inode, @@ -327,7 +329,7 @@ mext_insert_across_blocks(handle_t *handle, struct inode *orig_inode, if (end_flag) { err = get_ext_path(orig_inode, le32_to_cpu(end_ext->ee_block) - 1, &orig_path); - if (orig_path == NULL) + if (err) goto out; if (ext4_ext_insert_extent(handle, orig_inode, @@ -673,12 +675,12 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode, /* Get the original extent for the block "orig_off" */ err = get_ext_path(orig_inode, orig_off, &orig_path); - if (orig_path == NULL) + if (err) goto out; /* Get the donor extent for the head */ err = get_ext_path(donor_inode, donor_off, &donor_path); - if (donor_path == NULL) + if (err) goto out; depth = ext_depth(orig_inode); oext = orig_path[depth].p_ext; @@ -733,7 +735,7 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode, if (orig_path) ext4_ext_drop_refs(orig_path); err = get_ext_path(orig_inode, orig_off, &orig_path); - if (orig_path == NULL) + if (err) goto out; depth = ext_depth(orig_inode); oext = orig_path[depth].p_ext; @@ -747,7 +749,7 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode, if (donor_path) ext4_ext_drop_refs(donor_path); err = get_ext_path(donor_inode, donor_off, &donor_path); - if (donor_path == NULL) + if (err) goto out; depth = ext_depth(donor_inode); dext = donor_path[depth].p_ext; @@ -1221,7 +1223,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, donor_start, &len, *moved_len); mext_double_up_read(orig_inode, donor_inode); if (ret1) - goto out2; + goto out; file_end = (i_size_read(orig_inode) - 1) >> orig_inode->i_blkbits; block_end = block_start + len - 1; @@ -1229,20 +1231,16 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, len -= block_end - file_end; ret1 = get_ext_path(orig_inode, block_start, &orig_path); - if (orig_path == NULL) - goto out2; + if (ret1) + goto out; /* Get path structure to check the hole */ ret1 = get_ext_path(orig_inode, block_start, &holecheck_path); - if (holecheck_path == NULL) + if (ret1) goto out; depth = ext_depth(orig_inode); ext_cur = holecheck_path[depth].p_ext; - if (ext_cur == NULL) { - ret1 = -EINVAL; - goto out; - } /* * Get proper extent whose ee_block is beyond block_start @@ -1371,7 +1369,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, if (holecheck_path) ext4_ext_drop_refs(holecheck_path); ret1 = get_ext_path(orig_inode, seq_start, &holecheck_path); - if (holecheck_path == NULL) + if (ret1) break; depth = holecheck_path->p_depth; @@ -1379,7 +1377,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, if (orig_path) ext4_ext_drop_refs(orig_path); ret1 = get_ext_path(orig_inode, seq_start, &orig_path); - if (orig_path == NULL) + if (ret1) break; ext_cur = holecheck_path[depth].p_ext; @@ -1396,7 +1394,7 @@ out: ext4_ext_drop_refs(holecheck_path); kfree(holecheck_path); } -out2: + ret2 = mext_inode_double_unlock(orig_inode, donor_inode); if (ret1) -- cgit v1.2.3 From c40ce3c9ea97425a12d7e44031a98fe50add6fc1 Mon Sep 17 00:00:00 2001 From: Akira Fujita Date: Wed, 16 Sep 2009 14:25:39 -0400 Subject: ext4: Fix different block exchange issue in EXT4_IOC_MOVE_EXT If logical block offset of original file which is passed to EXT4_IOC_MOVE_EXT is different from donor file's, a calculation error occurs in ext4_calc_swap_extents(), therefore wrong block is exchanged between original file and donor file. As a result, we hit ext4_error() in check_block_validity(). To detect the logical offset difference in EXT4_IOC_MOVE_EXT, add checks to mext_calc_swap_extents() and handle it as error, since data exchange must be done between the same blocks in EXT4_IOC_MOVE_EXT. Reported-by: Peng Tao Signed-off-by: Akira Fujita Signed-off-by: "Theodore Ts'o" --- fs/ext4/move_extent.c | 46 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 9 deletions(-) (limited to 'fs/ext4/move_extent.c') diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c index 1c509d54913..1f027b1ec43 100644 --- a/fs/ext4/move_extent.c +++ b/fs/ext4/move_extent.c @@ -597,8 +597,10 @@ out: * @orig_off: block offset of original inode * @donor_off: block offset of donor inode * @max_count: the maximun length of extents + * + * Return 0 on success, or a negative error value on failure. */ -static void +static int mext_calc_swap_extents(struct ext4_extent *tmp_dext, struct ext4_extent *tmp_oext, ext4_lblk_t orig_off, ext4_lblk_t donor_off, @@ -607,6 +609,19 @@ mext_calc_swap_extents(struct ext4_extent *tmp_dext, ext4_lblk_t diff, orig_diff; struct ext4_extent dext_old, oext_old; + BUG_ON(orig_off != donor_off); + + /* original and donor extents have to cover the same block offset */ + if (orig_off < le32_to_cpu(tmp_oext->ee_block) || + le32_to_cpu(tmp_oext->ee_block) + + ext4_ext_get_actual_len(tmp_oext) - 1 < orig_off) + return -ENODATA; + + if (orig_off < le32_to_cpu(tmp_dext->ee_block) || + le32_to_cpu(tmp_dext->ee_block) + + ext4_ext_get_actual_len(tmp_dext) - 1 < orig_off) + return -ENODATA; + dext_old = *tmp_dext; oext_old = *tmp_oext; @@ -634,6 +649,8 @@ mext_calc_swap_extents(struct ext4_extent *tmp_dext, copy_extent_status(&oext_old, tmp_dext); copy_extent_status(&dext_old, tmp_oext); + + return 0; } /** @@ -690,8 +707,10 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode, dext = donor_path[depth].p_ext; tmp_dext = *dext; - mext_calc_swap_extents(&tmp_dext, &tmp_oext, orig_off, + err = mext_calc_swap_extents(&tmp_dext, &tmp_oext, orig_off, donor_off, count); + if (err) + goto out; /* Loop for the donor extents */ while (1) { @@ -760,9 +779,10 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode, } tmp_dext = *dext; - mext_calc_swap_extents(&tmp_dext, &tmp_oext, orig_off, - donor_off, - count - replaced_count); + err = mext_calc_swap_extents(&tmp_dext, &tmp_oext, orig_off, + donor_off, count - replaced_count); + if (err) + goto out; } out: @@ -1243,11 +1263,15 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, ext_cur = holecheck_path[depth].p_ext; /* - * Get proper extent whose ee_block is beyond block_start - * if block_start was within the hole. + * Get proper starting location of block replacement if block_start was + * within the hole. */ if (le32_to_cpu(ext_cur->ee_block) + ext4_ext_get_actual_len(ext_cur) - 1 < block_start) { + /* + * The hole exists between extents or the tail of + * original file. + */ last_extent = mext_next_extent(orig_inode, holecheck_path, &ext_cur); if (last_extent < 0) { @@ -1260,8 +1284,12 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, ret1 = last_extent; goto out; } - } - seq_start = block_start; + seq_start = le32_to_cpu(ext_cur->ee_block); + } else if (le32_to_cpu(ext_cur->ee_block) > block_start) + /* The hole exists at the beginning of original file. */ + seq_start = le32_to_cpu(ext_cur->ee_block); + else + seq_start = block_start; /* No blocks within the specified range. */ if (le32_to_cpu(ext_cur->ee_block) > block_end) { -- cgit v1.2.3 From 0a80e9867db154966b2a771042e10452ac110e1e Mon Sep 17 00:00:00 2001 From: Eric Sandeen Date: Thu, 17 Sep 2009 11:55:58 -0400 Subject: ext4: replace MAX_DEFRAG_SIZE with EXT_MAX_BLOCK There's no reason to redefine the maximum allowable offset in an extent-based file just for defrag; EXT_MAX_BLOCK already does this. Signed-off-by: Eric Sandeen Signed-off-by: "Theodore Ts'o" --- fs/ext4/move_extent.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'fs/ext4/move_extent.c') diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c index 1f027b1ec43..c07a2915e40 100644 --- a/fs/ext4/move_extent.c +++ b/fs/ext4/move_extent.c @@ -1040,12 +1040,12 @@ mext_check_arguments(struct inode *orig_inode, return -EINVAL; } - if ((orig_start > MAX_DEFRAG_SIZE) || - (donor_start > MAX_DEFRAG_SIZE) || - (*len > MAX_DEFRAG_SIZE) || - (orig_start + *len > MAX_DEFRAG_SIZE)) { - ext4_debug("ext4 move extent: Can't handle over [%lu] blocks " - "[ino:orig %lu, donor %lu]\n", MAX_DEFRAG_SIZE, + if ((orig_start > EXT_MAX_BLOCK) || + (donor_start > EXT_MAX_BLOCK) || + (*len > EXT_MAX_BLOCK) || + (orig_start + *len > EXT_MAX_BLOCK)) { + ext4_debug("ext4 move extent: Can't handle over [%u] blocks " + "[ino:orig %lu, donor %lu]\n", EXT_MAX_BLOCK, orig_inode->i_ino, donor_inode->i_ino); return -EINVAL; } -- cgit v1.2.3 From 0031462b5b392f90d17f1d75abb795883c44e969 Mon Sep 17 00:00:00 2001 From: Mingming Cao Date: Mon, 28 Sep 2009 15:49:08 -0400 Subject: ext4: Split uninitialized extents for direct I/O When writing into an unitialized extent via direct I/O, and the direct I/O doesn't exactly cover the unitialized extent, split the extent into uninitialized and initialized extents before submitting the I/O. This avoids needing to deal with an ENOSPC error in the end_io callback that gets used for direct I/O. When the IO is complete, the written extent will be marked as initialized. Singed-Off-By: Mingming Cao Signed-off-by: "Theodore Ts'o" --- fs/ext4/move_extent.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'fs/ext4/move_extent.c') diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c index c07a2915e40..5332fd4c402 100644 --- a/fs/ext4/move_extent.c +++ b/fs/ext4/move_extent.c @@ -322,7 +322,7 @@ mext_insert_across_blocks(handle_t *handle, struct inode *orig_inode, goto out; if (ext4_ext_insert_extent(handle, orig_inode, - orig_path, new_ext)) + orig_path, new_ext, 0)) goto out; } @@ -333,7 +333,7 @@ mext_insert_across_blocks(handle_t *handle, struct inode *orig_inode, goto out; if (ext4_ext_insert_extent(handle, orig_inode, - orig_path, end_ext)) + orig_path, end_ext, 0)) goto out; } out: -- cgit v1.2.3 From f3ce8064b388ccf420012c5a4907aae4f13fe9d0 Mon Sep 17 00:00:00 2001 From: Theodore Ts'o Date: Mon, 28 Sep 2009 15:58:29 -0400 Subject: ext4: EXT4_IOC_MOVE_EXT: Check for different original and donor inodes first Move the check to make sure the original and donor inodes are different earlier, to avoid a potential deadlock by trying to lock the same inode twice. Signed-off-by: "Theodore Ts'o" --- fs/ext4/move_extent.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'fs/ext4/move_extent.c') diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c index 5332fd4c402..25b6b145736 100644 --- a/fs/ext4/move_extent.c +++ b/fs/ext4/move_extent.c @@ -1001,14 +1001,6 @@ mext_check_arguments(struct inode *orig_inode, return -EINVAL; } - /* orig and donor should be different file */ - if (orig_inode->i_ino == donor_inode->i_ino) { - ext4_debug("ext4 move extent: The argument files should not " - "be same file [ino:orig %lu, donor %lu]\n", - orig_inode->i_ino, donor_inode->i_ino); - return -EINVAL; - } - /* Ext4 move extent supports only extent based file */ if (!(EXT4_I(orig_inode)->i_flags & EXT4_EXTENTS_FL)) { ext4_debug("ext4 move extent: orig file is not extents " @@ -1232,6 +1224,14 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, int block_len_in_page; int uninit; + /* orig and donor should be different file */ + if (orig_inode->i_ino == donor_inode->i_ino) { + ext4_debug("ext4 move extent: The argument files should not " + "be same file [ino:orig %lu, donor %lu]\n", + orig_inode->i_ino, donor_inode->i_ino); + return -EINVAL; + } + /* protect orig and donor against a truncate */ ret1 = mext_inode_double_lock(orig_inode, donor_inode); if (ret1 < 0) -- cgit v1.2.3