ext4: port direct I/O to iomap infrastructure

Separate the iomap field population chunk into a separate simple helper routine. This helps reducing the overall clutter within the ext4_iomap_begin() callback, especially as we move across more code to make use of iomap infrastructure.

Signed-off-by Matthew Bobrowski <[email protected]>

---
 fs/ext4/inode.c | 65 ++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 28 deletions(-)

fs/ext4/inode.c +17 -0

index 516faa280ced..1ccdc14c4d69 100644
@@ -3406,10 +3406,43 @@ static bool ext4_inode_datasync_dirty(struct inode *inode)
 	return inode->i_state & I_DIRTY_DATASYNC;
 }
 
+static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type,
+			  unsigned long first_block, struct ext4_map_blocks *map)
+{
+	u8 blkbits = inode->i_blkbits;
+
+	iomap->flags = 0;
+	if (ext4_inode_datasync_dirty(inode))
+		iomap->flags |= IOMAP_F_DIRTY;
+	iomap->bdev = inode->i_sb->s_bdev;
+	iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev;
+	iomap->offset = (u64) first_block << blkbits;
+	iomap->length = (u64) map->m_len << blkbits;
+
+	if (type) {
+		iomap->type = type;
+		iomap->addr = IOMAP_NULL_ADDR;
+	} else {

fs/ext4/inode.c +10 -28

+
+	if (map->m_flags & EXT4_MAP_NEW)
+		iomap->flags |= IOMAP_F_NEW;
+	return 0;
+}
+
 static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 			    unsigned flags, struct iomap *iomap)
 {
-	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
+	u16 type = 0;
 	unsigned int blkbits = inode->i_blkbits;
 	unsigned long first_block, last_block;
 	struct ext4_map_blocks map;

@@ -3523,33 +3556,9 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 			return ret;
 	}
 
-	iomap->flags = 0;
-	if (ext4_inode_datasync_dirty(inode))
-		iomap->flags |= IOMAP_F_DIRTY;
-	iomap->bdev = inode->i_sb->s_bdev;
-	iomap->dax_dev = sbi->s_daxdev;
-	iomap->offset = (u64)first_block << blkbits;
-	iomap->length = (u64)map.m_len << blkbits;
-
-	if (ret == 0) {
-		iomap->type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE;
-		iomap->addr = IOMAP_NULL_ADDR;
-	} else {
-		if (map.m_flags & EXT4_MAP_MAPPED) {
-			iomap->type = IOMAP_MAPPED;
-		} else if (map.m_flags & EXT4_MAP_UNWRITTEN) {
-			iomap->type = IOMAP_UNWRITTEN;
-		} else {
-			WARN_ON_ONCE(1);
-			return -EIO;
-		}
-		iomap->addr = (u64)map.m_pblk << blkbits;
-	}
-
-	if (map.m_flags & EXT4_MAP_NEW)
-		iomap->flags |= IOMAP_F_NEW;
-
-	return 0;
+	if (!ret)
+		type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE;
+	return ext4_set_iomap(inode, iomap, type, first_block, &map);
 }
 
 static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,

Signature · 1 line

2.20.1

Quoted 22 lines · Matthew Bobrowski

fs/ext4/inode.c +22 -0

+static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type,
+			  unsigned long first_block, struct ext4_map_blocks *map)
+{
+	u8 blkbits = inode->i_blkbits;
+
+	iomap->flags = 0;
+	if (ext4_inode_datasync_dirty(inode))
+		iomap->flags |= IOMAP_F_DIRTY;
+	iomap->bdev = inode->i_sb->s_bdev;
+	iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev;
+	iomap->offset = (u64) first_block << blkbits;
+	iomap->length = (u64) map->m_len << blkbits;
+
+	if (type) {
+		iomap->type = type;
+		iomap->addr = IOMAP_NULL_ADDR;
+	} else {
+		if (map->m_flags & EXT4_MAP_MAPPED) {
+			iomap->type = IOMAP_MAPPED;
+		} else if (map->m_flags & EXT4_MAP_UNWRITTEN) {
+			iomap->type = IOMAP_UNWRITTEN;
+		} else {

fs/ext4/inode.c static bool ext4_inode_datasync_dirty(struct inode *inode) · line 3435

Quoted 5 lines

fs/ext4/inode.c +5 -0

+			WARN_ON_ONCE(1);
+			return -EIO;
+		}
+		iomap->addr = (u64) map->m_pblk << blkbits;
+	}

Looking at this function now, the 'type' argument looks a bit weird. Can we perhaps just remove the 'type' argument and change the above to:

	if (map->m_flags & (EXT4_MAP_MAPPED | EXT4_MAP_UNWRITTEN)) {
		if (map->m_flags & EXT4_MAP_MAPPED)
			iomap->type = IOMAP_MAPPED;
		else if (map->m_flags & EXT4_MAP_UNWRITTEN)
			iomap->type = IOMAP_UNWRITTEN;
		iomap->addr = (u64) map->m_pblk << blkbits;
	} else {
		iomap->type = IOMAP_HOLE;
		iomap->addr = IOMAP_NULL_ADDR;
	}

And then in ext4_iomap_begin() we overwrite the type to:

	if (delalloc && iomap->type == IOMAP_HOLE)
		iomap->type = IOMAP_DELALLOC;

That would IMO make ext4_set_iomap() arguments harder to get wrong.

Quoted 23 lines · Jan Kara

Quoted 22 lines · Matthew Bobrowski

fs/ext4/inode.c +22 -0

+static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type,
+			  unsigned long first_block, struct ext4_map_blocks *map)
+{
+	u8 blkbits = inode->i_blkbits;
+
+	iomap->flags = 0;
+	if (ext4_inode_datasync_dirty(inode))
+		iomap->flags |= IOMAP_F_DIRTY;
+	iomap->bdev = inode->i_sb->s_bdev;
+	iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev;
+	iomap->offset = (u64) first_block << blkbits;
+	iomap->length = (u64) map->m_len << blkbits;
+
+	if (type) {
+		iomap->type = type;
+		iomap->addr = IOMAP_NULL_ADDR;
+	} else {
+		if (map->m_flags & EXT4_MAP_MAPPED) {
+			iomap->type = IOMAP_MAPPED;
+		} else if (map->m_flags & EXT4_MAP_UNWRITTEN) {
+			iomap->type = IOMAP_UNWRITTEN;
+		} else {

Quoted 8 lines

fs/ext4/inode.c static bool ext4_inode_datasync_dirty(struct inode *inode) · line 3435

Quoted 5 lines

fs/ext4/inode.c +5 -0

+			WARN_ON_ONCE(1);
+			return -EIO;
+		}
+		iomap->addr = (u64) map->m_pblk << blkbits;
+	}

Looking at this function now, the 'type' argument looks a bit weird. Can we perhaps just remove the 'type' argument and change the above to:

We can, but refer to the point below.

Quoted 11 lines

	if (map->m_flags & (EXT4_MAP_MAPPED | EXT4_MAP_UNWRITTEN)) {
		if (map->m_flags & EXT4_MAP_MAPPED)
			iomap->type = IOMAP_MAPPED;
		else if (map->m_flags & EXT4_MAP_UNWRITTEN)
			iomap->type = IOMAP_UNWRITTEN;
		iomap->addr = (u64) map->m_pblk << blkbits;
	} else {
		iomap->type = IOMAP_HOLE;
		iomap->addr = IOMAP_NULL_ADDR;
	}

Quoted 6 lines

And then in ext4_iomap_begin() we overwrite the type to:

	if (delalloc && iomap->type == IOMAP_HOLE)
		iomap->type = IOMAP_DELALLOC;

That would IMO make ext4_set_iomap() arguments harder to get wrong.

I was thinking about this while doing a bunch of other things at work today. I'm kind of aligned with what Christoph mentioned around possibly duplicating some of the post 'iomap->type' setting from both current and any future ext4_set_iomap() callers. In addition to this, my thought was that if we're populating the iomap structure with values respectively, then it would make most sense to encapsulate those routines, if possible, within the ext4_set_iomap() as that's the sole purpose of the function.

Quoted 53 lines · Matthew Bobrowski

Quoted 23 lines · Jan Kara

Quoted 22 lines · Matthew Bobrowski

fs/ext4/inode.c +22 -0

+static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type,
+			  unsigned long first_block, struct ext4_map_blocks *map)
+{
+	u8 blkbits = inode->i_blkbits;
+
+	iomap->flags = 0;
+	if (ext4_inode_datasync_dirty(inode))
+		iomap->flags |= IOMAP_F_DIRTY;
+	iomap->bdev = inode->i_sb->s_bdev;
+	iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev;
+	iomap->offset = (u64) first_block << blkbits;
+	iomap->length = (u64) map->m_len << blkbits;
+
+	if (type) {
+		iomap->type = type;
+		iomap->addr = IOMAP_NULL_ADDR;
+	} else {
+		if (map->m_flags & EXT4_MAP_MAPPED) {
+			iomap->type = IOMAP_MAPPED;
+		} else if (map->m_flags & EXT4_MAP_UNWRITTEN) {
+			iomap->type = IOMAP_UNWRITTEN;
+		} else {

Quoted 8 lines

fs/ext4/inode.c static bool ext4_inode_datasync_dirty(struct inode *inode) · line 3435

Quoted 5 lines

fs/ext4/inode.c +5 -0

+			WARN_ON_ONCE(1);
+			return -EIO;
+		}
+		iomap->addr = (u64) map->m_pblk << blkbits;
+	}

Looking at this function now, the 'type' argument looks a bit weird. Can we perhaps just remove the 'type' argument and change the above to:

We can, but refer to the point below.

Quoted 11 lines

	if (map->m_flags & (EXT4_MAP_MAPPED | EXT4_MAP_UNWRITTEN)) {
		if (map->m_flags & EXT4_MAP_MAPPED)
			iomap->type = IOMAP_MAPPED;
		else if (map->m_flags & EXT4_MAP_UNWRITTEN)
			iomap->type = IOMAP_UNWRITTEN;
		iomap->addr = (u64) map->m_pblk << blkbits;
	} else {
		iomap->type = IOMAP_HOLE;
		iomap->addr = IOMAP_NULL_ADDR;
	}

Quoted 6 lines

And then in ext4_iomap_begin() we overwrite the type to:

	if (delalloc && iomap->type == IOMAP_HOLE)
		iomap->type = IOMAP_DELALLOC;

That would IMO make ext4_set_iomap() arguments harder to get wrong.

Quoted 8 lines

I was thinking about this while doing a bunch of other things at work today. I'm kind of aligned with what Christoph mentioned around possibly duplicating some of the post 'iomap->type' setting from both current and any future ext4_set_iomap() callers. In addition to this, my thought was that if we're populating the iomap structure with values respectively, then it would make most sense to encapsulate those routines, if possible, within the ext4_set_iomap() as that's the sole purpose of the function.

Well, what I dislike about 'type' argument is the inconsistency in it's handling. It is useful only for HOLE/DELALLOC, anything else will just give you invalid iomap and you have to be careful to pass 0 in that case.

I understand the concern about possible duplication but since only IOMAP_REPORT cares about IOMAP_DELALLOC, I'm not much concerned about it. But another sensible API would be to optionally pass 'struct extent_status *es' argument to ext4_set_iomap() and if this argument is non-NULL, ext4_set_iomap() will handle the intersection of ext4_map_blocks and extent_status and deduce appropriate resulting type from that.

Separate the iomap field population chunk into a separate simple helper routine. This helps reducing the overall clutter within the ext4_iomap_begin() callback, especially as we move across more code to make use of iomap infrastructure.

Signed-off-by Matthew Bobrowski <[email protected]>

---
  fs/ext4/inode.c | 65 ++++++++++++++++++++++++++++---------------------
  1 file changed, 37 insertions(+), 28 deletions(-)

fs/ext4/inode.c +0 -0

index 516faa280ced..1ccdc14c4d69 100644

fs/ext4/inode.c +35 -28

+{
+	u8 blkbits = inode->i_blkbits;
+
+	iomap->flags = 0;
+	if (ext4_inode_datasync_dirty(inode))
+		iomap->flags |= IOMAP_F_DIRTY;
+	iomap->bdev = inode->i_sb->s_bdev;
+	iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev;
+	iomap->offset = (u64) first_block << blkbits;
+	iomap->length = (u64) map->m_len << blkbits;
+
+	if (type) {
+		iomap->type = type;
+		iomap->addr = IOMAP_NULL_ADDR;
+	} else {
+		if (map->m_flags & EXT4_MAP_MAPPED) {
+			iomap->type = IOMAP_MAPPED;
+		} else if (map->m_flags & EXT4_MAP_UNWRITTEN) {
+			iomap->type = IOMAP_UNWRITTEN;
+		} else {
+			WARN_ON_ONCE(1);
+			return -EIO;
+		}
+		iomap->addr = (u64) map->m_pblk << blkbits;
+	}
+
+	if (map->m_flags & EXT4_MAP_NEW)
+		iomap->flags |= IOMAP_F_NEW;
+	return 0;
+}
+
  static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
  			    unsigned flags, struct iomap *iomap)
  {
-	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
+	u16 type = 0;
  	unsigned int blkbits = inode->i_blkbits;
  	unsigned long first_block, last_block;
  	struct ext4_map_blocks map;

@@ -3523,33 +3556,9 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
  			return ret;
  	}
 
-	iomap->flags = 0;
-	if (ext4_inode_datasync_dirty(inode))
-		iomap->flags |= IOMAP_F_DIRTY;
-	iomap->bdev = inode->i_sb->s_bdev;
-	iomap->dax_dev = sbi->s_daxdev;
-	iomap->offset = (u64)first_block << blkbits;
-	iomap->length = (u64)map.m_len << blkbits;
-
-	if (ret == 0) {
-		iomap->type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE;
-		iomap->addr = IOMAP_NULL_ADDR;
-	} else {
-		if (map.m_flags & EXT4_MAP_MAPPED) {
-			iomap->type = IOMAP_MAPPED;
-		} else if (map.m_flags & EXT4_MAP_UNWRITTEN) {
-			iomap->type = IOMAP_UNWRITTEN;
-		} else {
-			WARN_ON_ONCE(1);
-			return -EIO;
-		}
-		iomap->addr = (u64)map.m_pblk << blkbits;
-	}
-
-	if (map.m_flags & EXT4_MAP_NEW)
-		iomap->flags |= IOMAP_F_NEW;
-
-	return 0;
+	if (!ret)
+		type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE;
+	return ext4_set_iomap(inode, iomap, type, first_block, &map);
  }
 
  static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,

We can also get rid of "first_block" argument here.

Quoted 1 line · Ritesh Harjani

We can also get rid of "first_block" argument here.

That would just duplicate filling it out in all callers, so why?

Quoted 5 lines · Christoph Hellwig

Quoted 1 line · Ritesh Harjani

We can also get rid of "first_block" argument here.

That would just duplicate filling it out in all callers, so why?

What I meant is "first_block" is same as map->m_lblk. (unless ext4_map_blocks can change map->m_lblk, which AFAICT, it should not).
So why pass an extra argument when we are already passing 'map' structure.
So we can fill iomap->offset using map->m_lblk in ext4_set_iomap() function.

In preparation for porting across the direct I/O path to iomap, split out the IOMAP_WRITE logic into a separate helper. This way, we don't need to clutter the ext4_iomap_begin() callback.

Signed-off-by Matthew Bobrowski <[email protected]>

---
 fs/ext4/inode.c | 110 ++++++++++++++++++++++++++----------------------
 1 file changed, 60 insertions(+), 50 deletions(-)

fs/ext4/inode.c +60 -50

index 1ccdc14c4d69..caeb3dec0dec 100644
@@ -3439,6 +3439,62 @@ static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type,
 	return 0;
 }
 
+static int ext4_iomap_alloc(struct inode *inode,
+			    unsigned flags,
+			    unsigned long first_block,
+			    struct ext4_map_blocks *map)
+{
+	handle_t *handle;
+	u8 blkbits = inode->i_blkbits;
+	int ret, dio_credits, retries = 0;
+
+	/*
+	 * Trim mapping request to the maximum value that we can map
+	 * at once for direct I/O.
+	 */
+	if (map->m_len > DIO_MAX_BLOCKS)
+		map->m_len = DIO_MAX_BLOCKS;
+	dio_credits = ext4_chunk_trans_blocks(inode, map->m_len);
+retry:
+	/*
+	 * Either we allocate blocks and then we don't get unwritten
+	 * extent so we have reserved enough credits, or the blocks
+	 * are already allocated and unwritten. In that case, the
+	 * extent conversion fits in the credits as well.
+	 */
+	handle = ext4_journal_start(inode, EXT4_HT_MAP_BLOCKS, dio_credits);
+	if (IS_ERR(handle))
+		return PTR_ERR(handle);
+
+	ret = ext4_map_blocks(handle, inode, map, EXT4_GET_BLOCKS_CREATE_ZERO);
+	if (ret < 0)
+		goto journal_stop;
+
+	/*
+	 * If we have allocated blocks beyond the EOF, we need to make
+	 * sure that they get truncate if we crash before updating the
+	 * inode size metadata in ext4_iomap_end(). For faults, we
+	 * don't need to do that (and cannot due to the orphan list
+	 * operations needing an inode_lock()). If we happen to
+	 * instantiate blocks beyond EOF, it is because we race with a
+	 * truncate operation, which already has added the inode onto
+	 * the orphan list.
+	 */
+	if (!(flags & IOMAP_FAULT) && first_block + map->m_len >
+	    (i_size_read(inode) + (1 << blkbits) - 1) >> blkbits) {
+		int err;
+
+		err = ext4_orphan_add(handle, inode);
+		if (err < 0)
+			ret = err;
+	}
+journal_stop:
+	ext4_journal_stop(handle);
+	if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
+		goto retry;
+	return ret;
+}
+
 static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 			    unsigned flags, struct iomap *iomap)
 {

@@ -3500,62 +3556,16 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 			}
 		}
 	} else if (flags & IOMAP_WRITE) {
-		int dio_credits;
-		handle_t *handle;
-		int retries = 0;
-
-		/* Trim mapping request to maximum we can map at once for DIO */
-		if (map.m_len > DIO_MAX_BLOCKS)
-			map.m_len = DIO_MAX_BLOCKS;
-		dio_credits = ext4_chunk_trans_blocks(inode, map.m_len);
-retry:
-		/*
-		 * Either we allocate blocks and then we don't get unwritten
-		 * extent so we have reserved enough credits, or the blocks
-		 * are already allocated and unwritten and in that case
-		 * extent conversion fits in the credits as well.
-		 */
-		handle = ext4_journal_start(inode, EXT4_HT_MAP_BLOCKS,
-					    dio_credits);
-		if (IS_ERR(handle))
-			return PTR_ERR(handle);
-
-		ret = ext4_map_blocks(handle, inode, &map,
-				      EXT4_GET_BLOCKS_CREATE_ZERO);
-		if (ret < 0) {
-			ext4_journal_stop(handle);
-			if (ret == -ENOSPC &&
-			    ext4_should_retry_alloc(inode->i_sb, &retries))
-				goto retry;
-			return ret;
-		}
-
-		/*
-		 * If we added blocks beyond i_size, we need to make sure they
-		 * will get truncated if we crash before updating i_size in
-		 * ext4_iomap_end(). For faults we don't need to do that (and
-		 * even cannot because for orphan list operations inode_lock is
-		 * required) - if we happen to instantiate block beyond i_size,
-		 * it is because we race with truncate which has already added
-		 * the inode to the orphan list.
-		 */
-		if (!(flags & IOMAP_FAULT) && first_block + map.m_len >
-		    (i_size_read(inode) + (1 << blkbits) - 1) >> blkbits) {
-			int err;
-
-			err = ext4_orphan_add(handle, inode);
-			if (err < 0) {
-				ext4_journal_stop(handle);
-				return err;
-			}
-		}
-		ext4_journal_stop(handle);
+		ret = ext4_iomap_alloc(inode, flags, first_block, &map);
 	} else {
 		ret = ext4_map_blocks(NULL, inode, &map, 0);
 		if (ret < 0)
 			return ret;
 	}
 
+	if (ret < 0)
+		return ret;
+
 	if (!ret)
 		type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE;
 	return ext4_set_iomap(inode, iomap, type, first_block, &map);

Signature · 1 line

2.20.1

Quoted 4 lines · Matthew Bobrowski

In preparation for porting across the direct I/O path to iomap, split out the IOMAP_WRITE logic into a separate helper. This way, we don't need to clutter the ext4_iomap_begin() callback.

Quoted 1 line

Signed-off-by Matthew Bobrowski <[email protected]>

The patch looks good to me. You can add:

Reviewed-by Jan Kara <[email protected]>

Just please reformat the comments to use full 80 column lines. Your Emacs still doesn't seem to get it :)

In preparation for porting across the direct I/O path to iomap, split out the IOMAP_WRITE logic into a separate helper. This way, we don't need to clutter the ext4_iomap_begin() callback.

Signed-off-by Matthew Bobrowski <[email protected]>

---
  fs/ext4/inode.c | 110 ++++++++++++++++++++++++++----------------------
  1 file changed, 60 insertions(+), 50 deletions(-)

fs/ext4/inode.c +56 -41

index 1ccdc14c4d69..caeb3dec0dec 100644
@@ -3439,6 +3439,62 @@ static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type,
  	return 0;
  }
 
+static int ext4_iomap_alloc(struct inode *inode,
+			    unsigned flags,
+			    unsigned long first_block,
+			    struct ext4_map_blocks *map)
+{
+	handle_t *handle;
+	u8 blkbits = inode->i_blkbits;
+	int ret, dio_credits, retries = 0;
+
+	/*
+	 * Trim mapping request to the maximum value that we can map
+	 * at once for direct I/O.
+	 */
+	if (map->m_len > DIO_MAX_BLOCKS)
+		map->m_len = DIO_MAX_BLOCKS;
+	dio_credits = ext4_chunk_trans_blocks(inode, map->m_len);
+retry:
+	/*
+	 * Either we allocate blocks and then we don't get unwritten
+	 * extent so we have reserved enough credits, or the blocks
+	 * are already allocated and unwritten. In that case, the
+	 * extent conversion fits in the credits as well.
+	 */
+	handle = ext4_journal_start(inode, EXT4_HT_MAP_BLOCKS, dio_credits);
+	if (IS_ERR(handle))
+		return PTR_ERR(handle);
+
+	ret = ext4_map_blocks(handle, inode, map, EXT4_GET_BLOCKS_CREATE_ZERO);
+	if (ret < 0)
+		goto journal_stop;
+
+	/*
+	 * If we have allocated blocks beyond the EOF, we need to make
+	 * sure that they get truncate if we crash before updating the
+	 * inode size metadata in ext4_iomap_end(). For faults, we
+	 * don't need to do that (and cannot due to the orphan list
+	 * operations needing an inode_lock()). If we happen to
+	 * instantiate blocks beyond EOF, it is because we race with a
+	 * truncate operation, which already has added the inode onto
+	 * the orphan list.
+	 */
+	if (!(flags & IOMAP_FAULT) && first_block + map->m_len >
+	    (i_size_read(inode) + (1 << blkbits) - 1) >> blkbits) {
+		int err;
+
+		err = ext4_orphan_add(handle, inode);
+		if (err < 0)
+			ret = err;
+	}
+journal_stop:
+	ext4_journal_stop(handle);
+	if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
+		goto retry;
+	return ret;
+}
+
  static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
  			    unsigned flags, struct iomap *iomap)
  {

@@ -3500,62 +3556,16 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
  			}
  		}
  	} else if (flags & IOMAP_WRITE) {
-		int dio_credits;
-		handle_t *handle;
-		int retries = 0;
-
-		/* Trim mapping request to maximum we can map at once for DIO */
-		if (map.m_len > DIO_MAX_BLOCKS)
-			map.m_len = DIO_MAX_BLOCKS;
-		dio_credits = ext4_chunk_trans_blocks(inode, map.m_len);
-retry:
-		/*
-		 * Either we allocate blocks and then we don't get unwritten
-		 * extent so we have reserved enough credits, or the blocks
-		 * are already allocated and unwritten and in that case
-		 * extent conversion fits in the credits as well.
-		 */
-		handle = ext4_journal_start(inode, EXT4_HT_MAP_BLOCKS,
-					    dio_credits);
-		if (IS_ERR(handle))
-			return PTR_ERR(handle);
-
-		ret = ext4_map_blocks(handle, inode, &map,
-				      EXT4_GET_BLOCKS_CREATE_ZERO);
-		if (ret < 0) {
-			ext4_journal_stop(handle);
-			if (ret == -ENOSPC &&
-			    ext4_should_retry_alloc(inode->i_sb, &retries))
-				goto retry;
-			return ret;
-		}
-
-		/*
-		 * If we added blocks beyond i_size, we need to make sure they
-		 * will get truncated if we crash before updating i_size in
-		 * ext4_iomap_end(). For faults we don't need to do that (and
-		 * even cannot because for orphan list operations inode_lock is
-		 * required) - if we happen to instantiate block beyond i_size,
-		 * it is because we race with truncate which has already added
-		 * the inode to the orphan list.
-		 */
-		if (!(flags & IOMAP_FAULT) && first_block + map.m_len >
-		    (i_size_read(inode) + (1 << blkbits) - 1) >> blkbits) {

fs/ext4/inode.c +3 -0

  	} else {
  		ret = ext4_map_blocks(NULL, inode, &map, 0);
  		if (ret < 0)
  			return ret;
  	}
 
+	if (ret < 0)
+		return ret;
+
  	if (!ret)
  		type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE;
  	return ext4_set_iomap(inode, iomap, type, first_block, &map);

On 10/3/19 5:03 PM, Matthew Bobrowski wrote:
Minor comment, but otherwise.
Patch looks good to me. You may add:

Reviewed-by Ritesh Harjani <[email protected]>

Quoted 53 lines

static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
unsigned flags, struct iomap *iomap)
{

fs/ext4/inode.c +0 -46

@@ -3500,62 +3556,16 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
  			}
  		}
  	} else if (flags & IOMAP_WRITE) {
-		int dio_credits;
-		handle_t *handle;
-		int retries = 0;
-
-		/* Trim mapping request to maximum we can map at once for DIO */
-		if (map.m_len > DIO_MAX_BLOCKS)
-			map.m_len = DIO_MAX_BLOCKS;
-		dio_credits = ext4_chunk_trans_blocks(inode, map.m_len);
-retry:
-		/*
-		 * Either we allocate blocks and then we don't get unwritten
-		 * extent so we have reserved enough credits, or the blocks
-		 * are already allocated and unwritten and in that case
-		 * extent conversion fits in the credits as well.
-		 */
-		handle = ext4_journal_start(inode, EXT4_HT_MAP_BLOCKS,
-					    dio_credits);
-		if (IS_ERR(handle))
-			return PTR_ERR(handle);
-
-		ret = ext4_map_blocks(handle, inode, &map,
-				      EXT4_GET_BLOCKS_CREATE_ZERO);
-		if (ret < 0) {
-			ext4_journal_stop(handle);
-			if (ret == -ENOSPC &&
-			    ext4_should_retry_alloc(inode->i_sb, &retries))
-				goto retry;
-			return ret;
-		}
-
-		/*
-		 * If we added blocks beyond i_size, we need to make sure they
-		 * will get truncated if we crash before updating i_size in
-		 * ext4_iomap_end(). For faults we don't need to do that (and
-		 * even cannot because for orphan list operations inode_lock is
-		 * required) - if we happen to instantiate block beyond i_size,
-		 * it is because we race with truncate which has already added
-		 * the inode to the orphan list.
-		 */
-		if (!(flags & IOMAP_FAULT) && first_block + map.m_len >
-		    (i_size_read(inode) + (1 << blkbits) - 1) >> blkbits) {
-			int err;
-
-			err = ext4_orphan_add(handle, inode);
-			if (err < 0) {
-				ext4_journal_stop(handle);

fs/ext4/inode.c static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length, · line 3559

Quoted 5 lines

fs/ext4/inode.c +1 -4

-				return err;
-			}
-		}
-		ext4_journal_stop(handle);
+		ret = ext4_iomap_alloc(inode, flags, first_block, &map);

We don't need "first_block" argument here. Since map->m_lblk saves first_block directly above in the same function.

No strong objection against ext4_iomap_alloc, but maybe ext4_iomap_map_write sounds better? Either way is fine though.

As part of ext4_iomap_begin() cleanups and port across direct I/O path to make use of iomap infrastructure, we split IOMAP_REPORT operations into a separate ->iomap_begin() handler.

Signed-off-by Matthew Bobrowski <[email protected]>

fs/ext4/inode.c +16 -0

+static u16 ext4_iomap_check_delalloc(struct inode *inode,
+				     struct ext4_map_blocks *map)
+{
+	struct extent_status es;
+	ext4_lblk_t end = map->m_lblk + map->m_len - 1;
+
+	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, map->m_lblk,
+				  end, &es);
+
+	/* Entire range is a hole */
+	if (!es.es_len || es.es_lblk > end)
+		return IOMAP_HOLE;
+	if (es.es_lblk <= map->m_lblk) {
+		ext4_lblk_t offset = 0;
+
+		if (es.es_lblk < map->m_lblk)

fs/ext4/inode.c +49 -54

+
+static int ext4_iomap_begin_report(struct inode *inode, loff_t offset,
+				   loff_t length, unsigned flags,
+				   struct iomap *iomap)
+{
+	int ret;
+	u16 type = 0;
+	struct ext4_map_blocks map;
+	u8 blkbits = inode->i_blkbits;
+	unsigned long first_block, last_block;
+
+	if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK)
+		return -EINVAL;
+	first_block = offset >> blkbits;
+	last_block = min_t(loff_t, (offset + length - 1) >> blkbits,
+			   EXT4_MAX_LOGICAL_BLOCK);
+
+	if (ext4_has_inline_data(inode)) {
+		ret = ext4_inline_data_iomap(inode, iomap);
+		if (ret != -EAGAIN) {
+			if (ret == 0 && offset >= iomap->length)
+				ret = -ENOENT;
+			return ret;
+		}
+	}
+
+	map.m_lblk = first_block;
+	map.m_len = last_block = first_block + 1;
+	ret = ext4_map_blocks(NULL, inode, &map, 0);
+	if (ret < 0)
+		return ret;
+	if (ret == 0)
+		type = ext4_iomap_check_delalloc(inode, &map);
+	return ext4_set_iomap(inode, iomap, type, first_block, &map);
+}
+
+const struct iomap_ops ext4_iomap_report_ops = {
+	.iomap_begin = ext4_iomap_begin_report,
+};
+
 static int ext4_iomap_alloc(struct inode *inode,
 			    unsigned flags,
 			    unsigned long first_block,

@@ -3498,12 +3564,10 @@ static int ext4_iomap_alloc(struct inode *inode,
 static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 			    unsigned flags, struct iomap *iomap)
 {
-	u16 type = 0;
-	unsigned int blkbits = inode->i_blkbits;
-	unsigned long first_block, last_block;
-	struct ext4_map_blocks map;
-	bool delalloc = false;
 	int ret;
+	struct ext4_map_blocks map;
+	u8 blkbits = inode->i_blkbits;
+	unsigned long first_block, last_block;
 
 	if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK)
 		return -EINVAL;

@@ -3511,64 +3575,21 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 	last_block = min_t(loff_t, (offset + length - 1) >> blkbits,
 			   EXT4_MAX_LOGICAL_BLOCK);
 
-	if (flags & IOMAP_REPORT) {
-		if (ext4_has_inline_data(inode)) {
-			ret = ext4_inline_data_iomap(inode, iomap);
-			if (ret != -EAGAIN) {
-				if (ret == 0 && offset >= iomap->length)
-					ret = -ENOENT;
-				return ret;
-			}
-		}
-	} else {
-		if (WARN_ON_ONCE(ext4_has_inline_data(inode)))
-			return -ERANGE;
-	}
+	if (WARN_ON_ONCE(ext4_has_inline_data(inode)))
+		return -ERANGE;
 
 	map.m_lblk = first_block;
 	map.m_len = last_block - first_block + 1;
 
-	if (flags & IOMAP_REPORT) {
-		ret = ext4_map_blocks(NULL, inode, &map, 0);
-		if (ret < 0)
-			return ret;
-
-		if (ret == 0) {
-			ext4_lblk_t end = map.m_lblk + map.m_len - 1;
-			struct extent_status es;
-
-			ext4_es_find_extent_range(inode, &ext4_es_is_delayed,
-						  map.m_lblk, end, &es);
-
-			if (!es.es_len || es.es_lblk > end) {
-				/* entire range is a hole */
-			} else if (es.es_lblk > map.m_lblk) {
-				/* range starts with a hole */
-				map.m_len = es.es_lblk - map.m_lblk;
-			} else {
-				ext4_lblk_t offs = 0;
-
-				if (es.es_lblk < map.m_lblk)
-					offs = map.m_lblk - es.es_lblk;
-				map.m_lblk = es.es_lblk + offs;
-				map.m_len = es.es_len - offs;
-				delalloc = true;
-			}
-		}
-	} else if (flags & IOMAP_WRITE) {
+	if (flags & IOMAP_WRITE)
 		ret = ext4_iomap_alloc(inode, flags, first_block, &map);
-	} else {
+	else
 		ret = ext4_map_blocks(NULL, inode, &map, 0);
-		if (ret < 0)
-			return ret;
-	}
 
 	if (ret < 0)
 		return ret;
-
-	if (!ret)
-		type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE;
-	return ext4_set_iomap(inode, iomap, type, first_block, &map);
+	return ext4_set_iomap(inode, iomap, ret ? 0 : IOMAP_HOLE, first_block,
+			      &map);
 }
 
 static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,

Signature · 1 line

2.20.1

On Thu 03-10-19 21:33:45, Matthew Bobrowski wrote:
The patch looks good to me. You can add:

Reviewed-by Jan Kara <[email protected]>

It would just need small adjustments if you change patch 1 as I suggested:

Quoted 21 lines

fs/ext4/inode.c +21 -0

+static u16 ext4_iomap_check_delalloc(struct inode *inode,
+				     struct ext4_map_blocks *map)
+{
+	struct extent_status es;
+	ext4_lblk_t end = map->m_lblk + map->m_len - 1;
+
+	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, map->m_lblk,
+				  end, &es);
+
+	/* Entire range is a hole */
+	if (!es.es_len || es.es_lblk > end)
+		return IOMAP_HOLE;
+	if (es.es_lblk <= map->m_lblk) {
+		ext4_lblk_t offset = 0;
+
+		if (es.es_lblk < map->m_lblk)
+			offset = map->m_lblk - es.es_lblk;
+		map->m_lblk = es.es_lblk + offset;
+		map->m_len = es.es_len - offset;
+		return IOMAP_DELALLOC;
+	}

fs/ext4/inode.c static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type, · line 3467

Quoted 5 lines

fs/ext4/inode.c +5 -0

+
+	/* Range starts with a hole */
+	map->m_len = es.es_lblk - map->m_lblk;
+	return IOMAP_HOLE;
+}

This function would then be IMO better off to directly update 'iomap' as needed after ext4_set_iomap() sets hole there.

As part of ext4_iomap_begin() cleanups and port across direct I/O path to make use of iomap infrastructure, we split IOMAP_REPORT operations into a separate ->iomap_begin() handler.

Signed-off-by Matthew Bobrowski <[email protected]>

---
  fs/ext4/ext4.h  |   1 +
  fs/ext4/file.c  |   6 ++-
  fs/ext4/inode.c | 129 ++++++++++++++++++++++++++++--------------------
  3 files changed, 80 insertions(+), 56 deletions(-)

fs/ext4/ext4.h +1 -0

index 03db3e71676c..d0d88f411a44 100644
@@ -3379,6 +3379,7 @@ static inline void ext4_clear_io_unwritten_flag(ext4_io_end_t *io_end)
  }
 
  extern const struct iomap_ops ext4_iomap_ops;
+extern const struct iomap_ops ext4_iomap_report_ops;
 
  static inline int ext4_buffer_uptodate(struct buffer_head *bh)
  {

fs/ext4/file.c +4 -2

index 8d2bbcc2d813..ab75aee3e687 100644
@@ -494,12 +494,14 @@ loff_t ext4_llseek(struct file *file, loff_t offset, int whence)
  						maxbytes, i_size_read(inode));
  	case SEEK_HOLE:
  		inode_lock_shared(inode);
-		offset = iomap_seek_hole(inode, offset, &ext4_iomap_ops);
+		offset = iomap_seek_hole(inode, offset,
+					 &ext4_iomap_report_ops);
  		inode_unlock_shared(inode);
  		break;
  	case SEEK_DATA:
  		inode_lock_shared(inode);
-		offset = iomap_seek_data(inode, offset, &ext4_iomap_ops);
+		offset = iomap_seek_data(inode, offset,
+					 &ext4_iomap_report_ops);
  		inode_unlock_shared(inode);
  		break;
  	}

fs/ext4/inode.c +8 -0

index caeb3dec0dec..1dace576b8bd 100644
@@ -3439,6 +3439,72 @@ static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type,
  	return 0;
  }
 
+static u16 ext4_iomap_check_delalloc(struct inode *inode,
+				     struct ext4_map_blocks *map)
+{
+	struct extent_status es;
+	ext4_lblk_t end = map->m_lblk + map->m_len - 1;
+
+	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, map->m_lblk,
+				  end, &es);

fs/ext4/inode.c +32 -0

+		map->m_len = es.es_len - offset;
+		return IOMAP_DELALLOC;
+	}
+
+	/* Range starts with a hole */
+	map->m_len = es.es_lblk - map->m_lblk;
+	return IOMAP_HOLE;
+}
+
+static int ext4_iomap_begin_report(struct inode *inode, loff_t offset,
+				   loff_t length, unsigned flags,
+				   struct iomap *iomap)
+{
+	int ret;
+	u16 type = 0;
+	struct ext4_map_blocks map;
+	u8 blkbits = inode->i_blkbits;
+	unsigned long first_block, last_block;
+
+	if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK)
+		return -EINVAL;
+	first_block = offset >> blkbits;
+	last_block = min_t(loff_t, (offset + length - 1) >> blkbits,
+			   EXT4_MAX_LOGICAL_BLOCK);
+
+	if (ext4_has_inline_data(inode)) {
+		ret = ext4_inline_data_iomap(inode, iomap);
+		if (ret != -EAGAIN) {
+			if (ret == 0 && offset >= iomap->length)
+				ret = -ENOENT;
+			return ret;
+		}

fs/ext4/inode.c +15 -54

+}
+
+const struct iomap_ops ext4_iomap_report_ops = {
+	.iomap_begin = ext4_iomap_begin_report,
+};
+
  static int ext4_iomap_alloc(struct inode *inode,
  			    unsigned flags,
  			    unsigned long first_block,

@@ -3498,12 +3564,10 @@ static int ext4_iomap_alloc(struct inode *inode,
  static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
  			    unsigned flags, struct iomap *iomap)
  {
-	u16 type = 0;
-	unsigned int blkbits = inode->i_blkbits;
-	unsigned long first_block, last_block;
-	struct ext4_map_blocks map;
-	bool delalloc = false;
  	int ret;
+	struct ext4_map_blocks map;
+	u8 blkbits = inode->i_blkbits;
+	unsigned long first_block, last_block;
 
  	if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK)
  		return -EINVAL;

@@ -3511,64 +3575,21 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
  	last_block = min_t(loff_t, (offset + length - 1) >> blkbits,
  			   EXT4_MAX_LOGICAL_BLOCK);
 
-	if (flags & IOMAP_REPORT) {
-		if (ext4_has_inline_data(inode)) {
-			ret = ext4_inline_data_iomap(inode, iomap);
-			if (ret != -EAGAIN) {
-				if (ret == 0 && offset >= iomap->length)
-					ret = -ENOENT;
-				return ret;
-			}
-		}
-	} else {
-		if (WARN_ON_ONCE(ext4_has_inline_data(inode)))
-			return -ERANGE;
-	}
+	if (WARN_ON_ONCE(ext4_has_inline_data(inode)))
+		return -ERANGE;
 
  	map.m_lblk = first_block;
  	map.m_len = last_block - first_block + 1;
 
-	if (flags & IOMAP_REPORT) {
-		ret = ext4_map_blocks(NULL, inode, &map, 0);
-		if (ret < 0)
-			return ret;
-
-		if (ret == 0) {
-			ext4_lblk_t end = map.m_lblk + map.m_len - 1;
-			struct extent_status es;
-
-			ext4_es_find_extent_range(inode, &ext4_es_is_delayed,
-						  map.m_lblk, end, &es);
-
-			if (!es.es_len || es.es_lblk > end) {
-				/* entire range is a hole */
-			} else if (es.es_lblk > map.m_lblk) {
-				/* range starts with a hole */
-				map.m_len = es.es_lblk - map.m_lblk;
-			} else {
-				ext4_lblk_t offs = 0;
-
-				if (es.es_lblk < map.m_lblk)
-					offs = map.m_lblk - es.es_lblk;
-				map.m_lblk = es.es_lblk + offs;
-				map.m_len = es.es_len - offs;
-				delalloc = true;
-			}
-		}
-	} else if (flags & IOMAP_WRITE) {
+	if (flags & IOMAP_WRITE)
  		ret = ext4_iomap_alloc(inode, flags, first_block, &map);
-	} else {
+	else
  		ret = ext4_map_blocks(NULL, inode, &map, 0);
-		if (ret < 0)
-			return ret;
-	}
 
  	if (ret < 0)
  		return ret;
-
-	if (!ret)
-		type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE;
-	return ext4_set_iomap(inode, iomap, type, first_block, &map);
+	return ext4_set_iomap(inode, iomap, ret ? 0 : IOMAP_HOLE, first_block,
+			      &map);
  }
 
  static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,

Quoted 12 lines

fs/ext4/inode.c +12 -0

+static u16 ext4_iomap_check_delalloc(struct inode *inode,
+				     struct ext4_map_blocks *map)
+{
+	struct extent_status es;
+	ext4_lblk_t end = map->m_lblk + map->m_len - 1;
+
+	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, map->m_lblk,
+				  end, &es);
+
+	/* Entire range is a hole */
+	if (!es.es_len || es.es_lblk > end)
+		return IOMAP_HOLE;

fs/ext4/inode.c static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type, · line 3459

Quoted 6 lines

fs/ext4/inode.c +6 -0

+	if (es.es_lblk <= map->m_lblk) {
+		ext4_lblk_t offset = 0;
+
+		if (es.es_lblk < map->m_lblk)
+			offset = map->m_lblk - es.es_lblk;
+		map->m_lblk = es.es_lblk + offset;

This looks redundant no? map->m_lblk never changes actually. So this is not needed here.

Quoted 6 lines

fs/ext4/inode.c +6 -0

+	map.m_lblk = first_block;
+	map.m_len = last_block = first_block + 1;
+	ret = ext4_map_blocks(NULL, inode, &map, 0);
+	if (ret < 0)
+		return ret;
+	if (ret == 0)

fs/ext4/inode.c static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type, · line 3501

Quoted 2 lines

fs/ext4/inode.c +2 -0

+		type = ext4_iomap_check_delalloc(inode, &map);
+	return ext4_set_iomap(inode, iomap, type, first_block, &map);

We don't need to send first_block here. Since map->m_lblk is same as first_block.
Also with Jan comment, we don't even need 'type' parameter. Then we should be able to rename the function ext4_set_iomap ==> ext4_map_to_iomap. This better reflects what it is doing. Thoughts?

Quoted 12 lines · Ritesh Harjani

Quoted 12 lines

fs/ext4/inode.c +12 -0

+static u16 ext4_iomap_check_delalloc(struct inode *inode,
+				     struct ext4_map_blocks *map)
+{
+	struct extent_status es;
+	ext4_lblk_t end = map->m_lblk + map->m_len - 1;
+
+	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, map->m_lblk,
+				  end, &es);
+
+	/* Entire range is a hole */
+	if (!es.es_len || es.es_lblk > end)
+		return IOMAP_HOLE;

Quoted 8 lines

fs/ext4/inode.c static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type, · line 3459

Quoted 6 lines

fs/ext4/inode.c +6 -0

+	if (es.es_lblk <= map->m_lblk) {
+		ext4_lblk_t offset = 0;
+
+		if (es.es_lblk < map->m_lblk)
+			offset = map->m_lblk - es.es_lblk;
+		map->m_lblk = es.es_lblk + offset;

This looks redundant no? map->m_lblk never changes actually. So this is not needed here.

Well, it depends if map->m_lblk == es.es_lblk + offset prior to the assignment? If that's always true, then sure, it'd be redundant. But honestly, I don't know what the downstream effect would be if this was removed. I'd have to look at the code, perform some tests, and figure it out.

Quoted 15 lines

fs/ext4/inode.c static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type, · line 3501

Quoted 9 lines

fs/ext4/inode.c +8 -0

+	map.m_lblk = first_block;
+	map.m_len = last_block = first_block + 1;
+	ret = ext4_map_blocks(NULL, inode, &map, 0);
+	if (ret < 0)
+		return ret;
+	if (ret == 0)
+		type = ext4_iomap_check_delalloc(inode, &map);
+	return ext4_set_iomap(inode, iomap, type, first_block, &map);

We don't need to send first_block here. Since map->m_lblk is same as first_block.
Also with Jan comment, we don't even need 'type' parameter. Then we should be able to rename the function ext4_set_iomap ==> ext4_map_to_iomap. This better reflects what it is doing. Thoughts?

Depends on what we conclude in 1/8. :)

I'm for removing 'first_block', but still not convinced removing 'type' is heading down the right track if I were to forward think a little.

This patch introduces a new direct I/O read path that makes use of the iomap infrastructure.

The new function ext4_dio_read_iter() is responsible for calling into the iomap infrastructure via iomap_dio_rw(). If the read operation being performed on the inode does not pass the preliminary checks performed within ext4_dio_supported(), then we simply fallback to buffered I/O in order to fulfil the request.

Existing direct I/O read buffer_head code has been removed as it's now redundant.

Signed-off-by Matthew Bobrowski <[email protected]>

Quoted 12 lines · Matthew Bobrowski

This patch introduces a new direct I/O read path that makes use of the iomap infrastructure.

The new function ext4_dio_read_iter() is responsible for calling into the iomap infrastructure via iomap_dio_rw(). If the read operation being performed on the inode does not pass the preliminary checks performed within ext4_dio_supported(), then we simply fallback to buffered I/O in order to fulfil the request.

Existing direct I/O read buffer_head code has been removed as it's now redundant.

Quoted 1 line

Signed-off-by Matthew Bobrowski <[email protected]>

The patch looks good to me. Just one small nit below. With that fixed, you can add:

Reviewed-by Jan Kara <[email protected]>

Quoted 6 lines

fs/ext4/file.c +6 -0

+	/*
+	 * Get exclusion from truncate and other inode operations.
+	 */
+	if (!inode_trylock_shared(inode)) {
+		if (iocb->ki_flags & IOCB_NOWAIT)
+			return -EAGAIN;

Review

Quoted 2 lines

(quoted diff) +2 -0

+		inode_lock_shared(inode);
+	}

I've noticed here you actually introduce new trylock pattern - previously we had unconditional inode_lock_shared() in ext4_direct_IO_read(). So the cleanest would be to just use unconditional inode_lock_shared() here and then fixup IOCB_NOWAIT handling (I agree that was missing in the original code) in a separate patch.

And the pattern should rather look like:

	if (iocb->ki_flags & IOCB_NOWAIT) {
		if (!inode_trylock_shared(inode))
			return -EAGAIN;
	} else {
		inode_lock_shared(inode);
	}

to avoid two atomical operations instead of one in the fast path. No need to repeat old mistakes when we know better :).

This patch introduces a new direct I/O read path that makes use of the iomap infrastructure.

The new function ext4_dio_read_iter() is responsible for calling into the iomap infrastructure via iomap_dio_rw(). If the read operation being performed on the inode does not pass the preliminary checks performed within ext4_dio_supported(), then we simply fallback to buffered I/O in order to fulfil the request.

Existing direct I/O read buffer_head code has been removed as it's now redundant.

Signed-off-by Matthew Bobrowski <[email protected]>

---
  fs/ext4/file.c  | 58 +++++++++++++++++++++++++++++++++++++++++++++----
  fs/ext4/inode.c | 32 +--------------------------
  2 files changed, 55 insertions(+), 35 deletions(-)

fs/ext4/file.c +16 -0

index ab75aee3e687..69ac042fb74b 100644
@@ -34,6 +34,53 @@
  #include "xattr.h"
  #include "acl.h"
 
+static bool ext4_dio_supported(struct inode *inode)
+{
+	if (IS_ENABLED(CONFIG_FS_ENCRYPTION) && IS_ENCRYPTED(inode))
+		return false;
+	if (fsverity_active(inode))
+		return false;
+	if (ext4_should_journal_data(inode))
+		return false;
+	if (ext4_has_inline_data(inode))
+		return false;
+	return true;
+}
+
+static ssize_t ext4_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
+{
+	ssize_t ret;

fs/ext4/file.c +28 -4

+
+	if (!ext4_dio_supported(inode)) {
+		inode_unlock_shared(inode);
+		/*
+		 * Fallback to buffered I/O if the operation being
+		 * performed on the inode is not supported by direct
+		 * I/O. The IOCB_DIRECT flag needs to be cleared here
+		 * in order to ensure that the direct I/O path withiin
+		 * generic_file_read_iter() is not taken.
+		 */
+		iocb->ki_flags &= ~IOCB_DIRECT;
+		return generic_file_read_iter(iocb, to);
+	}
+
+	ret = iomap_dio_rw(iocb, to, &ext4_iomap_ops, NULL);
+	inode_unlock_shared(inode);
+
+	file_accessed(iocb->ki_filp);
+	return ret;
+}
+
  #ifdef CONFIG_FS_DAX
  static ssize_t ext4_dax_read_iter(struct kiocb *iocb, struct iov_iter *to)
  {

@@ -64,16 +111,19 @@ static ssize_t ext4_dax_read_iter(struct kiocb *iocb, struct iov_iter *to)
 
  static ssize_t ext4_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
  {
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(file_inode(iocb->ki_filp)->i_sb))))
+	struct inode *inode = file_inode(iocb->ki_filp);
+
+	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
  		return -EIO;
 
  	if (!iov_iter_count(to))
  		return 0; /* skip atime */
 
-#ifdef CONFIG_FS_DAX
-	if (IS_DAX(file_inode(iocb->ki_filp)))
+	if (IS_DAX(inode))
  		return ext4_dax_read_iter(iocb, to);
-#endif
+
+	if (iocb->ki_flags & IOCB_DIRECT)
+		return ext4_dio_read_iter(iocb, to);
  	return generic_file_read_iter(iocb, to);
  }
 

fs/ext4/inode.c +1 -31

index 1dace576b8bd..159ffb92f82d 100644
@@ -863,9 +863,6 @@ int ext4_dio_get_block(struct inode *inode, sector_t iblock,
  {
  	/* We don't expect handle for direct IO */
  	WARN_ON_ONCE(ext4_journal_current_handle());
-
-	if (!create)
-		return _ext4_get_block(inode, iblock, bh, 0);
  	return ext4_get_block_trans(inode, iblock, bh, EXT4_GET_BLOCKS_CREATE);
  }
 

@@ -3855,30 +3852,6 @@ static ssize_t ext4_direct_IO_write(struct kiocb *iocb, struct iov_iter *iter)
  	return ret;
  }
 
-static ssize_t ext4_direct_IO_read(struct kiocb *iocb, struct iov_iter *iter)
-{
-	struct address_space *mapping = iocb->ki_filp->f_mapping;
-	struct inode *inode = mapping->host;
-	size_t count = iov_iter_count(iter);
-	ssize_t ret;
-
-	/*
-	 * Shared inode_lock is enough for us - it protects against concurrent
-	 * writes & truncates and since we take care of writing back page cache,
-	 * we are protected against page writeback as well.
-	 */
-	inode_lock_shared(inode);
-	ret = filemap_write_and_wait_range(mapping, iocb->ki_pos,
-					   iocb->ki_pos + count - 1);
-	if (ret)
-		goto out_unlock;
-	ret = __blockdev_direct_IO(iocb, inode, inode->i_sb->s_bdev,
-				   iter, ext4_dio_get_block, NULL, NULL, 0);
-out_unlock:
-	inode_unlock_shared(inode);
-	return ret;
-}
-
  static ssize_t ext4_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
  {
  	struct file *file = iocb->ki_filp;

@@ -3905,10 +3878,7 @@ static ssize_t ext4_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
  		return 0;
 
  	trace_ext4_direct_IO_enter(inode, offset, count, iov_iter_rw(iter));
-	if (iov_iter_rw(iter) == READ)
-		ret = ext4_direct_IO_read(iocb, iter);
-	else
-		ret = ext4_direct_IO_write(iocb, iter);
+	ret = ext4_direct_IO_write(iocb, iter);
  	trace_ext4_direct_IO_exit(inode, offset, count, iov_iter_rw(iter), ret);
  	return ret;
  }

In preparation for implementing the iomap direct I/O write path modifications, the inode extension/truncate code needs to be moved out from ext4_iomap_end(). For direct I/O, if the current code remained within ext4_iomap_end() it would behave incorrectly. Updating the inode size prior to converting unwritten extents to written extents will potentially allow a racing direct I/O read operation to find unwritten extents before they've been correctly converted.

The inode extension/truncate code has been moved out into a new helper ext4_handle_inode_extension(). This function has been designed so that it can be used by both DAX and direct I/O paths.

Signed-off-by Matthew Bobrowski <[email protected]>

On Thu 03-10-19 21:34:18, Matthew Bobrowski wrote:
Looks good to me. Fell free to add:

Reviewed-by Jan Kara <[email protected]>

Just small nits below:

fs/ext4/file.c static ssize_t ext4_write_checks(struct kiocb *iocb, struct iov_iter *from) · line 240

Quoted 4 lines

fs/ext4/file.c +4 -0

+static int ext4_handle_inode_extension(struct inode *inode, loff_t offset,
+				       ssize_t written, size_t count)
+{
+	int ret = 0;

I think both the function and callsites may be slightly simpler if you let the function return 'written' or error (not 0 or error). But I'll leave that decision upto you.

Quoted 7 lines

fs/ext4/file.c +7 -0

+	handle_t *handle;
+	bool truncate = false;
+	u8 blkbits = inode->i_blkbits;
+	ext4_lblk_t written_blk, end_blk;
+
+	/*
+         * Note that EXT4_I(inode)->i_disksize can get extended up to

fs/ext4/file.c static ssize_t ext4_write_checks(struct kiocb *iocb, struct iov_iter *from) · line 253

Quoted 6 lines

fs/ext4/file.c +6 -0

+         * inode->i_size while the IO was running due to writeback of
+         * delalloc blocks. But the code in ext4_iomap_alloc() is careful
+         * to use zeroed / unwritten extents if this is possible and thus
+         * we won't leave uninitialized blocks in a file even if we didn't
+         * succeed in writing as much as we planned.
+         */

Whitespace damaged here...

Quoted 10 lines

Just small nits below:

fs/ext4/file.c static ssize_t ext4_write_checks(struct kiocb *iocb, struct iov_iter *from) · line 240

Quoted 4 lines

fs/ext4/file.c +4 -0

+static int ext4_handle_inode_extension(struct inode *inode, loff_t offset,
+				       ssize_t written, size_t count)
+{
+	int ret = 0;

I think both the function and callsites may be slightly simpler if you let the function return 'written' or error (not 0 or error). But I'll leave that decision upto you.

Hm, don't we actually need to return 0 for success cases so that iomap_dio_complete() behaves correctly i.e. increments iocb->ki_pos, etc?

Quoted 11 lines · Matthew Bobrowski

Quoted 10 lines

Just small nits below:

fs/ext4/file.c static ssize_t ext4_write_checks(struct kiocb *iocb, struct iov_iter *from) · line 240

Quoted 4 lines

fs/ext4/file.c +4 -0

+static int ext4_handle_inode_extension(struct inode *inode, loff_t offset,
+				       ssize_t written, size_t count)
+{
+	int ret = 0;

I think both the function and callsites may be slightly simpler if you let the function return 'written' or error (not 0 or error). But I'll leave that decision upto you.

Quoted 3 lines

Hm, don't we actually need to return 0 for success cases so that iomap_dio_complete() behaves correctly i.e. increments iocb->ki_pos, etc?

Correct, iomap_dio_complete() expects 0 on success. So if we keep calling ext4_handle_inode_extension() from ->end_io handler, we'd need some specialcasing there and I agree that changing ext4_handle_inode_extension() return convention isn't then very beneficial. If we stop calling ext4_handle_inode_extension() from ->end_io handler (patch 8/8 discussion pending), then the change would be a clear win.

In preparation for implementing the iomap direct I/O write path modifications, the inode extension/truncate code needs to be moved out from ext4_iomap_end(). For direct I/O, if the current code remained within ext4_iomap_end() it would behave incorrectly. Updating the inode size prior to converting unwritten extents to written extents will potentially allow a racing direct I/O read operation to find unwritten extents before they've been correctly converted.

The inode extension/truncate code has been moved out into a new helper ext4_handle_inode_extension(). This function has been designed so that it can be used by both DAX and direct I/O paths.

Signed-off-by Matthew Bobrowski <[email protected]>

---
  fs/ext4/file.c  | 79 ++++++++++++++++++++++++++++++++++++++++++++++++-
  fs/ext4/inode.c | 48 +-----------------------------
  2 files changed, 79 insertions(+), 48 deletions(-)

fs/ext4/file.c +78 -1

index 69ac042fb74b..2883711e8a33 100644
@@ -33,6 +33,7 @@
  #include "ext4_jbd2.h"
  #include "xattr.h"
  #include "acl.h"
+#include "truncate.h"
 
  static bool ext4_dio_supported(struct inode *inode)
  {

@@ -233,12 +234,82 @@ static ssize_t ext4_write_checks(struct kiocb *iocb, struct iov_iter *from)
  	return iov_iter_count(from);
  }
 
+static int ext4_handle_inode_extension(struct inode *inode, loff_t offset,
+				       ssize_t written, size_t count)
+{
+	int ret = 0;
+	handle_t *handle;
+	bool truncate = false;
+	u8 blkbits = inode->i_blkbits;
+	ext4_lblk_t written_blk, end_blk;
+
+	/*
+         * Note that EXT4_I(inode)->i_disksize can get extended up to
+         * inode->i_size while the IO was running due to writeback of
+         * delalloc blocks. But the code in ext4_iomap_alloc() is careful
+         * to use zeroed / unwritten extents if this is possible and thus
+         * we won't leave uninitialized blocks in a file even if we didn't
+         * succeed in writing as much as we planned.
+         */
+	WARN_ON_ONCE(i_size_read(inode) < EXT4_I(inode)->i_disksize);
+	if (offset + count <= EXT4_I(inode)->i_disksize)
+		return written < 0 ? written : 0;
+
+	if (written < 0) {
+		ret = written;
+		goto truncate;
+	}
+
+	handle = ext4_journal_start(inode, EXT4_HT_INODE, 2);
+	if (IS_ERR(handle)) {
+		ret = PTR_ERR(handle);
+		goto truncate;
+	}
+
+	if (ext4_update_inode_size(inode, offset + written))
+		ext4_mark_inode_dirty(handle, inode);
+
+	/*
+	 * We may need to truncate allocated but not written blocks
+	 * beyond EOF.
+	 */
+	written_blk = ALIGN(offset + written, 1 << blkbits);
+	end_blk = ALIGN(offset + count, 1 << blkbits);
+	if (written_blk < end_blk && ext4_can_truncate(inode))
+		truncate = true;
+
+	/*
+	 * Remove the inode from the orphan list if it has been
+	 * extended and everything went OK.
+	 */
+	if (!truncate && inode->i_nlink)
+		ext4_orphan_del(handle, inode);
+	ext4_journal_stop(handle);
+
+	if (truncate) {
+truncate:
+		ext4_truncate_failed_write(inode);
+		/*
+		 * If the truncate operation failed early, then the
+		 * inode may still be on the orphan list. In that
+		 * case, we need to try remove the inode from the
+		 * in-memory linked list.
+		 */
+		if (inode->i_nlink)
+			ext4_orphan_del(NULL, inode);
+	}
+	return ret;
+}
+
  #ifdef CONFIG_FS_DAX
  static ssize_t
  ext4_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
  {
-	struct inode *inode = file_inode(iocb->ki_filp);
+	int error;
  	ssize_t ret;
+	size_t count;
+	loff_t offset;
+	struct inode *inode = file_inode(iocb->ki_filp);
 
  	if (!inode_trylock(inode)) {
  		if (iocb->ki_flags & IOCB_NOWAIT)

@@ -255,7 +326,13 @@ ext4_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
  	if (ret)
  		goto out;
 
+	offset = iocb->ki_pos;
+	count = iov_iter_count(from);
  	ret = dax_iomap_rw(iocb, from, &ext4_iomap_ops);
+
+	error = ext4_handle_inode_extension(inode, offset, ret, count);
+	if (error)
+		ret = error;
  out:
  	inode_unlock(inode);
  	if (ret > 0)

fs/ext4/inode.c +1 -47

index 159ffb92f82d..d616062b603e 100644
@@ -3592,53 +3592,7 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
  static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,
  			  ssize_t written, unsigned flags, struct iomap *iomap)
  {
-	int ret = 0;
-	handle_t *handle;
-	int blkbits = inode->i_blkbits;
-	bool truncate = false;
-
-	if (!(flags & IOMAP_WRITE) || (flags & IOMAP_FAULT))
-		return 0;
-
-	handle = ext4_journal_start(inode, EXT4_HT_INODE, 2);
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
-		goto orphan_del;
-	}
-	if (ext4_update_inode_size(inode, offset + written))
-		ext4_mark_inode_dirty(handle, inode);
-	/*
-	 * We may need to truncate allocated but not written blocks beyond EOF.
-	 */
-	if (iomap->offset + iomap->length >
-	    ALIGN(inode->i_size, 1 << blkbits)) {
-		ext4_lblk_t written_blk, end_blk;
-
-		written_blk = (offset + written) >> blkbits;
-		end_blk = (offset + length) >> blkbits;
-		if (written_blk < end_blk && ext4_can_truncate(inode))
-			truncate = true;
-	}
-	/*
-	 * Remove inode from orphan list if we were extending a inode and
-	 * everything went fine.
-	 */
-	if (!truncate && inode->i_nlink &&
-	    !list_empty(&EXT4_I(inode)->i_orphan))
-		ext4_orphan_del(handle, inode);
-	ext4_journal_stop(handle);
-	if (truncate) {
-		ext4_truncate_failed_write(inode);
-orphan_del:
-		/*
-		 * If truncate failed early the inode might still be on the
-		 * orphan list; we need to make sure the inode is removed from
-		 * the orphan list in that case.
-		 */
-		if (inode->i_nlink)
-			ext4_orphan_del(NULL, inode);
-	}
-	return ret;
+	return 0;
  }
 
  const struct iomap_ops ext4_iomap_ops = {

Quoted 12 lines · Matthew Bobrowski

In preparation for implementing the iomap direct I/O write path modifications, the inode extension/truncate code needs to be moved out from ext4_iomap_end(). For direct I/O, if the current code remained within ext4_iomap_end() it would behave incorrectly. Updating the inode size prior to converting unwritten extents to written extents will potentially allow a racing direct I/O read operation to find unwritten extents before they've been correctly converted.

The inode extension/truncate code has been moved out into a new helper ext4_handle_inode_extension(). This function has been designed so that it can be used by both DAX and direct I/O paths.

Quoted 1 line

Signed-off-by Matthew Bobrowski <[email protected]>

checkpatch shows some whitespaces error in your comments in this patch.
But apart from that, patch looks good to me.

You may add:

Reviewed-by Ritesh Harjani <[email protected]>

We lift the inode extension/orphan list handling logic out from ext4_iomap_alloc() and place it within the caller ext4_dax_write_iter().

Signed-off-by Matthew Bobrowski <[email protected]>

---
 fs/ext4/file.c  | 17 +++++++++++++++++
 fs/ext4/inode.c | 22 ----------------------
 2 files changed, 17 insertions(+), 22 deletions(-)

fs/ext4/file.c +17 -0

index 2883711e8a33..f64da0c590b2 100644
@@ -309,6 +309,7 @@ ext4_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	ssize_t ret;
 	size_t count;
 	loff_t offset;
+	handle_t *handle;
 	struct inode *inode = file_inode(iocb->ki_filp);
 
 	if (!inode_trylock(inode)) {

@@ -328,6 +329,22 @@ ext4_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
 
 	offset = iocb->ki_pos;
 	count = iov_iter_count(from);
+
+	if (offset + count > EXT4_I(inode)->i_disksize) {
+		handle = ext4_journal_start(inode, EXT4_HT_INODE, 2);
+		if (IS_ERR(handle)) {
+			ret = PTR_ERR(handle);
+			goto out;
+		}
+
+		ret = ext4_orphan_add(handle, inode);
+		if (ret) {
+			ext4_journal_stop(handle);
+			goto out;
+		}
+		ext4_journal_stop(handle);
+	}
+
 	ret = dax_iomap_rw(iocb, from, &ext4_iomap_ops);
 
 	error = ext4_handle_inode_extension(inode, offset, ret, count);

fs/ext4/inode.c +0 -22

index d616062b603e..e133dda55063 100644
@@ -3508,7 +3508,6 @@ static int ext4_iomap_alloc(struct inode *inode,
 			    struct ext4_map_blocks *map)
 {
 	handle_t *handle;
-	u8 blkbits = inode->i_blkbits;
 	int ret, dio_credits, retries = 0;
 
 	/*

@@ -3530,28 +3529,7 @@ static int ext4_iomap_alloc(struct inode *inode,
 		return PTR_ERR(handle);
 
 	ret = ext4_map_blocks(handle, inode, map, EXT4_GET_BLOCKS_CREATE_ZERO);
-	if (ret < 0)
-		goto journal_stop;
 
-	/*
-	 * If we have allocated blocks beyond the EOF, we need to make
-	 * sure that they get truncate if we crash before updating the
-	 * inode size metadata in ext4_iomap_end(). For faults, we
-	 * don't need to do that (and cannot due to the orphan list
-	 * operations needing an inode_lock()). If we happen to
-	 * instantiate blocks beyond EOF, it is because we race with a
-	 * truncate operation, which already has added the inode onto
-	 * the orphan list.
-	 */
-	if (!(flags & IOMAP_FAULT) && first_block + map->m_len >
-	    (i_size_read(inode) + (1 << blkbits) - 1) >> blkbits) {
-		int err;
-
-		err = ext4_orphan_add(handle, inode);
-		if (err < 0)
-			ret = err;
-	}
-journal_stop:
 	ext4_journal_stop(handle);
 	if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
 		goto retry;

Signature · 1 line

2.20.1

Quoted 5 lines · Matthew Bobrowski

We lift the inode extension/orphan list handling logic out from ext4_iomap_alloc() and place it within the caller ext4_dax_write_iter().

Signed-off-by Matthew Bobrowski <[email protected]>

The patch looks good to me. You can add:

Reviewed-by Jan Kara <[email protected]>

We lift the inode extension/orphan list handling logic out from ext4_iomap_alloc() and place it within the caller ext4_dax_write_iter().

Signed-off-by Matthew Bobrowski <[email protected]>

---
  fs/ext4/file.c  | 17 +++++++++++++++++
  fs/ext4/inode.c | 22 ----------------------
  2 files changed, 17 insertions(+), 22 deletions(-)

fs/ext4/file.c +17 -0

index 2883711e8a33..f64da0c590b2 100644
@@ -309,6 +309,7 @@ ext4_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
  	ssize_t ret;
  	size_t count;
  	loff_t offset;
+	handle_t *handle;
  	struct inode *inode = file_inode(iocb->ki_filp);
 
  	if (!inode_trylock(inode)) {

@@ -328,6 +329,22 @@ ext4_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
 
  	offset = iocb->ki_pos;
  	count = iov_iter_count(from);
+
+	if (offset + count > EXT4_I(inode)->i_disksize) {
+		handle = ext4_journal_start(inode, EXT4_HT_INODE, 2);
+		if (IS_ERR(handle)) {
+			ret = PTR_ERR(handle);
+			goto out;
+		}
+
+		ret = ext4_orphan_add(handle, inode);
+		if (ret) {
+			ext4_journal_stop(handle);
+			goto out;
+		}
+		ext4_journal_stop(handle);
+	}
+
  	ret = dax_iomap_rw(iocb, from, &ext4_iomap_ops);
 
  	error = ext4_handle_inode_extension(inode, offset, ret, count);

fs/ext4/inode.c +0 -22

index d616062b603e..e133dda55063 100644
@@ -3508,7 +3508,6 @@ static int ext4_iomap_alloc(struct inode *inode,
  			    struct ext4_map_blocks *map)
  {
  	handle_t *handle;
-	u8 blkbits = inode->i_blkbits;
  	int ret, dio_credits, retries = 0;
 
  	/*

@@ -3530,28 +3529,7 @@ static int ext4_iomap_alloc(struct inode *inode,
  		return PTR_ERR(handle);
 
  	ret = ext4_map_blocks(handle, inode, map, EXT4_GET_BLOCKS_CREATE_ZERO);
-	if (ret < 0)
-		goto journal_stop;
 
-	/*
-	 * If we have allocated blocks beyond the EOF, we need to make
-	 * sure that they get truncate if we crash before updating the
-	 * inode size metadata in ext4_iomap_end(). For faults, we
-	 * don't need to do that (and cannot due to the orphan list
-	 * operations needing an inode_lock()). If we happen to
-	 * instantiate blocks beyond EOF, it is because we race with a
-	 * truncate operation, which already has added the inode onto
-	 * the orphan list.
-	 */
-	if (!(flags & IOMAP_FAULT) && first_block + map->m_len >
-	    (i_size_read(inode) + (1 << blkbits) - 1) >> blkbits) {
-		int err;
-
-		err = ext4_orphan_add(handle, inode);
-		if (err < 0)
-			ret = err;
-	}
-journal_stop:
  	ext4_journal_stop(handle);
  	if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
  		goto retry;

On 10/3/19 5:04 PM, Matthew Bobrowski wrote:
This looks good. Should solve our previous lengthy discussion about orphan handling :)

You may add:

Reviewed-by Ritesh Harjani <[email protected]>

For iomap direct IO write code path changes, we need to accommodate for the case where the block mapping flags passed to ext4_map_blocks() will result in m_flags having both EXT4_MAP_MAPPED and EXT4_MAP_UNWRITTEN bits set. In order for the allocated unwritten extents to be converted properly in the end_io handler, iomap->type must be set to IOMAP_UNWRITTEN, so we need to reshuffle the conditional statement in order to achieve this.

This change is a no-op for DAX code path as the block mapping flag passed to ext4_map_blocks() when IS_DAX(inode) never results in EXT4_MAP_MAPPED and EXT4_MAP_UNWRITTEN being set at once.

Signed-off-by Matthew Bobrowski <[email protected]>

---
 fs/ext4/inode.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

fs/ext4/inode.c +13 -3

index e133dda55063..63ad23ae05b8 100644
@@ -3420,10 +3420,20 @@ static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type,
 		iomap->type = type;
 		iomap->addr = IOMAP_NULL_ADDR;
 	} else {
-		if (map->m_flags & EXT4_MAP_MAPPED) {
-			iomap->type = IOMAP_MAPPED;
-		} else if (map->m_flags & EXT4_MAP_UNWRITTEN) {
+		/*
+		 * Flags passed to ext4_map_blocks() for direct I/O
+		 * writes can result in map->m_flags having both
+		 * EXT4_MAP_MAPPED and EXT4_MAP_UNWRITTEN bits set. In
+		 * order for allocated extents to be converted to
+		 * written extents in the ->end_io handler correctly,
+		 * we need to ensure that the iomap->type is set
+		 * approprately. Thus, we need to check whether
+		 * EXT4_MAP_UNWRITTEN is set first.
+		 */
+		if (map->m_flags & EXT4_MAP_UNWRITTEN) {
 			iomap->type = IOMAP_UNWRITTEN;
+		} else if (map->m_flags & EXT4_MAP_MAPPED) {
+			iomap->type = IOMAP_MAPPED;
 		} else {
 			WARN_ON_ONCE(1);
 			return -EIO;

Signature · 1 line

2.20.1

The patch looks good to me. You can add:

Reviewed-by Jan Kara <[email protected]>

Just those 72 columns limited comment lines... ;)

For iomap direct IO write code path changes, we need to accommodate for the case where the block mapping flags passed to ext4_map_blocks() will result in m_flags having both EXT4_MAP_MAPPED and EXT4_MAP_UNWRITTEN bits set. In order for the allocated unwritten extents to be converted properly in the end_io handler, iomap->type must be set to IOMAP_UNWRITTEN, so we need to reshuffle the conditional statement in order to achieve this.

This change is a no-op for DAX code path as the block mapping flag passed to ext4_map_blocks() when IS_DAX(inode) never results in EXT4_MAP_MAPPED and EXT4_MAP_UNWRITTEN being set at once.

Signed-off-by Matthew Bobrowski <[email protected]>

---
  fs/ext4/inode.c | 16 +++++++++++++---
  1 file changed, 13 insertions(+), 3 deletions(-)

fs/ext4/inode.c +13 -3

index e133dda55063..63ad23ae05b8 100644
@@ -3420,10 +3420,20 @@ static int ext4_set_iomap(struct inode *inode, struct iomap *iomap, u16 type,
  		iomap->type = type;
  		iomap->addr = IOMAP_NULL_ADDR;
  	} else {
-		if (map->m_flags & EXT4_MAP_MAPPED) {
-			iomap->type = IOMAP_MAPPED;
-		} else if (map->m_flags & EXT4_MAP_UNWRITTEN) {
+		/*
+		 * Flags passed to ext4_map_blocks() for direct I/O
+		 * writes can result in map->m_flags having both
+		 * EXT4_MAP_MAPPED and EXT4_MAP_UNWRITTEN bits set. In
+		 * order for allocated extents to be converted to
+		 * written extents in the ->end_io handler correctly,
+		 * we need to ensure that the iomap->type is set
+		 * approprately. Thus, we need to check whether
+		 * EXT4_MAP_UNWRITTEN is set first.
+		 */
+		if (map->m_flags & EXT4_MAP_UNWRITTEN) {
  			iomap->type = IOMAP_UNWRITTEN;
+		} else if (map->m_flags & EXT4_MAP_MAPPED) {
+			iomap->type = IOMAP_MAPPED;
  		} else {
  			WARN_ON_ONCE(1);
  			return -EIO;

You may be changing the function parameters & name here, (in ext4_set_iomap)

But functionality wise the patch looks good to me.

You may add:

Reviewed-by Ritesh Harjani <[email protected]>

This patch introduces a new direct I/O write code path implementation that makes use of the iomap infrastructure.

All direct I/O write operations are now passed from the ->write_iter() callback to the new function ext4_dio_write_iter(). This function is responsible for calling into iomap infrastructure via iomap_dio_rw(). Snippets of the direct I/O code from within ext4_file_write_iter(), such as checking whether the IO request is unaligned asynchronous I/O, or whether it will ber overwriting allocated and initialized blocks has been moved out and into ext4_dio_write_iter().

The block mapping flags that are passed to ext4_map_blocks() from within ext4_dio_get_block() and friends have effectively been taken out and introduced within the ext4_iomap_alloc().

For inode extension cases, the ->end_io() callback ext4_dio_write_end_io() is responsible for calling into ext4_handle_inode_extension() and performing the necessary metadata updates. Failed writes that we're intended to be extend the inode will have blocks truncated accordingly. The ->end_io() handler is also responsible for converting allocated unwritten extents to written extents.

In the instance of a short write, we fallback to buffered I/O and use that method to complete whatever is left over in 'iter'.

Any blocks
that have been allocated in preparation for direct I/O write will be reused by buffered I/O, so there's no issue with leaving allocated blocks beyond EOF.

Existing direct I/O write buffer_head code has been removed as it's now redundant.

Signed-off-by Matthew Bobrowski <[email protected]>

Seeing how difficult it is when a filesystem wants to complete the iocb synchronously (regardless whether it is async or sync) and have all the information in one place for further processing, I think it would be the easiest to provide iomap_dio_rw_wait() that forces waiting for the iocb to complete *and* returns the appropriate return value instead of pretty useless EIOCBQUEUED. It is actually pretty trivial (patch attached). With this we can then just call iomap_dio_rw_sync() for the inode extension case with ->end_io doing just the unwritten extent processing and then call ext4_handle_inode_extension() from ext4_direct_write_iter() where we would have all the information we need.

Christoph, Darrick, what do you think about extending iomap like in the attached patch (plus sample use in XFS)?

Quoted 11 lines · Jan Kara

Seeing how difficult it is when a filesystem wants to complete the iocb synchronously (regardless whether it is async or sync) and have all the information in one place for further processing, I think it would be the easiest to provide iomap_dio_rw_wait() that forces waiting for the iocb to complete *and* returns the appropriate return value instead of pretty useless EIOCBQUEUED. It is actually pretty trivial (patch attached). With this we can then just call iomap_dio_rw_sync() for the inode extension case with ->end_io doing just the unwritten extent processing and then call ext4_handle_inode_extension() from ext4_direct_write_iter() where we would have all the information we need.

Quoted 2 lines

Christoph, Darrick, what do you think about extending iomap like in the attached patch (plus sample use in XFS)?

I vaguely remember suggesting something like this but Brian or Dave convinced me it wasn't a good idea. This will require a trip to the xfs or fsdevel archives from when the inode_dio_wait was added in XFS.

But if we decide it actully works this time around please don't add the __ variant but just add the parameter to iomap_dio_rw directly.

Quoted 4 lines · Matthew Bobrowski

Any blocks
that have been allocated in preparation for direct I/O write will be reused by buffered I/O, so there's no issue with leaving allocated blocks beyond EOF.

This actually is not true as ext4_truncate_failed_write() will trim blocks beyond EOF. Also this would not be 100% reliable as if we crash between DIO short write succeeding and buffered write happening, we would leave inode with blocks beyond EOF. So I'd just remove this sentence.

Quoted 4 lines

Existing direct I/O write buffer_head code has been removed as it's now redundant.

Signed-off-by Matthew Bobrowski <[email protected]>

...

Review

Quoted 12 lines

(quoted diff) +12 -0

+static int ext4_dio_write_end_io(struct kiocb *iocb, ssize_t size,
+                                int error, unsigned int flags)
+{
+       loff_t offset = iocb->ki_pos;
+       struct inode *inode = file_inode(iocb->ki_filp);
+
+       if (!error && (flags & IOMAP_DIO_UNWRITTEN))
+               error = ext4_convert_unwritten_extents(NULL, inode, offset,
+                                                      size);
+       return ext4_handle_inode_extension(inode, offset, error ? : size,
+                                          size);
+}

I was pondering about this and I don't think we have it quite correct.

Still :-|. The problem is that iomap_dio_complete() will pass dio->size as 'size', which is the amount of submitted IO but not necessarily the amount of blocks that were mapped (that can be larger). Thus ext4_handle_inode_extension() can miss the fact that there are blocks beyond EOF that need to be trimmed. And we have no way of finding that out inside our ->end_io handler. Even iomap_dio_complete() doesn't have that information so we'd need to add 'original length' to struct iomap_dio and then pass it do ->end_io.

Seeing how difficult it is when a filesystem wants to complete the iocb synchronously (regardless whether it is async or sync) and have all the information in one place for further processing, I think it would be the easiest to provide iomap_dio_rw_wait() that forces waiting for the iocb to complete *and* returns the appropriate return value instead of pretty useless EIOCBQUEUED. It is actually pretty trivial (patch attached). With this we can then just call iomap_dio_rw_sync() for the inode extension case with ->end_io doing just the unwritten extent processing and then call ext4_handle_inode_extension() from ext4_direct_write_iter() where we would have all the information we need.