ext4: port direct I/O to iomap infrastructure

Separate the iomap field population chunk of code that is currently within ext4_iomap_begin() into a new helper called ext4_set_iomap(). The intent of this function is self explanatory, however the rationale behind doing so is to also reduce the overall clutter that we currently have within the ext4_iomap_begin() callback.

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

---
 fs/ext4/inode.c | 59 +++++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 26 deletions(-)

Separate the iomap field population chunk of code that is currently within ext4_iomap_begin() into a new helper called ext4_set_iomap(). The intent of this function is self explanatory, however the rationale behind doing so is to also reduce the overall clutter that we currently have within the ext4_iomap_begin() callback.

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

---
  fs/ext4/inode.c | 59 +++++++++++++++++++++++++++----------------------
  1 file changed, 33 insertions(+), 26 deletions(-)

fs/ext4/inode.c +33 -26

index 516faa280ced..158eea9a1944 100644
@@ -3406,10 +3406,39 @@ static bool ext4_inode_datasync_dirty(struct inode *inode)
  	return inode->i_state & I_DIRTY_DATASYNC;
  }
 
+static void ext4_set_iomap(struct inode *inode, struct iomap *iomap,
+			   struct ext4_map_blocks *map, loff_t offset,
+			   loff_t length)
+{
+	u8 blkbits = inode->i_blkbits;
+
+	iomap->flags = 0;
+	if (ext4_inode_datasync_dirty(inode))
+		iomap->flags |= IOMAP_F_DIRTY;
+
+	if (map->m_flags & EXT4_MAP_NEW)
+		iomap->flags |= IOMAP_F_NEW;
+
+	iomap->bdev = inode->i_sb->s_bdev;
+	iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev;
+	iomap->offset = (u64) map->m_lblk << blkbits;
+	iomap->length = (u64) map->m_len << blkbits;
+
+	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;
+	}
+}
+
  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);
  	unsigned int blkbits = inode->i_blkbits;
  	unsigned long first_block, last_block;
  	struct ext4_map_blocks map;

@@ -3523,31 +3552,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;
+	ext4_set_iomap(inode, iomap, &map, offset, length);
+	if (delalloc && iomap->type == IOMAP_HOLE)
+		iomap->type = IOMAP_DELALLOC;
 
  	return 0;
  }

Quoted 6 lines · Matthew Bobrowski

Separate the iomap field population chunk of code that is currently within ext4_iomap_begin() into a new helper called ext4_set_iomap(). The intent of this function is self explanatory, however the rationale behind doing so is to also reduce the overall clutter that we currently have within the ext4_iomap_begin() callback.

Quoted 1 line

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

Could you please re-arrange patch sequence in this fashion.

1. Patch-11 (re-ordering of unwritten flags)
2. Patch-8 (trylock in IOCB_NOWAIT cases)
3. Patch-2 (should explain offset & len in this patch)
4. Patch-1 (this patch).

This is so that some of these are anyway fixes or refactoring which can be picked up easily, either for backporting or sometimes this helps in getting some of the patches in, if the patch series gets bigger.
Also others (like me) can also pick some of these changes then to meet their dependency. :)

This patch looks good to me. You may add:

Reviewed-by Ritesh Harjani <[email protected]>

This patch is effectively addressed what Dave Chinner had found and fixed within this commit: 8a23414ee345. Justification for needing this modification has been provided below:

When doing a direct IO that spans the current EOF, and there are written blocks beyond EOF that extend beyond the current write, the only metadata update that needs to be done is a file size extension.

However, we don't mark such iomaps as IOMAP_F_DIRTY to indicate that there is IO completion metadata updates required, and hence we may fail to correctly sync file size extensions made in IO completion when O_DSYNC writes are being used and the hardware supports FUA.

Hence when setting IOMAP_F_DIRTY, we need to also take into account whether the iomap spans the current EOF. If it does, then we need to mark it dirty so that IO completion will call generic_write_sync() to flush the inode size update to stable storage correctly.

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

---
 fs/ext4/inode.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

fs/ext4/inode.c +7 -1

index 158eea9a1944..0dd29ae5cc8c 100644
@@ -3412,8 +3412,14 @@ static void ext4_set_iomap(struct inode *inode, struct iomap *iomap,
 {
 	u8 blkbits = inode->i_blkbits;
 
+	/*
+	 * Writes that span EOF might trigger an I/O size update on completion,
+	 * so consider them to be dirty for the purposes of O_DSYNC, even if
+	 * there is no other metadata changes being made or are pending here.
+	 */
 	iomap->flags = 0;
-	if (ext4_inode_datasync_dirty(inode))
+	if (ext4_inode_datasync_dirty(inode) ||
+	    offset + length > i_size_read(inode))
 		iomap->flags |= IOMAP_F_DIRTY;
 
 	if (map->m_flags & EXT4_MAP_NEW)

Signature · 3 lines

2.20.1

--<M>--

Quoted 18 lines · Matthew Bobrowski

This patch is effectively addressed what Dave Chinner had found and fixed within this commit: 8a23414ee345. Justification for needing this modification has been provided below:

When doing a direct IO that spans the current EOF, and there are written blocks beyond EOF that extend beyond the current write, the only metadata update that needs to be done is a file size extension.

However, we don't mark such iomaps as IOMAP_F_DIRTY to indicate that there is IO completion metadata updates required, and hence we may fail to correctly sync file size extensions made in IO completion when O_DSYNC writes are being used and the hardware supports FUA.

Hence when setting IOMAP_F_DIRTY, we need to also take into account whether the iomap spans the current EOF. If it does, then we need to mark it dirty so that IO completion will call generic_write_sync() to flush the inode size update to stable storage correctly.

Quoted 1 line

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

Looks good to me. You could possibly also comment in the changelog that currently, this change doesn't have user visible impact for ext4 as none of current users of ext4_iomap_begin() that extend files depend of IOMAP_F_DIRTY.

Also this patch would make slightly more sense to be before 1/12 so that you don't have there those two strange unused arguments. But these are just small nits.

Feel free to add:

Reviewed-by Jan Kara <[email protected]>

This patch is effectively addressed what Dave Chinner had found and fixed within this commit: 8a23414ee345. Justification for needing this modification has been provided below:

When doing a direct IO that spans the current EOF, and there are written blocks beyond EOF that extend beyond the current write, the only metadata update that needs to be done is a file size extension.

However, we don't mark such iomaps as IOMAP_F_DIRTY to indicate that there is IO completion metadata updates required, and hence we may fail to correctly sync file size extensions made in IO completion when O_DSYNC writes are being used and the hardware supports FUA.

Hence when setting IOMAP_F_DIRTY, we need to also take into account whether the iomap spans the current EOF. If it does, then we need to mark it dirty so that IO completion will call generic_write_sync() to flush the inode size update to stable storage correctly.

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

---
  fs/ext4/inode.c | 8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)

fs/ext4/inode.c +7 -1

index 158eea9a1944..0dd29ae5cc8c 100644
@@ -3412,8 +3412,14 @@ static void ext4_set_iomap(struct inode *inode, struct iomap *iomap,
  {
  	u8 blkbits = inode->i_blkbits;
 
+	/*
+	 * Writes that span EOF might trigger an I/O size update on completion,
+	 * so consider them to be dirty for the purposes of O_DSYNC, even if
+	 * there is no other metadata changes being made or are pending here.
+	 */
  	iomap->flags = 0;
-	if (ext4_inode_datasync_dirty(inode))
+	if (ext4_inode_datasync_dirty(inode) ||
+	    offset + length > i_size_read(inode))
  		iomap->flags |= IOMAP_F_DIRTY;
 
  	if (map->m_flags & EXT4_MAP_NEW)

Quoted 3 lines · Matthew Bobrowski

This patch is effectively addressed what Dave Chinner had found and fixed within this commit: 8a23414ee345. Justification for needing this modification has been provided below:

Not sure if this is a valid commit id. I couldn't find it.

In preparation for porting across the ext4 direct I/O path for reads and writes over to the iomap infrastructure, split up the IOMAP_WRITE chunk of code into a separate helper ext4_alloc_iomap(). This way, when we add the necessary bits for direct I/O, we don't end up with ext4_iomap_begin() becoming a behemoth twisty maze.

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

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

fs/ext4/inode.c +60 -52

index 0dd29ae5cc8c..3dc92bd8a944 100644
@@ -3442,6 +3442,62 @@ static void ext4_set_iomap(struct inode *inode, struct iomap *iomap,
 	}
 }
 
+static int ext4_iomap_alloc(struct inode *inode, struct ext4_map_blocks *map,
+			    unsigned int flags)
+{
+	handle_t *handle;
+	u8 blkbits = inode->i_blkbits;
+	int ret, dio_credits, retries = 0;
+
+	/*
+	 * Trim the 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 don't get an unwritten extent, so
+	 * in that case we have reserved enough credits. Or, the blocks are
+	 * already allocated and and unwritten. In that case, the extent
+	 * conversion fits into the credits too.
+	 */
+	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 EOF, we need to ensure that
+	 * they're truncated if we crash before updating the inode size
+	 * metadata within 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) && map->m_lblk + 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)
 {

@@ -3502,62 +3558,14 @@ 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, &map, flags);
 	} else {
 		ret = ext4_map_blocks(NULL, inode, &map, 0);
-		if (ret < 0)
-			return ret;
 	}
 
+	if (ret < 0)
+		return ret;
+
 	ext4_set_iomap(inode, iomap, &map, offset, length);
 	if (delalloc && iomap->type == IOMAP_HOLE)
 		iomap->type = IOMAP_DELALLOC;

Signature · 3 lines

2.20.1

--<M>--

In preparation for porting across the ext4 direct I/O path for reads and writes over to the iomap infrastructure, split up the IOMAP_WRITE chunk of code into a separate helper ext4_alloc_iomap(). This way, when we add the necessary bits for direct I/O, we don't end up with ext4_iomap_begin() becoming a behemoth twisty maze.

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

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

fs/ext4/inode.c +60 -52

index 0dd29ae5cc8c..3dc92bd8a944 100644
@@ -3442,6 +3442,62 @@ static void ext4_set_iomap(struct inode *inode, struct iomap *iomap,
  	}
  }
 
+static int ext4_iomap_alloc(struct inode *inode, struct ext4_map_blocks *map,
+			    unsigned int flags)
+{
+	handle_t *handle;
+	u8 blkbits = inode->i_blkbits;
+	int ret, dio_credits, retries = 0;
+
+	/*
+	 * Trim the 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 don't get an unwritten extent, so
+	 * in that case we have reserved enough credits. Or, the blocks are
+	 * already allocated and and unwritten. In that case, the extent
+	 * conversion fits into the credits too.
+	 */
+	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 EOF, we need to ensure that
+	 * they're truncated if we crash before updating the inode size
+	 * metadata within 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) && map->m_lblk + 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)
  {

@@ -3502,62 +3558,14 @@ 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, &map, flags);
  	} else {
  		ret = ext4_map_blocks(NULL, inode, &map, 0);
-		if (ret < 0)
-			return ret;
  	}
 
+	if (ret < 0)
+		return ret;
+
  	ext4_set_iomap(inode, iomap, &map, offset, length);
  	if (delalloc && iomap->type == IOMAP_HOLE)
  		iomap->type = IOMAP_DELALLOC;

As part of the ext4_iomap_begin() cleanups that precede this patch, here we also split up the IOMAP_REPORT branch into a completely separate ->iomap_begin() callback named ext4_iomap_begin_report(). Again, the raionale for this change is to reduce the overall clutter that's starting to become apparent as we start to port more functionality over to the iomap infrastructure.

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

On Mon 21-10-19 20:18:09, Matthew Bobrowski wrote:
One nit below.

fs/ext4/inode.c const struct iomap_ops ext4_iomap_ops = { · line 3608

Quoted 12 lines

fs/ext4/inode.c +12 -0

+	ext4_es_find_extent_range(inode, &ext4_es_is_delayed,
+				  map->m_lblk, end, &es);
+
+	if (!es.es_len || es.es_lblk > end)
+		return false;
+
+	if (es.es_lblk > map->m_lblk) {
+		map->m_len = es.es_lblk - map->m_lblk;
+		return false;
+	}
+
+	if (es.es_lblk <= map->m_lblk)

This condition must be always true AFAICT.

fs/ext4/inode.c const struct iomap_ops ext4_iomap_ops = { · line 3615

Quoted 7 lines

fs/ext4/inode.c +7 -0

+		offset = map->m_lblk - es.es_lblk;
+
+	map->m_lblk = es.es_lblk + offset;
+	map->m_len = es.es_len - offset;
+
+	return true;
+}

Quoted 16 lines · Jan Kara

On Mon 21-10-19 20:18:09, Matthew Bobrowski wrote:
One nit below.

fs/ext4/inode.c const struct iomap_ops ext4_iomap_ops = { · line 3608

Quoted 12 lines

fs/ext4/inode.c +12 -0

+	ext4_es_find_extent_range(inode, &ext4_es_is_delayed,
+				  map->m_lblk, end, &es);
+
+	if (!es.es_len || es.es_lblk > end)
+		return false;
+
+	if (es.es_lblk > map->m_lblk) {
+		map->m_len = es.es_lblk - map->m_lblk;
+		return false;
+	}
+
+	if (es.es_lblk <= map->m_lblk)

This condition must be always true AFAICT.

That would make sense. I will remove this condition in v6.

Quoted 3 lines

fs/ext4/inode.c const struct iomap_ops ext4_iomap_ops = { · line 3611

Quoted 3 lines

fs/ext4/inode.c +3 -0

+		offset = map->m_lblk - es.es_lblk;
+
+	map->m_lblk = es.es_lblk + offset;

Quoted 4 lines

fs/ext4/inode.c const struct iomap_ops ext4_iomap_ops = { · line 3615

Quoted 4 lines

fs/ext4/inode.c +4 -0

+	map->m_len = es.es_len - offset;
+
+	return true;
+}

--<M>--

Quoted 3 lines · Matthew Bobrowski

Quoted 2 lines · Jan Kara

On Mon 21-10-19 20:18:09, Matthew Bobrowski wrote:
One nit below.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

fs/ext4/inode.c +6 -0

+	ext4_es_find_extent_range(inode, &ext4_es_is_delayed,
+				  map->m_lblk, end, &es);
+
+	if (!es.es_len || es.es_lblk > end)
+		return false;
+

Quoted 8 lines

Quoted 8 lines

fs/ext4/inode.c const struct iomap_ops ext4_iomap_ops = { · line 3611

Quoted 8 lines

fs/ext4/inode.c +8 -0

+	if (es.es_lblk > map->m_lblk) {
+		map->m_len = es.es_lblk - map->m_lblk;
+		return false;
+	}
+
+	if (es.es_lblk <= map->m_lblk)
+		offset = map->m_lblk - es.es_lblk;
+	map->m_lblk = es.es_lblk + offset;

And so, this above line will also be redundant.

This patch has already been posted through by Jan, but I've just included it within this patch series to mark that it's a clear dependency.

Quoted 3 lines · On Mon, Oct 21, 2019 at 08:18:18PM +1100, [email protected] wrote:

This patch has already been posted through by Jan, but I've just included it within this patch series to mark that it's a clear dependency.

You probably want to resend the next iteration against the iomap-for-next branch, which includes this plus an iomap_begin API change. Darrick plans to have it as a stable branch, so that it can also be pulled into the ext4 tree.

Use iomap_dio_rw() to wait for unaligned direct IO instead of opencoding the wait.

Signed-off-by Jan Kara <[email protected]>

---

This patch has already been posted through by Jan, but I've just included it within this patch series to mark that it's a clear dependency.

 fs/xfs/xfs_file.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

fs/xfs/xfs_file.c +4 -8

index 0739ba72a82e..c0620135a279 100644
@@ -547,16 +547,12 @@ xfs_file_dio_aio_write(
 	}
 
 	trace_xfs_file_direct_write(ip, count, iocb->ki_pos);
-	ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, &xfs_dio_write_ops,
-			   is_sync_kiocb(iocb));
-
 	/*
-	 * If unaligned, this is the only IO in-flight. If it has not yet
-	 * completed, wait on it before we release the iolock to prevent
-	 * subsequent overlapping IO.
+	 * If unaligned, this is the only IO in-flight. Wait on it before we
+	 * release the iolock to prevent subsequent overlapping IO.
 	 */
-	if (ret == -EIOCBQUEUED && unaligned_io)
-		inode_dio_wait(inode);
+	ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, &xfs_dio_write_ops,
+			   is_sync_kiocb(iocb) || unaligned_io);
 out:
 	xfs_iunlock(ip, iolock);
 

Signature · 3 lines

2.20.1

--<M>--

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

The new function ext4_do_read_iter() is responsible for calling into the iomap infrastructure via iomap_dio_rw(). If the read operation performed on the inode is not supported, which is checked via ext4_dio_supported(), then we simply fallback and complete the I/O using buffered I/O.

Existing direct I/O read code path has been removed, as it is no longer required.

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

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

fs/ext4/file.c +45 -1

index ab75aee3e687..6ea7e00e0204 100644
@@ -34,6 +34,46 @@
 #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 int ext4_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
+{
+	ssize_t ret;
+	struct inode *inode = file_inode(iocb->ki_filp);
+
+	inode_lock_shared(inode);
+	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 within 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,
+			   is_sync_kiocb(iocb));
+	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,7 +104,9 @@ 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))

@@ -74,6 +116,8 @@ static ssize_t ext4_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	if (IS_DAX(file_inode(iocb->ki_filp)))
 		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 ebeedbf3900f..03a9e2b85e46 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);
 }
 

@@ -3865,30 +3862,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;

@@ -3915,10 +3888,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;
 }

Signature · 3 lines

2.20.1

--<M>--

Quoted 9 lines · Matthew Bobrowski

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

The new function ext4_do_read_iter() is responsible for calling into the iomap infrastructure via iomap_dio_rw(). If the read operation performed on the inode is not supported, which is checked via ext4_dio_supported(), then we simply fallback and complete the I/O using buffered I/O.

Quoted 4 lines

Existing direct I/O read code path has been removed, as it is no longer required.

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

Looks good to me. You can add:

Reviewed-by Jan Kara <[email protected]>

BTW, I think I gave you my Reviewed-by tag for this patch also last time and this patch didn't change since then. You can just include the tag in your posting in that case.

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

The new function ext4_do_read_iter() is responsible for calling into the iomap infrastructure via iomap_dio_rw(). If the read operation performed on the inode is not supported, which is checked via ext4_dio_supported(), then we simply fallback and complete the I/O using buffered I/O.

Existing direct I/O read code path has been removed, as it is no longer required.

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

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

fs/ext4/file.c +45 -1

index ab75aee3e687..6ea7e00e0204 100644
@@ -34,6 +34,46 @@
  #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 int ext4_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
+{
+	ssize_t ret;
+	struct inode *inode = file_inode(iocb->ki_filp);
+
+	inode_lock_shared(inode);
+	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 within 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,
+			   is_sync_kiocb(iocb));
+	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,7 +104,9 @@ 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))

@@ -74,6 +116,8 @@ static ssize_t ext4_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
  	if (IS_DAX(file_inode(iocb->ki_filp)))
  		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 ebeedbf3900f..03a9e2b85e46 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);
  }
 

@@ -3865,30 +3862,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;

@@ -3915,10 +3888,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;
  }

This patch updates the lock pattern in ext4_dio_read_iter() to only perform the trylock in IOCB_NOWAIT cases.

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

---
 fs/ext4/file.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

fs/ext4/file.c +7 -1

index 6ea7e00e0204..8420686b90f5 100644
@@ -52,7 +52,13 @@ static int ext4_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	ssize_t ret;
 	struct inode *inode = file_inode(iocb->ki_filp);
 
-	inode_lock_shared(inode);
+	if (iocb->ki_flags & IOCB_NOWAIT) {
+		if (!inode_trylock_shared(inode))
+			return -EAGAIN;
+	} else {
+		inode_lock_shared(inode);
+	}
+
 	if (!ext4_dio_supported(inode)) {
 		inode_unlock_shared(inode);
 		/*

Quoted 2 lines · Matthew Bobrowski

This patch updates the lock pattern in ext4_dio_read_iter() to only perform the trylock in IOCB_NOWAIT cases.

The changelog is actually misleading. It should say something like "This patch updates the lock pattern in ext4_dio_read_iter() to not block on inode lock in case of IOCB_NOWAIT direct IO reads."

Also to ease backporting of easy fixes, we try to put patches like this early in the series (fixing code in ext4_direct_IO_read(), and then the fixed code would just carry over to ext4_dio_read_iter()).

Quoted 11 lines · Jan Kara

Quoted 2 lines · Matthew Bobrowski

This patch updates the lock pattern in ext4_dio_read_iter() to only perform the trylock in IOCB_NOWAIT cases.

The changelog is actually misleading. It should say something like "This patch updates the lock pattern in ext4_dio_read_iter() to not block on inode lock in case of IOCB_NOWAIT direct IO reads."

Also to ease backporting of easy fixes, we try to put patches like this early in the series (fixing code in ext4_direct_IO_read(), and then the fixed code would just carry over to ext4_dio_read_iter()).

OK, understood. Now I know this for next time. :)

Providing that I have this patch precede the ext4_dio_read_iter() patch and implement this lock pattern in ext4_direct_IO_read(), am I OK to add the 'Reviewed-by' tag?

This patch updates the lock pattern in ext4_dio_read_iter() to only perform the trylock in IOCB_NOWAIT cases.

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

---
  fs/ext4/file.c | 8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)

fs/ext4/file.c +7 -1

index 6ea7e00e0204..8420686b90f5 100644
@@ -52,7 +52,13 @@ static int ext4_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
  	ssize_t ret;
  	struct inode *inode = file_inode(iocb->ki_filp);
 
-	inode_lock_shared(inode);
+	if (iocb->ki_flags & IOCB_NOWAIT) {
+		if (!inode_trylock_shared(inode))
+			return -EAGAIN;
+	} else {
+		inode_lock_shared(inode);
+	}
+
  	if (!ext4_dio_supported(inode)) {
  		inode_unlock_shared(inode);
  		/*

In preparation for implementing the iomap direct I/O modifications, the inode extension/truncate code needs to be moved out from the ext4_iomap_end() callback. For direct I/O, if the current code remained, it would behave incorrrectly. Updating the inode size prior to converting unwritten extents would potentially allow a racing direct I/O read to find unwritten extents before being converted correctly.

The inode extension/truncate code now resides within a new helper ext4_handle_inode_extension(). This function has been designed so that it can accommodate for both DAX and direct I/O extension/truncate operations.

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

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

Quoted 13 lines · Matthew Bobrowski

In preparation for implementing the iomap direct I/O modifications, the inode extension/truncate code needs to be moved out from the ext4_iomap_end() callback. For direct I/O, if the current code remained, it would behave incorrrectly. Updating the inode size prior to converting unwritten extents would potentially allow a racing direct I/O read to find unwritten extents before being converted correctly.

The inode extension/truncate code now resides within a new helper ext4_handle_inode_extension(). This function has been designed so that it can accommodate for both DAX and direct I/O extension/truncate operations.

Quoted 6 lines

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

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

The patch looks good to me. You can add:

Reviewed-by Jan Kara <[email protected]>

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

Quoted 2 lines

fs/ext4/file.c +2 -0

+static ssize_t ext4_handle_inode_extension(struct inode *inode, ssize_t written,
+					   loff_t offset, size_t count)

IMHO a bit more logical ordering of arguments would be 'inode, offset, written, count'...

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

All direct I/O writes are now passed from the ->write_iter() callback through to the new direct I/O handler ext4_dio_write_iter(). This function is responsible for calling into the iomap infrastructure via iomap_dio_rw().

Code snippets from the existing direct I/O write code within ext4_file_write_iter() such as, checking whether the I/O request is unaligned asynchronous I/O, or whether the write will result in an overwrite have effectively been moved out and into the new direct I/O ->write_iter() handler.

The block mapping flags that are eventually passed down to ext4_map_blocks() from the *_get_block_*() suite of routines have been taken out and introduced within ext4_iomap_alloc().

For inode extension cases, ext4_handle_inode_extension() is effectively the function responsible for performing such metadata updates. This is called after iomap_dio_rw() has returned so that we can safely determine whether we need to potentially truncate any allocated blocks that may have been prepared for this direct I/O write. We don't perform the inode extension, or truncate operations from the ->end_io() handler as we don't have the original I/O 'length' available there. The ->end_io() however is responsible fo converting allocated unwritten extents to written extents.

In the instance of a short write, we fallback and complete the remainder of the I/O using buffered I/O via ext4_buffered_write_iter().

The existing buffer_head direct I/O implementation has been removed as it's now redundant.

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

---
 fs/ext4/ext4.h    |   3 -
 fs/ext4/extents.c |   4 +-
 fs/ext4/file.c    | 236 ++++++++++++++++++--------
 fs/ext4/inode.c   | 411 +++++-----------------------------------------
 4 files changed, 207 insertions(+), 447 deletions(-)

fs/ext4/file.c +10 -0

+	if (extend) {
+		ret = ext4_handle_inode_extension(inode, ret, offset, count);
+
+		/*
+		 * We may have failed to remove the inode from the orphan list
+		 * in the case that the i_disksize got update due to delalloc
+		 * writeback while the direct I/O was running. We need to make
+		 * sure we remove it from the orphan list as if we've
+		 * prematurely popped it onto the list.
+		 */

Quoted 35 lines · Matthew Bobrowski

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

All direct I/O writes are now passed from the ->write_iter() callback through to the new direct I/O handler ext4_dio_write_iter(). This function is responsible for calling into the iomap infrastructure via iomap_dio_rw().

Code snippets from the existing direct I/O write code within ext4_file_write_iter() such as, checking whether the I/O request is unaligned asynchronous I/O, or whether the write will result in an overwrite have effectively been moved out and into the new direct I/O ->write_iter() handler.

The block mapping flags that are eventually passed down to ext4_map_blocks() from the *_get_block_*() suite of routines have been taken out and introduced within ext4_iomap_alloc().

For inode extension cases, ext4_handle_inode_extension() is effectively the function responsible for performing such metadata updates. This is called after iomap_dio_rw() has returned so that we can safely determine whether we need to potentially truncate any allocated blocks that may have been prepared for this direct I/O write. We don't perform the inode extension, or truncate operations from the ->end_io() handler as we don't have the original I/O 'length' available there. The ->end_io() however is responsible fo converting allocated unwritten extents to written extents.

In the instance of a short write, we fallback and complete the remainder of the I/O using buffered I/O via ext4_buffered_write_iter().

The existing buffer_head direct I/O implementation has been removed as it's now redundant.

Quoted 7 lines

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

---
 fs/ext4/ext4.h    |   3 -
 fs/ext4/extents.c |   4 +-
 fs/ext4/file.c    | 236 ++++++++++++++++++--------
 fs/ext4/inode.c   | 411 +++++-----------------------------------------
 4 files changed, 207 insertions(+), 447 deletions(-)

The patch looks good to me! You can add:

Reviewed-by Jan Kara <[email protected]>

One nitpick below:

Quoted 15 lines

fs/ext4/file.c +15 -0

+	if (extend) {
+		ret = ext4_handle_inode_extension(inode, ret, offset, count);
+
+		/*
+		 * We may have failed to remove the inode from the orphan list
+		 * in the case that the i_disksize got update due to delalloc
+		 * writeback while the direct I/O was running. We need to make
+		 * sure we remove it from the orphan list as if we've
+		 * prematurely popped it onto the list.
+		 */
+		if (!list_empty(&EXT4_I(inode)->i_orphan)) {
+			handle = ext4_journal_start(inode, EXT4_HT_INODE, 2);
+			if (IS_ERR(handle)) {
+				ret = PTR_ERR(handle);
+				if (inode->i_nlink)

fs/ext4/file.c static ssize_t ext4_handle_inode_extension(struct inode *inode, ssize_t written, · line 432

Quoted 5 lines

fs/ext4/file.c +5 -0

+					ext4_orphan_del(NULL, inode);
+				goto out;
+			}
+
+			if (inode->i_nlink)

This check can be joined with the list_empty() check above to save us from unnecessarily starting a transaction.

Also I was wondering whether it would not make more sense have this orphan handling bit also in ext4_handle_inode_extension(). ext4_dax_write_iter() doesn't strictly need it (as for DAX i_disksize cannot currently change while ext4_dax_write_iter() is running) but it would look more robust to me for the future users and it certainly doesn't hurt ext4_dax_write_iter() case.

Quoted 15 lines · Jan Kara

Quoted 15 lines

fs/ext4/file.c +15 -0

+	if (extend) {
+		ret = ext4_handle_inode_extension(inode, ret, offset, count);
+
+		/*
+		 * We may have failed to remove the inode from the orphan list
+		 * in the case that the i_disksize got update due to delalloc
+		 * writeback while the direct I/O was running. We need to make
+		 * sure we remove it from the orphan list as if we've
+		 * prematurely popped it onto the list.
+		 */
+		if (!list_empty(&EXT4_I(inode)->i_orphan)) {
+			handle = ext4_journal_start(inode, EXT4_HT_INODE, 2);
+			if (IS_ERR(handle)) {
+				ret = PTR_ERR(handle);
+				if (inode->i_nlink)

Quoted 8 lines

fs/ext4/file.c static ssize_t ext4_handle_inode_extension(struct inode *inode, ssize_t written, · line 432

Quoted 5 lines

fs/ext4/file.c +5 -0

+					ext4_orphan_del(NULL, inode);
+				goto out;
+			}
+
+			if (inode->i_nlink)

This check can be joined with the list_empty() check above to save us from unnecessarily starting a transaction.

Yes, easy done.

Quoted 7 lines

Also I was wondering whether it would not make more sense have this orphan handling bit also in ext4_handle_inode_extension(). ext4_dax_write_iter() doesn't strictly need it (as for DAX i_disksize cannot currently change while ext4_dax_write_iter() is running) but it would look more robust to me for the future users and it certainly doesn't hurt ext4_dax_write_iter() case.

I was thinking the same, but to be honest I wasn't entirely sure how it would pan out for the DAX code path. However, seeing as though you don't forsee there being any problems, then I can't really think of a reason not to roll this up into ext4_handle_inode_extension().

So, in ext4_handle_inode_extension() for the initial check against i_disksize, rather than returning 'written' and then having ext4_dio_write_iter() perform the cleanup, we could simply jump to a chunk of code in ext4_handle_inode_extension() and deal with it there, or quite literally just cleanup if that branch is taken there and then seeing as though it's not really needed in any other case? What do you think?

Hi Matthew, thanks for your work on this patch series!

I applied it against 4c3, and ran a quick test run on it, and found the following locking problem. To reproduce:

kvm-xfstests -c nojournal generic/113

generic/113 [09:27:19][ 5.841937] run fstests generic/113 at 2019-10-21 09:27:19

[    7.959477] 
[    7.959798] ============================================
[    7.960518] WARNING: possible recursive locking detected
[    7.961225] 5.4.0-rc3-xfstests-00012-g7fe6ea084e48 #1238 Not tainted
[    7.961991] --------------------------------------------
[    7.962569] aio-stress/1516 is trying to acquire lock:
[    7.963129] ffff9fd4791148c8 (&sb->s_type->i_mutex_key#12){++++}, at: __generic_file_fsync+0x3e/0xb0
[    7.964109] 
[    7.964109] but task is already holding lock:
[    7.964740] ffff9fd4791148c8 (&sb->s_type->i_mutex_key#12){++++}, at: ext4_dio_write_iter+0x15b/0x430

Quoted 8 lines · Theodore Y. Ts'o

Hi Matthew, thanks for your work on this patch series!

I applied it against 4c3, and ran a quick test run on it, and found the following locking problem. To reproduce:

kvm-xfstests -c nojournal generic/113

generic/113 [09:27:19][ 5.841937] run fstests generic/113 at 2019-10-21 09:27:19

Quoted 10 lines

[    7.959477] 
[    7.959798] ============================================
[    7.960518] WARNING: possible recursive locking detected
[    7.961225] 5.4.0-rc3-xfstests-00012-g7fe6ea084e48 #1238 Not tainted
[    7.961991] --------------------------------------------
[    7.962569] aio-stress/1516 is trying to acquire lock:
[    7.963129] ffff9fd4791148c8 (&sb->s_type->i_mutex_key#12){++++}, at: __generic_file_fsync+0x3e/0xb0
[    7.964109] 
[    7.964109] but task is already holding lock:
[    7.964740] ffff9fd4791148c8 (&sb->s_type->i_mutex_key#12){++++}, at: ext4_dio_write_iter+0x15b/0x430

This is going to be a tricky one. With iomap, the inode locking is handled by the filesystem while calling generic_write_sync() is done by iomap_dio_rw(). I would really prefer to avoid tweaking iomap_dio_rw() not to call generic_write_sync().

So we need to remove inode_lock from
__generic_file_fsync() (used from ext4_sync_file()). This locking is mostly for legacy purposes and we don't need this in ext4 AFAICT - but removing the lock from __generic_file_fsync() would mean auditing all legacy filesystems that use this to make sure flushing inode & its metadata buffer list while it is possibly changing cannot result in something unexpected. I don't want to clutter this series with it so we are left with reimplementing __generic_file_fsync() inside ext4 without inode_lock. Not too bad but not great either. Thoughts?

Quoted 20 lines · Jan Kara

Quoted 8 lines · Theodore Y. Ts'o

Hi Matthew, thanks for your work on this patch series!

I applied it against 4c3, and ran a quick test run on it, and found the following locking problem. To reproduce:

kvm-xfstests -c nojournal generic/113

generic/113 [09:27:19][ 5.841937] run fstests generic/113 at 2019-10-21 09:27:19

Quoted 10 lines

[    7.959477] 
[    7.959798] ============================================
[    7.960518] WARNING: possible recursive locking detected
[    7.961225] 5.4.0-rc3-xfstests-00012-g7fe6ea084e48 #1238 Not tainted
[    7.961991] --------------------------------------------
[    7.962569] aio-stress/1516 is trying to acquire lock:
[    7.963129] ffff9fd4791148c8 (&sb->s_type->i_mutex_key#12){++++}, at: __generic_file_fsync+0x3e/0xb0
[    7.964109] 
[    7.964109] but task is already holding lock:
[    7.964740] ffff9fd4791148c8 (&sb->s_type->i_mutex_key#12){++++}, at: ext4_dio_write_iter+0x15b/0x430

Quoted 4 lines

This is going to be a tricky one. With iomap, the inode locking is handled by the filesystem while calling generic_write_sync() is done by iomap_dio_rw(). I would really prefer to avoid tweaking iomap_dio_rw() not to call generic_write_sync().

You can't remove it from there, because that will break O_DSYNC AIO+DIO. i.e. generic_write_sync() needs to be called before iocb->ki_complete() is called in the AIO completion path, and that means filesystems using iomap_dio_rw need to be be able to run generic_write_sync() without taking the inode_lock().

Quoted 9 lines

So we need to remove inode_lock from
__generic_file_fsync() (used from ext4_sync_file()). This locking is mostly for legacy purposes and we don't need this in ext4 AFAICT - but removing the lock from __generic_file_fsync() would mean auditing all legacy filesystems that use this to make sure flushing inode & its metadata buffer list while it is possibly changing cannot result in something unexpected. I don't want to clutter this series with it so we are left with reimplementing __generic_file_fsync() inside ext4 without inode_lock. Not too bad but not great either. Thoughts?

XFS has implemented it's own ->fsync operation pretty much forever without issues. It's basically:

1. flush dirty cached data w/ WB_SYNC_ALL
2. flush dirty cached metadata (i.e. journal force)
3. flush device caches if journal force didn't, keeping in mind the requirements of data and journal being placed on different devices.

The ext4 variant shouldn't need to be any more complex than that...

Quoted 8 lines · Theodore Y. Ts'o

Hi Matthew, thanks for your work on this patch series!

I applied it against 4c3, and ran a quick test run on it, and found the following locking problem. To reproduce:

kvm-xfstests -c nojournal generic/113

generic/113 [09:27:19][ 5.841937] run fstests generic/113 at 2019-10-21 09:27:19

Quoted 10 lines

[    7.959477] 
[    7.959798] ============================================
[    7.960518] WARNING: possible recursive locking detected
[    7.961225] 5.4.0-rc3-xfstests-00012-g7fe6ea084e48 #1238 Not tainted
[    7.961991] --------------------------------------------
[    7.962569] aio-stress/1516 is trying to acquire lock:
[    7.963129] ffff9fd4791148c8 (&sb->s_type->i_mutex_key#12){++++}, at: __generic_file_fsync+0x3e/0xb0
[    7.964109] 
[    7.964109] but task is already holding lock:
[    7.964740] ffff9fd4791148c8 (&sb->s_type->i_mutex_key#12){++++}, at: ext4_dio_write_iter+0x15b/0x430

This is going to be a tricky one. With iomap, the inode locking is handled by the filesystem while calling generic_write_sync() is done by iomap_dio_rw(). I would really prefer to avoid tweaking iomap_dio_rw() not to call generic_write_sync(). So we need to remove inode_lock from __generic_file_fsync() (used from ext4_sync_file()). This locking is mostly for legacy purposes and we don't need this in ext4 AFAICT - but removing the lock from __generic_file_fsync() would mean auditing all legacy filesystems that use this to make sure flushing inode & its metadata buffer list while it is possibly changing cannot result in something unexpected. I don't want to clutter this series with it so we are left with reimplementing __generic_file_fsync() inside ext4 without inode_lock. Not too bad but not great either. Thoughts?

Quoted 20 lines · Jan Kara

Quoted 8 lines · Theodore Y. Ts'o

Hi Matthew, thanks for your work on this patch series!

I applied it against 4c3, and ran a quick test run on it, and found the following locking problem. To reproduce:

kvm-xfstests -c nojournal generic/113

generic/113 [09:27:19][ 5.841937] run fstests generic/113 at 2019-10-21 09:27:19

Quoted 10 lines

[    7.959477] 
[    7.959798] ============================================
[    7.960518] WARNING: possible recursive locking detected
[    7.961225] 5.4.0-rc3-xfstests-00012-g7fe6ea084e48 #1238 Not tainted
[    7.961991] --------------------------------------------
[    7.962569] aio-stress/1516 is trying to acquire lock:
[    7.963129] ffff9fd4791148c8 (&sb->s_type->i_mutex_key#12){++++}, at: __generic_file_fsync+0x3e/0xb0
[    7.964109] 
[    7.964109] but task is already holding lock:
[    7.964740] ffff9fd4791148c8 (&sb->s_type->i_mutex_key#12){++++}, at: ext4_dio_write_iter+0x15b/0x430

Quoted 12 lines

This is going to be a tricky one. With iomap, the inode locking is handled by the filesystem while calling generic_write_sync() is done by iomap_dio_rw(). I would really prefer to avoid tweaking iomap_dio_rw() not to call generic_write_sync(). So we need to remove inode_lock from __generic_file_fsync() (used from ext4_sync_file()). This locking is mostly for legacy purposes and we don't need this in ext4 AFAICT - but removing the lock from __generic_file_fsync() would mean auditing all legacy filesystems that use this to make sure flushing inode & its metadata buffer list while it is possibly changing cannot result in something unexpected. I don't want to clutter this series with it so we are left with reimplementing __generic_file_fsync() inside ext4 without inode_lock. Not too bad but not great either. Thoughts?

So, I just looked at this on my lunch break and I think the simplest approach would be to just transfer the necessary chunks of code from within __generic_file_fsync() into ext4_sync_file() for !journal cases, minus the inode lock, and minus calling into __generic_file_fsync(). I don't forsee this causing any issues, but feel free to correct me if I'm wrong.

If this is deemed to be OK, then I will go ahead and include this as a separate patch in my series.

Quoted 34 lines · Matthew Bobrowski

Quoted 20 lines · Jan Kara

Quoted 8 lines · Theodore Y. Ts'o

Hi Matthew, thanks for your work on this patch series!

I applied it against 4c3, and ran a quick test run on it, and found the following locking problem. To reproduce:

kvm-xfstests -c nojournal generic/113

generic/113 [09:27:19][ 5.841937] run fstests generic/113 at 2019-10-21 09:27:19

Quoted 10 lines

[    7.959477] 
[    7.959798] ============================================
[    7.960518] WARNING: possible recursive locking detected
[    7.961225] 5.4.0-rc3-xfstests-00012-g7fe6ea084e48 #1238 Not tainted
[    7.961991] --------------------------------------------
[    7.962569] aio-stress/1516 is trying to acquire lock:
[    7.963129] ffff9fd4791148c8 (&sb->s_type->i_mutex_key#12){++++}, at: __generic_file_fsync+0x3e/0xb0
[    7.964109] 
[    7.964109] but task is already holding lock:
[    7.964740] ffff9fd4791148c8 (&sb->s_type->i_mutex_key#12){++++}, at: ext4_dio_write_iter+0x15b/0x430

Quoted 12 lines

This is going to be a tricky one. With iomap, the inode locking is handled by the filesystem while calling generic_write_sync() is done by iomap_dio_rw(). I would really prefer to avoid tweaking iomap_dio_rw() not to call generic_write_sync(). So we need to remove inode_lock from __generic_file_fsync() (used from ext4_sync_file()). This locking is mostly for legacy purposes and we don't need this in ext4 AFAICT - but removing the lock from __generic_file_fsync() would mean auditing all legacy filesystems that use this to make sure flushing inode & its metadata buffer list while it is possibly changing cannot result in something unexpected. I don't want to clutter this series with it so we are left with reimplementing __generic_file_fsync() inside ext4 without inode_lock. Not too bad but not great either. Thoughts?

Quoted 6 lines

So, I just looked at this on my lunch break and I think the simplest approach would be to just transfer the necessary chunks of code from within __generic_file_fsync() into ext4_sync_file() for !journal cases, minus the inode lock, and minus calling into __generic_file_fsync(). I don't forsee this causing any issues, but feel free to correct me if I'm wrong.

Yes, that's what I'd suggest as well. In fact when doing that you can share file_write_and_wait_range() call with the one already in ext4_sync_file() use for other cases. Similarly with file_check_and_advance_wb_err(). So the copied bit will be really only:

        ret = sync_mapping_buffers(inode->i_mapping);
        if (!(inode->i_state & I_DIRTY_ALL))
                goto out;
        if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
                goto out;

        err = sync_inode_metadata(inode, 1);
        if (ret == 0)
                ret = err;

Quoted 2 lines

If this is deemed to be OK, then I will go ahead and include this as a separate patch in my series.

Yes, please.

Maybe something like the version below which declutter the thing a bit?