delalloc and reflink fixes & tweaks

From: Dave Chinner <[email protected]>

When xfs_reflink_allocate_cow() allocates a transaction, it drops the ILOCK to perform the operation. This Introduces a race condition where another thread modifying the file can perform the COW allocation operation underneath us. This result in the retry loop finding an allocated block and jumping straight to the conversion code. It does not, however, cancel the transaction it holds and so this gets leaked. This results in a lockdep warning:

================================================
WARNING: lock held when returning to user space!
4.18.5 #1 Not tainted
------------------------------------------------
worker/6123 is leaving the kernel with locks still held!
1 lock held by worker/6123:
#0: 000000009eab4f1b (sb_internal#2){.+.+}, at: xfs_trans_alloc+0x17c/0x220

And eventually the filesystem deadlocks because it runs out of log space that is reserved by the leaked transaction and never gets released.

The logic flow in xfs_reflink_allocate_cow() is a convoluted mess of gotos - it's no surprise that it has bug where the flow through several goto jumps then fails to clean up context from a non-obvious logic path. CLean up the logic flow and make sure every path does the right thing.

Reported-by Alexander Y. Fomichev <[email protected]>

Tested-by Alexander Y. Fomichev <[email protected]>

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

Signed-off-by Dave Chinner <[email protected]>

[hch: slight refactor]

Signed-off-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_reflink.c | 127 ++++++++++++++++++++++++++-----------------
 1 file changed, 77 insertions(+), 50 deletions(-)

fs/xfs/xfs_reflink.c +77 -50

index 38f405415b88..d60d0eeed7b9 100644
@@ -352,6 +352,47 @@ xfs_reflink_convert_cow(
 	return error;
 }
 
+/*
+ * Find the extent that maps the given range in the COW fork. Even if the extent
+ * is not shared we might have a preallocation for it in the COW fork. If so we
+ * use it that rather than trigger a new allocation.
+ */
+static int
+xfs_find_trim_cow_extent(
+	struct xfs_inode	*ip,
+	struct xfs_bmbt_irec	*imap,
+	bool			*shared,
+	bool			*found)
+{
+	xfs_fileoff_t		offset_fsb = imap->br_startoff;
+	xfs_filblks_t		count_fsb = imap->br_blockcount;
+	struct xfs_iext_cursor	icur;
+	struct xfs_bmbt_irec	got;
+	bool			trimmed;
+
+	*found = false;
+
+	/*
+	 * If we don't find an overlapping extent, trim the range we need to
+	 * allocate to fit the hole we found.
+	 */
+	if (!xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, &icur, &got) ||
+	    got.br_startoff > offset_fsb)
+		return xfs_reflink_trim_around_shared(ip, imap, shared, &trimmed);
+
+	*shared = true;
+	if (isnullstartblock(got.br_startblock)) {
+		xfs_trim_extent(imap, got.br_startoff, got.br_blockcount);
+		return 0;
+	}
+
+	/* real extent found - no need to allocate */
+	xfs_trim_extent(&got, offset_fsb, count_fsb);
+	*imap = got;
+	*found = true;
+	return 0;
+}
+
 /* Allocate all CoW reservations covering a range of blocks in a file. */
 int
 xfs_reflink_allocate_cow(

@@ -363,78 +404,64 @@ xfs_reflink_allocate_cow(
 	struct xfs_mount	*mp = ip->i_mount;
 	xfs_fileoff_t		offset_fsb = imap->br_startoff;
 	xfs_filblks_t		count_fsb = imap->br_blockcount;
-	struct xfs_bmbt_irec	got;
-	struct xfs_trans	*tp = NULL;
+	struct xfs_trans	*tp;
 	int			nimaps, error = 0;
-	bool			trimmed;
+	bool			found;
 	xfs_filblks_t		resaligned;
 	xfs_extlen_t		resblks = 0;
-	struct xfs_iext_cursor	icur;
 
-retry:
-	ASSERT(xfs_is_reflink_inode(ip));
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_reflink_inode(ip));
 
-	/*
-	 * Even if the extent is not shared we might have a preallocation for
-	 * it in the COW fork.  If so use it.
-	 */
-	if (xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, &icur, &got) &&
-	    got.br_startoff <= offset_fsb) {
-		*shared = true;
-
-		/* If we have a real allocation in the COW fork we're done. */
-		if (!isnullstartblock(got.br_startblock)) {
-			xfs_trim_extent(&got, offset_fsb, count_fsb);
-			*imap = got;
-			goto convert;
-		}
+	error = xfs_find_trim_cow_extent(ip, imap, shared, &found);
+	if (error || !*shared)
+		return error;
+	if (found)
+		goto convert;
 
-		xfs_trim_extent(imap, got.br_startoff, got.br_blockcount);
-	} else {
-		error = xfs_reflink_trim_around_shared(ip, imap, shared, &trimmed);
-		if (error || !*shared)
-			goto out;
-	}
+	resaligned = xfs_aligned_fsb_count(imap->br_startoff,
+		imap->br_blockcount, xfs_get_cowextsz_hint(ip));
+	resblks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
 
-	if (!tp) {
-		resaligned = xfs_aligned_fsb_count(imap->br_startoff,
-			imap->br_blockcount, xfs_get_cowextsz_hint(ip));
-		resblks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
+	xfs_iunlock(ip, *lockmode);
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0, &tp);
+	*lockmode = XFS_ILOCK_EXCL;
+	xfs_ilock(ip, *lockmode);
 
-		xfs_iunlock(ip, *lockmode);
-		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0, &tp);
-		*lockmode = XFS_ILOCK_EXCL;
-		xfs_ilock(ip, *lockmode);
+	if (error)
+		return error;
 
-		if (error)
-			return error;
+	error = xfs_qm_dqattach_locked(ip, false);
+	if (error)
+		goto out_trans_cancel;
 
-		error = xfs_qm_dqattach_locked(ip, false);
-		if (error)
-			goto out;
-		goto retry;
+	/*
+	 * Check for an overlapping extent again now that we dropped the ilock.
+	 */
+	error = xfs_find_trim_cow_extent(ip, imap, shared, &found);
+	if (error || !*shared)
+		goto out_trans_cancel;
+	if (found) {
+		xfs_trans_cancel(tp);
+		goto convert;
 	}
 
 	error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0,
 			XFS_QMOPT_RES_REGBLKS);
 	if (error)
-		goto out;
+		goto out_trans_cancel;
 
 	xfs_trans_ijoin(tp, ip, 0);
 
-	nimaps = 1;
-
 	/* Allocate the entire reservation as unwritten blocks. */
+	nimaps = 1;
 	error = xfs_bmapi_write(tp, ip, imap->br_startoff, imap->br_blockcount,
 			XFS_BMAPI_COWFORK | XFS_BMAPI_PREALLOC,
 			resblks, imap, &nimaps);
 	if (error)
-		goto out_trans_cancel;
+		goto out_unreserve;
 
 	xfs_inode_set_cowblocks_tag(ip);
-
-	/* Finish up. */
 	error = xfs_trans_commit(tp);
 	if (error)
 		return error;

@@ -447,12 +474,12 @@ xfs_reflink_allocate_cow(
 		return -ENOSPC;
 convert:
 	return xfs_reflink_convert_cow_extent(ip, imap, offset_fsb, count_fsb);
-out_trans_cancel:
+
+out_unreserve:
 	xfs_trans_unreserve_quota_nblks(tp, ip, (long)resblks, 0,
 			XFS_QMOPT_RES_REGBLKS);
-out:
-	if (tp)
-		xfs_trans_cancel(tp);
+out_trans_cancel:
+	xfs_trans_cancel(tp);
 	return error;
 }
 

Signature · 1 line

2.18.0

This function is only used to punch out delayed allocations on I/O failure, which means we need to have read the extents earlier.

Signed-off-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_bmap_util.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

fs/xfs/xfs_bmap_util.c +2 -6

index addbd74ecd8e..418e1e725f3a 100644
@@ -702,13 +702,9 @@ xfs_bmap_punch_delalloc_range(
 	struct xfs_iext_cursor	icur;
 	int			error = 0;
 
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
-		error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK);
-		if (error)
-			goto out_unlock;
-	}
+	ASSERT(ifp->if_flags & XFS_IFEXTENTS);
 
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	if (!xfs_iext_lookup_extent_before(ip, ifp, &end_fsb, &icur, &got))
 		goto out_unlock;
 

Signature · 1 line

2.18.0

The invalid state isn't any different from a hole, so merge the two states. Use the more descriptive hole name, but keep it as the first value of the enum to catch uninitialized fields.

Signed-off-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_aops.c |  4 ++--
 fs/xfs/xfs_aops.h | 14 ++++++--------
 2 files changed, 8 insertions(+), 10 deletions(-)

fs/xfs/xfs_aops.c +0 -0

index 49f5f5896a43..338b9d9984e0 100644

fs/xfs/xfs_aops.c +1 -1

 	};
 	int			ret;
 

@@ -933,7 +933,7 @@ xfs_vm_writepages(
 	struct writeback_control *wbc)
 {
 	struct xfs_writepage_ctx wpc = {
-		.io_type = XFS_IO_INVALID,
+		.io_type = XFS_IO_HOLE,
 	};
 	int			ret;
 

fs/xfs/xfs_aops.h +6 -8

index 9af867951a10..494b4338446e 100644
@@ -12,21 +12,19 @@ extern struct bio_set xfs_ioend_bioset;
  * Types of I/O for bmap clustering and I/O completion tracking.
  */
 enum {
-	XFS_IO_INVALID,		/* initial state */
+	XFS_IO_HOLE,		/* covers region without any block allocation */
 	XFS_IO_DELALLOC,	/* covers delalloc region */
 	XFS_IO_UNWRITTEN,	/* covers allocated but uninitialized data */
 	XFS_IO_OVERWRITE,	/* covers already allocated extent */
 	XFS_IO_COW,		/* covers copy-on-write extent */
-	XFS_IO_HOLE,		/* covers region without any block allocation */
 };
 
 #define XFS_IO_TYPES \
-	{ XFS_IO_INVALID,		"invalid" }, \
-	{ XFS_IO_DELALLOC,		"delalloc" }, \
-	{ XFS_IO_UNWRITTEN,		"unwritten" }, \
-	{ XFS_IO_OVERWRITE,		"overwrite" }, \
-	{ XFS_IO_COW,			"CoW" }, \
-	{ XFS_IO_HOLE,			"hole" }
+	{ XFS_IO_HOLE,			"hole" },	\
+	{ XFS_IO_DELALLOC,		"delalloc" },	\
+	{ XFS_IO_UNWRITTEN,		"unwritten" },	\
+	{ XFS_IO_OVERWRITE,		"overwrite" },	\
+	{ XFS_IO_COW,			"CoW" }
 
 /*
  * Structure for buffered I/O completions.

Signature · 1 line

2.18.0

fs/xfs/xfs_aops.c xfs_vm_writepage( · line 920

Quoted 4 lines

{

fs/xfs/xfs_aops.c +1 -1

 	struct xfs_writepage_ctx wpc = {
-		.io_type = XFS_IO_INVALID,
+		.io_type = XFS_IO_HOLE,

Hm. So I guess if we ever saw XFS_IO_INVALID that meant "we never did find any extents and so never set io_type", right?

Merge the two cases for reflink vs not into a single one.

Signed-off-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_iomap.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

fs/xfs/xfs_iomap.c +9 -9

index 6320aca39f39..9595a3c59ade 100644
@@ -1048,16 +1048,20 @@ xfs_file_iomap_begin(
 	if (!(flags & (IOMAP_WRITE | IOMAP_ZERO)))
 		goto out_found;
 
+	/*
+	 * Don't need to allocate over holes when doing zeroing operations,
+	 * unless we need to COW and have an existing extent.
+	 */
+	if ((flags & IOMAP_ZERO) &&
+	    (!xfs_is_reflink_inode(ip) ||
+	     !needs_cow_for_zeroing(&imap, nimaps)))
+		goto out_found;
+
 	/*
 	 * Break shared extents if necessary. Checks for non-blocking IO have
 	 * been done up front, so we don't need to do them here.
 	 */
 	if (xfs_is_reflink_inode(ip)) {
-		/* if zeroing doesn't need COW allocation, then we are done. */
-		if ((flags & IOMAP_ZERO) &&
-		    !needs_cow_for_zeroing(&imap, nimaps))
-			goto out_found;
-
 		if (flags & IOMAP_DIRECT) {
 			/* may drop and re-acquire the ilock */
 			error = xfs_reflink_allocate_cow(ip, &imap, &shared,

@@ -1074,10 +1078,6 @@ xfs_file_iomap_begin(
 		length = XFS_FSB_TO_B(mp, end_fsb) - offset;
 	}
 
-	/* Don't need to allocate over holes when doing zeroing operations. */
-	if (flags & IOMAP_ZERO)
-		goto out_found;
-
 	if (!imap_needs_alloc(inode, &imap, nimaps))
 		goto out_found;
 

Signature · 1 line

2.18.0

Merge the two cases for reflink vs not into a single one.

Signed-off-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_iomap.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

fs/xfs/xfs_iomap.c +9 -5

index 6320aca39f39..9595a3c59ade 100644
@@ -1048,16 +1048,20 @@ xfs_file_iomap_begin(
 	if (!(flags & (IOMAP_WRITE | IOMAP_ZERO)))
 		goto out_found;
 
+	/*
+	 * Don't need to allocate over holes when doing zeroing operations,
+	 * unless we need to COW and have an existing extent.
+	 */
+	if ((flags & IOMAP_ZERO) &&
+	    (!xfs_is_reflink_inode(ip) ||
+	     !needs_cow_for_zeroing(&imap, nimaps)))
+		goto out_found;
+
 	/*
 	 * Break shared extents if necessary. Checks for non-blocking IO have
 	 * been done up front, so we don't need to do them here.
 	 */
 	if (xfs_is_reflink_inode(ip)) {
-		/* if zeroing doesn't need COW allocation, then we are done. */
-		if ((flags & IOMAP_ZERO) &&
-		    !needs_cow_for_zeroing(&imap, nimaps))
-			goto out_found;
-
 		if (flags & IOMAP_DIRECT) {
 			/* may drop and re-acquire the ilock */
 			error = xfs_reflink_allocate_cow(ip, &imap, &shared,

fs/xfs/xfs_iomap.c +0 -0

 	if (!imap_needs_alloc(inode, &imap, nimaps))
 		goto out_found;
 

Signature · 1 line

2.18.0

With this change, it looks like that scenario plays out the same until we get to imap_needs_alloc(), which returns true and brings us to do an allocation... I guess this changes a bit in the follow on patches, but the IOMAP_ZERO check that remains still makes the logic look funny. Should we ever get here with IOMAP_ZERO after the final patch to switch to separate ops?

For files with extent size hints we always allocate unwritten extents first and then convert them to real extents later to avoid exposing stale data outside the blocks actually written. But there is no reason we can't do unwritten extent allocations from delalloc blocks, in fact we already do that for COW operations.

Note that this does not handle files on the RT subvolume (yet), as we removed code handling RT allocations from the delalloc path this would be a slightly bigger project, and it doesn't really help with simplifying the COW path either.

Signed-off-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_iomap.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

fs/xfs/xfs_iomap.c +0 -1

index e12ff5e9a8ec..2d08ace09e25 100644
@@ -533,7 +533,6 @@ xfs_file_iomap_begin_delay(
 	xfs_fsblock_t		prealloc_blocks = 0;
 
 	ASSERT(!XFS_IS_REALTIME_INODE(ip));
-	ASSERT(!xfs_get_extsz_hint(ip));
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 

fs/xfs/xfs_iomap.c +3 -11

 		/* Reserve delalloc blocks for regular writeback. */
 		return xfs_file_iomap_begin_delay(inode, offset, length, flags,
 				iomap);

@@ -1088,17 +1087,9 @@ xfs_file_iomap_begin(
 	 * been done up front, so we don't need to do them here.
 	 */
 	if (xfs_is_reflink_inode(ip)) {
-		if (flags & IOMAP_DIRECT) {
-			/* may drop and re-acquire the ilock */
-			error = xfs_reflink_allocate_cow(ip, &imap, &shared,
-					&lockmode);
-			if (error)
-				goto out_unlock;
-		} else {
-			error = xfs_reflink_reserve_cow(ip, &imap, &shared);
-			if (error)
-				goto out_unlock;
-		}
+		error = xfs_reflink_allocate_cow(ip, &imap, &shared, &lockmode);
+		if (error)
+			goto out_unlock;
 
 		end_fsb = imap.br_startoff + imap.br_blockcount;
 		length = XFS_FSB_TO_B(mp, end_fsb) - offset;

Signature · 1 line

2.18.0

I'd guess this is due to the extent overlap check in xfs_bmap_extsize_align(), but I haven't verified.

Introduce a new xfs_delalloc_iomap_ops instance that is passed to iomap_apply when we are doing delayed allocations. This keeps the code more neatly separated for future work.

Signed-off-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_bmap_util.c |   3 +-
 fs/xfs/xfs_file.c      |   6 +-
 fs/xfs/xfs_iomap.c     | 177 ++++++++++++++++++++---------------------
 fs/xfs/xfs_iomap.h     |   1 +
 fs/xfs/xfs_iops.c      |   4 +-
 fs/xfs/xfs_reflink.c   |   2 +-
 6 files changed, 95 insertions(+), 98 deletions(-)

fs/xfs/xfs_iomap.c +1 -0

index 9079196bbc35..1bfc40ce581a 100644
@@ -512,8 +512,16 @@ xfs_iomap_prealloc_size(
 	return alloc_blocks;
 }
 
+static int xfs_file_iomap_begin(struct inode *inode, loff_t offset,

fs/xfs/xfs_iomap.c +0 -0

 	struct inode		*inode,
 	loff_t			offset,
 	loff_t			count,

fs/xfs/xfs_iomap.c +7 -0

@@ -658,6 +671,73 @@ xfs_file_iomap_begin_delay(
 	return error;
 }
 
+static int
+xfs_delalloc_iomap_end(
+	struct inode		*inode,
+	loff_t			offset,
+	loff_t			length,
+	ssize_t			written,
+	unsigned		flags,

fs/xfs/xfs_iomap.c +54 -89

+
+	/*
+	 * Behave as if the write failed if drop writes is enabled. Set the NEW
+	 * flag to force delalloc cleanup.
+	 */
+	if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_DROP_WRITES)) {
+		iomap->flags |= IOMAP_F_NEW;
+		written = 0;
+	}
+
+	/*
+	 * start_fsb refers to the first unused block after a short write. If
+	 * nothing was written, round offset down to point at the first block in
+	 * the range.
+	 */
+	if (unlikely(!written))
+		start_fsb = XFS_B_TO_FSBT(mp, offset);
+	else
+		start_fsb = XFS_B_TO_FSB(mp, offset + written);
+	end_fsb = XFS_B_TO_FSB(mp, offset + length);
+
+	/*
+	 * Trim delalloc blocks if they were allocated by this write and we
+	 * didn't manage to write the whole range.
+	 *
+	 * We don't need to care about racing delalloc as we hold i_mutex
+	 * across the reserve/allocate/unreserve calls. If there are delalloc
+	 * blocks in the range, they are ours.
+	 */
+	if ((iomap->flags & IOMAP_F_NEW) && start_fsb < end_fsb) {
+		truncate_pagecache_range(VFS_I(ip), XFS_FSB_TO_B(mp, start_fsb),
+					 XFS_FSB_TO_B(mp, end_fsb) - 1);
+
+		error = xfs_bmap_punch_delalloc_range(ip, start_fsb,
+					       end_fsb - start_fsb);
+		if (error && !XFS_FORCED_SHUTDOWN(mp)) {
+			xfs_alert(mp, "%s: unable to clean up ino %lld",
+				__func__, ip->i_ino);
+			return error;
+		}
+	}
+
+	return 0;
+}
+
+const struct iomap_ops xfs_delalloc_iomap_ops = {
+	.iomap_begin		= xfs_delalloc_iomap_begin,
+	.iomap_end		= xfs_delalloc_iomap_end,
+};
+
 /*
  * Pass in a delayed allocate extent, convert it to real extents;
  * return to the caller the extent we create which maps on top of

@@ -1028,16 +1108,13 @@ xfs_file_iomap_begin(
 	bool			shared = false;
 	unsigned		lockmode;
 
+	ASSERT(!(flags & (IOMAP_WRITE | IOMAP_ZERO)) ||
+		(flags & IOMAP_DIRECT) || IS_DAX(inode));
+	ASSERT(!(flags & IOMAP_ZERO) || XFS_IS_REALTIME_INODE(ip));
+
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
 
-	if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && !(flags & IOMAP_DIRECT) &&
-			!IS_DAX(inode) && !XFS_IS_REALTIME_INODE(ip)) {
-		/* Reserve delalloc blocks for regular writeback. */
-		return xfs_file_iomap_begin_delay(inode, offset, length, flags,
-				iomap);
-	}
-
 	/*
 	 * Lock the inode in the manner required for the specified operation and
 	 * check for as many conditions that would result in blocking as

@@ -1070,15 +1147,6 @@ xfs_file_iomap_begin(
 	if (!(flags & (IOMAP_WRITE | IOMAP_ZERO)))
 		goto out_found;
 
-	/*
-	 * Don't need to allocate over holes when doing zeroing operations,
-	 * unless we need to COW and have an existing extent.
-	 */
-	if ((flags & IOMAP_ZERO) &&
-	    (!xfs_is_reflink_inode(ip) ||
-	     !needs_cow_for_zeroing(&imap, nimaps)))
-		goto out_found;
-
 	/*
 	 * Break shared extents if necessary. Checks for non-blocking IO have
 	 * been done up front, so we don't need to do them here.

@@ -1148,81 +1216,8 @@ xfs_file_iomap_begin(
 	return error;
 }
 
-static int
-xfs_file_iomap_end_delalloc(
-	struct xfs_inode	*ip,
-	loff_t			offset,
-	loff_t			length,
-	ssize_t			written,
-	struct iomap		*iomap)
-{
-	struct xfs_mount	*mp = ip->i_mount;
-	xfs_fileoff_t		start_fsb;
-	xfs_fileoff_t		end_fsb;
-	int			error = 0;
-
-	/*
-	 * Behave as if the write failed if drop writes is enabled. Set the NEW
-	 * flag to force delalloc cleanup.
-	 */
-	if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_DROP_WRITES)) {
-		iomap->flags |= IOMAP_F_NEW;
-		written = 0;
-	}
-
-	/*
-	 * start_fsb refers to the first unused block after a short write. If
-	 * nothing was written, round offset down to point at the first block in
-	 * the range.
-	 */
-	if (unlikely(!written))
-		start_fsb = XFS_B_TO_FSBT(mp, offset);
-	else
-		start_fsb = XFS_B_TO_FSB(mp, offset + written);
-	end_fsb = XFS_B_TO_FSB(mp, offset + length);
-
-	/*
-	 * Trim delalloc blocks if they were allocated by this write and we
-	 * didn't manage to write the whole range.
-	 *
-	 * We don't need to care about racing delalloc as we hold i_mutex
-	 * across the reserve/allocate/unreserve calls. If there are delalloc
-	 * blocks in the range, they are ours.
-	 */
-	if ((iomap->flags & IOMAP_F_NEW) && start_fsb < end_fsb) {
-		truncate_pagecache_range(VFS_I(ip), XFS_FSB_TO_B(mp, start_fsb),
-					 XFS_FSB_TO_B(mp, end_fsb) - 1);
-
-		error = xfs_bmap_punch_delalloc_range(ip, start_fsb,
-					       end_fsb - start_fsb);
-		if (error && !XFS_FORCED_SHUTDOWN(mp)) {
-			xfs_alert(mp, "%s: unable to clean up ino %lld",
-				__func__, ip->i_ino);
-			return error;
-		}
-	}
-
-	return 0;
-}
-
-static int
-xfs_file_iomap_end(
-	struct inode		*inode,
-	loff_t			offset,
-	loff_t			length,
-	ssize_t			written,
-	unsigned		flags,
-	struct iomap		*iomap)
-{
-	if ((flags & IOMAP_WRITE) && iomap->type == IOMAP_DELALLOC)
-		return xfs_file_iomap_end_delalloc(XFS_I(inode), offset,
-				length, written, iomap);
-	return 0;
-}
-
 const struct iomap_ops xfs_iomap_ops = {
 	.iomap_begin		= xfs_file_iomap_begin,
-	.iomap_end		= xfs_file_iomap_end,
 };
 
 static int

fs/xfs/xfs_iomap.h +1 -0

index c6170548831b..fe4cde2c30c9 100644
@@ -42,6 +42,7 @@ xfs_aligned_fsb_count(
 }
 
 extern const struct iomap_ops xfs_iomap_ops;
+extern const struct iomap_ops xfs_delalloc_iomap_ops;
 extern const struct iomap_ops xfs_xattr_iomap_ops;
 
 #endif /* __XFS_IOMAP_H__*/

fs/xfs/xfs_iops.c +2 -2

index c3e74f9128e8..fb8035e3ba0b 100644
@@ -865,10 +865,10 @@ xfs_setattr_size(
 	if (newsize > oldsize) {
 		trace_xfs_zero_eof(ip, oldsize, newsize - oldsize);
 		error = iomap_zero_range(inode, oldsize, newsize - oldsize,
-				&did_zeroing, &xfs_iomap_ops);
+				&did_zeroing, &xfs_delalloc_iomap_ops);
 	} else {
 		error = iomap_truncate_page(inode, newsize, &did_zeroing,
-				&xfs_iomap_ops);
+				&xfs_delalloc_iomap_ops);
 	}
 
 	if (error)

fs/xfs/xfs_reflink.c +1 -1

index e0111d31140b..ac94ace45424 100644
@@ -1393,7 +1393,7 @@ xfs_reflink_dirty_extents(
 			if (fpos + flen > isize)
 				flen = isize - fpos;
 			error = iomap_file_dirty(VFS_I(ip), fpos, flen,
-					&xfs_iomap_ops);
+					&xfs_delalloc_iomap_ops);
 			xfs_ilock(ip, XFS_ILOCK_EXCL);
 			if (error)
 				goto out;

Signature · 1 line

2.18.0

fs/xfs/xfs_iomap.c xfs_iomap_prealloc_size( · line 524

Quoted 3 lines

static int

fs/xfs/xfs_iomap.c +1 -1

-xfs_file_iomap_begin_delay(
+xfs_delalloc_iomap_begin(

Ok, but wouldn't something like xfs_buffered_iomap_begin/end() be a better name? Technically a real extent may already exist in this codepath, so I find the delalloc name kind of confusing.

fs/xfs/xfs_iomap.c xfs_file_iomap_begin_delay( · line 690

Quoted 6 lines

fs/xfs/xfs_iomap.c +6 -0

+	xfs_fileoff_t		start_fsb;
+	xfs_fileoff_t		end_fsb;
+	int			error = 0;
+
+	if (iomap->type != IOMAP_DELALLOC)
+		return 0;

Any reason we don't continue to filter out !IOMAP_WRITE as well?

Hi all,

this series starts with a for-4.19 fix originally from Dave, and then has various tweaks, including a fix for a potential uninitialized data exposure that are 4.20 material.

Do you have any numbers that demonstrate the performance impact of the preallocation change? We've always intended to fix this exposure issue as you've done, I'm just interested in the sort of impact it has (if any).

Also, with this change to use unwritten extents for all delalloc extents, we can start doing speculative preallocation for writes into holes inside EOF without leaving uninitialised/unzeroed blocks laying around.

Using spec. prealloc for "extend-via-truncate, sequential write to fill hole" workloads would greatly benefit from that. Did you have any plans to look at that code?

Quoted 4 lines · Dave Chinner

Do you have any numbers that demonstrate the performance impact of the preallocation change? We've always intended to fix this exposure issue as you've done, I'm just interested in the sort of impact it has (if any).

For the absolute worst case - completely random 4k writes to a sparse file I see a slowdown of about 3%. Which is less than the improvement that we saw from removing buffer heads.

Quoted 4 lines

Also, with this change to use unwritten extents for all delalloc extents, we can start doing speculative preallocation for writes into holes inside EOF without leaving uninitialised/unzeroed blocks laying around.

Careful. We already have issues because delalloc blocks before EOF don't ever get reclaimed. This triggers up on xfs/442 with 1k blocksize for me. I actually have a fix for that now, but that will require dropping one of the cleanup patches from this series, so expect a respin.

If we want to more generic preallocation I guess we should follow the example of the COW direct I/O path and mark all preallocated extents as unwritten - that way we know delalloc extents that have the unwritten bit set can be safely reclaimed. But that is more work than I want to do for this merge window at least.

Quoted 4 lines

Careful. We already have issues because delalloc blocks before EOF don't ever get reclaimed. This triggers up on xfs/442 with 1k blocksize for me. I actually have a fix for that now, but that will require dropping one of the cleanup patches from this series, so expect a respin.

Yeah, I didn't say everything already worked, just that using preallocation for delalloc gets rid of the stale data exposure problem that has prevented us from doing this in the past.

That's sounds like a good idea, and a good direction to head towards. No immediate hurry, just trying to understand where you might be going with these changes.