[PATCH 0/7] ext4: cleanups and regression fixes for grow/shrink logic V3

[PATCH 0/7] ext4: cleanups and regression fixes for grow/shrink logic V3

[Dmitry Monakhov]

changes from v2 (in responce to comments from tytso@)
 - split punch_hole handler patch in to reviewable peaces.
 - redesign error path for ext4_ext_map_blocks

##First patch is a fix for real issue
 ext4: Restore old EOFBLOCKS flag state after error
##Code cleanup for shrink logic.
 ext4: move inode indepth shrink logic to didicated function
##The rest are punch hole logic fixes
 ext4: Move punch hole logic to didicated function
 ext4: punch_hole fix extent conversion
 ext4: Update inode's transaction id after punch_hole
 ext4: punch hole should be restarted after transaction restart
 ext4: update EOFBLOCK flags after punch hole

[PATCH 1/7] ext4: Restore old EOFBLOCKS flag state after error

[Dmitry Monakhov]

ext4_ext_insert_extent() may fail due to number of reasons (ENOSPC),
restore it's state before exit.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/extents.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 61abc13..eae9680 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3723,6 +3723,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 	ext4_fsblk_t newblock = 0;
 	int free_on_err = 0, err = 0, depth, ret;
+	int eof_fl = 0;
 	unsigned int allocated = 0, offset = 0;
 	unsigned int allocated_clusters = 0, reserved_clusters = 0;
 	unsigned int punched_out = 0;
@@ -4056,9 +4057,11 @@ got_allocated_blocks:
 	}
 
 	err = 0;
-	if ((flags & EXT4_GET_BLOCKS_KEEP_SIZE) == 0)
+	if ((flags & EXT4_GET_BLOCKS_KEEP_SIZE) == 0) {
+		eof_fl = ext4_test_inode_flag(inode, EXT4_INODE_EOFBLOCKS);
 		err = check_eofblocks_fl(handle, inode, map->m_lblk,
 					 path, ar.len);
+	}
 	if (!err)
 		err = ext4_ext_insert_extent(handle, inode, path,
 					     &newex, flags);
@@ -4071,6 +4074,12 @@ got_allocated_blocks:
 		ext4_discard_preallocations(inode);
 		ext4_free_blocks(handle, inode, NULL, ext4_ext_pblock(&newex),
 				 ext4_ext_get_actual_len(&newex), fb_flags);
+		/* Restore EOFBLOCKS flag state */
+		if (eof_fl &&
+		    !ext4_test_inode_flag(inode, EXT4_INODE_EOFBLOCKS)) {
+			ext4_set_inode_flag(inode, EXT4_INODE_EOFBLOCKS);
+			ext4_mark_inode_dirty(handle, inode);
+		}
 		goto out2;
 	}
 
-- 
1.7.1

[PATCH 2/7] ext4: move inode indepth shrink logic to didicated function

[Dmitry Monakhov]

- add ext4_ext_try_shrink helper
- ext4_mark_inode_dirty() called externally in order to allow
  caller to butch several inode updates in to one mark_dirty call.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/extents.c |   59 ++++++++++++++++++++++------------------------------
 1 files changed, 25 insertions(+), 34 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index eae9680..6e3ce38 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2496,6 +2496,22 @@ ext4_ext_more_to_rm(struct ext4_ext_path *path)
 	return 1;
 }
 
+static int ext4_ext_try_shrink(handle_t *handle, struct inode *inode)
+{
+	/* TODO: flexible tree reduction should be here */
+	if (ext_depth(inode) && ext_inode_hdr(inode)->eh_entries == 0) {
+		/*
+		 * truncate to zero freed all the tree,
+		 * so we need to correct eh_depth
+		 */
+		ext_inode_hdr(inode)->eh_depth = 0;
+		ext_inode_hdr(inode)->eh_max =
+			cpu_to_le16(ext4_ext_space_root(inode, 0));
+		return 1;
+	}
+	return 0;
+}
+
 static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
 {
 	struct super_block *sb = inode->i_sb;
@@ -2503,7 +2519,7 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start)
 	struct ext4_ext_path *path;
 	ext4_fsblk_t partial_cluster = 0;
 	handle_t *handle;
-	int i, err;
+	int i, err, err2;
 
 	ext_debug("truncate since %u\n", start);
 
@@ -2629,29 +2645,18 @@ again:
 				 EXT4_SB(sb)->s_cluster_ratio, flags);
 		partial_cluster = 0;
 	}
-
-	/* TODO: flexible tree reduction should be here */
-	if (path->p_hdr->eh_entries == 0) {
-		/*
-		 * truncate to zero freed all the tree,
-		 * so we need to correct eh_depth
-		 */
-		err = ext4_ext_get_access(handle, inode, path);
-		if (err == 0) {
-			ext_inode_hdr(inode)->eh_depth = 0;
-			ext_inode_hdr(inode)->eh_max =
-				cpu_to_le16(ext4_ext_space_root(inode, 0));
-			err = ext4_ext_dirty(handle, inode, path);
-		}
-	}
+	if(ext4_ext_try_shrink(handle, inode))
+		err2 = ext4_mark_inode_dirty(handle, inode);
+	if (!err)
+		err = err2;
 out:
 	ext4_ext_drop_refs(path);
 	kfree(path);
 	if (err == -EAGAIN)
 		goto again;
-	ext4_journal_stop(handle);
+	err2 = ext4_journal_stop(handle);
 
-	return err;
+	return err ? err : err2;
 }
 
 /*
@@ -3895,22 +3900,8 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 					       &partial_cluster, map->m_lblk,
 					       map->m_lblk + punched_out);
 
-			if (!err && path->p_hdr->eh_entries == 0) {
-				/*
-				 * Punch hole freed all of this sub tree,
-				 * so we need to correct eh_depth
-				 */
-				err = ext4_ext_get_access(handle, inode, path);
-				if (err == 0) {
-					ext_inode_hdr(inode)->eh_depth = 0;
-					ext_inode_hdr(inode)->eh_max =
-					cpu_to_le16(ext4_ext_space_root(
-						inode, 0));

[PATCH 3/7] ext4: Move punch hole logic to didicated function

[Dmitry Monakhov]

punch hole logic is sited directly in ext4_ext_map_blocks() on 3rd controll
level, IMHO one can easily screw-up his eyes while invastigating that code.
We have  nothing to hide aren't we? Let's move it to didicate function.
Do it similar to uninitialized extent handlers. Logic moved as is.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/extents.c |  145 +++++++++++++++++++++++++----------------------------
 1 files changed, 68 insertions(+), 77 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 6e3ce38..02dfe38 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3225,6 +3225,70 @@ static void unmap_underlying_metadata_blocks(struct block_device *bdev,
                 unmap_underlying_metadata(bdev, block + i);
 }
 
+static int
+ext4_ext_handle_punched_extent(handle_t *handle, struct inode *inode,
+			struct ext4_map_blocks *map, struct ext4_ext_path *path)
+{
+	struct ext4_extent *ex = path[path->p_depth].p_ext;
+	ext4_lblk_t ee_block =  ext4_ext_get_actual_len(ex);
+	unsigned short ee_len = le32_to_cpu(ex->ee_block);
+	struct ext4_map_blocks punch_map;
+	ext4_fsblk_t partial_cluster = 0;
+	unsigned int punched_out = 0;
+   	int err;
+
+	/* Punch out the map length, but only to the end of the extent */
+	punched_out = ee_len - (map->m_lblk - ee_block);
+	if (punched_out > map->m_len)
+		punched_out = map->m_len;
+	/*
+	 * Sense extents need to be converted to uninitialized, they must
+	 * fit in an uninitialized extent
+	 */
+	if (punched_out > EXT_UNINIT_MAX_LEN)
+		punched_out = EXT_UNINIT_MAX_LEN;
+
+	punch_map.m_lblk = map->m_lblk;
+	punch_map.m_pblk = map->m_lblk - ee_block + ext4_ext_pblock(ex);;
+	punch_map.m_len = punched_out;
+	punch_map.m_flags = 0;
+
+	/* Check to see if the extent needs to be split */
+	if (punch_map.m_len != ee_len || punch_map.m_lblk != ee_block) {
+		err = ext4_split_extent(handle, inode, path, &punch_map, 0,
+					EXT4_GET_BLOCKS_PUNCH_OUT_EXT |
+					EXT4_GET_BLOCKS_PRE_IO);
+		if (err < 0)
+			goto out;
+		/*
+		 * find extent for the block at the start of the hole
+		 */
+		ext4_ext_drop_refs(path);
+		kfree(path);
+
+		path = ext4_ext_find_extent(inode, map->m_lblk, NULL);
+		if (IS_ERR(path)) {
+			err = PTR_ERR(path);
+			path = NULL;
+			goto out;
+		}
+		ex = path[path->p_depth].p_ext;
+	}
+
+	ext4_ext_mark_uninitialized(ex);
+	ext4_ext_invalidate_cache(inode);
+	err = ext4_ext_rm_leaf(handle, inode, path, &partial_cluster,
+			       map->m_lblk, map->m_lblk + punched_out);
+	if (!err && ext4_ext_try_shrink(handle, inode))
+		err = ext4_mark_inode_dirty(handle, inode);
+out:
+	if (path) {
+		ext4_ext_drop_refs(path);
+		kfree(path);
+	}
+
+	return err ? err : punched_out;
+}
 /*
  * Handle EOFBLOCKS_FL flag, clearing it if necessary
  */
@@ -3731,12 +3795,9 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 	int eof_fl = 0;
 	unsigned int allocated = 0, offset = 0;
 	unsigned int allocated_clusters = 0, reserved_clusters = 0;
-	unsigned int punched_out = 0;
-	unsigned int result = 0;
 	struct ext4_allocation_request ar;
 	ext4_io_end_t *io = EXT4_I(inode)->cur_aio_dio;
 	ext4_lblk_t cluster_offset;
-	struct ext4_map_blocks punch_map;
 
 	ext_debug("blocks %u/%u requested for inode %lu\n",
 		  map->m_lblk, map->m_len, inode->i_ino);
@@ -3812,8 +3873,6 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 
 		/* if found extent covers block, simply return it */
 		if (in_range(map->m_lblk, ee_block, ee_len)) {
-			ext4_fsblk_t partial_cluster = 0;
-
 			newblock = map->m_lblk - ee_block + ee_start;
 			/* number of remaining blocks in the extent */
 			allocated = ee_len - (map->m_lblk - ee_block);
@@ -3835,75 +3894,9 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 					allocated, newblock);
 				return ret;
 			}
-
-			/*
-			 * Punch out the map length, but only to the
-			 * end of the extent
-			 */
-			punched_out = allocated < map->m_len ?
-				allocated : map->m_len;
-
-			/*
-			 * Sense extents need to be converted to
-			 * uninitialized, they must fit in an
-			 * uninitialized extent
-			 */
-			if (punched_out > EXT_UNINIT_MAX_LEN)
-				punched_out = EXT_UNINIT_MAX_LEN;
-
-			punch_map.m_lblk = map->m_lblk;
-			punch_map.m_pblk = newblock;
-			punch_map.m_len = punched_out;
-			punch_map.m_flags = 0;
-
-			/* Check to see if the extent needs to be split */
-			if (punch_map.m_len != ee_len ||
-				punch_map.m_lblk != ee_block) {
-
-				ret = ext4_split_extent(handle, inode,
-				path, &punch_map, 0,
-				EXT4_GET_BLOCKS_PUNCH_OUT_EXT |
-				EXT4_GET_BLOCKS_PRE_IO);
-
-				if (ret < 0) {
-					err = ret;
-					goto out2;
-				}
-				/*
-				 * find extent for the block at
-				 * the start of the hole
-				 */
-				ext4_ext_drop_refs(path);
-				kfree(path);
-
-				path = ext4_ext_find_extent(inode,
-				map->m_lblk, NULL);
-				if (IS_ERR(path)) {
-					err = PTR_ERR(path);
-					path = NULL;
-					goto out2;
-				}
-
-				depth = ext_depth(inode);
-				ex = path[depth].p_ext;
-				ee_len = ext4_ext_get_actual_len(ex);
-				ee_block = le32_to_cpu(ex->ee_block);
-				ee_start = ext4_ext_pblock(ex);
-
-			}
-
-			ext4_ext_mark_uninitialized(ex);
-
-			ext4_ext_invalidate_cache(inode);
-
-			err = ext4_ext_rm_leaf(handle, inode, path,
-					       &partial_cluster, map->m_lblk,
-					       map->m_lblk + punched_out);
-
-			if (!err && ext4_ext_try_shrink(handle, inode))
-				err = ext4_mark_inode_dirty(handle, inode);

[PATCH 4/7] ext4: punch_hole fix extent conversion

[Dmitry Monakhov]

Currently punched extent converted to uninitialized incorrectly,
in fact i'm not shure whenever we actually need that conversion,
but if we do that let's do it right.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/extents.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 02dfe38..0365173 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3275,8 +3275,15 @@ ext4_ext_handle_punched_extent(handle_t *handle, struct inode *inode,
 		ex = path[path->p_depth].p_ext;
 	}
 
-	ext4_ext_mark_uninitialized(ex);
+	err = ext4_ext_get_access(handle, inode, path + path->p_depth);
+	if (err)
+		goto out;
 	ext4_ext_invalidate_cache(inode);
+	ext4_ext_mark_uninitialized(ex);
+	err = ext4_ext_dirty(handle, inode, path + path->p_depth);
+	if (err)
+		goto out;
+
 	err = ext4_ext_rm_leaf(handle, inode, path, &partial_cluster,
 			       map->m_lblk, map->m_lblk + punched_out);
 	if (!err && ext4_ext_try_shrink(handle, inode))
-- 
1.7.1

[PATCH 5/7] ext4: Update inode's transaction id after punch_hole

[Dmitry Monakhov]

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/extents.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 0365173..5cce39a 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3289,6 +3289,7 @@ ext4_ext_handle_punched_extent(handle_t *handle, struct inode *inode,
 	if (!err && ext4_ext_try_shrink(handle, inode))
 		err = ext4_mark_inode_dirty(handle, inode);
 out:
+	ext4_update_inode_fsync_trans(handle, inode, 0);
 	if (path) {
 		ext4_ext_drop_refs(path);
 		kfree(path);
-- 
1.7.1

[PATCH 6/7] ext4: punch hole should be restarted after transaction restart

[Dmitry Monakhov]

ext4_ext_rm_leaf() may return -EAGAIN due to transaction restart,
repeat attempt in that case.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/extents.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 5cce39a..a77cde2 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3795,7 +3795,7 @@ static int get_implied_cluster_alloc(struct super_block *sb,
 int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 			struct ext4_map_blocks *map, int flags)
 {
-	struct ext4_ext_path *path = NULL;
+	struct ext4_ext_path *path;
 	struct ext4_extent newex, *ex, *ex2;
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 	ext4_fsblk_t newblock = 0;
@@ -3811,6 +3811,9 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 		  map->m_lblk, map->m_len, inode->i_ino);
 	trace_ext4_ext_map_blocks_enter(inode, map->m_lblk, map->m_len, flags);
 
+again:
+	path = NULL;
+
 	/* check in cache */
 	if (!(flags & EXT4_GET_BLOCKS_PUNCH_OUT_EXT) &&
 		ext4_ext_in_cache(inode, map->m_lblk, &newex)) {
@@ -3904,6 +3907,8 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 			}
 			ret = ext4_ext_handle_punched_extent(handle, inode,
 							map, path);
+			if (ret == -EAGAIN)
+				goto again;
 			return ret;
 		}
 	}
-- 
1.7.1

[PATCH 7/7] ext4: update EOFBLOCK flags after punch hole

[Dmitry Monakhov]

Clear EOFBLOCK if punched extent is the only uninitialized extent
beyond eof, unfortunately we can not reuse check_eofblock_fl()
logic here because it purpose to handle file expansion,
but in our case we have to recheck base invariant that:
clear_eof_flag = (eof_block >= last_allocated_block)

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/extents.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index a77cde2..40f1248 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3235,7 +3235,7 @@ ext4_ext_handle_punched_extent(handle_t *handle, struct inode *inode,
 	struct ext4_map_blocks punch_map;
 	ext4_fsblk_t partial_cluster = 0;
 	unsigned int punched_out = 0;
-   	int err;
+   	int err, inode_changed = 0;
 
 	/* Punch out the map length, but only to the end of the extent */
 	punched_out = ee_len - (map->m_lblk - ee_block);
@@ -3286,8 +3286,55 @@ ext4_ext_handle_punched_extent(handle_t *handle, struct inode *inode,
 
 	err = ext4_ext_rm_leaf(handle, inode, path, &partial_cluster,
 			       map->m_lblk, map->m_lblk + punched_out);
-	if (!err && ext4_ext_try_shrink(handle, inode))
-		err = ext4_mark_inode_dirty(handle, inode);
+	if (err)
+		goto out;
+
+	inode_changed = ext4_ext_try_shrink(handle, inode);
+	/*
+	 * We have punched out an extent, if it was the only extent beyond
+	 * i_size  eofblocks flag should be cleared.
+	 */
+	if (ext4_test_inode_flag(inode, EXT4_INODE_EOFBLOCKS)) {
+		ext4_fsblk_t eof_block =
+			(inode->i_size + (1 << inode->i_blkbits) - 1) >>
+				inode->i_blkbits;
+		/* find the latest extent */
+		ext4_ext_drop_refs(path);
+		kfree(path);
+		path = ext4_ext_find_extent(inode, EXT_MAX_BLOCKS -1, NULL);
+		if (IS_ERR(path)) {
+			err = PTR_ERR(path);
+			path = NULL;
+			goto out;
+		}
+		ex = path[path->p_depth].p_ext;
+		if (ex) {
+			ee_len = ext4_ext_get_actual_len(ex);
+			ee_block = le32_to_cpu(ex->ee_block);
+		} else {
+			/* Inode is empty */
+			ee_block = ee_len = 0;
+		}
+		if (eof_block >= ee_block + ee_len) {
+			ext4_clear_inode_flag(inode, EXT4_INODE_EOFBLOCKS);
+			inode_changed = 1;
+		} else if (!ext4_ext_is_uninitialized(ex)) {
+			EXT4_ERROR_INODE(inode, "initialized extent beyond "
+					"EOF i_size: %lld, ex[%u:%u] "
+					"depth: %d pblock %lld",
+					inode->i_size, ee_block, ee_len,
+					path->p_depth,
+					path[path->p_depth].p_block);
+			err = -EIO;
+			/* Continue, because inode shrink should be
+			 * accomplished regardless to staled eof blocks */
+		}
+	}
+	if (inode_changed) {
+		int err2 = ext4_mark_inode_dirty(handle, inode);
+		if (!err)
+			err = err2;
+	}
 out:
 	ext4_update_inode_fsync_trans(handle, inode, 0);
 	if (path) {
-- 
1.7.1

Re: [PATCH 0/7] ext4: cleanups and regression fixes for grow/shrink logic V3

[Allison Henderson]

On 10/28/2011 10:00 AM, Dmitry Monakhov wrote:
> changes from v2 (in responce to comments from tytso@)
>   - split punch_hole handler patch in to reviewable peaces.
>   - redesign error path for ext4_ext_map_blocks
>
> ##First patch is a fix for real issue
>   ext4: Restore old EOFBLOCKS flag state after error
> ##Code cleanup for shrink logic.
>   ext4: move inode indepth shrink logic to didicated function
> ##The rest are punch hole logic fixes
>   ext4: Move punch hole logic to didicated function
>   ext4: punch_hole fix extent conversion
>   ext4: Update inode's transaction id after punch_hole
>   ext4: punch hole should be restarted after transaction restart
>   ext4: update EOFBLOCK flags after punch hole

Hi Dmitry,

I went through this set, and I think it looks good.  I didnt catch 
anything that hasnt already been explained in earlier versions, and I 
think it looks cleaner now.  Thx!

Allison Henderson