Re: [PATCH v4 5/8] ext4: move inode extension/truncate code out from ->iomap_end() callback

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 10/3/19 5:04 PM, Matthew Bobrowski wrote:
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 <mbobrowski@xxxxxxxxxxxxxx>

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 <riteshh@xxxxxxxxxxxxx>


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

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 69ac042fb74b..2883711e8a33 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -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)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 159ffb92f82d..d616062b603e 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -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 = {





[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux