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

login
register
mail settings
Submitter Yongqiang Yang
Date Nov. 17, 2011, 2:03 a.m.
Message ID <1321495405-9583-4-git-send-email-xiaoqiangnk@gmail.com>
Download mbox | patch
Permalink /patch/126110/
State New
Headers show

Comments

Yongqiang Yang - Nov. 17, 2011, 2:03 a.m.
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(-)
Andreas Dilger - Nov. 17, 2011, 4:56 p.m.
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
--
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
Yongqiang Yang - Nov. 18, 2011, 3:19 a.m.
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
>
Allison Henderson - Dec. 1, 2011, 8:55 p.m.
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

--
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
Yongqiang Yang - Dec. 2, 2011, 1:09 a.m.
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
>
>

Patch

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;