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 <[email protected]>
---
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
npath is set to NULL and is not set again before BUG_ON, so
the BUG_ON is useless.
Signed-off-by: Yongqiang Yang <[email protected]>
---
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
ext4_ext_try_to_merge merges both to right and left.
Signed-off-by: Yongqiang Yang <[email protected]>
---
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 */
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 <[email protected]>
---
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
New allocated blocks have been checked after allocating,
so there is no need to recheck them.
Signed-off-by: Yongqiang Yang <[email protected]>
---
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
The work checking if the lead is full can be done before we do actual splitting
work.
Signed-off-by: Yongqiang Yang <[email protected]>
---
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
On 2011-11-16, at 19:03, Yongqiang Yang <[email protected]> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 18, 2011 at 12:56 AM, Andreas Dilger <[email protected]> wrote:
> On 2011-11-16, at 19:03, Yongqiang Yang <[email protected]> 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 <[email protected]>
>> ---
>> 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 [email protected]
>> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>
--
Best Wishes
Yongqiang Yang
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<[email protected]>
> ---
> 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
On Fri, Dec 2, 2011 at 4:55 AM, Allison Henderson
<[email protected]> 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<[email protected]>
>> ---
>> ?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