[PATCH 1/6] ext4: remove unnecessary var in ext4_ext_insert_extent

[PATCH 1/6] ext4: remove unnecessary var in ext4_ext_insert_extent

[Yongqiang Yang]

Var(uninitialized) is not necessary.  As the comment says,
two extent are either initialized or uninitialized.  So
checking newext is ok.

Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
---
 fs/ext4/extents.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 61fa9e1..6b4a558 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -1665,7 +1665,6 @@ int ext4_ext_insert_extent(handle_t *handle, struct inode *inode,
 	struct ext4_ext_path *npath = NULL;
 	int depth, len, err;
 	ext4_lblk_t next;
-	unsigned uninitialized = 0;
 	int flags = 0;
 
 	if (unlikely(ext4_ext_get_actual_len(newext) == 0)) {
@@ -1698,11 +1697,9 @@ int ext4_ext_insert_extent(handle_t *handle, struct inode *inode,
 		 * both extents are uninitialized, or both aren't. Thus we
 		 * need to check only one of them here.
 		 */
-		if (ext4_ext_is_uninitialized(ex))
-			uninitialized = 1;
 		ex->ee_len = cpu_to_le16(ext4_ext_get_actual_len(ex)
 					+ ext4_ext_get_actual_len(newext));
-		if (uninitialized)
+		if (ext4_ext_is_uninitialized(newext))
 			ext4_ext_mark_uninitialized(ex);
 		eh = path[depth].p_hdr;
 		nearex = ex;
-- 
1.7.5.1

[PATCH 2/6] ext4: remove useless BUG_ON in ext4_ext_insert_extent

[Yongqiang Yang]

npath is set to NULL and is not set again before BUG_ON, so
the BUG_ON is useless.

Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
---
 fs/ext4/extents.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 6b4a558..8591bc8 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -1718,7 +1718,6 @@ int ext4_ext_insert_extent(handle_t *handle, struct inode *inode,
 		next = ext4_ext_next_leaf_block(path);
 	if (next != EXT_MAX_BLOCKS) {
 		ext_debug("next leaf block - %u\n", next);
-		BUG_ON(npath != NULL);
 		npath = ext4_ext_find_extent(inode, next, NULL);
 		if (IS_ERR(npath))
 			return PTR_ERR(npath);
-- 
1.7.5.1

[PATCH 3/6] ext4: correct comment on extent merging in ext4_ext_insert_extent

[Yongqiang Yang]

ext4_ext_try_to_merge merges both to right and left.

Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
---
 fs/ext4/extents.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 8591bc8..6888d1a 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -1804,12 +1804,10 @@ has_space:
 	nearex->ee_len = newext->ee_len;
 
 merge:
-	/* try to merge extents to the right */
+	/* try to merge extents */
 	if (!(flag & EXT4_GET_BLOCKS_PRE_IO))
 		ext4_ext_try_to_merge(inode, path, nearex);
 
-	/* try to merge extents to the left */

[PATCH 4/6] ext4: remove code related to punching hole from ext4_ext_insert_extent

[Yongqiang Yang]

Punch hole should never call ext4_ext_insert_extent, so this patch
removes code related to it from ext4_ext_insert_extent.

Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
---
 fs/ext4/extents.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 6888d1a..720070d 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -1737,8 +1737,6 @@ int ext4_ext_insert_extent(handle_t *handle, struct inode *inode,
 	 * There is no free space in the found leaf.
 	 * We're gonna add a new leaf in the tree.
 	 */
-	if (flag & EXT4_GET_BLOCKS_PUNCH_OUT_EXT)
-		flags = EXT4_MB_USE_ROOT_BLOCKS;
 	err = ext4_ext_create_new_leaf(handle, inode, flags, path, newext);
 	if (err)
 		goto cleanup;
-- 
1.7.5.1

Re: [PATCH 4/6] ext4: remove code related to punching hole from ext4_ext_insert_extent

[Andreas Dilger]

On 2011-11-16, at 19:03, Yongqiang Yang <xiaoqiangnk@gmail.com> wrote:

> Punch hole should never call ext4_ext_insert_extent, so this patch
> removes code related to it from ext4_ext_insert_extent.

Is that still true if punch hole is used beyond the end of the file?  I don't recall whether the semantics of this mean to extend the file size or to shrink it?

> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
> ---
> fs/ext4/extents.c |    2 --
> 1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 6888d1a..720070d 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -1737,8 +1737,6 @@ int ext4_ext_insert_extent(handle_t *handle, struct inode *inode,
>     * There is no free space in the found leaf.
>     * We're gonna add a new leaf in the tree.
>     */
> -    if (flag & EXT4_GET_BLOCKS_PUNCH_OUT_EXT)
> -        flags = EXT4_MB_USE_ROOT_BLOCKS;
>    err = ext4_ext_create_new_leaf(handle, inode, flags, path, newext);
>    if (err)
>        goto cleanup;
> -- 
> 1.7.5.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] ext4: remove code related to punching hole from ext4_ext_insert_extent

[Yongqiang Yang]

On Fri, Nov 18, 2011 at 12:56 AM, Andreas Dilger <adilger@dilger.ca> wrote:
> On 2011-11-16, at 19:03, Yongqiang Yang <xiaoqiangnk@gmail.com> wrote:
>
>> Punch hole should never call ext4_ext_insert_extent, so this patch
>> removes code related to it from ext4_ext_insert_extent.
>
> Is that still true if punch hole is used beyond the end of the file?  I don't recall whether the semantics of this mean to extend the file size or to shrink it?
Add Allison to cc list.

Punch hole is defined to 'free blocks' in documentation, so the part
beyond the end of a file should be just ignored.  Both ext4 and btrfs
act this way.

Yongqiang.
>
>> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
>> ---
>> fs/ext4/extents.c |    2 --
>> 1 files changed, 0 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index 6888d1a..720070d 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -1737,8 +1737,6 @@ int ext4_ext_insert_extent(handle_t *handle, struct inode *inode,
>>     * There is no free space in the found leaf.
>>     * We're gonna add a new leaf in the tree.
>>     */
>> -    if (flag & EXT4_GET_BLOCKS_PUNCH_OUT_EXT)
>> -        flags = EXT4_MB_USE_ROOT_BLOCKS;
>>    err = ext4_ext_create_new_leaf(handle, inode, flags, path, newext);
>>    if (err)
>>        goto cleanup;
>> --
>> 1.7.5.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] ext4: remove code related to punching hole from ext4_ext_insert_extent

[Allison Henderson]

On 11/16/2011 07:03 PM, Yongqiang Yang wrote:
> Punch hole should never call ext4_ext_insert_extent, so this patch
> removes code related to it from ext4_ext_insert_extent.
>
> Signed-off-by: Yongqiang Yang<xiaoqiangnk@gmail.com>
> ---
>   fs/ext4/extents.c |    2 --
>   1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 6888d1a..720070d 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -1737,8 +1737,6 @@ int ext4_ext_insert_extent(handle_t *handle, struct inode *inode,
>   	 * There is no free space in the found leaf.
>   	 * We're gonna add a new leaf in the tree.
>   	 */
> -	if (flag&  EXT4_GET_BLOCKS_PUNCH_OUT_EXT)
> -		flags = EXT4_MB_USE_ROOT_BLOCKS;
>   	err = ext4_ext_create_new_leaf(handle, inode, flags, path, newext);
>   	if (err)
>   		goto cleanup;

Hi Yongqiang,

Actually I believe it does end up inserting an extent if an extent gets 
split. For example, we punch a hole in the middle of an extent, so we 
first split the extent into three pieces, and remove the middle piece.

Because inserting the extra extents can require extra blocks, the 
operation may temporarily consume blocks. The problem that this causes 
is that if the file system is really full, it can fail with ENOSPC, even 
though punch hole is an operation that is supposed to free blocks.  The 
above EXT4_MB_USE_ROOT_BLOCKS flag was put in so that when we punch 
holes, we can borrow reserved blocks to complete the operation to avoid 
the ENOSPC problem.  I had a script to catch this and added to xfstests 
(test 256). So unless there was a change somewhere that I missed, it 
does not make sense to me to take it out at this point.

Allison Henderson

Re: [PATCH 4/6] ext4: remove code related to punching hole from ext4_ext_insert_extent

[Yongqiang Yang]

On Fri, Dec 2, 2011 at 4:55 AM, Allison Henderson
<achender@linux.vnet.ibm.com> wrote:
> On 11/16/2011 07:03 PM, Yongqiang Yang wrote:
>>
>> Punch hole should never call ext4_ext_insert_extent, so this patch
>> removes code related to it from ext4_ext_insert_extent.
>>
>> Signed-off-by: Yongqiang Yang<xiaoqiangnk@gmail.com>
>> ---
>>  fs/ext4/extents.c |    2 --
>>  1 files changed, 0 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index 6888d1a..720070d 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -1737,8 +1737,6 @@ int ext4_ext_insert_extent(handle_t *handle, struct
>> inode *inode,
>>         * There is no free space in the found leaf.
>>         * We're gonna add a new leaf in the tree.
>>         */
>> -       if (flag&  EXT4_GET_BLOCKS_PUNCH_OUT_EXT)
>> -               flags = EXT4_MB_USE_ROOT_BLOCKS;
>>        err = ext4_ext_create_new_leaf(handle, inode, flags, path, newext);
>>        if (err)
>>                goto cleanup;
>
> Hi Yongqiang,
>
> Actually I believe it does end up inserting an extent if an extent gets
> split. For example, we punch a hole in the middle of an extent, so we first
> split the extent into three pieces, and remove the middle piece.
>
> Because inserting the extra extents can require extra blocks, the operation
> may temporarily consume blocks. The problem that this causes is that if the
> file system is really full, it can fail with ENOSPC, even though punch hole
> is an operation that is supposed to free blocks.  The above
> EXT4_MB_USE_ROOT_BLOCKS flag was put in so that when we punch holes, we can
> borrow reserved blocks to complete the operation to avoid the ENOSPC
> problem.  I had a script to catch this and added to xfstests (test 256). So
> unless there was a change somewhere that I missed, it does not make sense to
> me to take it out at this point.
Got it.  Thank you for your explanation.

Yongqiang.
>
> Allison Henderson
>
>



-- 
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] ext4: remove useless code in ext4_ext_split

[Yongqiang Yang]

New allocated blocks have been checked after allocating,
so there is no need to recheck them.

Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
---
 fs/ext4/extents.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 720070d..d483635 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -857,11 +857,6 @@ static int ext4_ext_split(handle_t *handle, struct inode *inode,
 
 	/* initialize new leaf */
 	newblock = ablocks[--a];
-	if (unlikely(newblock == 0)) {
-		EXT4_ERROR_INODE(inode, "newblock == 0!");
-		err = -EIO;
-		goto cleanup;
-	}
 	bh = sb_getblk(inode->i_sb, newblock);
 	if (!bh) {
 		err = -EIO;
-- 
1.7.5.1

[PATCH 6/6] ext4: move code checking if leaf is full to the beginning of ext4_ext_split

[Yongqiang Yang]

The work checking if the lead is full can be done before we do actual splitting
work.

Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
---
 fs/ext4/extents.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index d483635..6f0300e 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -817,6 +817,15 @@ static int ext4_ext_split(handle_t *handle, struct inode *inode,
 		EXT4_ERROR_INODE(inode, "p_ext > EXT_MAX_EXTENT!");
 		return -EIO;
 	}
+
+	if (unlikely(path[depth].p_hdr->eh_entries !=
+		     path[depth].p_hdr->eh_max)) {
+		EXT4_ERROR_INODE(inode, "eh_entries %d != eh_max %d!",
+				 path[depth].p_hdr->eh_entries,
+				 path[depth].p_hdr->eh_max);
+		return -EIO;
+	}
+
 	if (path[depth].p_ext != EXT_MAX_EXTENT(path[depth].p_hdr)) {
 		border = path[depth].p_ext[1].ee_block;
 		ext_debug("leaf will be split."
@@ -875,14 +884,6 @@ static int ext4_ext_split(handle_t *handle, struct inode *inode,
 	neh->eh_depth = 0;
 
 	/* move remainder of path[depth] to the new leaf */
-	if (unlikely(path[depth].p_hdr->eh_entries !=
-		     path[depth].p_hdr->eh_max)) {
-		EXT4_ERROR_INODE(inode, "eh_entries %d != eh_max %d!",
-				 path[depth].p_hdr->eh_entries,
-				 path[depth].p_hdr->eh_max);
-		err = -EIO;
-		goto cleanup;
-	}
 	/* start copy from next extent */
 	m = EXT_MAX_EXTENT(path[depth].p_hdr) - path[depth].p_ext++;
 	ext4_ext_show_move(inode, path, newblock, depth);
-- 
1.7.5.1