Add flag to files with blocks intentionally past EOF

PATCH Superseded Eric Sandeen 6 participants 11 messages Jan 19, 2010 – Feb 24, 2010

Subsystems (2)

VFS ext4 file system

lore

Share & tools

lore

Eric Sandeen

author

2010-01-19 21:45 UTC #0 lore raw

PATCH 1/1 Add flag to files with blocks intentionally past EOF

From: Jiaying Zhang <[email protected]>

fallocate() may potentially instantiate blocks past EOF, depending on the flags used when it is called.

e2fsck currently has a test for blocks past i_size, and it sometimes trips up - noticeably on xfstests 013 which runs fsstress.

This patch from Jiayang does fix it up for me - it (along with e2fsprogs updates and other patches recently from Aneesh) has survived many fsstress runs in a row.

The setattr interface may also be used to clear the flag and remove any blocks past EOF.

Signed-off-by Eric Sandeen <[email protected]>

---

(just resending this since it probably got lost in the previous thread - Jiaying didn't have a SOB line, but maybe that should be added. I have included the proper From: line for authorship)

fs/ext4/ext4.h +3 -2

index 874d169..4c7cd9b 100644
@@ -284,10 +284,11 @@ struct flex_groups {
 #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_EOFBLOCKS_FL		0x00400000 /* Blocks allocated beyond EOF (bit reserved in fs.h) */
 #define EXT4_RESERVED_FL		0x80000000 /* reserved for ext4 lib */
 
-#define EXT4_FL_USER_VISIBLE		0x000BDFFF /* User visible flags */
-#define EXT4_FL_USER_MODIFIABLE		0x000B80FF /* User modifiable flags */
+#define EXT4_FL_USER_VISIBLE		0x004BDFFF /* User visible flags */
+#define EXT4_FL_USER_MODIFIABLE		0x004B80FF /* User modifiable flags */
 
 /* Flags that should be inherited by new inodes from their parent. */
 #define EXT4_FL_INHERITED (EXT4_SECRM_FL | EXT4_UNRM_FL | EXT4_COMPR_FL |\

fs/ext4/extents.c +16 -1

index 765a482..e7d5ba2 100644
@@ -3185,7 +3185,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;
+	struct ext4_extent newex, *ex, *last_ex;
 	ext4_fsblk_t newblock;
 	int err = 0, depth, ret, cache_type;
 	unsigned int allocated = 0;

@@ -3366,6 +3366,14 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
 					EXT4_STATE_DIO_UNWRITTEN;;
 		}
 	}
+
+	if (unlikely(inode->i_flags & EXT4_EOFBLOCKS_FL)) {
+		BUG_ON(!eh->eh_entries);
+		last_ex = EXT_LAST_EXTENT(eh);
+		if (iblock + ar.len > le32_to_cpu(last_ex->ee_block)
+					+ ext4_ext_get_actual_len(last_ex))
+			inode->i_flags &= ~EXT4_EOFBLOCKS_FL;
+	}
 	err = ext4_ext_insert_extent(handle, inode, path, &newex, flags);
 	if (err) {
 		/* free data blocks we just allocated */

@@ -3499,6 +3507,13 @@ static void ext4_falloc_update_inode(struct inode *inode,
 			i_size_write(inode, new_size);
 		if (new_size > EXT4_I(inode)->i_disksize)
 			ext4_update_i_disksize(inode, new_size);
+	} else {
+		/*
+		 * Mark that we allocate beyond EOF so the subsequent truncate
+		 * can proceed even if the new size is the same as i_size.
+		 */
+		if (new_size > i_size_read(inode))
+			inode->i_flags |= EXT4_EOFBLOCKS_FL;
 	}
 
 }

fs/ext4/inode.c +14 -3

index cbf56da..bc31ea6 100644
@@ -4429,6 +4429,8 @@ void ext4_truncate(struct inode *inode)
 	if (!ext4_can_truncate(inode))
 		return;
 
+	inode->i_flags &= ~EXT4_EOFBLOCKS_FL;
+
 	if (inode->i_size == 0 && !test_opt(inode->i_sb, NO_AUTO_DA_ALLOC))
 		ei->i_state |= EXT4_STATE_DA_ALLOC_CLOSE;

@@ -4741,8 +4743,8 @@ 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);
+	ei->i_flags &= ~(EXT4_SYNC_FL|EXT4_APPEND_FL|EXT4_IMMUTABLE_FL|
+			EXT4_NOATIME_FL|EXT4_DIRSYNC_FL|EXT4_EOFBLOCKS_FL);
 	if (flags & S_SYNC)
 		ei->i_flags |= EXT4_SYNC_FL;
 	if (flags & S_APPEND)

@@ -4753,6 +4755,8 @@ void ext4_get_inode_flags(struct ext4_inode_info *ei)
 		ei->i_flags |= EXT4_NOATIME_FL;
 	if (flags & S_DIRSYNC)
 		ei->i_flags |= EXT4_DIRSYNC_FL;
+	if (flags & FS_EOFBLOCKS_FL)
+		ei->i_flags |= EXT4_EOFBLOCKS_FL;
 }
 
 static blkcnt_t ext4_inode_blocks(struct ext4_inode *raw_inode,

@@ -5284,7 +5288,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) {
+	    attr->ia_valid & ATTR_SIZE &&
+	    (attr->ia_size < inode->i_size ||
+	     (inode->i_flags & EXT4_EOFBLOCKS_FL))) {
 		handle_t *handle;
 
 		handle = ext4_journal_start(inode, 3);

@@ -5315,6 +5321,11 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 				goto err_out;
 			}
 		}
+		if ((inode->i_flags & EXT4_EOFBLOCKS_FL)) {
+			rc = vmtruncate(inode, attr->ia_size);
+			if (rc)
+				goto err_out;
+		}
 	}
 
 	rc = inode_setattr(inode, attr);

fs/ext4/ioctl.c +10 -0

index b63d193..71f578e 100644
@@ -92,6 +92,16 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			flags &= ~EXT4_EXTENTS_FL;
 		}
 
+		if (flags & EXT4_EOFBLOCKS_FL) {
+			/* we don't support adding EOFBLOCKS flag */
+			if (!(oldflags & EXT4_EOFBLOCKS_FL)) {
+				err = -EOPNOTSUPP;
+				goto flags_out;
+			}
+		} else if (oldflags & EXT4_EOFBLOCKS_FL)
+			/* free the space reserved with fallocate KEEPSIZE */
+			vmtruncate(inode, inode->i_size);
+
 		handle = ext4_journal_start(inode, 1);
 		if (IS_ERR(handle)) {
 			err = PTR_ERR(handle);

include/linux/fs.h +3 -2

index 9147ca8..db3ffb6 100644
@@ -349,10 +349,11 @@ struct inodes_stat_t {
 #define FS_TOPDIR_FL			0x00020000 /* Top of directory hierarchies*/
 #define FS_EXTENT_FL			0x00080000 /* Extents */
 #define FS_DIRECTIO_FL			0x00100000 /* Use direct i/o */
+#define FS_EOFBLOCKS_FL			0x00400000 /* Blocks allocated beyond EOF */
 #define FS_RESERVED_FL			0x80000000 /* reserved for ext2 lib */
 
-#define FS_FL_USER_VISIBLE		0x0003DFFF /* User visible flags */
-#define FS_FL_USER_MODIFIABLE		0x000380FF /* User modifiable flags */
+#define FS_FL_USER_VISIBLE		0x0043DFFF /* User visible flags */
+#define FS_FL_USER_MODIFIABLE		0x004380FF /* User modifiable flags */
 
 
 #define SYNC_FILE_RANGE_WAIT_BEFORE	1

Mingming

2010-01-20 00:46 UTC #1 lore raw

Quoted 54 lines · Eric Sandeen

From: Jiaying Zhang <[email protected]>
fallocate() may potentially instantiate blocks past EOF, depending on the flags used when it is called.
e2fsck currently has a test for blocks past i_size, and it sometimes trips up - noticeably on xfstests 013 which runs fsstress.
This patch from Jiayang does fix it up for me - it (along with e2fsprogs updates and other patches recently from Aneesh) has survived many fsstress runs in a row.
The setattr interface may also be used to clear the flag and remove any blocks past EOF.
Signed-off-by Eric Sandeen <[email protected]>
---
(just resending this since it probably got lost in the previous thread - Jiaying didn't have a SOB line, but maybe that should be added. I have included the proper From: line for authorship)
fs/ext4/ext4.h +3 -2
index 874d169..4c7cd9b 100644
@@ -284,10 +284,11 @@ struct flex_groups {
 #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_EOFBLOCKS_FL		0x00400000 /* Blocks allocated beyond EOF (bit reserved in fs.h) */
 #define EXT4_RESERVED_FL		0x80000000 /* reserved for ext4 lib */
 
-#define EXT4_FL_USER_VISIBLE		0x000BDFFF /* User visible flags */
-#define EXT4_FL_USER_MODIFIABLE		0x000B80FF /* User modifiable flags */
+#define EXT4_FL_USER_VISIBLE		0x004BDFFF /* User visible flags */
+#define EXT4_FL_USER_MODIFIABLE		0x004B80FF /* User modifiable flags */
 
 /* Flags that should be inherited by new inodes from their parent. */
 #define EXT4_FL_INHERITED (EXT4_SECRM_FL | EXT4_UNRM_FL | EXT4_COMPR_FL |\
fs/ext4/extents.c +1 -1
index 765a482..e7d5ba2 100644
@@ -3185,7 +3185,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;
+	struct ext4_extent newex, *ex, *last_ex;
 	ext4_fsblk_t newblock;
 	int err = 0, depth, ret, cache_type;
 	unsigned int allocated = 0;

fs/ext4/extents.c int ext4_ext_get_blocks(handle_t *handle, struct inode *inode, · line 3371

Quoted 6 lines

fs/ext4/extents.c +3 -0

@@ -3366,6 +3366,14 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
 					EXT4_STATE_DIO_UNWRITTEN;;
 		}
 	}
+
+	if (unlikely(inode->i_flags & EXT4_EOFBLOCKS_FL)) {
+		BUG_ON(!eh->eh_entries);

Perhaps BUG_ON() is too strong? Maybe add some warning messages first.

Quoted 67 lines

fs/ext4/extents.c +12 -0

+		last_ex = EXT_LAST_EXTENT(eh);
+		if (iblock + ar.len > le32_to_cpu(last_ex->ee_block)
+					+ ext4_ext_get_actual_len(last_ex))
+			inode->i_flags &= ~EXT4_EOFBLOCKS_FL;
+	}
 	err = ext4_ext_insert_extent(handle, inode, path, &newex, flags);
 	if (err) {
 		/* free data blocks we just allocated */

@@ -3499,6 +3507,13 @@ static void ext4_falloc_update_inode(struct inode *inode,
 			i_size_write(inode, new_size);
 		if (new_size > EXT4_I(inode)->i_disksize)
 			ext4_update_i_disksize(inode, new_size);
+	} else {
+		/*
+		 * Mark that we allocate beyond EOF so the subsequent truncate
+		 * can proceed even if the new size is the same as i_size.
+		 */
+		if (new_size > i_size_read(inode))
+			inode->i_flags |= EXT4_EOFBLOCKS_FL;
 	}
 
 }

fs/ext4/inode.c +9 -3

index cbf56da..bc31ea6 100644
@@ -4429,6 +4429,8 @@ void ext4_truncate(struct inode *inode)
 	if (!ext4_can_truncate(inode))
 		return;
 
+	inode->i_flags &= ~EXT4_EOFBLOCKS_FL;
+
 	if (inode->i_size == 0 && !test_opt(inode->i_sb, NO_AUTO_DA_ALLOC))
 		ei->i_state |= EXT4_STATE_DA_ALLOC_CLOSE;

@@ -4741,8 +4743,8 @@ 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);
+	ei->i_flags &= ~(EXT4_SYNC_FL|EXT4_APPEND_FL|EXT4_IMMUTABLE_FL|
+			EXT4_NOATIME_FL|EXT4_DIRSYNC_FL|EXT4_EOFBLOCKS_FL);
 	if (flags & S_SYNC)
 		ei->i_flags |= EXT4_SYNC_FL;
 	if (flags & S_APPEND)

@@ -4753,6 +4755,8 @@ void ext4_get_inode_flags(struct ext4_inode_info *ei)
 		ei->i_flags |= EXT4_NOATIME_FL;
 	if (flags & S_DIRSYNC)
 		ei->i_flags |= EXT4_DIRSYNC_FL;
+	if (flags & FS_EOFBLOCKS_FL)
+		ei->i_flags |= EXT4_EOFBLOCKS_FL;
 }
 
 static blkcnt_t ext4_inode_blocks(struct ext4_inode *raw_inode,

@@ -5284,7 +5288,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) {
+	    attr->ia_valid & ATTR_SIZE &&
+	    (attr->ia_size < inode->i_size ||
+	     (inode->i_flags & EXT4_EOFBLOCKS_FL))) {
 		handle_t *handle;
 
 		handle = ext4_journal_start(inode, 3);

fs/ext4/inode.c int ext4_setattr(struct dentry *dentry, struct iattr *attr) · line 5330

Quoted 10 lines

fs/ext4/inode.c +5 -0

@@ -5315,6 +5321,11 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 				goto err_out;
 			}
 		}
+		if ((inode->i_flags & EXT4_EOFBLOCKS_FL)) {
+			rc = vmtruncate(inode, attr->ia_size);
+			if (rc)
+				goto err_out;
+		}
 	}

I am a little lost why doing vmtruncate here...

Quoted 12 lines

fs/ext4/inode.c +0 -0

 	rc = inode_setattr(inode, attr);

fs/ext4/ioctl.c +3 -0

index b63d193..71f578e 100644
@@ -92,6 +92,16 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			flags &= ~EXT4_EXTENTS_FL;
 		}
 
+		if (flags & EXT4_EOFBLOCKS_FL) {
+			/* we don't support adding EOFBLOCKS flag */
+			if (!(oldflags & EXT4_EOFBLOCKS_FL)) {

fs/ext4/ioctl.c long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) · line 107

Quoted 10 lines

fs/ext4/ioctl.c +7 -0

+				err = -EOPNOTSUPP;
+				goto flags_out;
+			}
+		} else if (oldflags & EXT4_EOFBLOCKS_FL)
+			/* free the space reserved with fallocate KEEPSIZE */
+			vmtruncate(inode, inode->i_size);
+
 		handle = ext4_journal_start(inode, 1);
 		if (IS_ERR(handle)) {
 			err = PTR_ERR(handle);

And here...

Quoted 23 lines

include/linux/fs.h +3 -3

index 9147ca8..db3ffb6 100644
@@ -349,10 +349,11 @@ struct inodes_stat_t {
 #define FS_TOPDIR_FL			0x00020000 /* Top of directory hierarchies*/
 #define FS_EXTENT_FL			0x00080000 /* Extents */
 #define FS_DIRECTIO_FL			0x00100000 /* Use direct i/o */
+#define FS_EOFBLOCKS_FL			0x00400000 /* Blocks allocated beyond EOF */
 #define FS_RESERVED_FL			0x80000000 /* reserved for ext2 lib */
 
-#define FS_FL_USER_VISIBLE		0x0003DFFF /* User visible flags */
-#define FS_FL_USER_MODIFIABLE		0x000380FF /* User modifiable flags */
+#define FS_FL_USER_VISIBLE		0x0043DFFF /* User visible flags */
+#define FS_FL_USER_MODIFIABLE		0x004380FF /* User modifiable flags */
 
 
 #define SYNC_FILE_RANGE_WAIT_BEFORE	1
 
--

To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Sandeen

author

2010-01-20 03:59 UTC #2 lore raw

Quoted 55 lines · Mingming

Quoted 54 lines · Eric Sandeen
From: Jiaying Zhang <[email protected]>
fallocate() may potentially instantiate blocks past EOF, depending on the flags used when it is called.
e2fsck currently has a test for blocks past i_size, and it sometimes trips up - noticeably on xfstests 013 which runs fsstress.
This patch from Jiayang does fix it up for me - it (along with e2fsprogs updates and other patches recently from Aneesh) has survived many fsstress runs in a row.
The setattr interface may also be used to clear the flag and remove any blocks past EOF.
Signed-off-by Eric Sandeen <[email protected]>
---
(just resending this since it probably got lost in the previous thread - Jiaying didn't have a SOB line, but maybe that should be added. I have included the proper From: line for authorship)
fs/ext4/ext4.h +3 -2
index 874d169..4c7cd9b 100644
@@ -284,10 +284,11 @@ struct flex_groups {
 #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_EOFBLOCKS_FL		0x00400000 /* Blocks allocated beyond EOF (bit reserved in fs.h) */
 #define EXT4_RESERVED_FL		0x80000000 /* reserved for ext4 lib */
 
-#define EXT4_FL_USER_VISIBLE		0x000BDFFF /* User visible flags */
-#define EXT4_FL_USER_MODIFIABLE		0x000B80FF /* User modifiable flags */
+#define EXT4_FL_USER_VISIBLE		0x004BDFFF /* User visible flags */
+#define EXT4_FL_USER_MODIFIABLE		0x004B80FF /* User modifiable flags */
 
 /* Flags that should be inherited by new inodes from their parent. */
 #define EXT4_FL_INHERITED (EXT4_SECRM_FL | EXT4_UNRM_FL | EXT4_COMPR_FL |\
fs/ext4/extents.c +1 -1
index 765a482..e7d5ba2 100644
@@ -3185,7 +3185,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;
+	struct ext4_extent newex, *ex, *last_ex;
 	ext4_fsblk_t newblock;
 	int err = 0, depth, ret, cache_type;
 	unsigned int allocated = 0;
@@ -3366,6 +3366,14 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,

Quoted 8 lines

fs/ext4/extents.c int ext4_ext_get_blocks(handle_t *handle, struct inode *inode, · line 3371
Quoted 6 lines
fs/ext4/extents.c +3 -0
 					EXT4_STATE_DIO_UNWRITTEN;;
 		}
 	}
+
+	if (unlikely(inode->i_flags & EXT4_EOFBLOCKS_FL)) {
+		BUG_ON(!eh->eh_entries);
Perhaps BUG_ON() is too strong? Maybe add some warning messages first.

perhaps ... not sure how we would get here w/ no entries.

...

Quoted 5 lines

fs/ext4/extents.c +1 -0

@@ -5315,6 +5321,11 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 				goto err_out;
 			}
 		}
+		if ((inode->i_flags & EXT4_EOFBLOCKS_FL)) {

Quoted 8 lines

fs/ext4/extents.c int ext4_setattr(struct dentry *dentry, struct iattr *attr) · line 5330
Quoted 6 lines
fs/ext4/extents.c +4 -0
+			rc = vmtruncate(inode, attr->ia_size);
+			if (rc)
+				goto err_out;
+		}
 	}
 
I am a little lost why doing vmtruncate here...

Hm first off I assume vmtruncate will clear blocks past that size, but tonight I'm not seeing how it gets there.

Anyway, it looks like any setting of the size, truncate up or down (or to current size) will clear any blocks past EOF.

Quoted 9 lines

fs/ext4/extents.c +0 -0

 	rc = inode_setattr(inode, attr);

fs/ext4/ioctl.c +0 -0

index b63d193..71f578e 100644
@@ -92,6 +92,16 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			flags &= ~EXT4_EXTENTS_FL;
 		}

Quoted 15 lines

fs/ext4/ioctl.c long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) · line 107

Quoted 13 lines

fs/ext4/ioctl.c +10 -0

+		if (flags & EXT4_EOFBLOCKS_FL) {
+			/* we don't support adding EOFBLOCKS flag */
+			if (!(oldflags & EXT4_EOFBLOCKS_FL)) {
+				err = -EOPNOTSUPP;
+				goto flags_out;
+			}
+		} else if (oldflags & EXT4_EOFBLOCKS_FL)
+			/* free the space reserved with fallocate KEEPSIZE */
+			vmtruncate(inode, inode->i_size);
+
 		handle = ext4_journal_start(inode, 1);
 		if (IS_ERR(handle)) {
 			err = PTR_ERR(handle);

And here...

Well here we are clearing the EOFBLOCKS flag so we'd want to clear any blocks past EOF.... but now, does vmtruncate do that?

Ok, count me as confused too, but mostly jsut so far as how does vmtruncate clear the blocks beyond eof. I guess I glossed over this when reading it.

-Eric

Andreas Dilger

2010-01-20 08:37 UTC #3 lore raw

Quoted 9 lines · Eric Sandeen

Quoted 2 lines · Mingming
I am a little lost why doing vmtruncate here...
Hm first off I assume vmtruncate will clear blocks past that size, but tonight I'm not seeing how it gets there.
Anyway, it looks like any setting of the size, truncate up or down (or to current size) will clear any blocks past EOF.

Why not just call ext4_truncate(inode)? Since these blocks are only fallocated, and pages couldn't (shouldn't?) be instantiated beyond i_size, we don't need to do anything at the mapping level, only at the inode level.

Quoted 5 lines

Well here we are clearing the EOFBLOCKS flag so we'd want to clear any blocks past EOF.... but now, does vmtruncate do that?
Ok, count me as confused too, but mostly jsut so far as how does vmtruncate clear the blocks beyond eof. I guess I glossed over this when reading it.

Has the new truncate API patchset gone into the kernel? I recall there was some work to restructure the API and maybe you are a victim of understanding the old API?

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Aneesh Kumar K. V

2010-01-20 09:15 UTC #4 lore raw

In reply to Eric Sandeen

Quoted 83 lines · Eric Sandeen

Quoted 55 lines · Mingming
Quoted 54 lines · Eric Sandeen
From: Jiaying Zhang <[email protected]>
fallocate() may potentially instantiate blocks past EOF, depending on the flags used when it is called.
e2fsck currently has a test for blocks past i_size, and it sometimes trips up - noticeably on xfstests 013 which runs fsstress.
This patch from Jiayang does fix it up for me - it (along with e2fsprogs updates and other patches recently from Aneesh) has survived many fsstress runs in a row.
The setattr interface may also be used to clear the flag and remove any blocks past EOF.
Signed-off-by Eric Sandeen <[email protected]>
---
(just resending this since it probably got lost in the previous thread - Jiaying didn't have a SOB line, but maybe that should be added. I have included the proper From: line for authorship)
fs/ext4/ext4.h +3 -2
index 874d169..4c7cd9b 100644
@@ -284,10 +284,11 @@ struct flex_groups {
 #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_EOFBLOCKS_FL		0x00400000 /* Blocks allocated beyond EOF (bit reserved in fs.h) */
 #define EXT4_RESERVED_FL		0x80000000 /* reserved for ext4 lib */
 
-#define EXT4_FL_USER_VISIBLE		0x000BDFFF /* User visible flags */
-#define EXT4_FL_USER_MODIFIABLE		0x000B80FF /* User modifiable flags */
+#define EXT4_FL_USER_VISIBLE		0x004BDFFF /* User visible flags */
+#define EXT4_FL_USER_MODIFIABLE		0x004B80FF /* User modifiable flags */
 
 /* Flags that should be inherited by new inodes from their parent. */
 #define EXT4_FL_INHERITED (EXT4_SECRM_FL | EXT4_UNRM_FL | EXT4_COMPR_FL |\
fs/ext4/extents.c +1 -1
index 765a482..e7d5ba2 100644
@@ -3185,7 +3185,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;
+	struct ext4_extent newex, *ex, *last_ex;
 	ext4_fsblk_t newblock;
 	int err = 0, depth, ret, cache_type;
 	unsigned int allocated = 0;
@@ -3366,6 +3366,14 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
Quoted 8 lines
fs/ext4/extents.c int ext4_ext_get_blocks(handle_t *handle, struct inode *inode, · line 3371
Quoted 6 lines
fs/ext4/extents.c +3 -0
 					EXT4_STATE_DIO_UNWRITTEN;;
 		}
 	}
+
+	if (unlikely(inode->i_flags & EXT4_EOFBLOCKS_FL)) {
+		BUG_ON(!eh->eh_entries);
Perhaps BUG_ON() is too strong? Maybe add some warning messages first.
perhaps ... not sure how we would get here w/ no entries.
...
Quoted 5 lines
Quoted 5 lines
fs/ext4/extents.c +1 -0
@@ -5315,6 +5321,11 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 				goto err_out;
 			}
 		}
+		if ((inode->i_flags & EXT4_EOFBLOCKS_FL)) {
Quoted 8 lines
fs/ext4/extents.c int ext4_setattr(struct dentry *dentry, struct iattr *attr) · line 5330
Quoted 6 lines
fs/ext4/extents.c +4 -0
+			rc = vmtruncate(inode, attr->ia_size);
+			if (rc)
+				goto err_out;
+		}
 	}
 
I am a little lost why doing vmtruncate here...

Quoted 5 lines

Hm first off I assume vmtruncate will clear blocks past that size, but tonight I'm not seeing how it gets there.
Anyway, it looks like any setting of the size, truncate up or down (or to current size) will clear any blocks past EOF.

yes. earlier we were doing truncate only when attr->ia_size < inode->i_size. Now we do when we find that inode have EXT4_EOFBLOCKS_FL set.

Quoted 26 lines

Quoted 10 lines

fs/ext4/extents.c +0 -0

 	rc = inode_setattr(inode, attr);

fs/ext4/ioctl.c +0 -0

index b63d193..71f578e 100644
@@ -92,6 +92,16 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			flags &= ~EXT4_EXTENTS_FL;
 		}

Quoted 15 lines

fs/ext4/ioctl.c long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) · line 107

Quoted 13 lines

fs/ext4/ioctl.c +10 -0

+		if (flags & EXT4_EOFBLOCKS_FL) {
+			/* we don't support adding EOFBLOCKS flag */
+			if (!(oldflags & EXT4_EOFBLOCKS_FL)) {
+				err = -EOPNOTSUPP;
+				goto flags_out;
+			}
+		} else if (oldflags & EXT4_EOFBLOCKS_FL)
+			/* free the space reserved with fallocate KEEPSIZE */
+			vmtruncate(inode, inode->i_size);
+
 		handle = ext4_journal_start(inode, 1);
 		if (IS_ERR(handle)) {
 			err = PTR_ERR(handle);

And here...

Quoted 5 lines

Well here we are clearing the EOFBLOCKS flag so we'd want to clear any blocks past EOF.... but now, does vmtruncate do that?
Ok, count me as confused too, but mostly jsut so far as how does vmtruncate clear the blocks beyond eof. I guess I glossed over this when reading it.

Yes. vmtruncate ends up calling ext4_truncate which will do that it not that is a bug. When we failed to copy for user space in case of write we did call vmtruncate to free up the allocated blocks. So i guess it should be working. But we should be able to check that easily.

Aneesh Kumar K. V

2010-01-20 09:03 UTC #5 lore raw

In reply to Eric Sandeen

Quoted 15 lines · Eric Sandeen

From: Jiaying Zhang <[email protected]>
fallocate() may potentially instantiate blocks past EOF, depending on the flags used when it is called.
e2fsck currently has a test for blocks past i_size, and it sometimes trips up - noticeably on xfstests 013 which runs fsstress.
This patch from Jiayang does fix it up for me - it (along with e2fsprogs updates and other patches recently from Aneesh) has survived many fsstress runs in a row.
The setattr interface may also be used to clear the flag and remove any blocks past EOF.

Quoted 7 lines

Signed-off-by Eric Sandeen <[email protected]>
---
(just resending this since it probably got lost in the previous thread - Jiaying didn't have a SOB line, but maybe that should be added. I have included the proper From: line for authorship)

The patch i looked earlier was was early RFC and i didn't look the full thread to find an updated patch was posted.(hmm getting used to reading mails with notmuchmail.org)

http://article.gmane.org/gmane.comp.file-systems.ext4/15035

Quoted 33 lines

fs/ext4/ext4.h +3 -2

index 874d169..4c7cd9b 100644
@@ -284,10 +284,11 @@ struct flex_groups {
 #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_EOFBLOCKS_FL		0x00400000 /* Blocks allocated beyond EOF (bit reserved in fs.h) */
 #define EXT4_RESERVED_FL		0x80000000 /* reserved for ext4 lib */
 
-#define EXT4_FL_USER_VISIBLE		0x000BDFFF /* User visible flags */
-#define EXT4_FL_USER_MODIFIABLE		0x000B80FF /* User modifiable flags */
+#define EXT4_FL_USER_VISIBLE		0x004BDFFF /* User visible flags */
+#define EXT4_FL_USER_MODIFIABLE		0x004B80FF /* User modifiable flags */
 
 /* Flags that should be inherited by new inodes from their parent. */
 #define EXT4_FL_INHERITED (EXT4_SECRM_FL | EXT4_UNRM_FL | EXT4_COMPR_FL |\

fs/ext4/extents.c +1 -1

index 765a482..e7d5ba2 100644
@@ -3185,7 +3185,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;
+	struct ext4_extent newex, *ex, *last_ex;
 	ext4_fsblk_t newblock;
 	int err = 0, depth, ret, cache_type;
 	unsigned int allocated = 0;

@@ -3366,6 +3366,14 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
 					EXT4_STATE_DIO_UNWRITTEN;;

fs/ext4/extents.c int ext4_ext_get_blocks(handle_t *handle, struct inode *inode, · line 3376

Quoted 10 lines

fs/ext4/extents.c +8 -0

 		}
 	}
+
+	if (unlikely(inode->i_flags & EXT4_EOFBLOCKS_FL)) {
+		BUG_ON(!eh->eh_entries);
+		last_ex = EXT_LAST_EXTENT(eh);
+		if (iblock + ar.len > le32_to_cpu(last_ex->ee_block)
+					+ ext4_ext_get_actual_len(last_ex))
+			inode->i_flags &= ~EXT4_EOFBLOCKS_FL;
+	}

Why do we need to set inode->i_flag ? Can we make it Ext4 specific and look at ext4_inode->i_flags . Also setting inode->i_flag with an EXT4 flag value is confusing.

Quoted 31 lines

fs/ext4/extents.c +7 -0

 	err = ext4_ext_insert_extent(handle, inode, path, &newex, flags);
 	if (err) {
 		/* free data blocks we just allocated */

@@ -3499,6 +3507,13 @@ static void ext4_falloc_update_inode(struct inode *inode,
 			i_size_write(inode, new_size);
 		if (new_size > EXT4_I(inode)->i_disksize)
 			ext4_update_i_disksize(inode, new_size);
+	} else {
+		/*
+		 * Mark that we allocate beyond EOF so the subsequent truncate
+		 * can proceed even if the new size is the same as i_size.
+		 */
+		if (new_size > i_size_read(inode))
+			inode->i_flags |= EXT4_EOFBLOCKS_FL;
 	}
 
 }

fs/ext4/inode.c +2 -0

index cbf56da..bc31ea6 100644
@@ -4429,6 +4429,8 @@ void ext4_truncate(struct inode *inode)
 	if (!ext4_can_truncate(inode))
 		return;
 
+	inode->i_flags &= ~EXT4_EOFBLOCKS_FL;
+
 	if (inode->i_size == 0 && !test_opt(inode->i_sb, NO_AUTO_DA_ALLOC))
 		ei->i_state |= EXT4_STATE_DA_ALLOC_CLOSE;

fs/ext4/inode.c void ext4_get_inode_flags(struct ext4_inode_info *ei) · line 4748

Quoted 8 lines

fs/ext4/inode.c +2 -2

@@ -4741,8 +4743,8 @@ 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);
+	ei->i_flags &= ~(EXT4_SYNC_FL|EXT4_APPEND_FL|EXT4_IMMUTABLE_FL|
+
 EXT4_NOATIME_FL|EXT4_DIRSYNC_FL|EXT4_EOFBLOCKS_FL);

Do we really need to allow the get and set of this flag. IMHO a truncate should be the only API and the flag should be remove implicitly for that.

Quoted 65 lines

fs/ext4/inode.c +0 -0

 	if (flags & S_SYNC)
 		ei->i_flags |= EXT4_SYNC_FL;

fs/ext4/inode.c +10 -1

 	if (flags & S_APPEND)

@@ -4753,6 +4755,8 @@ void ext4_get_inode_flags(struct ext4_inode_info *ei)
 		ei->i_flags |= EXT4_NOATIME_FL;
 	if (flags & S_DIRSYNC)
 		ei->i_flags |= EXT4_DIRSYNC_FL;
+	if (flags & FS_EOFBLOCKS_FL)
+		ei->i_flags |= EXT4_EOFBLOCKS_FL;
 }
 
 static blkcnt_t ext4_inode_blocks(struct ext4_inode *raw_inode,

@@ -5284,7 +5288,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) {
+	    attr->ia_valid & ATTR_SIZE &&
+	    (attr->ia_size < inode->i_size ||
+	     (inode->i_flags & EXT4_EOFBLOCKS_FL))) {
 		handle_t *handle;
 
 		handle = ext4_journal_start(inode, 3);

@@ -5315,6 +5321,11 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 				goto err_out;
 			}
 		}
+		if ((inode->i_flags & EXT4_EOFBLOCKS_FL)) {
+			rc = vmtruncate(inode, attr->ia_size);
+			if (rc)
+				goto err_out;
+		}
 	}
 
 	rc = inode_setattr(inode, attr);

fs/ext4/ioctl.c +10 -0

index b63d193..71f578e 100644
@@ -92,6 +92,16 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			flags &= ~EXT4_EXTENTS_FL;
 		}
 
+		if (flags & EXT4_EOFBLOCKS_FL) {
+			/* we don't support adding EOFBLOCKS flag */
+			if (!(oldflags & EXT4_EOFBLOCKS_FL)) {
+				err = -EOPNOTSUPP;
+				goto flags_out;
+			}
+		} else if (oldflags & EXT4_EOFBLOCKS_FL)
+			/* free the space reserved with fallocate KEEPSIZE */
+			vmtruncate(inode, inode->i_size);
+
 		handle = ext4_journal_start(inode, 1);
 		if (IS_ERR(handle)) {
 			err = PTR_ERR(handle);

include/linux/fs.h +0 -0

index 9147ca8..db3ffb6 100644
@@ -349,10 +349,11 @@ struct inodes_stat_t {
 #define FS_TOPDIR_FL			0x00020000 /* Top of directory hierarchies*/
 #define FS_EXTENT_FL			0x00080000 /* Extents */
 #define FS_DIRECTIO_FL			0x00100000 /* Use direct i/o */

include/linux/fs.h struct inodes_stat_t { · line 359

Quoted 10 lines

include/linux/fs.h +3 -2

+#define FS_EOFBLOCKS_FL			0x00400000 /* Blocks allocated beyond EOF */
 #define FS_RESERVED_FL			0x80000000 /* reserved for ext2 lib */
 
-#define FS_FL_USER_VISIBLE		0x0003DFFF /* User visible flags */
-#define FS_FL_USER_MODIFIABLE		0x000380FF /* User modifiable flags */
+#define FS_FL_USER_VISIBLE		0x0043DFFF /* User visible flags */
+#define FS_FL_USER_MODIFIABLE		0x004380FF /* User modifiable flags */
 
 
 #define SYNC_FILE_RANGE_WAIT_BEFORE	1

If we remove ext4_ioctl support i guess that patch can become much simpler.

-aneesh

Andreas Dilger

2010-01-22 00:32 UTC #6 lore raw

Quoted 5 lines · Aneesh Kumar K. V

Quoted 5 lines

fs/ext4/inode.c +0 -0

@@ -4741,8 +4743,8 @@ void ext4_get_inode_flags(struct  
 ext4_inode_info *ei)
 {
 	unsigned int flags = ei->vfs_inode.i_flags;

Quoted 9 lines

fs/ext4/inode.c void ext4_get_inode_flags(struct ext4_inode_info *ei)
Quoted 5 lines
fs/ext4/inode.c +2 -2
-	ei->i_flags &= ~(EXT4_SYNC_FL|EXT4_APPEND_FL|
-			EXT4_IMMUTABLE_FL|EXT4_NOATIME_FL|EXT4_DIRSYNC_FL);
+	ei->i_flags &= ~(EXT4_SYNC_FL|EXT4_APPEND_FL|EXT4_IMMUTABLE_FL|
+
 EXT4_NOATIME_FL|EXT4_DIRSYNC_FL|EXT4_EOFBLOCKS_FL);
Do we really need to allow the get and set of this flag. IMHO a truncate should be the only API and the flag should be remove implicitly for that.

Since this flag is set on disk in the inode flags, it makes sense to allow clearing it via "chattr", just like "chattr +e" will remap a file to extent format. The risk with setting it via truncate() is that this is racy with some other process writing to the file. If we allow setting it via "chattr" this can be done in a non-racy manner, by locking the inode and dropping only the blocks beyond EOF.

Quoted 5 lines

fs/ext4/ioctl.c +0 -0

@@ -92,6 +92,16 @@ long ext4_ioctl(struct file *filp, unsigned int  
 cmd, unsigned long arg)
 			flags &= ~EXT4_EXTENTS_FL;
 		}

Quoted 16 lines

Quoted 12 lines

fs/ext4/ioctl.c +10 -0

+		if (flags & EXT4_EOFBLOCKS_FL) {
+			/* we don't support adding EOFBLOCKS flag */
+			if (!(oldflags & EXT4_EOFBLOCKS_FL)) {
+				err = -EOPNOTSUPP;
+				goto flags_out;
+			}
+		} else if (oldflags & EXT4_EOFBLOCKS_FL)
+			/* free the space reserved with fallocate KEEPSIZE */
+			vmtruncate(inode, inode->i_size);
+
 		handle = ext4_journal_start(inode, 1);
 		if (IS_ERR(handle)) {

Quoted 1 line

			err = PTR_ERR(handle);

If we remove ext4_ioctl support i guess that patch can become much simpler.

Sure it will be simpler, but less useful. The point of exposing this flag via lsattr is to allow userspace to determine which files are holding blocks beyond EOF, so that if the filesystem is getting too full it is possible to run e.g. "lsattr -R" and find files with this EOF attribute and truncate them. Without keeping at least the EOFBLOCKS flag in USER_VISIBLE this is impossible.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Eric Sandeen

author

2010-01-22 17:41 UTC #7 lore raw

Quoted 1 line · Andreas Dilger

On 2010-01-20, at 02:03, Aneesh Kumar K. V wrote:

...

Quoted 10 lines

Quoted 2 lines
If we remove ext4_ioctl support i guess that patch can become much simpler.
Sure it will be simpler, but less useful. The point of exposing this flag via lsattr is to allow userspace to determine which files are holding blocks beyond EOF, so that if the filesystem is getting too full it is possible to run e.g. "lsattr -R" and find files with this EOF attribute and truncate them. Without keeping at least the EOFBLOCKS flag in USER_VISIBLE this is impossible.

Well, I submitted an updated patch without it. We can add another when we sort out what we really want with the flags, but fixing the corruption e2fsck finds seems paramount.

I think the case of stray files w/ blocks past EOF is probably pretty rare - but I guess I agree, at least seeing the flags would be nice.

But let's handle that separately...

-Eric

Eric Sandeen

author

2010-01-21 18:00 UTC #8 lore raw

PATCH v2 1/1 Add flag to files with blocks intentionally past EOF

In reply to Eric Sandeen

From: Jiaying Zhang <[email protected]>

fallocate() may potentially instantiate blocks past EOF, depending on the flags used when it is called.

e2fsck currently has a test for blocks past i_size, and it sometimes trips up - noticeably on xfstests 013 which runs fsstress.

This patch from Jiayang does fix it up - it (along with e2fsprogs updates and other patches recently from Aneesh) has survived many fsstress runs in a row.

(Eric Sandeen: removed ioctl interface and minor cleanups)

Signed-off-by Eric Sandeen <[email protected]>

---

fs/ext4/ext4.h +1 -0

index 874d169..1f6b936 100644
@@ -284,6 +284,7 @@ struct flex_groups {
 #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_EOFBLOCKS_FL		0x00400000 /* Blocks allocated beyond EOF */
 #define EXT4_RESERVED_FL		0x80000000 /* reserved for ext4 lib */
 
 #define EXT4_FL_USER_VISIBLE		0x000BDFFF /* User visible flags */

fs/ext4/extents.c +21 -1

index 765a482..95e94ae 100644
@@ -3185,7 +3185,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;
+	struct ext4_extent newex, *ex, *last_ex;
 	ext4_fsblk_t newblock;
 	int err = 0, depth, ret, cache_type;
 	unsigned int allocated = 0;

@@ -3366,6 +3366,19 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
 					EXT4_STATE_DIO_UNWRITTEN;;
 		}
 	}
+
+	if (unlikely(EXT4_I(inode)->i_flags & EXT4_EOFBLOCKS_FL)) {
+                if (eh->eh_entries) {
+			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;
+		} else {
+                	WARN_ON(eh->eh_entries == 0);
+			ext4_error(inode->i_sb, __func__,
+				"inode#%lu, eh->eh_entries = 0!", inode->i_ino);
+		}
+	}
 	err = ext4_ext_insert_extent(handle, inode, path, &newex, flags);
 	if (err) {
 		/* free data blocks we just allocated */

@@ -3499,6 +3512,13 @@ static void ext4_falloc_update_inode(struct inode *inode,
 			i_size_write(inode, new_size);
 		if (new_size > EXT4_I(inode)->i_disksize)
 			ext4_update_i_disksize(inode, new_size);
+	} else {
+		/*
+		 * Mark that we allocate beyond EOF so the subsequent truncate
+		 * 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;
 	}
 
 }

fs/ext4/inode.c +8 -1

index cbf56da..f5802e9 100644
@@ -4429,6 +4429,8 @@ void ext4_truncate(struct inode *inode)
 	if (!ext4_can_truncate(inode))
 		return;
 
+	EXT4_I(inode)->i_flags &= ~EXT4_EOFBLOCKS_FL;
+
 	if (inode->i_size == 0 && !test_opt(inode->i_sb, NO_AUTO_DA_ALLOC))
 		ei->i_state |= EXT4_STATE_DA_ALLOC_CLOSE;

@@ -5284,7 +5286,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) {
+	    attr->ia_valid & ATTR_SIZE &&
+	    (attr->ia_size < inode->i_size ||
+	     (EXT4_I(inode)->i_flags & EXT4_EOFBLOCKS_FL))) {
 		handle_t *handle;
 
 		handle = ext4_journal_start(inode, 3);

@@ -5315,6 +5319,9 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 				goto err_out;
 			}
 		}
+		/* ext4_truncate will clear the flag */
+		if ((EXT4_I(inode)->i_flags & EXT4_EOFBLOCKS_FL))
+			ext4_truncate(inode);
 	}
 
 	rc = inode_setattr(inode, attr);

Jiaying Zhang

2010-01-21 20:32 UTC #9 lore raw

Eric,

Thank you very much for bringing the patch up-to-date and improving it! I have been thinking to re-sync it and send it again but was pulled away but other things.

The patch looks good. I think it is ok to drop the ioctl support at this time. We can add it later if there are users need that feature.

Jiaying

Quoted 104 lines · Eric Sandeen

From: Jiaying Zhang <[email protected]>

fallocate() may potentially instantiate blocks past EOF, depending on the flags used when it is called.

e2fsck currently has a test for blocks past i_size, and it sometimes trips up - noticeably on xfstests 013 which runs fsstress.

This patch from Jiayang does fix it up - it (along with e2fsprogs updates and other patches recently from Aneesh) has survived many fsstress runs in a row.

(Eric Sandeen: removed ioctl interface and minor cleanups)

Signed-off-by Eric Sandeen <[email protected]>

---

fs/ext4/ext4.h +1 -0

index 874d169..1f6b936 100644
@@ -284,6 +284,7 @@ struct flex_groups {
  #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_EOFBLOCKS_FL              0x00400000 /* Blocks allocated beyond EOF */
  #define EXT4_RESERVED_FL               0x80000000 /* reserved for ext4 lib */
 
  #define EXT4_FL_USER_VISIBLE           0x000BDFFF /* User visible flags */

fs/ext4/extents.c +21 -1

index 765a482..95e94ae 100644
@@ -3185,7 +3185,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;
+       struct ext4_extent newex, *ex, *last_ex;
        ext4_fsblk_t newblock;
        int err = 0, depth, ret, cache_type;
        unsigned int allocated = 0;

@@ -3366,6 +3366,19 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
                                        EXT4_STATE_DIO_UNWRITTEN;;
                }
        }
+
+       if (unlikely(EXT4_I(inode)->i_flags & EXT4_EOFBLOCKS_FL)) {
+                if (eh->eh_entries) {
+                       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;
+               } else {
+                       WARN_ON(eh->eh_entries == 0);
+                       ext4_error(inode->i_sb, __func__,
+                               "inode#%lu, eh->eh_entries = 0!", inode->i_ino);
+               }
+       }
        err = ext4_ext_insert_extent(handle, inode, path, &newex, flags);
        if (err) {
                /* free data blocks we just allocated */

@@ -3499,6 +3512,13 @@ static void ext4_falloc_update_inode(struct inode *inode,
                        i_size_write(inode, new_size);
                if (new_size > EXT4_I(inode)->i_disksize)
                        ext4_update_i_disksize(inode, new_size);
+       } else {
+               /*
+                * Mark that we allocate beyond EOF so the subsequent truncate
+                * 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;
        }
 
  }

fs/ext4/inode.c +2 -0

index cbf56da..f5802e9 100644
@@ -4429,6 +4429,8 @@ void ext4_truncate(struct inode *inode)
        if (!ext4_can_truncate(inode))
                return;
 
+       EXT4_I(inode)->i_flags &= ~EXT4_EOFBLOCKS_FL;
+
        if (inode->i_size == 0 && !test_opt(inode->i_sb, NO_AUTO_DA_ALLOC))
                ei->i_state |= EXT4_STATE_DA_ALLOC_CLOSE;

@@ -5284,7 +5286,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) {
+ attr->ia_valid & ATTR_SIZE &&
+ (attr->ia_size < inode->i_size ||
+ (EXT4_I(inode)->i_flags & EXT4_EOFBLOCKS_FL))) { handle_t *handle;

handle = ext4_journal_start(inode, 3);

fs/ext4/inode.c int ext4_setattr(struct dentry *dentry, struct iattr *attr) · line 5327

Quoted 9 lines

fs/ext4/inode.c +3 -0

@@ -5315,6 +5319,9 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
                                goto err_out;
                        }
                }
+               /* ext4_truncate will clear the flag */
+               if ((EXT4_I(inode)->i_flags & EXT4_EOFBLOCKS_FL))
+                       ext4_truncate(inode);
        }
 
        rc = inode_setattr(inode, attr);

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

[email protected]

maintainer Applied

2010-02-24 16:26 UTC #10 lore raw

In reply to Eric Sandeen

Quoted 8 lines · Eric Sandeen

From: Jiaying Zhang <[email protected]>
fallocate() may potentially instantiate blocks past EOF, depending on the flags used when it is called.
e2fsck currently has a test for blocks past i_size, and it sometimes trips up - noticeably on xfstests 013 which runs fsstress.

Quoted 8 lines

This patch from Jiayang does fix it up - it (along with e2fsprogs updates and other patches recently from Aneesh) has survived many fsstress runs in a row.
(Eric Sandeen: removed ioctl interface and minor cleanups)
Signed-off-by Eric Sandeen <[email protected]>

Applied to the maint branch, although I added back the ability to truncate the fallocated blocks via chattr.

- Ted