All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] ext4: Use bitops to read/modify EXT4_I(inode)->i_flags
@ 2010-04-19 14:32 Dmitry Monakhov
  2010-04-19 14:32 ` [PATCH 2/2] ext4: fix eofblock flag handling Dmitry Monakhov
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Dmitry Monakhov @ 2010-04-19 14:32 UTC (permalink / raw)
  To: linux-ext4; +Cc: Dmitry Monakhov

At several places we modify EXT4_I(inode)->i_flags without holding
i_mutex (ext4_do_update_inode, ...). These modifications are racy and we can
lose updates to i_flags. So convert handling of i_flags to use bitops
which are atomic.
https://bugzilla.kernel.org/show_bug.cgi?id=15792

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/dir.c         |    4 +-
 fs/ext4/ext4.h        |  103 +++++++++++++++++++++++++++++++-----------------
 fs/ext4/ext4_jbd2.h   |    8 ++--
 fs/ext4/extents.c     |   10 ++--
 fs/ext4/file.c        |    2 +-
 fs/ext4/ialloc.c      |    4 +-
 fs/ext4/inode.c       |   71 ++++++++++++++++++++--------------
 fs/ext4/mballoc.c     |    4 +-
 fs/ext4/migrate.c     |    4 +-
 fs/ext4/move_extent.c |    4 +-
 fs/ext4/namei.c       |   10 ++--
 fs/ext4/xattr.c       |    4 +-
 12 files changed, 135 insertions(+), 93 deletions(-)

diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index 86cb6d8..a696746 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -111,7 +111,7 @@ static int ext4_readdir(struct file *filp,
 
 	if (EXT4_HAS_COMPAT_FEATURE(inode->i_sb,
 				    EXT4_FEATURE_COMPAT_DIR_INDEX) &&
-	    ((EXT4_I(inode)->i_flags & EXT4_INDEX_FL) ||
+	    (ext4_test_inode_flag(inode, EXT4_INODE_INDEX) ||
 	     ((inode->i_size >> sb->s_blocksize_bits) == 1))) {
 		err = ext4_dx_readdir(filp, dirent, filldir);
 		if (err != ERR_BAD_DX_DIR) {
@@ -122,7 +122,7 @@ static int ext4_readdir(struct file *filp,
 		 * We don't set the inode dirty flag since it's not
 		 * critical that it get flushed back to the disk.
 		 */
-		EXT4_I(filp->f_path.dentry->d_inode)->i_flags &= ~EXT4_INDEX_FL;
+		ext4_clear_inode_flag(inode, EXT4_INODE_INDEX);
 	}
 	stored = 0;
 	offset = filp->f_pos & (sb->s_blocksize - 1);
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index bf938cf..6641c58 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -268,31 +268,57 @@ struct flex_groups {
 /*
  * Inode flags
  */
-#define	EXT4_SECRM_FL			0x00000001 /* Secure deletion */
-#define	EXT4_UNRM_FL			0x00000002 /* Undelete */
-#define	EXT4_COMPR_FL			0x00000004 /* Compress file */
-#define EXT4_SYNC_FL			0x00000008 /* Synchronous updates */
-#define EXT4_IMMUTABLE_FL		0x00000010 /* Immutable file */
-#define EXT4_APPEND_FL			0x00000020 /* writes to file may only append */
-#define EXT4_NODUMP_FL			0x00000040 /* do not dump file */
-#define EXT4_NOATIME_FL			0x00000080 /* do not update atime */
+enum {
+	EXT4_INODE_SECRM	= 0, 	/* Secure deletion */
+	EXT4_INODE_UNORM	= 1, 	/* Undelete */
+	EXT4_INODE_COMPR 	= 2,	/* Compress file */
+	EXT4_INODE_SYNC		= 3,	/* Synchronous updates */
+	EXT4_INODE_IMMUTABLE	= 4,	/* Immutable file */
+	EXT4_INODE_APPEND	= 5,	/* writes to file may only append */
+	EXT4_INODE_NODUMP	= 6,	/* do not dump file */
+	EXT4_INODE_NOATIME	= 7,	/* do not update atime */
 /* Reserved for compression usage... */
-#define EXT4_DIRTY_FL			0x00000100
-#define EXT4_COMPRBLK_FL		0x00000200 /* One or more compressed clusters */
-#define EXT4_NOCOMPR_FL			0x00000400 /* Don't compress */
-#define EXT4_ECOMPR_FL			0x00000800 /* Compression error */
+	EXT4_INODE_DIRTY	= 8,
+	EXT4_INODE_COMPRBLK	= 9,	/* One or more compressed clusters */
+	EXT4_INODE_NOCOMPR	= 10,	/* Don't compress */
+	EXT4_INODE_ECOMPR	= 11,	/* Compression error */
 /* End compression flags --- maybe not all used */
-#define EXT4_INDEX_FL			0x00001000 /* hash-indexed directory */
-#define EXT4_IMAGIC_FL			0x00002000 /* AFS directory */
-#define EXT4_JOURNAL_DATA_FL		0x00004000 /* file data should be journaled */
-#define EXT4_NOTAIL_FL			0x00008000 /* file tail should not be merged */
-#define EXT4_DIRSYNC_FL			0x00010000 /* dirsync behaviour (directories only) */
-#define EXT4_TOPDIR_FL			0x00020000 /* Top of directory hierarchies*/
-#define EXT4_HUGE_FILE_FL               0x00040000 /* Set to each huge file */
-#define EXT4_EXTENTS_FL			0x00080000 /* Inode uses extents */
-#define EXT4_EA_INODE_FL	        0x00200000 /* Inode used for large EA */
-#define EXT4_EOFBLOCKS_FL		0x00400000 /* Blocks allocated beyond EOF */
-#define EXT4_RESERVED_FL		0x80000000 /* reserved for ext4 lib */
+	EXT4_INODE_INDEX	= 12,	/* hash-indexed directory */
+	EXT4_INODE_IMAGIC	= 13,	/* AFS directory */
+	EXT4_INODE_JOURNAL_DATA	= 14,	/* file data should be journaled */
+	EXT4_INODE_NOTAIL	= 15,	/* file tail should not be merged */
+	EXT4_INODE_DIRSYNC	= 16,	/* dirsync behaviour (directories only) */
+	EXT4_INODE_TOPDIR	= 17,	/* Top of directory hierarchies*/
+	EXT4_INODE_HUGE_FILE	= 18,	/* Set to each huge file */
+	EXT4_INODE_EXTENTS	= 19,	/* Inode uses extents */
+	EXT4_INODE_EA_INODE	= 20,	/* Inode used for large EA */
+	EXT4_INODE_EOFBLOCKS	= 22,	/* Blocks allocated beyond EOF */
+	EXT4_INODE_RESERVED	= 23,	/* reserved for ext4 lib */
+};
+
+#define	EXT4_SECRM_FL		(1 << EXT4_INODE_SECRM)
+#define	EXT4_UNRM_FL		(1 << EXT4_INODE_UNORM)
+#define	EXT4_COMPR_FL		(1 << EXT4_INODE_COMPR)
+#define EXT4_SYNC_FL		(1 << EXT4_INODE_SYNC)
+#define EXT4_IMMUTABLE_FL	(1 << EXT4_INODE_IMMUTABLE)
+#define EXT4_APPEND_FL		(1 << EXT4_INODE_APPEND)
+#define EXT4_NODUMP_FL		(1 << EXT4_INODE_NODUMP)
+#define EXT4_NOATIME_FL		(1 << EXT4_INODE_NOATIME)
+#define EXT4_DIRTY_FL		(1 << EXT4_INODE_DIRTY)
+#define EXT4_COMPRBLK_FL	(1 << EXT4_INODE_COMPRBLK)
+#define EXT4_NOCOMPR_FL		(1 << EXT4_INODE_NOCOMPR)
+#define EXT4_ECOMPR_FL		(1 << EXT4_INODE_ECOMPR)
+#define EXT4_INDEX_FL		(1 << EXT4_INODE_INDEX)
+#define EXT4_IMAGIC_FL		(1 << EXT4_INODE_IMAGIC)
+#define EXT4_JOURNAL_DATA_FL	(1 << EXT4_INODE_JOURNAL_DATA)
+#define EXT4_NOTAIL_FL		(1 << EXT4_INODE_NOTAIL)
+#define EXT4_DIRSYNC_FL		(1 << EXT4_INODE_DIRSYNC)
+#define EXT4_TOPDIR_FL		(1 << EXT4_INODE_TOPDIR)
+#define EXT4_HUGE_FILE_FL	(1 << EXT4_INODE_HUGE_FILE)
+#define EXT4_EXTENTS_FL		(1 << EXT4_INODE_EXTENTS)
+#define EXT4_EA_INODE_FL	(1 << EXT4_INODE_EA_INODE)
+#define EXT4_EOFBLOCKS_FL	(1 << EXT4_INODE_EOFBLOCKS)
+#define EXT4_RESERVED_FL	(1 << EXT4_INODE_RESERVED)
 
 #define EXT4_FL_USER_VISIBLE		0x004BDFFF /* User visible flags */
 #define EXT4_FL_USER_MODIFIABLE		0x004B80FF /* User modifiable flags */
@@ -616,7 +642,6 @@ struct ext4_ext_cache {
  */
 struct ext4_inode_info {
 	__le32	i_data[15];	/* unconverted */
-	__u32	i_flags;
 	ext4_fsblk_t	i_file_acl;
 	__u32	i_dtime;
 
@@ -628,8 +653,10 @@ struct ext4_inode_info {
 	 * near to their parent directory's inode.
 	 */
 	ext4_group_t	i_block_group;
+	unsigned long	i_flags;
 	unsigned long	i_state_flags;		/* Dynamic state flags */
 
+
 	ext4_lblk_t		i_dir_start_lookup;
 #ifdef CONFIG_EXT4_FS_XATTR
 	/*
@@ -1064,20 +1091,22 @@ enum {
 	EXT4_STATE_DIO_UNWRITTEN,	/* need convert on dio done*/
 };
 
-static inline int ext4_test_inode_state(struct inode *inode, int bit)
-{
-	return test_bit(bit, &EXT4_I(inode)->i_state_flags);
-}
-
-static inline void ext4_set_inode_state(struct inode *inode, int bit)
-{
-	set_bit(bit, &EXT4_I(inode)->i_state_flags);
+#define EXT4_INODE_BIT_FNS(name, field)					\
+static inline int ext4_test_inode_##name(struct inode *inode, int bit)	\
+{									\
+	return test_bit(bit, &EXT4_I(inode)->i_##field);		\
+}									\
+static inline void ext4_set_inode_##name(struct inode *inode, int bit) 	\
+{									\
+	set_bit(bit, &EXT4_I(inode)->i_##field);			\
+}									\
+static inline void ext4_clear_inode_##name(struct inode *inode, int bit) \
+{									\
+	clear_bit(bit, &EXT4_I(inode)->i_##field);			\
 }
 
-static inline void ext4_clear_inode_state(struct inode *inode, int bit)
-{
-	clear_bit(bit, &EXT4_I(inode)->i_state_flags);
-}
+EXT4_INODE_BIT_FNS(flag, flags)
+EXT4_INODE_BIT_FNS(state, state_flags)
 #else
 /* Assume that user mode programs are passing in an ext4fs superblock, not
  * a kernel struct super_block.  This will allow us to call the feature-test
@@ -1264,7 +1293,7 @@ struct ext4_dir_entry_2 {
 
 #define is_dx(dir) (EXT4_HAS_COMPAT_FEATURE(dir->i_sb, \
 				      EXT4_FEATURE_COMPAT_DIR_INDEX) && \
-		      (EXT4_I(dir)->i_flags & EXT4_INDEX_FL))
+			ext4_test_inode_flag((dir), EXT4_INODE_INDEX))
 #define EXT4_DIR_LINK_MAX(dir) (!is_dx(dir) && (dir)->i_nlink >= EXT4_LINK_MAX)
 #define EXT4_DIR_LINK_EMPTY(dir) ((dir)->i_nlink == 2 || (dir)->i_nlink == 1)
 
diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
index b79ad51..f35d77c 100644
--- a/fs/ext4/ext4_jbd2.h
+++ b/fs/ext4/ext4_jbd2.h
@@ -273,7 +273,7 @@ static inline int ext4_should_journal_data(struct inode *inode)
 		return 1;
 	if (test_opt(inode->i_sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA)
 		return 1;
-	if (EXT4_I(inode)->i_flags & EXT4_JOURNAL_DATA_FL)
+	if (ext4_test_inode_flag(inode, EXT4_INODE_JOURNAL_DATA))
 		return 1;
 	return 0;
 }
@@ -284,7 +284,7 @@ static inline int ext4_should_order_data(struct inode *inode)
 		return 0;
 	if (!S_ISREG(inode->i_mode))
 		return 0;
-	if (EXT4_I(inode)->i_flags & EXT4_JOURNAL_DATA_FL)
+	if (ext4_test_inode_flag(inode, EXT4_INODE_JOURNAL_DATA))
 		return 0;
 	if (test_opt(inode->i_sb, DATA_FLAGS) == EXT4_MOUNT_ORDERED_DATA)
 		return 1;
@@ -297,7 +297,7 @@ static inline int ext4_should_writeback_data(struct inode *inode)
 		return 0;
 	if (EXT4_JOURNAL(inode) == NULL)
 		return 1;
-	if (EXT4_I(inode)->i_flags & EXT4_JOURNAL_DATA_FL)
+	if (ext4_test_inode_flag(inode, EXT4_INODE_JOURNAL_DATA))
 		return 0;
 	if (test_opt(inode->i_sb, DATA_FLAGS) == EXT4_MOUNT_WRITEBACK_DATA)
 		return 1;
@@ -321,7 +321,7 @@ static inline int ext4_should_dioread_nolock(struct inode *inode)
 		return 0;
 	if (!S_ISREG(inode->i_mode))
 		return 0;
-	if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL))
+	if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
 		return 0;
 	if (ext4_should_journal_data(inode))
 		return 0;
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 1938418..7c7b1d5 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3509,7 +3509,7 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
 			set_buffer_uninit(bh_result);
 	}
 
-	if (unlikely(EXT4_I(inode)->i_flags & EXT4_EOFBLOCKS_FL)) {
+	if (unlikely(ext4_test_inode_flag(inode, EXT4_INODE_EOFBLOCKS))) {
 		if (unlikely(!eh->eh_entries)) {
 			EXT4_ERROR_INODE(inode,
 					 "eh->eh_entries == 0 ee_block %d",
@@ -3520,7 +3520,7 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
 		last_ex = EXT_LAST_EXTENT(eh);
 		if (iblock + ar.len > le32_to_cpu(last_ex->ee_block)
 		    + ext4_ext_get_actual_len(last_ex))
-			EXT4_I(inode)->i_flags &= ~EXT4_EOFBLOCKS_FL;
+			ext4_clear_inode_flag(inode, EXT4_INODE_EOFBLOCKS);
 	}
 	err = ext4_ext_insert_extent(handle, inode, path, &newex, flags);
 	if (err) {
@@ -3661,7 +3661,7 @@ static void ext4_falloc_update_inode(struct inode *inode,
 		 * can proceed even if the new size is the same as i_size.
 		 */
 		if (new_size > i_size_read(inode))
-			EXT4_I(inode)->i_flags |= EXT4_EOFBLOCKS_FL;
+			ext4_set_inode_flag(inode, EXT4_INODE_EOFBLOCKS);
 	}
 
 }
@@ -3689,7 +3689,7 @@ long ext4_fallocate(struct inode *inode, int mode, loff_t offset, loff_t len)
 	 * currently supporting (pre)allocate mode for extent-based
 	 * files _only_
 	 */
-	if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL))
+	if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
 		return -EOPNOTSUPP;
 
 	/* preallocation to directories is currently not supported */
@@ -3933,7 +3933,7 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 	int error = 0;
 
 	/* fallback to generic here if not in extents fmt */
-	if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL))
+	if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
 		return generic_block_fiemap(inode, fieinfo, start, len,
 			ext4_get_block);
 
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index d0776e4..a708d15 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -66,7 +66,7 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 	 * is smaller than s_maxbytes, which is for extent-mapped files.
 	 */
 
-	if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL)) {
+	if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
 		struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 		size_t length = iov_length(iov, nr_segs);
 
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 25fe42f..8d3a4d6 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -499,7 +499,7 @@ static int find_group_orlov(struct super_block *sb, struct inode *parent,
 
 	if (S_ISDIR(mode) &&
 	    ((parent == sb->s_root->d_inode) ||
-	     (EXT4_I(parent)->i_flags & EXT4_TOPDIR_FL))) {
+		    ext4_test_inode_flag(parent, EXT4_INODE_TOPDIR))) {
 		int best_ndir = inodes_per_group;
 		int ret = -1;
 
@@ -1045,7 +1045,7 @@ got:
 	if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_EXTENTS)) {
 		/* set extent flag only for directory, file and normal symlink*/
 		if (S_ISDIR(mode) || S_ISREG(mode) || S_ISLNK(mode)) {
-			EXT4_I(inode)->i_flags |= EXT4_EXTENTS_FL;
+			ext4_set_inode_flag(inode, EXT4_INODE_EXTENTS);
 			ext4_ext_tree_init(handle, inode);
 		}
 	}
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 11119e0..8fe4e68 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -932,7 +932,7 @@ static int ext4_ind_get_blocks(handle_t *handle, struct inode *inode,
 	int count = 0;
 	ext4_fsblk_t first_block = 0;
 
-	J_ASSERT(!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL));
+	J_ASSERT(!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS));
 	J_ASSERT(handle != NULL || (flags & EXT4_GET_BLOCKS_CREATE) == 0);
 	depth = ext4_block_to_path(inode, iblock, offsets,
 				   &blocks_to_boundary);
@@ -1060,7 +1060,7 @@ static int ext4_indirect_calc_metadata_amount(struct inode *inode,
  */
 static int ext4_calc_metadata_amount(struct inode *inode, sector_t lblock)
 {
-	if (EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL)
+	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
 		return ext4_ext_calc_metadata_amount(inode, lblock);
 
 	return ext4_indirect_calc_metadata_amount(inode, lblock);
@@ -1249,7 +1249,7 @@ int ext4_get_blocks(handle_t *handle, struct inode *inode, sector_t block,
 	 * file system block.
 	 */
 	down_read((&EXT4_I(inode)->i_data_sem));
-	if (EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL) {
+	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
 		retval =  ext4_ext_get_blocks(handle, inode, block, max_blocks,
 				bh, 0);
 	} else {
@@ -1311,7 +1311,7 @@ int ext4_get_blocks(handle_t *handle, struct inode *inode, sector_t block,
 	 * We need to check for EXT4 here because migrate
 	 * could have changed the inode type in between
 	 */
-	if (EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL) {
+	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
 		retval =  ext4_ext_get_blocks(handle, inode, block, max_blocks,
 					      bh, flags);
 	} else {
@@ -2349,7 +2349,7 @@ static void mpage_add_bh_to_extent(struct mpage_da_data *mpd,
 	int nrblocks = mpd->b_size >> mpd->inode->i_blkbits;
 
 	/* check if thereserved journal credits might overflow */
-	if (!(EXT4_I(mpd->inode)->i_flags & EXT4_EXTENTS_FL)) {
+	if (!ext4_test_inode_flag(mpd->inode, EXT4_INODE_EXTENTS)) {
 		if (nrblocks >= EXT4_MAX_TRANS_DATA) {
 			/*
 			 * With non-extent format we are limited by the journal
@@ -2820,7 +2820,7 @@ static int ext4_da_writepages_trans_blocks(struct inode *inode)
 	 * number of contiguous block. So we will limit
 	 * number of contiguous block to a sane value
 	 */
-	if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL) &&
+	if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS) &&
 	    (max_blocks > EXT4_MAX_TRANS_DATA))
 		max_blocks = EXT4_MAX_TRANS_DATA;
 
@@ -3972,7 +3972,7 @@ static ssize_t ext4_direct_IO(int rw, struct kiocb *iocb,
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file->f_mapping->host;
 
-	if (EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL)
+	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
 		return ext4_ext_direct_IO(rw, iocb, iov, offset, nr_segs);
 
 	return ext4_ind_direct_IO(rw, iocb, iov, offset, nr_segs);
@@ -4611,12 +4611,12 @@ void ext4_truncate(struct inode *inode)
 	if (!ext4_can_truncate(inode))
 		return;
 
-	EXT4_I(inode)->i_flags &= ~EXT4_EOFBLOCKS_FL;
+	ext4_clear_inode_flag(inode, EXT4_INODE_EOFBLOCKS);
 
 	if (inode->i_size == 0 && !test_opt(inode->i_sb, NO_AUTO_DA_ALLOC))
 		ext4_set_inode_state(inode, EXT4_STATE_DA_ALLOC_CLOSE);
 
-	if (EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL) {
+	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
 		ext4_ext_truncate(inode);
 		return;
 	}
@@ -4922,19 +4922,32 @@ void ext4_set_inode_flags(struct inode *inode)
 void ext4_get_inode_flags(struct ext4_inode_info *ei)
 {
 	unsigned int flags = ei->vfs_inode.i_flags;
+	struct inode *inode =  &(ei->vfs_inode);
 
-	ei->i_flags &= ~(EXT4_SYNC_FL|EXT4_APPEND_FL|
-			EXT4_IMMUTABLE_FL|EXT4_NOATIME_FL|EXT4_DIRSYNC_FL);
 	if (flags & S_SYNC)
-		ei->i_flags |= EXT4_SYNC_FL;
+		ext4_set_inode_flag(inode, EXT4_INODE_SYNC);
+	else
+		ext4_clear_inode_flag(inode, EXT4_INODE_SYNC);
+
 	if (flags & S_APPEND)
-		ei->i_flags |= EXT4_APPEND_FL;
+		ext4_set_inode_flag(inode, EXT4_INODE_APPEND);
+	else
+		ext4_clear_inode_flag(inode, EXT4_INODE_APPEND);
+
 	if (flags & S_IMMUTABLE)
-		ei->i_flags |= EXT4_IMMUTABLE_FL;
+		ext4_set_inode_flag(inode, EXT4_INODE_IMMUTABLE);
+	else
+		ext4_clear_inode_flag(inode, EXT4_INODE_IMMUTABLE);
+
 	if (flags & S_NOATIME)
-		ei->i_flags |= EXT4_NOATIME_FL;
+		ext4_set_inode_flag(inode, EXT4_INODE_NOATIME);
+	else
+		ext4_clear_inode_flag(inode, EXT4_INODE_NOATIME);
+
 	if (flags & S_DIRSYNC)
-		ei->i_flags |= EXT4_DIRSYNC_FL;
+		ext4_set_inode_flag(inode, EXT4_INODE_DIRSYNC);
+	else
+		ext4_clear_inode_flag(inode, EXT4_INODE_DIRSYNC);
 }
 
 static blkcnt_t ext4_inode_blocks(struct ext4_inode *raw_inode,
@@ -4949,7 +4962,7 @@ static blkcnt_t ext4_inode_blocks(struct ext4_inode *raw_inode,
 		/* we are using combined 48 bit field */
 		i_blocks = ((u64)le16_to_cpu(raw_inode->i_blocks_high)) << 32 |
 					le32_to_cpu(raw_inode->i_blocks_lo);
-		if (ei->i_flags & EXT4_HUGE_FILE_FL) {
+		if (ext4_test_inode_flag(inode, EXT4_INODE_HUGE_FILE)) {
 			/* i_blocks represent file system block size */
 			return i_blocks  << (inode->i_blkbits - 9);
 		} else {
@@ -5099,7 +5112,7 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
 			   ei->i_file_acl, inode->i_ino);
 		ret = -EIO;
 		goto bad_inode;
-	} else if (ei->i_flags & EXT4_EXTENTS_FL) {
+	} else if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
 		if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
 		    (S_ISLNK(inode->i_mode) &&
 		     !ext4_inode_is_fast_symlink(inode)))
@@ -5171,7 +5184,7 @@ static int ext4_inode_blocks_set(handle_t *handle,
 		 */
 		raw_inode->i_blocks_lo   = cpu_to_le32(i_blocks);
 		raw_inode->i_blocks_high = 0;
-		ei->i_flags &= ~EXT4_HUGE_FILE_FL;
+		ext4_clear_inode_flag(inode, EXT4_INODE_HUGE_FILE);
 		return 0;
 	}
 	if (!EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_HUGE_FILE))
@@ -5184,9 +5197,9 @@ static int ext4_inode_blocks_set(handle_t *handle,
 		 */
 		raw_inode->i_blocks_lo   = cpu_to_le32(i_blocks);
 		raw_inode->i_blocks_high = cpu_to_le16(i_blocks >> 32);
-		ei->i_flags &= ~EXT4_HUGE_FILE_FL;
+		ext4_clear_inode_flag(inode, EXT4_INODE_HUGE_FILE);
 	} else {
-		ei->i_flags |= EXT4_HUGE_FILE_FL;
+		ext4_set_inode_flag(inode, EXT4_INODE_HUGE_FILE);
 		/* i_block is stored in file system block size */
 		i_blocks = i_blocks >> (inode->i_blkbits - 9);
 		raw_inode->i_blocks_lo   = cpu_to_le32(i_blocks);
@@ -5453,7 +5466,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 	}
 
 	if (attr->ia_valid & ATTR_SIZE) {
-		if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL)) {
+		if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {
 			struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 
 			if (attr->ia_size > sbi->s_bitmap_maxbytes) {
@@ -5464,9 +5477,9 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 	}
 
 	if (S_ISREG(inode->i_mode) &&
-	    attr->ia_valid & ATTR_SIZE &&
-	    (attr->ia_size < inode->i_size ||
-	     (EXT4_I(inode)->i_flags & EXT4_EOFBLOCKS_FL))) {
+		attr->ia_valid & ATTR_SIZE &&
+		(attr->ia_size < inode->i_size ||
+			ext4_test_inode_flag(inode, EXT4_INODE_EOFBLOCKS))) {
 		handle_t *handle;
 
 		handle = ext4_journal_start(inode, 3);
@@ -5498,7 +5511,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 			}
 		}
 		/* ext4_truncate will clear the flag */
-		if ((EXT4_I(inode)->i_flags & EXT4_EOFBLOCKS_FL))
+		if (ext4_test_inode_flag(inode, EXT4_INODE_EOFBLOCKS))
 			ext4_truncate(inode);
 	}
 
@@ -5574,7 +5587,7 @@ static int ext4_indirect_trans_blocks(struct inode *inode, int nrblocks,
 
 static int ext4_index_trans_blocks(struct inode *inode, int nrblocks, int chunk)
 {
-	if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL))
+	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
 		return ext4_indirect_trans_blocks(inode, nrblocks, chunk);
 	return ext4_ext_index_trans_blocks(inode, nrblocks, chunk);
 }
@@ -5909,9 +5922,9 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
 	 */
 
 	if (val)
-		EXT4_I(inode)->i_flags |= EXT4_JOURNAL_DATA_FL;
+		ext4_set_inode_flag(inode, EXT4_INODE_JOURNAL_DATA);
 	else
-		EXT4_I(inode)->i_flags &= ~EXT4_JOURNAL_DATA_FL;
+		ext4_clear_inode_flag(inode, EXT4_INODE_JOURNAL_DATA);
 	ext4_set_aops(inode);
 
 	jbd2_journal_unlock_updates(journal);
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 506713a..c24a1a3 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -1963,7 +1963,7 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
 	sbi = EXT4_SB(sb);
 	ngroups = ext4_get_groups_count(sb);
 	/* non-extent files are limited to low blocks/groups */
-	if (!(EXT4_I(ac->ac_inode)->i_flags & EXT4_EXTENTS_FL))
+	if (!ext4_test_inode_flag(ac->ac_inode, EXT4_INODE_EXTENTS))
 		ngroups = sbi->s_blockfile_groups;
 
 	BUG_ON(ac->ac_status == AC_STATUS_FOUND);
@@ -3121,7 +3121,7 @@ ext4_mb_use_preallocated(struct ext4_allocation_context *ac)
 			continue;
 
 		/* non-extent files can't have physical blocks past 2^32 */
-		if (!(EXT4_I(ac->ac_inode)->i_flags & EXT4_EXTENTS_FL) &&
+		if (!ext4_test_inode_flag(ac->ac_inode, EXT4_INODE_EXTENTS) &&
 			pa->pa_pstart + pa->pa_len > EXT4_MAX_BLOCK_FILE_PHYS)
 			continue;
 
diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
index 8b87bd0..7b8b52d 100644
--- a/fs/ext4/migrate.c
+++ b/fs/ext4/migrate.c
@@ -375,7 +375,7 @@ static int ext4_ext_swap_inode_data(handle_t *handle, struct inode *inode,
 	 * We have the extent map build with the tmp inode.
 	 * Now copy the i_data across
 	 */
-	ei->i_flags |= EXT4_EXTENTS_FL;
+	ext4_set_inode_flag(inode, EXT4_INODE_EXTENTS);
 	memcpy(ei->i_data, tmp_ei->i_data, sizeof(ei->i_data));
 
 	/*
@@ -474,7 +474,7 @@ int ext4_ext_migrate(struct inode *inode)
 	 */
 	if (!EXT4_HAS_INCOMPAT_FEATURE(inode->i_sb,
 				       EXT4_FEATURE_INCOMPAT_EXTENTS) ||
-	    (EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL))
+		ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
 		return -EINVAL;
 
 	if (S_ISLNK(inode->i_mode) && inode->i_blocks == 0)
diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
index b399358..b1c5b76 100644
--- a/fs/ext4/move_extent.c
+++ b/fs/ext4/move_extent.c
@@ -976,11 +976,11 @@ mext_check_arguments(struct inode *orig_inode,
 	}
 
 	/* Ext4 move extent supports only extent based file */
-	if (!(EXT4_I(orig_inode)->i_flags & EXT4_EXTENTS_FL)) {
+	if (!ext4_test_inode_flag(orig_inode, EXT4_INODE_EXTENTS)) {
 		ext4_debug("ext4 move extent: orig file is not extents "
 			"based file [ino:orig %lu]\n", orig_inode->i_ino);
 		return -EOPNOTSUPP;
-	} else if (!(EXT4_I(donor_inode)->i_flags & EXT4_EXTENTS_FL)) {
+	} else if (!ext4_test_inode_flag(donor_inode, EXT4_INODE_EXTENTS)) {
 		ext4_debug("ext4 move extent: donor file is not extents "
 			"based file [ino:donor %lu]\n", donor_inode->i_ino);
 		return -EOPNOTSUPP;
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 0c070fa..e3066d3 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -656,7 +656,7 @@ int ext4_htree_fill_tree(struct file *dir_file, __u32 start_hash,
 	dxtrace(printk(KERN_DEBUG "In htree_fill_tree, start hash: %x:%x\n", 
 		       start_hash, start_minor_hash));
 	dir = dir_file->f_path.dentry->d_inode;
-	if (!(EXT4_I(dir)->i_flags & EXT4_INDEX_FL)) {
+	if (!ext4_test_inode_flag(dir, EXT4_INODE_INDEX)) {
 		hinfo.hash_version = EXT4_SB(dir->i_sb)->s_def_hash_version;
 		if (hinfo.hash_version <= DX_HASH_TEA)
 			hinfo.hash_version +=
@@ -801,7 +801,7 @@ static void ext4_update_dx_flag(struct inode *inode)
 {
 	if (!EXT4_HAS_COMPAT_FEATURE(inode->i_sb,
 				     EXT4_FEATURE_COMPAT_DIR_INDEX))
-		EXT4_I(inode)->i_flags &= ~EXT4_INDEX_FL;
+		ext4_clear_inode_flag(inode, EXT4_INODE_INDEX);
 }
 
 /*
@@ -1418,7 +1418,7 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
 		brelse(bh);
 		return retval;
 	}
-	EXT4_I(dir)->i_flags |= EXT4_INDEX_FL;
+	ext4_set_inode_flag(dir, EXT4_INODE_INDEX);
 	data1 = bh2->b_data;
 
 	memcpy (data1, de, len);
@@ -1491,7 +1491,7 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
 		retval = ext4_dx_add_entry(handle, dentry, inode);
 		if (!retval || (retval != ERR_BAD_DX_DIR))
 			return retval;
-		EXT4_I(dir)->i_flags &= ~EXT4_INDEX_FL;
+		ext4_clear_inode_flag(dir, EXT4_INODE_INDEX);
 		dx_fallback++;
 		ext4_mark_inode_dirty(handle, dir);
 	}
@@ -2297,7 +2297,7 @@ retry:
 		}
 	} else {
 		/* clear the extent format for fast symlink */
-		EXT4_I(inode)->i_flags &= ~EXT4_EXTENTS_FL;
+		ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS);
 		inode->i_op = &ext4_fast_symlink_inode_operations;
 		memcpy((char *)&EXT4_I(inode)->i_data, symname, l);
 		inode->i_size = l-1;
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index b4c5aa8..277e9e6 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -820,7 +820,7 @@ inserted:
 						EXT4_I(inode)->i_block_group);
 
 			/* non-extent files can't have physical blocks past 2^32 */
-			if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL))
+			if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
 				goal = goal & EXT4_MAX_BLOCK_FILE_PHYS;
 
 			block = ext4_new_meta_blocks(handle, inode,
@@ -828,7 +828,7 @@ inserted:
 			if (error)
 				goto cleanup;
 
-			if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL))
+			if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
 				BUG_ON(block > EXT4_MAX_BLOCK_FILE_PHYS);
 
 			ea_idebug(inode, "creating block %d", block);
-- 
1.6.6.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/2] ext4: fix eofblock flag handling
  2010-04-19 14:32 [PATCH 1/2] ext4: Use bitops to read/modify EXT4_I(inode)->i_flags Dmitry Monakhov
@ 2010-04-19 14:32 ` Dmitry Monakhov
  2010-05-25  4:17   ` tytso
  2010-05-24  3:09 ` [PATCH 1/2] ext4: Use bitops to read/modify EXT4_I(inode)->i_flags tytso
  2010-05-24 20:49 ` [PATCH -v2] ext4: Use bitops to read/modify i_flags in struct ext4_inode_info Theodore Ts'o
  2 siblings, 1 reply; 17+ messages in thread
From: Dmitry Monakhov @ 2010-04-19 14:32 UTC (permalink / raw)
  To: linux-ext4; +Cc: Dmitry Monakhov

- Fix NULL pointer deference on error path
- Extent header we found may be not latest node of the inode. In order to
  find latest extent we have to traverse a path from very beginning.

https://bugzilla.kernel.org/show_bug.cgi?id=15792

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

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 7c7b1d5..13758c3 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3327,7 +3327,7 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
 {
 	struct ext4_ext_path *path = NULL;
 	struct ext4_extent_header *eh;
-	struct ext4_extent newex, *ex, *last_ex;
+	struct ext4_extent newex, *ex;
 	ext4_fsblk_t newblock;
 	int err = 0, depth, ret, cache_type;
 	unsigned int allocated = 0;
@@ -3510,17 +3510,38 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
 	}
 
 	if (unlikely(ext4_test_inode_flag(inode, EXT4_INODE_EOFBLOCKS))) {
-		if (unlikely(!eh->eh_entries)) {
-			EXT4_ERROR_INODE(inode,
-					 "eh->eh_entries == 0 ee_block %d",
-					 ex->ee_block);
-			err = -EIO;
-			goto out2;
-		}
-		last_ex = EXT_LAST_EXTENT(eh);
+		struct ext4_extent *last_ex = EXT_LAST_EXTENT(eh);
+		/*
+		 * Optimization: Extent header we found may not be the latest
+		 * extent of the inode, but it is already in-core memory.
+		 * Let's test against this EH to avoid unecessery IO.
+		*/
+		if (unlikely(!eh->eh_entries))
+			goto bad_eh;
 		if (iblock + ar.len > le32_to_cpu(last_ex->ee_block)
-		    + ext4_ext_get_actual_len(last_ex))
-			ext4_clear_inode_flag(inode, EXT4_INODE_EOFBLOCKS);
+			+ ext4_ext_get_actual_len(last_ex)) {
+			struct ext4_ext_path *path2;
+			struct ext4_extent_header *eh2;
+			/* Real search of the latests extent is necessery */
+			path2 = ext4_ext_find_extent(inode, EXT_MAX_BLOCK, NULL);
+			eh2 = path2[depth].p_hdr;
+			if (IS_ERR(path2)) {
+				err = PTR_ERR(path2);
+				goto out2;
+			}
+			last_ex = path2[depth].p_ext;
+			if (unlikely(!eh2->eh_entries || !last_ex)) {
+				ext4_ext_drop_refs(path2);
+				kfree(path2);
+				goto bad_eh;
+			}
+			if (iblock + ar.len > le32_to_cpu(last_ex->ee_block)
+				+ ext4_ext_get_actual_len(last_ex))
+				ext4_clear_inode_flag(inode, EXT4_INODE_EOFBLOCKS);
+			ext4_ext_drop_refs(path2);
+			kfree(path2);
+		}
+
 	}
 	err = ext4_ext_insert_extent(handle, inode, path, &newex, flags);
 	if (err) {
@@ -3570,6 +3591,12 @@ out2:
 		kfree(path);
 	}
 	return err ? err : allocated;
+
+bad_eh:
+	EXT4_ERROR_INODE(inode, "eh->eh_entries == 0, i_flags = %lx iblock = %u"
+			, EXT4_I(inode)->i_flags, iblock);
+	err = -EIO;
+	goto out2;
 }
 
 void ext4_ext_truncate(struct inode *inode)
@@ -3580,6 +3607,7 @@ void ext4_ext_truncate(struct inode *inode)
 	handle_t *handle;
 	int err = 0;
 
+	BUG_ON(ext4_test_inode_flag(inode, EXT4_INODE_EOFBLOCKS));
 	/*
 	 * probably first extent we're gonna free will be last in block
 	 */
-- 
1.6.6.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] ext4: Use bitops to read/modify EXT4_I(inode)->i_flags
  2010-04-19 14:32 [PATCH 1/2] ext4: Use bitops to read/modify EXT4_I(inode)->i_flags Dmitry Monakhov
  2010-04-19 14:32 ` [PATCH 2/2] ext4: fix eofblock flag handling Dmitry Monakhov
@ 2010-05-24  3:09 ` tytso
  2010-05-24 20:49 ` [PATCH -v2] ext4: Use bitops to read/modify i_flags in struct ext4_inode_info Theodore Ts'o
  2 siblings, 0 replies; 17+ messages in thread
From: tytso @ 2010-05-24  3:09 UTC (permalink / raw)
  To: Dmitry Monakhov; +Cc: linux-ext4

On Mon, Apr 19, 2010 at 06:32:16PM +0400, Dmitry Monakhov wrote:
> At several places we modify EXT4_I(inode)->i_flags without holding
> i_mutex (ext4_do_update_inode, ...). These modifications are racy and we can
> lose updates to i_flags. So convert handling of i_flags to use bitops
> which are atomic.
> https://bugzilla.kernel.org/show_bug.cgi?id=15792

I don't think it would cause any harm, since those flags aren't used
(yet) but this patch is buggy; The values for EXT4_INODE_EA_INODE, and
EXT4_INODE_RESERVED are wrong.  Fortunately I'm a paranoid b*stard, so
I caught it by *not* deleting the explicit EXT4_XXX_FL flags, and
using this:

#define TEST_FLAG_VALUE(FLAG) (EXT4_##FLAG##_FL == (1 << EXT4_INODE_##FLAG))
#define CHECK_FLAG_VALUE(FLAG) if (!TEST_FLAG_VALUE(FLAG)) { \
	printk("EXT4 flag fail: " #FLAG ": %d %d\n", EXT4_##FLAG##_FL, \
	       EXT4_INODE_##FLAG); BUG_ON(1); }

/*
 * Since it's pretty easy to mix up bit numbers and hex values, and we
 * can't do a compile-time test for ENUM values, we use a run-time
 * test to make sure that EXT4_XXX_FL is consistent with respect to
 * EXT4_INODE_XXX.  If all is well the printk and BUG_ON will all drop
 * out so it won't cost any extra space in the compiled kernel image.
 * But it's important that these values are the same, since we are
 * using EXT4_INODE_XXX to test for the flag values, but EXT4_XX_FL
 * must be consistent with the values of FS_XXX_FL defined in
 * include/linux/fs.h and the on-disk values found in ext2, ext3, and
 * ext4 filesystems, and of course the values defined in e2fsprogs.
 *
 * It's not paranoia if Murphy's Law really *is* out to get you.  :-)
 */
static inline void ext4_check_flag_values(void)
{
	CHECK_FLAG_VALUE(SECRM);
	CHECK_FLAG_VALUE(UNRM);
	CHECK_FLAG_VALUE(COMPR);
	CHECK_FLAG_VALUE(SYNC);
	CHECK_FLAG_VALUE(IMMUTABLE);
	CHECK_FLAG_VALUE(APPEND);
	CHECK_FLAG_VALUE(NODUMP);
	CHECK_FLAG_VALUE(NOATIME);
	CHECK_FLAG_VALUE(DIRTY);
	CHECK_FLAG_VALUE(COMPRBLK);
	CHECK_FLAG_VALUE(NOCOMPR);
	CHECK_FLAG_VALUE(ECOMPR);
	CHECK_FLAG_VALUE(INDEX);
	CHECK_FLAG_VALUE(IMAGIC);
	CHECK_FLAG_VALUE(JOURNAL_DATA);
	CHECK_FLAG_VALUE(NOTAIL);
	CHECK_FLAG_VALUE(DIRSYNC);
	CHECK_FLAG_VALUE(TOPDIR);
	CHECK_FLAG_VALUE(HUGE_FILE);
	CHECK_FLAG_VALUE(EXTENTS);
	CHECK_FLAG_VALUE(EA_INODE);
	CHECK_FLAG_VALUE(EOFBLOCKS);
	CHECK_FLAG_VALUE(RESERVED);
}

I'll send the full patch after I finish doing some testing, but I
thought I'd just point this out before I hit the sack...

	    	       	    	       	 - Ted

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH -v2] ext4: Use bitops to read/modify i_flags in struct ext4_inode_info
  2010-04-19 14:32 [PATCH 1/2] ext4: Use bitops to read/modify EXT4_I(inode)->i_flags Dmitry Monakhov
  2010-04-19 14:32 ` [PATCH 2/2] ext4: fix eofblock flag handling Dmitry Monakhov
  2010-05-24  3:09 ` [PATCH 1/2] ext4: Use bitops to read/modify EXT4_I(inode)->i_flags tytso
@ 2010-05-24 20:49 ` Theodore Ts'o
  2010-05-31  8:56   ` [PATCH] ext4: Use bitops to read/modify i_flags part2 Dmitry Monakhov
  2 siblings, 1 reply; 17+ messages in thread
From: Theodore Ts'o @ 2010-05-24 20:49 UTC (permalink / raw)
  To: Ext4 Developers List; +Cc: Theodore Ts'o, Dmitry Monakhov

From: "Theodore Ts'o" <tytso@mit.edu>

From: Dmitry Monakhov <dmonakhov@openvz.org>

At several places we modify EXT4_I(inode)->i_flags without holding
i_mutex (ext4_do_update_inode, ...). These modifications are racy and
we can lose updates to i_flags. So convert handling of i_flags to use
bitops which are atomic.

https://bugzilla.kernel.org/show_bug.cgi?id=15792

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/dir.c         |    4 +-
 fs/ext4/ext4.h        |  109 ++++++++++++++++++++++++++++++++++++++++++-------
 fs/ext4/ext4_jbd2.h   |    8 ++--
 fs/ext4/extents.c     |   10 ++--
 fs/ext4/file.c        |    2 +-
 fs/ext4/ialloc.c      |    4 +-
 fs/ext4/inode.c       |   30 +++++++-------
 fs/ext4/mballoc.c     |    4 +-
 fs/ext4/migrate.c     |    2 +-
 fs/ext4/move_extent.c |    4 +-
 fs/ext4/namei.c       |   10 ++--
 fs/ext4/super.c       |    1 +
 fs/ext4/xattr.c       |    4 +-
 13 files changed, 136 insertions(+), 56 deletions(-)

diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index 883d21e..42bae25 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -110,7 +110,7 @@ static int ext4_readdir(struct file *filp,
 
 	if (EXT4_HAS_COMPAT_FEATURE(inode->i_sb,
 				    EXT4_FEATURE_COMPAT_DIR_INDEX) &&
-	    ((EXT4_I(inode)->i_flags & EXT4_INDEX_FL) ||
+	    ((ext4_test_inode_flag(inode, EXT4_INODE_INDEX)) ||
 	     ((inode->i_size >> sb->s_blocksize_bits) == 1))) {
 		err = ext4_dx_readdir(filp, dirent, filldir);
 		if (err != ERR_BAD_DX_DIR) {
@@ -121,7 +121,7 @@ static int ext4_readdir(struct file *filp,
 		 * We don't set the inode dirty flag since it's not
 		 * critical that it get flushed back to the disk.
 		 */
-		EXT4_I(filp->f_path.dentry->d_inode)->i_flags &= ~EXT4_INDEX_FL;
+		ext4_clear_inode_flag(filp->f_path.dentry->d_inode, EXT4_INODE_INDEX);
 	}
 	stored = 0;
 	offset = filp->f_pos & (sb->s_blocksize - 1);
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 413321f..3b63837 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -344,6 +344,83 @@ static inline __u32 ext4_mask_flags(umode_t mode, __u32 flags)
 		return flags & EXT4_OTHER_FLMASK;
 }
 
+/*
+ * Inode flags used for atomic set/get
+ */
+enum {
+	EXT4_INODE_SECRM	= 0, 	/* Secure deletion */
+	EXT4_INODE_UNRM		= 1,	/* Undelete */
+	EXT4_INODE_COMPR 	= 2,	/* Compress file */
+	EXT4_INODE_SYNC		= 3,	/* Synchronous updates */
+	EXT4_INODE_IMMUTABLE	= 4,	/* Immutable file */
+	EXT4_INODE_APPEND	= 5,	/* writes to file may only append */
+	EXT4_INODE_NODUMP	= 6,	/* do not dump file */
+	EXT4_INODE_NOATIME	= 7,	/* do not update atime */
+/* Reserved for compression usage... */
+	EXT4_INODE_DIRTY	= 8,
+	EXT4_INODE_COMPRBLK	= 9,	/* One or more compressed clusters */
+	EXT4_INODE_NOCOMPR	= 10,	/* Don't compress */
+	EXT4_INODE_ECOMPR	= 11,	/* Compression error */
+/* End compression flags --- maybe not all used */
+	EXT4_INODE_INDEX	= 12,	/* hash-indexed directory */
+	EXT4_INODE_IMAGIC	= 13,	/* AFS directory */
+	EXT4_INODE_JOURNAL_DATA	= 14,	/* file data should be journaled */
+	EXT4_INODE_NOTAIL	= 15,	/* file tail should not be merged */
+	EXT4_INODE_DIRSYNC	= 16,	/* dirsync behaviour (directories only) */
+	EXT4_INODE_TOPDIR	= 17,	/* Top of directory hierarchies*/
+	EXT4_INODE_HUGE_FILE	= 18,	/* Set to each huge file */
+	EXT4_INODE_EXTENTS	= 19,	/* Inode uses extents */
+	EXT4_INODE_EA_INODE	= 21,	/* Inode used for large EA */
+	EXT4_INODE_EOFBLOCKS	= 22,	/* Blocks allocated beyond EOF */
+	EXT4_INODE_RESERVED	= 31,	/* reserved for ext4 lib */
+};
+
+#define TEST_FLAG_VALUE(FLAG) (EXT4_##FLAG##_FL == (1 << EXT4_INODE_##FLAG))
+#define CHECK_FLAG_VALUE(FLAG) if (!TEST_FLAG_VALUE(FLAG)) { \
+	printk("EXT4 flag fail: " #FLAG ": %d %d\n", EXT4_##FLAG##_FL, \
+	       EXT4_INODE_##FLAG); BUG_ON(1); }
+
+/*
+ * Since it's pretty easy to mix up bit numbers and hex values, and we
+ * can't do a compile-time test for ENUM values, we use a run-time
+ * test to make sure that EXT4_XXX_FL is consistent with respect to
+ * EXT4_INODE_XXX.  If all is well the printk and BUG_ON will all drop
+ * out so it won't cost any extra space in the compiled kernel image.
+ * But it's important that these values are the same, since we are
+ * using EXT4_INODE_XXX to test for the flag values, but EXT4_XX_FL
+ * must be consistent with the values of FS_XXX_FL defined in
+ * include/linux/fs.h and the on-disk values found in ext2, ext3, and
+ * ext4 filesystems, and of course the values defined in e2fsprogs.
+ *
+ * It's not paranoia if the Murphy's Law really *is* out to get you.  :-)
+ */
+static inline void ext4_check_flag_values(void)
+{
+	CHECK_FLAG_VALUE(SECRM);
+	CHECK_FLAG_VALUE(UNRM);
+	CHECK_FLAG_VALUE(COMPR);
+	CHECK_FLAG_VALUE(SYNC);
+	CHECK_FLAG_VALUE(IMMUTABLE);
+	CHECK_FLAG_VALUE(APPEND);
+	CHECK_FLAG_VALUE(NODUMP);
+	CHECK_FLAG_VALUE(NOATIME);
+	CHECK_FLAG_VALUE(DIRTY);
+	CHECK_FLAG_VALUE(COMPRBLK);
+	CHECK_FLAG_VALUE(NOCOMPR);
+	CHECK_FLAG_VALUE(ECOMPR);
+	CHECK_FLAG_VALUE(INDEX);
+	CHECK_FLAG_VALUE(IMAGIC);
+	CHECK_FLAG_VALUE(JOURNAL_DATA);
+	CHECK_FLAG_VALUE(NOTAIL);
+	CHECK_FLAG_VALUE(DIRSYNC);
+	CHECK_FLAG_VALUE(TOPDIR);
+	CHECK_FLAG_VALUE(HUGE_FILE);
+	CHECK_FLAG_VALUE(EXTENTS);
+	CHECK_FLAG_VALUE(EA_INODE);
+	CHECK_FLAG_VALUE(EOFBLOCKS);
+	CHECK_FLAG_VALUE(RESERVED);
+}
+
 /* Used to pass group descriptor data when online resize is done */
 struct ext4_new_group_input {
 	__u32 group;		/* Group number for this data */
@@ -639,9 +716,8 @@ struct ext4_ext_cache {
  */
 struct ext4_inode_info {
 	__le32	i_data[15];	/* unconverted */
-	__u32	i_flags;
-	ext4_fsblk_t	i_file_acl;
 	__u32	i_dtime;
+	ext4_fsblk_t	i_file_acl;
 
 	/*
 	 * i_block_group is the number of the block group which contains
@@ -652,6 +728,7 @@ struct ext4_inode_info {
 	 */
 	ext4_group_t	i_block_group;
 	unsigned long	i_state_flags;		/* Dynamic state flags */
+	unsigned long	i_flags;
 
 	ext4_lblk_t		i_dir_start_lookup;
 #ifdef CONFIG_EXT4_FS_XATTR
@@ -1087,20 +1164,22 @@ enum {
 	EXT4_STATE_DIO_UNWRITTEN,	/* need convert on dio done*/
 };
 
-static inline int ext4_test_inode_state(struct inode *inode, int bit)
-{
-	return test_bit(bit, &EXT4_I(inode)->i_state_flags);
+#define EXT4_INODE_BIT_FNS(name, field)					\
+static inline int ext4_test_inode_##name(struct inode *inode, int bit)	\
+{									\
+	return test_bit(bit, &EXT4_I(inode)->i_##field);		\
+}									\
+static inline void ext4_set_inode_##name(struct inode *inode, int bit) 	\
+{									\
+	set_bit(bit, &EXT4_I(inode)->i_##field);			\
+}									\
+static inline void ext4_clear_inode_##name(struct inode *inode, int bit) \
+{									\
+	clear_bit(bit, &EXT4_I(inode)->i_##field);			\
 }
 
-static inline void ext4_set_inode_state(struct inode *inode, int bit)
-{
-	set_bit(bit, &EXT4_I(inode)->i_state_flags);
-}
-
-static inline void ext4_clear_inode_state(struct inode *inode, int bit)
-{
-	clear_bit(bit, &EXT4_I(inode)->i_state_flags);
-}
+EXT4_INODE_BIT_FNS(flag, flags)
+EXT4_INODE_BIT_FNS(state, state_flags)
 #else
 /* Assume that user mode programs are passing in an ext4fs superblock, not
  * a kernel struct super_block.  This will allow us to call the feature-test
@@ -1287,7 +1366,7 @@ struct ext4_dir_entry_2 {
 
 #define is_dx(dir) (EXT4_HAS_COMPAT_FEATURE(dir->i_sb, \
 				      EXT4_FEATURE_COMPAT_DIR_INDEX) && \
-		      (EXT4_I(dir)->i_flags & EXT4_INDEX_FL))
+		    ext4_test_inode_flag((dir), EXT4_INODE_INDEX))
 #define EXT4_DIR_LINK_MAX(dir) (!is_dx(dir) && (dir)->i_nlink >= EXT4_LINK_MAX)
 #define EXT4_DIR_LINK_EMPTY(dir) ((dir)->i_nlink == 2 || (dir)->i_nlink == 1)
 
diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
index b79ad51..dade0c0 100644
--- a/fs/ext4/ext4_jbd2.h
+++ b/fs/ext4/ext4_jbd2.h
@@ -273,7 +273,7 @@ static inline int ext4_should_journal_data(struct inode *inode)
 		return 1;
 	if (test_opt(inode->i_sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA)
 		return 1;
-	if (EXT4_I(inode)->i_flags & EXT4_JOURNAL_DATA_FL)
+	if (ext4_test_inode_flag(inode, EXT4_INODE_JOURNAL_DATA))
 		return 1;
 	return 0;
 }
@@ -284,7 +284,7 @@ static inline int ext4_should_order_data(struct inode *inode)
 		return 0;
 	if (!S_ISREG(inode->i_mode))
 		return 0;
-	if (EXT4_I(inode)->i_flags & EXT4_JOURNAL_DATA_FL)
+	if (ext4_test_inode_flag(inode, EXT4_INODE_JOURNAL_DATA))
 		return 0;
 	if (test_opt(inode->i_sb, DATA_FLAGS) == EXT4_MOUNT_ORDERED_DATA)
 		return 1;
@@ -297,7 +297,7 @@ static inline int ext4_should_writeback_data(struct inode *inode)
 		return 0;
 	if (EXT4_JOURNAL(inode) == NULL)
 		return 1;
-	if (EXT4_I(inode)->i_flags & EXT4_JOURNAL_DATA_FL)
+	if (ext4_test_inode_flag(inode, EXT4_INODE_JOURNAL_DATA))
 		return 0;
 	if (test_opt(inode->i_sb, DATA_FLAGS) == EXT4_MOUNT_WRITEBACK_DATA)
 		return 1;
@@ -321,7 +321,7 @@ static inline int ext4_should_dioread_nolock(struct inode *inode)
 		return 0;
 	if (!S_ISREG(inode->i_mode))
 		return 0;
-	if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL))
+	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
 		return 0;
 	if (ext4_should_journal_data(inode))
 		return 0;
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index e21e110..595ab6d 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3498,7 +3498,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 			map->m_flags |= EXT4_MAP_UNINIT;
 	}
 
-	if (unlikely(EXT4_I(inode)->i_flags & EXT4_EOFBLOCKS_FL)) {
+	if (unlikely(ext4_test_inode_flag(inode, EXT4_INODE_EOFBLOCKS))) {
 		if (unlikely(!eh->eh_entries)) {
 			EXT4_ERROR_INODE(inode,
 					 "eh->eh_entries == 0 ee_block %d",
@@ -3509,7 +3509,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 		last_ex = EXT_LAST_EXTENT(eh);
 		if (map->m_lblk + ar.len > le32_to_cpu(last_ex->ee_block)
 		    + ext4_ext_get_actual_len(last_ex))
-			EXT4_I(inode)->i_flags &= ~EXT4_EOFBLOCKS_FL;
+			ext4_clear_inode_flag(inode, EXT4_INODE_EOFBLOCKS);
 	}
 	err = ext4_ext_insert_extent(handle, inode, path, &newex, flags);
 	if (err) {
@@ -3650,7 +3650,7 @@ static void ext4_falloc_update_inode(struct inode *inode,
 		 * can proceed even if the new size is the same as i_size.
 		 */
 		if (new_size > i_size_read(inode))
-			EXT4_I(inode)->i_flags |= EXT4_EOFBLOCKS_FL;
+			ext4_set_inode_flag(inode, EXT4_INODE_EOFBLOCKS);
 	}
 
 }
@@ -3677,7 +3677,7 @@ long ext4_fallocate(struct inode *inode, int mode, loff_t offset, loff_t len)
 	 * currently supporting (pre)allocate mode for extent-based
 	 * files _only_
 	 */
-	if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL))
+	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
 		return -EOPNOTSUPP;
 
 	/* preallocation to directories is currently not supported */
@@ -3922,7 +3922,7 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 	int error = 0;
 
 	/* fallback to generic here if not in extents fmt */
-	if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL))
+	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
 		return generic_block_fiemap(inode, fieinfo, start, len,
 			ext4_get_block);
 
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index d0776e4..5313ae4 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -66,7 +66,7 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 	 * is smaller than s_maxbytes, which is for extent-mapped files.
 	 */
 
-	if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL)) {
+	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {
 		struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 		size_t length = iov_length(iov, nr_segs);
 
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 52618d5..7f6b582 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -492,7 +492,7 @@ static int find_group_orlov(struct super_block *sb, struct inode *parent,
 
 	if (S_ISDIR(mode) &&
 	    ((parent == sb->s_root->d_inode) ||
-	     (EXT4_I(parent)->i_flags & EXT4_TOPDIR_FL))) {
+	     (ext4_test_inode_flag(parent, EXT4_INODE_TOPDIR)))) {
 		int best_ndir = inodes_per_group;
 		int ret = -1;
 
@@ -1038,7 +1038,7 @@ got:
 	if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_EXTENTS)) {
 		/* set extent flag only for directory, file and normal symlink*/
 		if (S_ISDIR(mode) || S_ISREG(mode) || S_ISLNK(mode)) {
-			EXT4_I(inode)->i_flags |= EXT4_EXTENTS_FL;
+			ext4_set_inode_flag(inode, EXT4_INODE_EXTENTS);
 			ext4_ext_tree_init(handle, inode);
 		}
 	}
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index a8c4c24..14a05db 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -931,7 +931,7 @@ static int ext4_ind_map_blocks(handle_t *handle, struct inode *inode,
 	int count = 0;
 	ext4_fsblk_t first_block = 0;
 
-	J_ASSERT(!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL));
+	J_ASSERT(!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)));
 	J_ASSERT(handle != NULL || (flags & EXT4_GET_BLOCKS_CREATE) == 0);
 	depth = ext4_block_to_path(inode, map->m_lblk, offsets,
 				   &blocks_to_boundary);
@@ -1059,7 +1059,7 @@ static int ext4_indirect_calc_metadata_amount(struct inode *inode,
  */
 static int ext4_calc_metadata_amount(struct inode *inode, sector_t lblock)
 {
-	if (EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL)
+	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
 		return ext4_ext_calc_metadata_amount(inode, lblock);
 
 	return ext4_indirect_calc_metadata_amount(inode, lblock);
@@ -1236,7 +1236,7 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
 	 * file system block.
 	 */
 	down_read((&EXT4_I(inode)->i_data_sem));
-	if (EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL) {
+	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
 		retval = ext4_ext_map_blocks(handle, inode, map, 0);
 	} else {
 		retval = ext4_ind_map_blocks(handle, inode, map, 0);
@@ -1295,7 +1295,7 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
 	 * We need to check for EXT4 here because migrate
 	 * could have changed the inode type in between
 	 */
-	if (EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL) {
+	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
 		retval = ext4_ext_map_blocks(handle, inode, map, flags);
 	} else {
 		retval = ext4_ind_map_blocks(handle, inode, map, flags);
@@ -2325,7 +2325,7 @@ static void mpage_add_bh_to_extent(struct mpage_da_data *mpd,
 		goto flush_it;
 
 	/* check if thereserved journal credits might overflow */
-	if (!(EXT4_I(mpd->inode)->i_flags & EXT4_EXTENTS_FL)) {
+	if (!(ext4_test_inode_flag(mpd->inode, EXT4_INODE_EXTENTS))) {
 		if (nrblocks >= EXT4_MAX_TRANS_DATA) {
 			/*
 			 * With non-extent format we are limited by the journal
@@ -2779,7 +2779,7 @@ static int ext4_da_writepages_trans_blocks(struct inode *inode)
 	 * number of contiguous block. So we will limit
 	 * number of contiguous block to a sane value
 	 */
-	if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL) &&
+	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) &&
 	    (max_blocks > EXT4_MAX_TRANS_DATA))
 		max_blocks = EXT4_MAX_TRANS_DATA;
 
@@ -3995,7 +3995,7 @@ static ssize_t ext4_direct_IO(int rw, struct kiocb *iocb,
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file->f_mapping->host;
 
-	if (EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL)
+	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
 		return ext4_ext_direct_IO(rw, iocb, iov, offset, nr_segs);
 
 	return ext4_ind_direct_IO(rw, iocb, iov, offset, nr_segs);
@@ -4631,12 +4631,12 @@ void ext4_truncate(struct inode *inode)
 	if (!ext4_can_truncate(inode))
 		return;
 
-	EXT4_I(inode)->i_flags &= ~EXT4_EOFBLOCKS_FL;
+	ext4_clear_inode_flag(inode, EXT4_INODE_EOFBLOCKS);
 
 	if (inode->i_size == 0 && !test_opt(inode->i_sb, NO_AUTO_DA_ALLOC))
 		ext4_set_inode_state(inode, EXT4_STATE_DA_ALLOC_CLOSE);
 
-	if (EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL) {
+	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
 		ext4_ext_truncate(inode);
 		return;
 	}
@@ -5473,7 +5473,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 	}
 
 	if (attr->ia_valid & ATTR_SIZE) {
-		if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL)) {
+		if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {
 			struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 
 			if (attr->ia_size > sbi->s_bitmap_maxbytes) {
@@ -5486,7 +5486,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 	if (S_ISREG(inode->i_mode) &&
 	    attr->ia_valid & ATTR_SIZE &&
 	    (attr->ia_size < inode->i_size ||
-	     (EXT4_I(inode)->i_flags & EXT4_EOFBLOCKS_FL))) {
+	     (ext4_test_inode_flag(inode, EXT4_INODE_EOFBLOCKS)))) {
 		handle_t *handle;
 
 		handle = ext4_journal_start(inode, 3);
@@ -5518,7 +5518,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 			}
 		}
 		/* ext4_truncate will clear the flag */
-		if ((EXT4_I(inode)->i_flags & EXT4_EOFBLOCKS_FL))
+		if ((ext4_test_inode_flag(inode, EXT4_INODE_EOFBLOCKS)))
 			ext4_truncate(inode);
 	}
 
@@ -5594,7 +5594,7 @@ static int ext4_indirect_trans_blocks(struct inode *inode, int nrblocks,
 
 static int ext4_index_trans_blocks(struct inode *inode, int nrblocks, int chunk)
 {
-	if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL))
+	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
 		return ext4_indirect_trans_blocks(inode, nrblocks, chunk);
 	return ext4_ext_index_trans_blocks(inode, nrblocks, chunk);
 }
@@ -5929,9 +5929,9 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
 	 */
 
 	if (val)
-		EXT4_I(inode)->i_flags |= EXT4_JOURNAL_DATA_FL;
+		ext4_set_inode_flag(inode, EXT4_INODE_JOURNAL_DATA);
 	else
-		EXT4_I(inode)->i_flags &= ~EXT4_JOURNAL_DATA_FL;
+		ext4_clear_inode_flag(inode, EXT4_INODE_JOURNAL_DATA);
 	ext4_set_aops(inode);
 
 	jbd2_journal_unlock_updates(journal);
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index e5dcdc9..0bdc018 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2006,7 +2006,7 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
 	sbi = EXT4_SB(sb);
 	ngroups = ext4_get_groups_count(sb);
 	/* non-extent files are limited to low blocks/groups */
-	if (!(EXT4_I(ac->ac_inode)->i_flags & EXT4_EXTENTS_FL))
+	if (!(ext4_test_inode_flag(ac->ac_inode, EXT4_INODE_EXTENTS)))
 		ngroups = sbi->s_blockfile_groups;
 
 	BUG_ON(ac->ac_status == AC_STATUS_FOUND);
@@ -3171,7 +3171,7 @@ ext4_mb_use_preallocated(struct ext4_allocation_context *ac)
 			continue;
 
 		/* non-extent files can't have physical blocks past 2^32 */
-		if (!(EXT4_I(ac->ac_inode)->i_flags & EXT4_EXTENTS_FL) &&
+		if (!(ext4_test_inode_flag(ac->ac_inode, EXT4_INODE_EXTENTS)) &&
 			pa->pa_pstart + pa->pa_len > EXT4_MAX_BLOCK_FILE_PHYS)
 			continue;
 
diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
index 34dcfc5..6f3a27e 100644
--- a/fs/ext4/migrate.c
+++ b/fs/ext4/migrate.c
@@ -475,7 +475,7 @@ int ext4_ext_migrate(struct inode *inode)
 	 */
 	if (!EXT4_HAS_INCOMPAT_FEATURE(inode->i_sb,
 				       EXT4_FEATURE_INCOMPAT_EXTENTS) ||
-	    (EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL))
+	    (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
 		return -EINVAL;
 
 	if (S_ISLNK(inode->i_mode) && inode->i_blocks == 0)
diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
index b8b8ea1..3a6c92a 100644
--- a/fs/ext4/move_extent.c
+++ b/fs/ext4/move_extent.c
@@ -977,11 +977,11 @@ mext_check_arguments(struct inode *orig_inode,
 	}
 
 	/* Ext4 move extent supports only extent based file */
-	if (!(EXT4_I(orig_inode)->i_flags & EXT4_EXTENTS_FL)) {
+	if (!(ext4_test_inode_flag(orig_inode, EXT4_INODE_EXTENTS))) {
 		ext4_debug("ext4 move extent: orig file is not extents "
 			"based file [ino:orig %lu]\n", orig_inode->i_ino);
 		return -EOPNOTSUPP;
-	} else if (!(EXT4_I(donor_inode)->i_flags & EXT4_EXTENTS_FL)) {
+	} else if (!(ext4_test_inode_flag(donor_inode, EXT4_INODE_EXTENTS))) {
 		ext4_debug("ext4 move extent: donor file is not extents "
 			"based file [ino:donor %lu]\n", donor_inode->i_ino);
 		return -EOPNOTSUPP;
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index bff77b0..7b4bf8f 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -656,7 +656,7 @@ int ext4_htree_fill_tree(struct file *dir_file, __u32 start_hash,
 	dxtrace(printk(KERN_DEBUG "In htree_fill_tree, start hash: %x:%x\n", 
 		       start_hash, start_minor_hash));
 	dir = dir_file->f_path.dentry->d_inode;
-	if (!(EXT4_I(dir)->i_flags & EXT4_INDEX_FL)) {
+	if (!(ext4_test_inode_flag(dir, EXT4_INODE_INDEX))) {
 		hinfo.hash_version = EXT4_SB(dir->i_sb)->s_def_hash_version;
 		if (hinfo.hash_version <= DX_HASH_TEA)
 			hinfo.hash_version +=
@@ -801,7 +801,7 @@ static void ext4_update_dx_flag(struct inode *inode)
 {
 	if (!EXT4_HAS_COMPAT_FEATURE(inode->i_sb,
 				     EXT4_FEATURE_COMPAT_DIR_INDEX))
-		EXT4_I(inode)->i_flags &= ~EXT4_INDEX_FL;
+		ext4_clear_inode_flag(inode, EXT4_INODE_INDEX);
 }
 
 /*
@@ -1416,7 +1416,7 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
 		brelse(bh);
 		return retval;
 	}
-	EXT4_I(dir)->i_flags |= EXT4_INDEX_FL;
+	ext4_set_inode_flag(dir, EXT4_INODE_INDEX);
 	data1 = bh2->b_data;
 
 	memcpy (data1, de, len);
@@ -1489,7 +1489,7 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
 		retval = ext4_dx_add_entry(handle, dentry, inode);
 		if (!retval || (retval != ERR_BAD_DX_DIR))
 			return retval;
-		EXT4_I(dir)->i_flags &= ~EXT4_INDEX_FL;
+		ext4_clear_inode_flag(dir, EXT4_INODE_INDEX);
 		dx_fallback++;
 		ext4_mark_inode_dirty(handle, dir);
 	}
@@ -2294,7 +2294,7 @@ retry:
 		}
 	} else {
 		/* clear the extent format for fast symlink */
-		EXT4_I(inode)->i_flags &= ~EXT4_EXTENTS_FL;
+		ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS);
 		inode->i_op = &ext4_fast_symlink_inode_operations;
 		memcpy((char *)&EXT4_I(inode)->i_data, symname, l);
 		inode->i_size = l-1;
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 2e8790e..9766b2a 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4148,6 +4148,7 @@ static int __init init_ext4_fs(void)
 {
 	int err;
 
+	ext4_check_flag_values();
 	err = init_ext4_system_zone();
 	if (err)
 		return err;
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 0a09a2e..47431bc 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -818,7 +818,7 @@ inserted:
 						EXT4_I(inode)->i_block_group);
 
 			/* non-extent files can't have physical blocks past 2^32 */
-			if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL))
+			if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
 				goal = goal & EXT4_MAX_BLOCK_FILE_PHYS;
 
 			block = ext4_new_meta_blocks(handle, inode,
@@ -826,7 +826,7 @@ inserted:
 			if (error)
 				goto cleanup;
 
-			if (!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL))
+			if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
 				BUG_ON(block > EXT4_MAX_BLOCK_FILE_PHYS);
 
 			ea_idebug(inode, "creating block %d", block);
-- 
1.6.6.1.1.g974db.dirty


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] ext4: fix eofblock flag handling
  2010-04-19 14:32 ` [PATCH 2/2] ext4: fix eofblock flag handling Dmitry Monakhov
@ 2010-05-25  4:17   ` tytso
  2010-05-25  4:18     ` [PATCH 1/2] ext4: Avoid crashing on NULL ptr dereference on a filesystem error Theodore Ts'o
  2010-05-25  4:18     ` [PATCH 2/2] ext4: Clear the EXT4_EOFBLOCKS_FL flag only when warranted Theodore Ts'o
  0 siblings, 2 replies; 17+ messages in thread
From: tytso @ 2010-05-25  4:17 UTC (permalink / raw)
  To: Dmitry Monakhov; +Cc: linux-ext4

On Mon, Apr 19, 2010 at 06:32:17PM +0400, Dmitry Monakhov wrote:
> - Fix NULL pointer deference on error path
> - Extent header we found may be not latest node of the inode. In order to
>   find latest extent we have to traverse a path from very beginning.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=15792
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>

I split this patch into two patches, since they are addressing two
very distinct and different bugs.  As it turns out I chose a
completely different way of tackling the second issue, which has the
advantage being much simpler, and not requiring a second call to
ext4_ext_find_extent(), which can end up requiring extra disk reads.

Also note that I supplied a test case to demonstrate the problem.
This was helpful in assuring that the problem was fixed, and also in
proving that there really *was* a problem; as it turns out triggering
it is quite difficult and I would be very surprised if it has really
happenned in real life.

To construct the test case I first of all used a 1k block file system,
and then generated an extremely fragmented free space:

mkdir a; cd a
seq -f %05.0f 1 65536 | xargs touch
seq -f %05.0f 1 65536 | xargs -L 1 fallocate -l 1k
seq -f %05.0f 1 2 65536 | xargs rm
cd ..

I then created the fragmented file with the EOFBLOCKS set:

fallocate -n -l 32m foo

This should generate a file with a two-deep extent tree.  (It is
otherwise *very* hard to create a deep extent tree.)  I then found the
last block in an leaf block in the middle of the tree, and deleted the
last extent in that leaf block, using the tst_extents program found in
lib/ext2fs in the e2fsprogs sources (it is built using "make check").
In the case described in the commit, this happened to be for the
logical block 17925.

Could such a file be generated in real life?  Yes, but you'd have to
be quite unlucky, as it would require extending a sparse, fragmented
file using an i_size-preserving fallocate call, where there was a hole
at precisely the right (wrong) place in the extent tree.

			      	      	 - Ted

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/2] ext4: Avoid crashing on NULL ptr dereference on a filesystem error
  2010-05-25  4:17   ` tytso
@ 2010-05-25  4:18     ` Theodore Ts'o
  2010-05-25  4:18     ` [PATCH 2/2] ext4: Clear the EXT4_EOFBLOCKS_FL flag only when warranted Theodore Ts'o
  1 sibling, 0 replies; 17+ messages in thread
From: Theodore Ts'o @ 2010-05-25  4:18 UTC (permalink / raw)
  To: Ext4 Developers List; +Cc: Theodore Ts'o

If the EOFBLOCK_FL flag is set when it should not be and the inode is
zero length, then eh_entries is zero, and ex is NULL, so dereferencing
ex to print ex->ee_block causes a kernel OOPS in
ext4_ext_map_blocks().

On top of that, the error message which is printed isn't very helpful.
So we fix this by printing something more explanatory which doesn't
involve trying to print ex->ee_block.

Addresses-Google-Bug: #2655740

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/extents.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 595ab6d..aeec5c7 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3370,8 +3370,9 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 	 */
 	if (unlikely(path[depth].p_ext == NULL && depth != 0)) {
 		EXT4_ERROR_INODE(inode, "bad extent address "
-				 "iblock: %d, depth: %d pblock %lld",
-				 map->m_lblk, depth, path[depth].p_block);
+				 "lblock: %lu, depth: %d pblock %lld",
+				 (unsigned long) map->m_lblk, depth,
+				 path[depth].p_block);
 		err = -EIO;
 		goto out2;
 	}
@@ -3501,8 +3502,8 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 	if (unlikely(ext4_test_inode_flag(inode, EXT4_INODE_EOFBLOCKS))) {
 		if (unlikely(!eh->eh_entries)) {
 			EXT4_ERROR_INODE(inode,
-					 "eh->eh_entries == 0 ee_block %d",
-					 ex->ee_block);
+					 "eh->eh_entries == 0 and "
+					 "EOFBLOCKS_FL set");
 			err = -EIO;
 			goto out2;
 		}
-- 
1.6.6.1.1.g974db.dirty


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/2] ext4: Clear the EXT4_EOFBLOCKS_FL flag only when warranted
  2010-05-25  4:17   ` tytso
  2010-05-25  4:18     ` [PATCH 1/2] ext4: Avoid crashing on NULL ptr dereference on a filesystem error Theodore Ts'o
@ 2010-05-25  4:18     ` Theodore Ts'o
  2010-05-25  7:23       ` Dmitry Monakhov
  1 sibling, 1 reply; 17+ messages in thread
From: Theodore Ts'o @ 2010-05-25  4:18 UTC (permalink / raw)
  To: Ext4 Developers List; +Cc: Theodore Ts'o

Dimitry Monakhov discovered an edge case where it was possible for the
EXT4_EOFBLOCKS_FL flag could get cleared unnecessarily.  This is true;
I have a test case that can be exercised via downloading and
decompressing the file:

wget ftp://ftp.kernel.org/pub/linux/kernel/people/tytso/ext4-testcases/eofblocks-fl-test-case.img.bz2
bunzip2 eofblocks-fl-test-case.img
dd if=/dev/zero of=eofblocks-fl-test-case.img bs=1k seek=17925 bs=1k count=1 conv=notrunc

However, triggering it in real life is highly unlikely since it
requires an extremely fragmented sparse file with a hole in exactly
the right place in the extent tree.  (It actually took quite a bit of
work to generate this test case.)  Still, it's nice to get even
extreme corner cases to be correct, so this patch makes sure that we
don't clear the EXT4_EOFBLOCKS_FL incorrectly even in this corner
case.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/extents.c |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index aeec5c7..c7c304f 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3319,7 +3319,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 	struct ext4_extent_header *eh;
 	struct ext4_extent newex, *ex, *last_ex;
 	ext4_fsblk_t newblock;
-	int err = 0, depth, ret, cache_type;
+	int i, err = 0, depth, ret, cache_type;
 	unsigned int allocated = 0;
 	struct ext4_allocation_request ar;
 	ext4_io_end_t *io = EXT4_I(inode)->cur_aio_dio;
@@ -3508,8 +3508,20 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 			goto out2;
 		}
 		last_ex = EXT_LAST_EXTENT(eh);
-		if (map->m_lblk + ar.len > le32_to_cpu(last_ex->ee_block)
-		    + ext4_ext_get_actual_len(last_ex))
+		/*
+		 * If the current leaf block was reached by looking at
+		 * the last index block all the way down the tree, and
+		 * we are extending the inode beyond the last extent
+		 * in the current leaf block, then clear the
+		 * EOFBLOCKS_FL flag.
+		 */
+		for (i=depth-1; i >= 0; i--) {
+			if (path[i].p_idx != EXT_LAST_INDEX(path[i].p_hdr))
+				break;
+		}
+		if ((i < 0) &&
+		    (map->m_lblk + ar.len > le32_to_cpu(last_ex->ee_block) + 
+		     ext4_ext_get_actual_len(last_ex)))
 			ext4_clear_inode_flag(inode, EXT4_INODE_EOFBLOCKS);
 	}
 	err = ext4_ext_insert_extent(handle, inode, path, &newex, flags);
-- 
1.6.6.1.1.g974db.dirty


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] ext4: Clear the EXT4_EOFBLOCKS_FL flag only when warranted
  2010-05-25  4:18     ` [PATCH 2/2] ext4: Clear the EXT4_EOFBLOCKS_FL flag only when warranted Theodore Ts'o
@ 2010-05-25  7:23       ` Dmitry Monakhov
  2010-05-25 13:03         ` tytso
  0 siblings, 1 reply; 17+ messages in thread
From: Dmitry Monakhov @ 2010-05-25  7:23 UTC (permalink / raw)
  To: Theodore Ts'o; +Cc: Ext4 Developers List

Theodore Ts'o <tytso@mit.edu> writes:

> Dimitry Monakhov discovered an edge case where it was possible for the
> EXT4_EOFBLOCKS_FL flag could get cleared unnecessarily.  This is true;
> I have a test case that can be exercised via downloading and
> decompressing the file:
>
> wget ftp://ftp.kernel.org/pub/linux/kernel/people/tytso/ext4-testcases/eofblocks-fl-test-case.img.bz2
> bunzip2 eofblocks-fl-test-case.img
> dd if=/dev/zero of=eofblocks-fl-test-case.img bs=1k seek=17925 bs=1k count=1 conv=notrunc
>
> However, triggering it in real life is highly unlikely since it
> requires an extremely fragmented sparse file with a hole in exactly
> the right place in the extent tree.  (It actually took quite a bit of
This condition was triggered during fsstress test. So I consider
it as rare but possible in real life. Nor than less it is better
to fix it now, than fix it in response from a midnight call from some
crazy customer :)
> work to generate this test case.)  Still, it's nice to get even
> extreme corner cases to be correct, so this patch makes sure that we
> don't clear the EXT4_EOFBLOCKS_FL incorrectly even in this corner
> case.
>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  fs/ext4/extents.c |   18 +++++++++++++++---
>  1 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index aeec5c7..c7c304f 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3319,7 +3319,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
>  	struct ext4_extent_header *eh;
>  	struct ext4_extent newex, *ex, *last_ex;
>  	ext4_fsblk_t newblock;
> -	int err = 0, depth, ret, cache_type;
> +	int i, err = 0, depth, ret, cache_type;
>  	unsigned int allocated = 0;
>  	struct ext4_allocation_request ar;
>  	ext4_io_end_t *io = EXT4_I(inode)->cur_aio_dio;
> @@ -3508,8 +3508,20 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
>  			goto out2;
>  		}
>  		last_ex = EXT_LAST_EXTENT(eh);
> -		if (map->m_lblk + ar.len > le32_to_cpu(last_ex->ee_block)
> -		    + ext4_ext_get_actual_len(last_ex))
> +		/*
> +		 * If the current leaf block was reached by looking at
> +		 * the last index block all the way down the tree, and
> +		 * we are extending the inode beyond the last extent
> +		 * in the current leaf block, then clear the
> +		 * EOFBLOCKS_FL flag.
> +		 */
> +		for (i=depth-1; i >= 0; i--) {
> +			if (path[i].p_idx != EXT_LAST_INDEX(path[i].p_hdr))
> +				break;
This approach definitely looks better.
> +		}
> +		if ((i < 0) &&
> +		    (map->m_lblk + ar.len > le32_to_cpu(last_ex->ee_block) + 
> +		     ext4_ext_get_actual_len(last_ex)))
>  			ext4_clear_inode_flag(inode, EXT4_INODE_EOFBLOCKS);
>  	}
>  	err = ext4_ext_insert_extent(handle, inode, path, &newex, flags);

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] ext4: Clear the EXT4_EOFBLOCKS_FL flag only when warranted
  2010-05-25  7:23       ` Dmitry Monakhov
@ 2010-05-25 13:03         ` tytso
  2010-05-25 13:12           ` Dmitry Monakhov
  0 siblings, 1 reply; 17+ messages in thread
From: tytso @ 2010-05-25 13:03 UTC (permalink / raw)
  To: Dmitry Monakhov; +Cc: Ext4 Developers List

On Tue, May 25, 2010 at 11:23:48AM +0400, Dmitry Monakhov wrote:
> This condition was triggered during fsstress test. So I consider
> it as rare but possible in real life. Nor than less it is better
> to fix it now, than fix it in response from a midnight call from some
> crazy customer :)

Fair enough, I just didn't have an 8-core system handy when I was
dealing with it, so hand-crafted an fs image.

BTW, whatever happened to the xfstests patch that you created in

https://bugzilla.kernel.org/show_bug.cgi?id=15792

Did you submit it to the xfstests folks for inclusion?

    	       	     	 	  	- Ted

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] ext4: Clear the EXT4_EOFBLOCKS_FL flag only when warranted
  2010-05-25 13:03         ` tytso
@ 2010-05-25 13:12           ` Dmitry Monakhov
  2010-05-25 13:15             ` tytso
  0 siblings, 1 reply; 17+ messages in thread
From: Dmitry Monakhov @ 2010-05-25 13:12 UTC (permalink / raw)
  To: tytso; +Cc: Ext4 Developers List

tytso@mit.edu writes:

> On Tue, May 25, 2010 at 11:23:48AM +0400, Dmitry Monakhov wrote:
>> This condition was triggered during fsstress test. So I consider
>> it as rare but possible in real life. Nor than less it is better
>> to fix it now, than fix it in response from a midnight call from some
>> crazy customer :)
>
> Fair enough, I just didn't have an 8-core system handy when I was
> dealing with it, so hand-crafted an fs image.
>
> BTW, whatever happened to the xfstests patch that you created in
>
> https://bugzilla.kernel.org/show_bug.cgi?id=15792
>
> Did you submit it to the xfstests folks for inclusion?
Yes but changes was requested, so i've prepared new version.
I'll post it against Jan's xfsqa quota branch.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] ext4: Clear the EXT4_EOFBLOCKS_FL flag only when warranted
  2010-05-25 13:12           ` Dmitry Monakhov
@ 2010-05-25 13:15             ` tytso
  2010-05-25 13:19               ` Dmitry Monakhov
  0 siblings, 1 reply; 17+ messages in thread
From: tytso @ 2010-05-25 13:15 UTC (permalink / raw)
  To: Dmitry Monakhov; +Cc: Ext4 Developers List

On Tue, May 25, 2010 at 05:12:36PM +0400, Dmitry Monakhov wrote:
> Yes but changes was requested, so i've prepared new version.
> I'll post it against Jan's xfsqa quota branch.

Is there a reason you don't just base it against the xfstests mainline
and send the patch directly to xfs@oss.sgi.com?  Your patch isn't
related to quota or has any dependency on other changes that might be
in Jan's branch, right?  Or am I missing something?

   	 	    	 	 	       	 - Ted



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] ext4: Clear the EXT4_EOFBLOCKS_FL flag only when warranted
  2010-05-25 13:15             ` tytso
@ 2010-05-25 13:19               ` Dmitry Monakhov
  0 siblings, 0 replies; 17+ messages in thread
From: Dmitry Monakhov @ 2010-05-25 13:19 UTC (permalink / raw)
  To: tytso; +Cc: Ext4 Developers List

tytso@mit.edu writes:

> On Tue, May 25, 2010 at 05:12:36PM +0400, Dmitry Monakhov wrote:
>> Yes but changes was requested, so i've prepared new version.
>> I'll post it against Jan's xfsqa quota branch.
>
> Is there a reason you don't just base it against the xfstests mainline
> and send the patch directly to xfs@oss.sgi.com?  Your patch isn't
> related to quota or has any dependency on other changes that might be
> in Jan's branch, right?  Or am I missing something?
They are related because test number always increasing, and i want to
use the test to run with and w/o quota enabled.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH] ext4: Use bitops to read/modify i_flags part2
  2010-05-24 20:49 ` [PATCH -v2] ext4: Use bitops to read/modify i_flags in struct ext4_inode_info Theodore Ts'o
@ 2010-05-31  8:56   ` Dmitry Monakhov
  2010-06-01  3:06     ` tytso
  2010-06-03  2:55     ` tytso
  0 siblings, 2 replies; 17+ messages in thread
From: Dmitry Monakhov @ 2010-05-31  8:56 UTC (permalink / raw)
  To: Theodore Ts'o; +Cc: Ext4 Developers List

[-- Attachment #1: Type: text/plain, Size: 362 bytes --]

"Theodore Ts'o" <tytso@mit.edu> writes:

> From: "Theodore Ts'o" <tytso@mit.edu>
>
> From: Dmitry Monakhov <dmonakhov@openvz.org>
Bad news. Bug still exist because you've missed several important chunks 
(ext4_set_inode_flags, ext4_inode_blocks) while porting original patch.
And I've missed this too on review cycle.
Please add following patch to patch-queue.


[-- Attachment #2: 0001-ext4-Use-bitops-to-read-modify-i_flags-part2.patch --]
[-- Type: text/plain, Size: 2792 bytes --]

>From 3e70168f78e832457e956f32ae7581e86f8daf15 Mon Sep 17 00:00:00 2001
From: Dmitry Monakhov <dmonakhov@openvz.org>
Date: Mon, 31 May 2010 12:41:40 +0400
Subject: [PATCH] ext4: Use bitops to read/modify i_flags part2

Some places still modified in non-atomic meaner without i_mutex held.

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

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index c3b4443..39d1c14 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4942,20 +4942,26 @@ void ext4_set_inode_flags(struct inode *inode)
 /* Propagate flags from i_flags to EXT4_I(inode)->i_flags */
 void ext4_get_inode_flags(struct ext4_inode_info *ei)
 {
-	unsigned int flags = ei->vfs_inode.i_flags;
-
-	ei->i_flags &= ~(EXT4_SYNC_FL|EXT4_APPEND_FL|
-			EXT4_IMMUTABLE_FL|EXT4_NOATIME_FL|EXT4_DIRSYNC_FL);
-	if (flags & S_SYNC)
-		ei->i_flags |= EXT4_SYNC_FL;
-	if (flags & S_APPEND)
-		ei->i_flags |= EXT4_APPEND_FL;
-	if (flags & S_IMMUTABLE)
-		ei->i_flags |= EXT4_IMMUTABLE_FL;
-	if (flags & S_NOATIME)
-		ei->i_flags |= EXT4_NOATIME_FL;
-	if (flags & S_DIRSYNC)
-		ei->i_flags |= EXT4_DIRSYNC_FL;
+	unsigned int vfs_fl;
+	unsigned long old_flags, new_flags;
+
+	do {
+		vfs_fl = ei->vfs_inode.i_flags;
+		old_fl = ei->i_flags;
+		new_fl = old_fl & ~(EXT4_SYNC_FL|EXT4_APPEND_FL|
+				EXT4_IMMUTABLE_FL|EXT4_NOATIME_FL|
+				EXT4_DIRSYNC_FL);
+		if (vfs_fl & S_SYNC)
+			new_fl |= EXT4_SYNC_FL;
+		if (vfs_fl & S_APPEND)
+			new_fl |= EXT4_APPEND_FL;
+		if (vfs_fl & S_IMMUTABLE)
+			new_fl |= EXT4_IMMUTABLE_FL;
+		if (vfs_fl & S_NOATIME)
+			new_fl |= EXT4_NOATIME_FL;
+		if (vfs_fl & S_DIRSYNC)
+			new_fl |= EXT4_DIRSYNC_FL;
+	} while (cmpxchg(&ei->i_flags, old_fl, new_fl) != old_fl);
 }
 
 static blkcnt_t ext4_inode_blocks(struct ext4_inode *raw_inode,
@@ -5191,7 +5197,7 @@ static int ext4_inode_blocks_set(handle_t *handle,
 		 */
 		raw_inode->i_blocks_lo   = cpu_to_le32(i_blocks);
 		raw_inode->i_blocks_high = 0;
-		ei->i_flags &= ~EXT4_HUGE_FILE_FL;
+		ext4_clear_inode_flag(inode, EXT4_INODE_HUGE_FILE);
 		return 0;
 	}
 	if (!EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_HUGE_FILE))
@@ -5204,9 +5210,9 @@ static int ext4_inode_blocks_set(handle_t *handle,
 		 */
 		raw_inode->i_blocks_lo   = cpu_to_le32(i_blocks);
 		raw_inode->i_blocks_high = cpu_to_le16(i_blocks >> 32);
-		ei->i_flags &= ~EXT4_HUGE_FILE_FL;
+		ext4_clear_inode_flag(inode, EXT4_INODE_HUGE_FILE);
 	} else {
-		ei->i_flags |= EXT4_HUGE_FILE_FL;
+		ext4_set_inode_flag(inode, EXT4_INODE_HUGE_FILE);
 		/* i_block is stored in file system block size */
 		i_blocks = i_blocks >> (inode->i_blkbits - 9);
 		raw_inode->i_blocks_lo   = cpu_to_le32(i_blocks);
-- 
1.6.6.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] ext4: Use bitops to read/modify i_flags part2
  2010-05-31  8:56   ` [PATCH] ext4: Use bitops to read/modify i_flags part2 Dmitry Monakhov
@ 2010-06-01  3:06     ` tytso
  2010-06-03  2:55     ` tytso
  1 sibling, 0 replies; 17+ messages in thread
From: tytso @ 2010-06-01  3:06 UTC (permalink / raw)
  To: Dmitry Monakhov; +Cc: Ext4 Developers List

On Mon, May 31, 2010 at 12:56:45PM +0400, Dmitry Monakhov wrote:
> Bad news. Bug still exist because you've missed several important chunks 
> (ext4_set_inode_flags, ext4_inode_blocks) while porting original patch.
> And I've missed this too on review cycle.
> Please add following patch to patch-queue.

What I should have insisted on (if I had time to review this sort of
patch earlier) is to split it up into that which could be implemented
via a perl script (i.e.):

#!/usr/bin/perl -i.bak

while (<>) {
    s/EXT4_I\(([^ ]*)\)->i_flags & EXT4_([^ ]*)_FL/ext4_test_inode_flag(\1, EXT4_INODE_\2)/;
    s/EXT4_I\(([^ ]*)\)->i_flags \|= EXT4_([^ ]*)_FL/ext4_set_inode_flag(\1, EXT4_INODE_\2)/;
    s/EXT4_I\(([^ ]*)\)->i_flags \&= ~EXT4_([^ ]*)_FL/ext4_clear_inode_flag(\1, EXT4_INODE_\2)/;
    print;
}

with a piece before and after it for the more complicated bits.  This
is why really large patches, really, *really* need to be split apart.
(Or needs to have enough description of what was being done where so
that I could split apart into more easily manageable --- and easily
backported to stable kernel -- pieces.)

But no worries, we'll treat this as the follow-on part of the patch,
and then add these to the additional stable series patches.  These
sorts of things happen, especially with these gigantic patches....

      	 			   	      - Ted

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ext4: Use bitops to read/modify i_flags part2
  2010-05-31  8:56   ` [PATCH] ext4: Use bitops to read/modify i_flags part2 Dmitry Monakhov
  2010-06-01  3:06     ` tytso
@ 2010-06-03  2:55     ` tytso
  2010-06-03  8:48       ` Dmitry Monakhov
  1 sibling, 1 reply; 17+ messages in thread
From: tytso @ 2010-06-03  2:55 UTC (permalink / raw)
  To: Dmitry Monakhov; +Cc: Ext4 Developers List

On Mon, May 31, 2010 at 12:56:45PM +0400, Dmitry Monakhov wrote:
> "Theodore Ts'o" <tytso@mit.edu> writes:
> 
> > From: "Theodore Ts'o" <tytso@mit.edu>
> >
> > From: Dmitry Monakhov <dmonakhov@openvz.org>
> Bad news. Bug still exist because you've missed several important chunks 
> (ext4_set_inode_flags, ext4_inode_blocks) while porting original patch.
> And I've missed this too on review cycle.
> Please add following patch to patch-queue.

Added to the ext4 patch queue (with minor updates to the commit
description).

					- Ted

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ext4: Use bitops to read/modify i_flags part2
  2010-06-03  2:55     ` tytso
@ 2010-06-03  8:48       ` Dmitry Monakhov
  2010-06-03 10:37         ` Theodore Tso
  0 siblings, 1 reply; 17+ messages in thread
From: Dmitry Monakhov @ 2010-06-03  8:48 UTC (permalink / raw)
  To: tytso; +Cc: Ext4 Developers List

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

tytso@mit.edu writes:

> On Mon, May 31, 2010 at 12:56:45PM +0400, Dmitry Monakhov wrote:
>> "Theodore Ts'o" <tytso@mit.edu> writes:
>> 
>> > From: "Theodore Ts'o" <tytso@mit.edu>
>> >
>> > From: Dmitry Monakhov <dmonakhov@openvz.org>
>> Bad news. Bug still exist because you've missed several important chunks 
>> (ext4_set_inode_flags, ext4_inode_blocks) while porting original patch.
>> And I've missed this too on review cycle.
>> Please add following patch to patch-queue.
>
> Added to the ext4 patch queue (with minor updates to the commit
> description).
BTW. The patch that i've sent would not compile because of misstype.
So it may looks like that i've sent untested random peace of crap
from a first glance. But the truth is that that misstype was fixed
in to topmost quilt's patch so i've missed it original patch.
And indeed the bug with non-atomic bit's manipulation has gone, at
least i can't reproduce it any more. Incremental fix attached.


[-- Attachment #2: ext4-Use-bitops-fix --]
[-- Type: text/plain, Size: 338 bytes --]

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 7b42cf8..39d1c14 100644
--- b/fs/ext4/inode.c
+++ a/fs/ext4/inode.c
@@ -4943,7 +4943,7 @@
 void ext4_get_inode_flags(struct ext4_inode_info *ei)
 {
 	unsigned int vfs_fl;
-	unsigned long old_flags, new_flags;
+	unsigned long old_fl, new_fl;
 
 	do {
 		vfs_fl = ei->vfs_inode.i_flags;

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] ext4: Use bitops to read/modify i_flags part2
  2010-06-03  8:48       ` Dmitry Monakhov
@ 2010-06-03 10:37         ` Theodore Tso
  0 siblings, 0 replies; 17+ messages in thread
From: Theodore Tso @ 2010-06-03 10:37 UTC (permalink / raw)
  To: Dmitry Monakhov; +Cc: Ext4 Developers List


On Jun 3, 2010, at 4:48 AM, Dmitry Monakhov wrote:

> tytso@mit.edu writes:
> 
>> On Mon, May 31, 2010 at 12:56:45PM +0400, Dmitry Monakhov wrote:
>>> "Theodore Ts'o" <tytso@mit.edu> writes:
>>> 
>>>> From: "Theodore Ts'o" <tytso@mit.edu>
>>>> 
>>>> From: Dmitry Monakhov <dmonakhov@openvz.org>
>>> Bad news. Bug still exist because you've missed several important chunks 
>>> (ext4_set_inode_flags, ext4_inode_blocks) while porting original patch.
>>> And I've missed this too on review cycle.
>>> Please add following patch to patch-queue.
>> 
>> Added to the ext4 patch queue (with minor updates to the commit
>> description).
> BTW. The patch that i've sent would not compile because of misstype.
> So it may looks like that i've sent untested random peace of crap
> from a first glance. But the truth is that that misstype was fixed
> in to topmost quilt's patch so i've missed it original patch.
> And indeed the bug with non-atomic bit's manipulation has gone, at
> least i can't reproduce it any more. Incremental fix attached.

Yeah, I caught that.  It caused me to review that part of the patch much
more carefully, but no harm done.

-- Ted


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2010-06-03 10:38 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-04-19 14:32 [PATCH 1/2] ext4: Use bitops to read/modify EXT4_I(inode)->i_flags Dmitry Monakhov
2010-04-19 14:32 ` [PATCH 2/2] ext4: fix eofblock flag handling Dmitry Monakhov
2010-05-25  4:17   ` tytso
2010-05-25  4:18     ` [PATCH 1/2] ext4: Avoid crashing on NULL ptr dereference on a filesystem error Theodore Ts'o
2010-05-25  4:18     ` [PATCH 2/2] ext4: Clear the EXT4_EOFBLOCKS_FL flag only when warranted Theodore Ts'o
2010-05-25  7:23       ` Dmitry Monakhov
2010-05-25 13:03         ` tytso
2010-05-25 13:12           ` Dmitry Monakhov
2010-05-25 13:15             ` tytso
2010-05-25 13:19               ` Dmitry Monakhov
2010-05-24  3:09 ` [PATCH 1/2] ext4: Use bitops to read/modify EXT4_I(inode)->i_flags tytso
2010-05-24 20:49 ` [PATCH -v2] ext4: Use bitops to read/modify i_flags in struct ext4_inode_info Theodore Ts'o
2010-05-31  8:56   ` [PATCH] ext4: Use bitops to read/modify i_flags part2 Dmitry Monakhov
2010-06-01  3:06     ` tytso
2010-06-03  2:55     ` tytso
2010-06-03  8:48       ` Dmitry Monakhov
2010-06-03 10:37         ` Theodore Tso

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.