Patchwork [2/6] ext4: move inode indepth shrink logic to didicated function

login
register
mail settings
Submitter Dmitri Monakho
Date Oct. 20, 2011, 9:08 p.m.
Message ID <1319144939-28801-3-git-send-email-dmonakhov@openvz.org>
Download mbox | patch
Permalink /patch/120881/
State New
Headers show

Comments

Dmitri Monakho - Oct. 20, 2011, 9:08 p.m.
- 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(-)
Theodore Ts'o - Oct. 25, 2011, 8:01 a.m.
On Fri, Oct 21, 2011 at 01:08:55AM +0400, Dmitry Monakhov wrote:
> - 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>

This patch is broken in two ways:

1) It drops the absolutely necessary calls to ext4_ext_get_access()
    and ext4_ext_dirty().  If you don't do this you will get file
    system corruptions.

2) Some of the callers to the new ext4_ext_try_shrink() helper depends
   on it return 0 or 1 depending on whether the tree was shrunk, but
   others assumed that it would return an error code.  Which is OK,
   since the error codes should be negative, but that means it's
   critical that the callers check to see whether return code is
   really an error before returning it.

Since this is just a cleanup, I'm going to skip this for now.  Dmitry,
could you fix this up and resubmit?   Thanks!!

						- Ted
--
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
Dmitri Monakho - Oct. 28, 2011, 10:47 a.m.
On Tue, 25 Oct 2011 04:01:35 -0400, "Ted Ts'o" <tytso@mit.edu> wrote:
> On Fri, Oct 21, 2011 at 01:08:55AM +0400, Dmitry Monakhov wrote:
> > - 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>
> 
> This patch is broken in two ways:
> 
> 1) It drops the absolutely necessary calls to ext4_ext_get_access()
>     and ext4_ext_dirty().  If you don't do this you will get file
>     system corruptions.
You almost caught me.. When i read this comment for the first time, i've
thought that you right, and this patch is crap. Patch was written two
month ago so i almost forget exact details. But still it was strange
because whole series was tested very hard, and never saw any corruption,
or complains. From other point of view usually meta data modification
w/o prior get_write_access call result in complain from JBD thread about dirty
bufs in transaction.c:warn_dirty_buffer(). This never happen in my case.

Answer is simple: ext4_ext_get_access(path[0]) is noop in case of inode body
because it has no bh attached. And ext4_ext_dirty() turns in to
ext4_mark_inode_dirty(). That's why patch is correct..
> 
> 2) Some of the callers to the new ext4_ext_try_shrink() helper depends
>    on it return 0 or 1 depending on whether the tree was shrunk, but
>    others assumed that it would return an error code.  Which is OK,
>    since the error codes should be negative, but that means it's
>    critical that the callers check to see whether return code is
>    really an error before returning it.
and {0:1} retcode is sufficient. Situation will change after flexible
tree reduction appear, but this happens in perfect future.
> 
> Since this is just a cleanup, I'm going to skip this for now.  Dmitry,
> could you fix this up and resubmit?   Thanks!!
> 
> 						- Ted
--
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

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 8eb3656..315775e 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2570,6 +2570,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;
@@ -2577,7 +2593,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);
 
@@ -2703,29 +2719,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;
 }
 
 /*
@@ -3965,22 +3970,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));
-
-					err = ext4_ext_dirty(
-						handle, inode, path);
-				}
-			}
+			if (!err && ext4_ext_try_shrink(handle, inode))
+				err = ext4_mark_inode_dirty(handle, inode);
 
 			goto out2;
 		}