Patchwork [3/4] ext4: clone indirect.c file from inode.c

login
register
mail settings
Submitter Amir G.
Date June 21, 2011, 9:39 a.m.
Message ID <1308649168-12543-4-git-send-email-amir73il@users.sourceforge.net>
Download mbox | patch
Permalink /patch/101265/
State Superseded
Headers show

Comments

Amir G. - June 21, 2011, 9:39 a.m.
From: Amir Goldstein <amir73il@users.sf.net>

The next patch is going to move ext4_ind_ functions to
indirect.c. First, we clone the file from inode.c and
only leave code that is going to be duplicated in both files.
This should keep the deleted lines count from inode.c in the next
patch the same as the added lines count to indirect.c.

Signed-off-by: Amir Goldstein <amir73il@users.sf.net>
---
 fs/ext4/indirect.c |   92 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 92 insertions(+), 0 deletions(-)
 create mode 100644 fs/ext4/indirect.c
Theodore Ts'o - June 27, 2011, 11:41 p.m.
On Tue, Jun 21, 2011 at 12:39:27PM +0300, amir73il@users.sourceforge.net wrote:
> From: Amir Goldstein <amir73il@users.sf.net>
> 
> The next patch is going to move ext4_ind_ functions to
> indirect.c. First, we clone the file from inode.c and
> only leave code that is going to be duplicated in both files.
> This should keep the deleted lines count from inode.c in the next
> patch the same as the added lines count to indirect.c.

I really dislike cloning functions.  This becomes a maintenance
headache, since bugs that get fixed in one file might not get
propagated to another.  Since I need to manually move all of the
functions to verify nothing else changed in patches that do massive
code movement, I used the first two patches in your patch series, but
replaced the last two patches in the patch series, and replaced it
with the following:

A) Move __ext4_check_blockref() to fs/ext4/block_validity.c and
declare it extern.

B) Move ext4_truncate_failed_write() and blocks_for_truncate() to a
new function, fs/ext4/truncate.h.

C) Movement of indirect-related code to fs/ext4/indirect.c.

   	       			     	- 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
Amir G. - June 28, 2011, 7:02 a.m.
On Tue, Jun 28, 2011 at 2:41 AM, Ted Ts'o <tytso@mit.edu> wrote:
> On Tue, Jun 21, 2011 at 12:39:27PM +0300, amir73il@users.sourceforge.net wrote:
>> From: Amir Goldstein <amir73il@users.sf.net>
>>
>> The next patch is going to move ext4_ind_ functions to
>> indirect.c. First, we clone the file from inode.c and
>> only leave code that is going to be duplicated in both files.
>> This should keep the deleted lines count from inode.c in the next
>> patch the same as the added lines count to indirect.c.
>
> I really dislike cloning functions.  This becomes a maintenance
> headache, since bugs that get fixed in one file might not get
> propagated to another.  Since I need to manually move all of the
> functions to verify nothing else changed in patches that do massive
> code movement, I used the first two patches in your patch series, but
> replaced the last two patches in the patch series, and replaced it
> with the following:
>
> A) Move __ext4_check_blockref() to fs/ext4/block_validity.c and
> declare it extern.
>
> B) Move ext4_truncate_failed_write() and blocks_for_truncate() to a
> new function, fs/ext4/truncate.h.

Nice cleanups!

>
> C) Movement of indirect-related code to fs/ext4/indirect.c.
>

Hmm.. in my patch the deleted lines from inode.c matched
the added lines in indirect.c (-1 newline at end of file).
Your patch has:
 fs/ext4/indirect.c       | 1510 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/ext4/inode.c          | 1486 ---------------------------------------------

This accounts for 28 lines added by header comment and includes in indirect.c
-1 newline at end of file, so where did 3 more lines go?
I am guessing these are just deleted newlines, but have no way of knowing.
Have you made other (maybe checkpatch) changes while moving or just
removed 'static'
from function declarations?
I am asking just in case I get 'git blamed' for it some day ;-)

Cheers,
Amir.
--
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/indirect.c b/fs/ext4/indirect.c
new file mode 100644
index 0000000..98cf221
--- /dev/null
+++ b/fs/ext4/indirect.c
@@ -0,0 +1,92 @@ 
+/*
+ *  linux/fs/ext4/indirect.c
+ *
+ *  from
+ *
+ *  linux/fs/ext4/inode.c
+ *
+ * Copyright (C) 1992, 1993, 1994, 1995
+ * Remy Card (card@masi.ibp.fr)
+ * Laboratoire MASI - Institut Blaise Pascal
+ * Universite Pierre et Marie Curie (Paris VI)
+ *
+ *  from
+ *
+ *  linux/fs/minix/inode.c
+ *
+ *  Copyright (C) 1991, 1992  Linus Torvalds
+ *
+ *  Goal-directed block allocation by Stephen Tweedie
+ *	(sct@redhat.com), 1993, 1998
+ *  Big-endian to little-endian byte-swapping/bitmaps by
+ *        David S. Miller (davem@caip.rutgers.edu), 1995
+ *  64-bit file support on 64-bit platforms by Jakub Jelinek
+ *	(jj@sunsite.ms.mff.cuni.cz)
+ *
+ *  Assorted race fixes, rewrite of ext4_get_block() by Al Viro, 2000
+ */
+
+#include <linux/module.h>
+#include "ext4_jbd2.h"
+
+#include <trace/events/ext4.h>
+
+static int __ext4_check_blockref(const char *function, unsigned int line,
+				 struct inode *inode,
+				 __le32 *p, unsigned int max)
+{
+	struct ext4_super_block *es = EXT4_SB(inode->i_sb)->s_es;
+	__le32 *bref = p;
+	unsigned int blk;
+
+	while (bref < p+max) {
+		blk = le32_to_cpu(*bref++);
+		if (blk &&
+		    unlikely(!ext4_data_block_valid(EXT4_SB(inode->i_sb),
+						    blk, 1))) {
+			es->s_last_error_block = cpu_to_le64(blk);
+			ext4_error_inode(inode, function, line, blk,
+					 "invalid block");
+			return -EIO;
+		}
+	}
+	return 0;
+}
+
+
+/*
+ * Truncate blocks that were not used by write. We have to truncate the
+ * pagecache as well so that corresponding buffers get properly unmapped.
+ */
+static void ext4_truncate_failed_write(struct inode *inode)
+{
+	truncate_inode_pages(inode->i_mapping, inode->i_size);
+	ext4_truncate(inode);
+}
+
+/*
+ * Work out how many blocks we need to proceed with the next chunk of a
+ * truncate transaction.
+ */
+static unsigned long blocks_for_truncate(struct inode *inode)
+{
+	ext4_lblk_t needed;
+
+	needed = inode->i_blocks >> (inode->i_sb->s_blocksize_bits - 9);
+
+	/* Give ourselves just enough room to cope with inodes in which
+	 * i_blocks is corrupt: we've seen disk corruptions in the past
+	 * which resulted in random data in an inode which looked enough
+	 * like a regular file for ext4 to try to delete it.  Things
+	 * will go a bit crazy if that happens, but at least we should
+	 * try not to panic the whole kernel. */
+	if (needed < 2)
+		needed = 2;
+
+	/* But we need to bound the transaction so we don't overflow the
+	 * journal. */
+	if (needed > EXT4_MAX_TRANS_DATA)
+		needed = EXT4_MAX_TRANS_DATA;
+
+	return EXT4_DATA_TRANS_BLOCKS(inode->i_sb) + needed;
+}