reflink COW improvements

reflink COW improvements

[Christoph Hellwig]

Hi all,

this series adds a few improvements to the direct I/O COW path.

Note that patch 2 at this point causes a regression in xfs/214:

@ -11,7 +11,7 @@
 CoW one of the files
 root      --       0       0       0              4     0     0       
 nobody    --       0       0       0              1     0     0       
-fsgqa     --    3520       0       0              4     0     0       
+fsgqa     --    3072       0       0              4     0     0       
 Remount the FS to see if accounting changes
 root      --       0       0       0              4     0     0       
 nobody    --       0       0       0              1     0     0 

But I think the test golden output actually is wrong - the test
claims it tests that we don't get blocks charged during the COW,
but the 3520 here means 512 bytes were charged, and later in the
golden output we actually see that going away after a remount.

[PATCH 1/3] xfs: reject all unaligned direct writes to reflinked files

[Christoph Hellwig]

We currently fall back from direct to buffered writes if we detect a
remaining shared extent in the iomap_begin callback.  But by the time
iomap_begin is called for the potentially unaligned end block we might
have already written most of the data to disk, which we'd now write
again using buffered I/O.  To avoid this reject all writes to reflinked
files before starting I/O so that we are guaranteed to only write the
data once.

The alternative would be to unshare the unaligned start and/or end block
before doing the I/O. I think that's doable, and will actually be
required to support reflinks on DAX file system.  But it will take a
little more time and I'd rather get rid of the double write ASAP.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_file.c  |  7 +++++++
 fs/xfs/xfs_iomap.c | 12 +-----------
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index f5effa6..873cd42 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -532,6 +532,13 @@ xfs_file_dio_aio_write(
 	if ((iocb->ki_pos & mp->m_blockmask) ||
 	    ((iocb->ki_pos + count) & mp->m_blockmask)) {
 		unaligned_io = 1;
+
+		/*
+		 * We can't properly handle unaligned direct I/O to reflink
+		 * files yet, as we can't unshare a partial block.
+		 */
+		if (xfs_is_reflink_inode(ip))
+			return -EREMCHG;
 		iolock = XFS_IOLOCK_EXCL;
 	} else {
 		iolock = XFS_IOLOCK_SHARED;
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 0d14742..78105db 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1026,17 +1026,7 @@ xfs_file_iomap_begin(
 		if (error)
 			goto out_unlock;
 
-		/*
-		 * We're here because we're trying to do a directio write to a
-		 * region that isn't aligned to a filesystem block.  If the
-		 * extent is shared, fall back to buffered mode to handle the
-		 * RMW.
-		 */
-		if (!(flags & IOMAP_REPORT) && shared) {
-			trace_xfs_reflink_bounce_dio_write(ip, &imap);
-			error = -EREMCHG;
-			goto out_unlock;
-		}
+		ASSERT((flags & IOMAP_REPORT) || !shared);
 	}
 
 	if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && xfs_is_reflink_inode(ip)) {
-- 
2.1.4

[PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Christoph Hellwig]

When we allocate COW fork blocks for direct I/O writes we currently first
create a delayed allocation, and then convert it to a real allocation
once we've got the delayed one.

As there is no good reason for that this patch instead makes use call
xfs_bmapi_write from the COW allocation path.  The only interesting bits
are a few tweaks the low-level allocator to allow for this, most notably
the need to remove the call to xfs_bmap_extsize_align for the cowextsize
in xfs_bmap_btalloc - for the existing convert case it's a no-op, but
for the direct allocation case it would blow up our block reservation
way beyond what we reserved for the transaction.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c |   9 ++---
 fs/xfs/xfs_reflink.c     | 101 ++++++++++++++++++++++++++++++++++-------------
 2 files changed, 76 insertions(+), 34 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 6f28814..a4a4327 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -2893,13 +2893,14 @@ xfs_bmap_add_extent_hole_real(
 	ASSERT(!isnullstartblock(new->br_startblock));
 	ASSERT(!bma->cur ||
 	       !(bma->cur->bc_private.b.flags & XFS_BTCUR_BPRV_WASDEL));
-	ASSERT(whichfork != XFS_COW_FORK);
 
 	XFS_STATS_INC(mp, xs_add_exlist);
 
 	state = 0;
 	if (whichfork == XFS_ATTR_FORK)
 		state |= BMAP_ATTRFORK;
+	if (whichfork == XFS_COW_FORK)
+		state |= BMAP_COWFORK;
 
 	/*
 	 * Check and set flags if this segment has a left neighbor.
@@ -3619,9 +3620,7 @@ xfs_bmap_btalloc(
 	else if (mp->m_dalign)
 		stripe_align = mp->m_dalign;
 
-	if (ap->flags & XFS_BMAPI_COWFORK)
-		align = xfs_get_cowextsz_hint(ap->ip);
-	else if (xfs_alloc_is_userdata(ap->datatype))
+	if (xfs_alloc_is_userdata(ap->datatype))
 		align = xfs_get_extsz_hint(ap->ip);
 	if (unlikely(align)) {
 		error = xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
@@ -4608,8 +4607,6 @@ xfs_bmapi_write(
 		 */
 		if (flags & XFS_BMAPI_REMAP)
 			ASSERT(inhole);
-		if (flags & XFS_BMAPI_COWFORK)
-			ASSERT(!inhole);
 
 		/*
 		 * First, deal with the hole before the allocated space
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 88fd03c..0a077e80 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -304,59 +304,104 @@ __xfs_reflink_allocate_cow(
 	xfs_fileoff_t		end_fsb)
 {
 	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_bmbt_irec	imap;
+	struct xfs_bmbt_irec	imap, got;
 	struct xfs_defer_ops	dfops;
 	struct xfs_trans	*tp;
 	xfs_fsblock_t		first_block;
-	int			nimaps = 1, error;
-	bool			shared;
+	int			nimaps, error, lockmode;
+	bool			shared, trimmed;
+	xfs_extlen_t		resblks, align;
+	xfs_extnum_t		idx;
 
-	xfs_defer_init(&dfops, &first_block);
+	align = xfs_eof_alignment(ip, xfs_get_cowextsz_hint(ip));
+	if (align)
+		end_fsb = roundup_64(end_fsb, align);
 
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, 0, 0,
-			XFS_TRANS_RESERVE, &tp);
+	resblks = XFS_DIOSTRAT_SPACE_RES(mp, end_fsb - *offset_fsb);
+
+	error = xfs_qm_dqattach(ip, 0);
 	if (error)
 		return error;
 
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-
-	/* Read extent from the source file. */
-	nimaps = 1;
-	error = xfs_bmapi_read(ip, *offset_fsb, end_fsb - *offset_fsb,
-			&imap, &nimaps, 0);
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0, &tp);
 	if (error)
-		goto out_unlock;
-	ASSERT(nimaps == 1);
+		return error;
 
-	error = xfs_reflink_reserve_cow(ip, &imap, &shared);
-	if (error)
-		goto out_trans_cancel;
+	lockmode = XFS_ILOCK_EXCL;
+	xfs_ilock(ip, lockmode);
 
-	if (!shared) {
-		*offset_fsb = imap.br_startoff + imap.br_blockcount;
-		goto out_trans_cancel;
+	/*
+	 * 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, &idx, &got) &&
+	    got.br_startoff <= *offset_fsb) {
+		/* If we have a real allocation in the COW fork we're done. */
+		if (!isnullstartblock(got.br_startblock)) {
+			xfs_trim_extent(&got, *offset_fsb,
+					end_fsb - *offset_fsb);
+			*offset_fsb = got.br_startoff + got.br_blockcount;
+			goto out_trans_cancel;
+		}
+	} else {
+		nimaps = 1;
+		error = xfs_bmapi_read(ip, *offset_fsb, end_fsb - *offset_fsb,
+				&imap, &nimaps, 0);
+		if (error)
+			goto out_trans_cancel;
+		ASSERT(nimaps == 1);
+
+		/* Trim the mapping to the nearest shared extent boundary. */
+		error = xfs_reflink_trim_around_shared(ip, &imap, &shared,
+				&trimmed);
+		if (error)
+			goto out_trans_cancel;
+
+		if (!shared) {
+			*offset_fsb = imap.br_startoff + imap.br_blockcount;
+			goto out_trans_cancel;
+		}
+
+		*offset_fsb = imap.br_startoff;
+		end_fsb = imap.br_startoff + imap.br_blockcount;
+		if (align)
+			end_fsb = roundup_64(end_fsb, align);
 	}
 
-	xfs_trans_ijoin(tp, ip, 0);
-	error = xfs_bmapi_write(tp, ip, imap.br_startoff, imap.br_blockcount,
-			XFS_BMAPI_COWFORK, &first_block,
-			XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK),
-			&imap, &nimaps, &dfops);
+	error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0,
+			XFS_QMOPT_RES_REGBLKS);
 	if (error)
 		goto out_trans_cancel;
 
+	xfs_trans_ijoin(tp, ip, 0);
+
+	xfs_defer_init(&dfops, &first_block);
+	nimaps = 1;
+	error = xfs_bmapi_write(tp, ip, *offset_fsb, end_fsb - *offset_fsb,
+			XFS_BMAPI_COWFORK, &first_block, resblks, &imap,
+			&nimaps, &dfops);
+	if (error)
+		goto out_bmap_cancel;
+
 	error = xfs_defer_finish(&tp, &dfops, NULL);
 	if (error)
-		goto out_trans_cancel;
+		goto out_bmap_cancel;
 
 	error = xfs_trans_commit(tp);
+	if (error)
+		goto out_unlock;
 
 	*offset_fsb = imap.br_startoff + imap.br_blockcount;
+
 out_unlock:
-	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	xfs_iunlock(ip, lockmode);
 	return error;
-out_trans_cancel:
+
+out_bmap_cancel:
 	xfs_defer_cancel(&dfops);
+	xfs_trans_unreserve_quota_nblks(tp, ip, (long)resblks, 0,
+			XFS_QMOPT_RES_REGBLKS);
+out_trans_cancel:
 	xfs_trans_cancel(tp);
 	goto out_unlock;
 }
-- 
2.1.4

[PATCH 3/3] xfs: allocate direct I/O COW blocks in iomap_begin

[Christoph Hellwig]

Instead of preallocating all the required COW blocks in the high-level
write code do it inside the iomap code, like we do for all other I/O.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_file.c    |   8 ---
 fs/xfs/xfs_iomap.c   |  31 ++++-------
 fs/xfs/xfs_reflink.c | 154 +++++++++++++++++++--------------------------------
 fs/xfs/xfs_reflink.h |   5 +-
 fs/xfs/xfs_trace.h   |   2 -
 5 files changed, 73 insertions(+), 127 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 873cd42..e6138f3 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -564,14 +564,6 @@ xfs_file_dio_aio_write(
 	}
 
 	trace_xfs_file_direct_write(ip, count, iocb->ki_pos);
-
-	/* If this is a block-aligned directio CoW, remap immediately. */
-	if (xfs_is_reflink_inode(ip) && !unaligned_io) {
-		ret = xfs_reflink_allocate_cow_range(ip, iocb->ki_pos, count);
-		if (ret)
-			goto out;
-	}
-
 	ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, xfs_dio_write_end_io);
 out:
 	xfs_iunlock(ip, iolock);
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 78105db..01c6e31 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1002,37 +1002,31 @@ xfs_file_iomap_begin(
 	offset_fsb = XFS_B_TO_FSBT(mp, offset);
 	end_fsb = XFS_B_TO_FSB(mp, offset + length);
 
-	if (xfs_is_reflink_inode(ip) &&
-	    (flags & IOMAP_WRITE) && (flags & IOMAP_DIRECT)) {
-		shared = xfs_reflink_find_cow_mapping(ip, offset, &imap);
-		if (shared) {
-			xfs_iunlock(ip, lockmode);
-			goto alloc_done;
-		}
-		ASSERT(!isnullstartblock(imap.br_startblock));
-	}
-
 	error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, &imap,
 			       &nimaps, 0);
 	if (error)
 		goto out_unlock;
 
-	if ((flags & IOMAP_REPORT) ||
-	    (xfs_is_reflink_inode(ip) &&
-	     (flags & IOMAP_WRITE) && (flags & IOMAP_DIRECT))) {
+	if (flags & IOMAP_REPORT) {
 		/* Trim the mapping to the nearest shared extent boundary. */
 		error = xfs_reflink_trim_around_shared(ip, &imap, &shared,
 				&trimmed);
 		if (error)
 			goto out_unlock;
-
-		ASSERT((flags & IOMAP_REPORT) || !shared);
 	}
 
 	if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && xfs_is_reflink_inode(ip)) {
-		error = xfs_reflink_reserve_cow(ip, &imap, &shared);
-		if (error)
-			goto out_unlock;
+		if (flags & IOMAP_DIRECT) {
+			/* may drop and re-acquire the ilock */
+			error = xfs_reflink_allocate_cow(ip, offset_fsb, end_fsb,
+					&imap, &shared, &lockmode);
+			if (error)
+				goto out_unlock;
+		} else {
+			error = xfs_reflink_reserve_cow(ip, &imap, &shared);
+			if (error)
+				goto out_unlock;
+		}
 
 		end_fsb = imap.br_startoff + imap.br_blockcount;
 		length = XFS_FSB_TO_B(mp, end_fsb) - offset;
@@ -1061,7 +1055,6 @@ xfs_file_iomap_begin(
 		if (error)
 			return error;
 
-alloc_done:
 		iomap->flags = IOMAP_F_NEW;
 		trace_xfs_iomap_alloc(ip, offset, length, 0, &imap);
 	} else {
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 0a077e80..2cddc9a 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -297,88 +297,92 @@ xfs_reflink_reserve_cow(
 }
 
 /* Allocate all CoW reservations covering a range of blocks in a file. */
-static int
-__xfs_reflink_allocate_cow(
+int
+xfs_reflink_allocate_cow(
 	struct xfs_inode	*ip,
-	xfs_fileoff_t		*offset_fsb,
-	xfs_fileoff_t		end_fsb)
+	xfs_fileoff_t		offset_fsb,
+	xfs_fileoff_t		end_fsb,
+	struct xfs_bmbt_irec	*imap,
+	bool			*shared,
+	uint			*lockmode)
 {
 	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_bmbt_irec	imap, got;
+	struct xfs_bmbt_irec	got;
 	struct xfs_defer_ops	dfops;
-	struct xfs_trans	*tp;
+	struct xfs_trans	*tp = NULL;
 	xfs_fsblock_t		first_block;
-	int			nimaps, error, lockmode;
-	bool			shared, trimmed;
+	int			nimaps, error = 0;
+	bool			trimmed;
 	xfs_extlen_t		resblks, align;
 	xfs_extnum_t		idx;
 
-	align = xfs_eof_alignment(ip, xfs_get_cowextsz_hint(ip));
-	if (align)
-		end_fsb = roundup_64(end_fsb, align);
-
-	resblks = XFS_DIOSTRAT_SPACE_RES(mp, end_fsb - *offset_fsb);
-
-	error = xfs_qm_dqattach(ip, 0);
-	if (error)
-		return error;
-
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0, &tp);
-	if (error)
-		return error;
-
-	lockmode = XFS_ILOCK_EXCL;
-	xfs_ilock(ip, lockmode);
+retry:
+	ASSERT(xfs_is_reflink_inode(ip));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
 
 	/*
 	 * 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, &idx, &got) &&
-	    got.br_startoff <= *offset_fsb) {
+	if (xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, &idx, &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,
-					end_fsb - *offset_fsb);
-			*offset_fsb = got.br_startoff + got.br_blockcount;
-			goto out_trans_cancel;
+			xfs_trim_extent(&got, offset_fsb, end_fsb - offset_fsb);
+			*imap = got;
+			goto out;
 		}
+
+		xfs_trim_extent(imap, got.br_startoff, got.br_blockcount);
 	} else {
-		nimaps = 1;
-		error = xfs_bmapi_read(ip, *offset_fsb, end_fsb - *offset_fsb,
-				&imap, &nimaps, 0);
+		error = xfs_reflink_trim_around_shared(ip, imap, shared, &trimmed);
+		if (error || !*shared)
+			goto out;
+	}
+
+	if (!tp) {
+		align = xfs_eof_alignment(ip, xfs_get_cowextsz_hint(ip));
+		if (align)
+			end_fsb = roundup_64(end_fsb, align);
+
+		resblks = XFS_DIOSTRAT_SPACE_RES(mp, end_fsb - offset_fsb);
+
+		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)
-			goto out_trans_cancel;
-		ASSERT(nimaps == 1);
+			return error;
 
-		/* Trim the mapping to the nearest shared extent boundary. */
-		error = xfs_reflink_trim_around_shared(ip, &imap, &shared,
-				&trimmed);
+		error = xfs_qm_dqattach_locked(ip, 0);
 		if (error)
-			goto out_trans_cancel;
+			goto out;
 
-		if (!shared) {
-			*offset_fsb = imap.br_startoff + imap.br_blockcount;
-			goto out_trans_cancel;
-		}
+		/* Read extent from the source file. */
+		nimaps = 1;
+		error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb,
+				imap, &nimaps, 0);
+		if (error)
+			goto out;
+		ASSERT(nimaps == 1);
 
-		*offset_fsb = imap.br_startoff;
-		end_fsb = imap.br_startoff + imap.br_blockcount;
-		if (align)
-			end_fsb = roundup_64(end_fsb, align);
+		goto retry;
 	}
 
 	error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0,
 			XFS_QMOPT_RES_REGBLKS);
 	if (error)
-		goto out_trans_cancel;
+		goto out;
 
 	xfs_trans_ijoin(tp, ip, 0);
 
 	xfs_defer_init(&dfops, &first_block);
 	nimaps = 1;
-	error = xfs_bmapi_write(tp, ip, *offset_fsb, end_fsb - *offset_fsb,
-			XFS_BMAPI_COWFORK, &first_block, resblks, &imap,
+	error = xfs_bmapi_write(tp, ip, imap->br_startoff, imap->br_blockcount,
+			XFS_BMAPI_COWFORK, &first_block, resblks, imap,
 			&nimaps, &dfops);
 	if (error)
 		goto out_bmap_cancel;
@@ -387,57 +391,15 @@ __xfs_reflink_allocate_cow(
 	if (error)
 		goto out_bmap_cancel;
 
-	error = xfs_trans_commit(tp);
-	if (error)
-		goto out_unlock;
-
-	*offset_fsb = imap.br_startoff + imap.br_blockcount;
-
-out_unlock:
-	xfs_iunlock(ip, lockmode);
-	return error;
+	return xfs_trans_commit(tp);
 
 out_bmap_cancel:
 	xfs_defer_cancel(&dfops);
 	xfs_trans_unreserve_quota_nblks(tp, ip, (long)resblks, 0,
 			XFS_QMOPT_RES_REGBLKS);
-out_trans_cancel:
-	xfs_trans_cancel(tp);
-	goto out_unlock;
-}
-
-/* Allocate all CoW reservations covering a part of a file. */
-int
-xfs_reflink_allocate_cow_range(
-	struct xfs_inode	*ip,
-	xfs_off_t		offset,
-	xfs_off_t		count)
-{
-	struct xfs_mount	*mp = ip->i_mount;
-	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
-	xfs_fileoff_t		end_fsb = XFS_B_TO_FSB(mp, offset + count);
-	int			error;
-
-	ASSERT(xfs_is_reflink_inode(ip));
-
-	trace_xfs_reflink_allocate_cow_range(ip, offset, count);
-
-	/*
-	 * Make sure that the dquots are there.
-	 */
-	error = xfs_qm_dqattach(ip, 0);
-	if (error)
-		return error;
-
-	while (offset_fsb < end_fsb) {
-		error = __xfs_reflink_allocate_cow(ip, &offset_fsb, end_fsb);
-		if (error) {
-			trace_xfs_reflink_allocate_cow_range_error(ip, error,
-					_RET_IP_);
-			break;
-		}
-	}
-
+out:
+	if (tp)
+		xfs_trans_cancel(tp);
 	return error;
 }
 
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index aa6a4d6..2813d49 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -28,8 +28,9 @@ extern int xfs_reflink_trim_around_shared(struct xfs_inode *ip,
 
 extern int xfs_reflink_reserve_cow(struct xfs_inode *ip,
 		struct xfs_bmbt_irec *imap, bool *shared);
-extern int xfs_reflink_allocate_cow_range(struct xfs_inode *ip,
-		xfs_off_t offset, xfs_off_t count);
+extern int xfs_reflink_allocate_cow(struct xfs_inode *ip,
+		xfs_fileoff_t offset_fsb, xfs_fileoff_t end_fsb,
+		struct xfs_bmbt_irec *imap, bool *shared, uint *lockmode);
 extern bool xfs_reflink_find_cow_mapping(struct xfs_inode *ip, xfs_off_t offset,
 		struct xfs_bmbt_irec *imap);
 extern void xfs_reflink_trim_irec_to_next_cow(struct xfs_inode *ip,
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 0907752..54aa1b2 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -3347,7 +3347,6 @@ DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_found);
 DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_enospc);
 
 DEFINE_RW_EVENT(xfs_reflink_reserve_cow);
-DEFINE_RW_EVENT(xfs_reflink_allocate_cow_range);
 
 DEFINE_INODE_IREC_EVENT(xfs_reflink_bounce_dio_write);
 DEFINE_IOMAP_EVENT(xfs_reflink_find_cow_mapping);
@@ -3357,7 +3356,6 @@ DEFINE_SIMPLE_IO_EVENT(xfs_reflink_cancel_cow_range);
 DEFINE_SIMPLE_IO_EVENT(xfs_reflink_end_cow);
 DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap);
 
-DEFINE_INODE_ERROR_EVENT(xfs_reflink_allocate_cow_range_error);
 DEFINE_INODE_ERROR_EVENT(xfs_reflink_cancel_cow_range_error);
 DEFINE_INODE_ERROR_EVENT(xfs_reflink_end_cow_error);
 
-- 
2.1.4

Re: reflink COW improvements

[Darrick J. Wong]

On Mon, Dec 05, 2016 at 10:05:21PM +0100, Christoph Hellwig wrote:
> Hi all,
> 
> this series adds a few improvements to the direct I/O COW path.
> 
> Note that patch 2 at this point causes a regression in xfs/214:
> 
> @ -11,7 +11,7 @@
>  CoW one of the files
>  root      --       0       0       0              4     0     0       
>  nobody    --       0       0       0              1     0     0       
> -fsgqa     --    3520       0       0              4     0     0       
> +fsgqa     --    3072       0       0              4     0     0       
>  Remount the FS to see if accounting changes
>  root      --       0       0       0              4     0     0       
>  nobody    --       0       0       0              1     0     0 
> 
> But I think the test golden output actually is wrong - the test
> claims it tests that we don't get blocks charged during the COW,
> but the 3520 here means 512 bytes were charged, and later in the
> golden output we actually see that going away after a remount.

I think it's correct for how the directio path used to work...

So xfs/214 writes a 1MB file and reflinks it twice, with the intent of
playing with file2.  So, file2 looks like this:

dddddddddddddddd (d = existing data, data fork fork)
                 (cow fork)

A 512K cowextsize hint is set and we write to the last 64K of one of
the files.  The old dio write code would make the usual delalloc
reservation, which rounds both up and down to the nearest cowextsize
boundary:

dddddddddddddddd (data fork)
        cccccccc (cow fork)

At this point, we should be charged for 3584K -- 1MB for each of the
three files, and 512K for the CoW delalloc.

Then we call fsync, which CoWs the last 64k out to disk.  Now we have:

dddddddddddddddD (D = replacement data, data fork)
        ccccccc  (cow fork)

The quota charge is now 3520K -- 1MB for each of the three files plus
the 448K that remains in the CoW fork anticipating a future write.
Remounting flushes out the cow fork:

dddddddddddddddD (data fork)
                 (cow fork)

So now the quota charge is back to an even 3072MB -- three 1MB files.

My suspicion is that since the new patchset bypasses the delalloc +
conversion dance and replaces it with allocating exactly as many blocks
as the dio write asked for + rounding up to the nearest cowextsize.  In
this case, we never make the delalloc reservation between 512K and 896K,
so the repquota output says 3072K instead of 3520K.

(That said, I've been tied up in meetings all day so I haven't had a
chance to apply the patches and find out for real.)


--D

Re: [PATCH 1/3] xfs: reject all unaligned direct writes to reflinked files

[Brian Foster]

On Mon, Dec 05, 2016 at 10:05:22PM +0100, Christoph Hellwig wrote:
> We currently fall back from direct to buffered writes if we detect a
> remaining shared extent in the iomap_begin callback.  But by the time
> iomap_begin is called for the potentially unaligned end block we might
> have already written most of the data to disk, which we'd now write
> again using buffered I/O.  To avoid this reject all writes to reflinked
> files before starting I/O so that we are guaranteed to only write the
> data once.
> 
> The alternative would be to unshare the unaligned start and/or end block
> before doing the I/O. I think that's doable, and will actually be
> required to support reflinks on DAX file system.  But it will take a
> little more time and I'd rather get rid of the double write ASAP.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_file.c  |  7 +++++++
>  fs/xfs/xfs_iomap.c | 12 +-----------
>  2 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index f5effa6..873cd42 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -532,6 +532,13 @@ xfs_file_dio_aio_write(
>  	if ((iocb->ki_pos & mp->m_blockmask) ||
>  	    ((iocb->ki_pos + count) & mp->m_blockmask)) {
>  		unaligned_io = 1;
> +
> +		/*
> +		 * We can't properly handle unaligned direct I/O to reflink
> +		 * files yet, as we can't unshare a partial block.
> +		 */
> +		if (xfs_is_reflink_inode(ip))
> +			return -EREMCHG;
>  		iolock = XFS_IOLOCK_EXCL;
>  	} else {
>  		iolock = XFS_IOLOCK_SHARED;
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 0d14742..78105db 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -1026,17 +1026,7 @@ xfs_file_iomap_begin(
>  		if (error)
>  			goto out_unlock;
>  
> -		/*
> -		 * We're here because we're trying to do a directio write to a
> -		 * region that isn't aligned to a filesystem block.  If the
> -		 * extent is shared, fall back to buffered mode to handle the
> -		 * RMW.
> -		 */
> -		if (!(flags & IOMAP_REPORT) && shared) {
> -			trace_xfs_reflink_bounce_dio_write(ip, &imap);
> -			error = -EREMCHG;
> -			goto out_unlock;
> -		}
> +		ASSERT((flags & IOMAP_REPORT) || !shared);
>  	}
>  
>  	if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && xfs_is_reflink_inode(ip)) {
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Brian Foster]

On Mon, Dec 05, 2016 at 10:05:23PM +0100, Christoph Hellwig wrote:
> When we allocate COW fork blocks for direct I/O writes we currently first
> create a delayed allocation, and then convert it to a real allocation
> once we've got the delayed one.
> 
> As there is no good reason for that this patch instead makes use call
> xfs_bmapi_write from the COW allocation path.  The only interesting bits
> are a few tweaks the low-level allocator to allow for this, most notably
> the need to remove the call to xfs_bmap_extsize_align for the cowextsize
> in xfs_bmap_btalloc - for the existing convert case it's a no-op, but
> for the direct allocation case it would blow up our block reservation
> way beyond what we reserved for the transaction.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c |   9 ++---
>  fs/xfs/xfs_reflink.c     | 101 ++++++++++++++++++++++++++++++++++-------------
>  2 files changed, 76 insertions(+), 34 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 6f28814..a4a4327 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -2893,13 +2893,14 @@ xfs_bmap_add_extent_hole_real(
>  	ASSERT(!isnullstartblock(new->br_startblock));
>  	ASSERT(!bma->cur ||
>  	       !(bma->cur->bc_private.b.flags & XFS_BTCUR_BPRV_WASDEL));
> -	ASSERT(whichfork != XFS_COW_FORK);
>  
>  	XFS_STATS_INC(mp, xs_add_exlist);
>  
>  	state = 0;
>  	if (whichfork == XFS_ATTR_FORK)
>  		state |= BMAP_ATTRFORK;
> +	if (whichfork == XFS_COW_FORK)
> +		state |= BMAP_COWFORK;
>  
>  	/*
>  	 * Check and set flags if this segment has a left neighbor.
> @@ -3619,9 +3620,7 @@ xfs_bmap_btalloc(
>  	else if (mp->m_dalign)
>  		stripe_align = mp->m_dalign;
>  
> -	if (ap->flags & XFS_BMAPI_COWFORK)
> -		align = xfs_get_cowextsz_hint(ap->ip);
> -	else if (xfs_alloc_is_userdata(ap->datatype))
> +	if (xfs_alloc_is_userdata(ap->datatype))

Doesn't this defeat the purpose (i.e., fragmentation avoidance via wider
allocations) of the cowextszhint for direct I/O? I think it would be
better to be consistent with the approach for traditional I/O + extsz
and incorporate the alignment into the reservation. Perhaps the hunk of
code that already does just that in xfs_iomap_write_direct() could be
converted to a small helper and reused..?

>  		align = xfs_get_extsz_hint(ap->ip);
>  	if (unlikely(align)) {
>  		error = xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
> @@ -4608,8 +4607,6 @@ xfs_bmapi_write(
>  		 */
>  		if (flags & XFS_BMAPI_REMAP)
>  			ASSERT(inhole);
> -		if (flags & XFS_BMAPI_COWFORK)
> -			ASSERT(!inhole);
>  
>  		/*
>  		 * First, deal with the hole before the allocated space
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 88fd03c..0a077e80 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -304,59 +304,104 @@ __xfs_reflink_allocate_cow(
>  	xfs_fileoff_t		end_fsb)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
> -	struct xfs_bmbt_irec	imap;
> +	struct xfs_bmbt_irec	imap, got;
>  	struct xfs_defer_ops	dfops;
>  	struct xfs_trans	*tp;
>  	xfs_fsblock_t		first_block;
> -	int			nimaps = 1, error;
> -	bool			shared;
> +	int			nimaps, error, lockmode;
> +	bool			shared, trimmed;
> +	xfs_extlen_t		resblks, align;
> +	xfs_extnum_t		idx;
>  
> -	xfs_defer_init(&dfops, &first_block);
> +	align = xfs_eof_alignment(ip, xfs_get_cowextsz_hint(ip));
> +	if (align)
> +		end_fsb = roundup_64(end_fsb, align);
>  
> -	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, 0, 0,
> -			XFS_TRANS_RESERVE, &tp);
> +	resblks = XFS_DIOSTRAT_SPACE_RES(mp, end_fsb - *offset_fsb);
> +
> +	error = xfs_qm_dqattach(ip, 0);

This is already in the (only) caller.

Brian

>  	if (error)
>  		return error;
>  
> -	xfs_ilock(ip, XFS_ILOCK_EXCL);
> -
> -	/* Read extent from the source file. */
> -	nimaps = 1;
> -	error = xfs_bmapi_read(ip, *offset_fsb, end_fsb - *offset_fsb,
> -			&imap, &nimaps, 0);
> +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0, &tp);
>  	if (error)
> -		goto out_unlock;
> -	ASSERT(nimaps == 1);
> +		return error;
>  
> -	error = xfs_reflink_reserve_cow(ip, &imap, &shared);
> -	if (error)
> -		goto out_trans_cancel;
> +	lockmode = XFS_ILOCK_EXCL;
> +	xfs_ilock(ip, lockmode);
>  
> -	if (!shared) {
> -		*offset_fsb = imap.br_startoff + imap.br_blockcount;
> -		goto out_trans_cancel;
> +	/*
> +	 * 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, &idx, &got) &&
> +	    got.br_startoff <= *offset_fsb) {
> +		/* If we have a real allocation in the COW fork we're done. */
> +		if (!isnullstartblock(got.br_startblock)) {
> +			xfs_trim_extent(&got, *offset_fsb,
> +					end_fsb - *offset_fsb);
> +			*offset_fsb = got.br_startoff + got.br_blockcount;
> +			goto out_trans_cancel;
> +		}
> +	} else {
> +		nimaps = 1;
> +		error = xfs_bmapi_read(ip, *offset_fsb, end_fsb - *offset_fsb,
> +				&imap, &nimaps, 0);
> +		if (error)
> +			goto out_trans_cancel;
> +		ASSERT(nimaps == 1);
> +
> +		/* Trim the mapping to the nearest shared extent boundary. */
> +		error = xfs_reflink_trim_around_shared(ip, &imap, &shared,
> +				&trimmed);
> +		if (error)
> +			goto out_trans_cancel;
> +
> +		if (!shared) {
> +			*offset_fsb = imap.br_startoff + imap.br_blockcount;
> +			goto out_trans_cancel;
> +		}
> +
> +		*offset_fsb = imap.br_startoff;
> +		end_fsb = imap.br_startoff + imap.br_blockcount;
> +		if (align)
> +			end_fsb = roundup_64(end_fsb, align);
>  	}
>  
> -	xfs_trans_ijoin(tp, ip, 0);
> -	error = xfs_bmapi_write(tp, ip, imap.br_startoff, imap.br_blockcount,
> -			XFS_BMAPI_COWFORK, &first_block,
> -			XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK),
> -			&imap, &nimaps, &dfops);
> +	error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0,
> +			XFS_QMOPT_RES_REGBLKS);
>  	if (error)
>  		goto out_trans_cancel;
>  
> +	xfs_trans_ijoin(tp, ip, 0);
> +
> +	xfs_defer_init(&dfops, &first_block);
> +	nimaps = 1;
> +	error = xfs_bmapi_write(tp, ip, *offset_fsb, end_fsb - *offset_fsb,
> +			XFS_BMAPI_COWFORK, &first_block, resblks, &imap,
> +			&nimaps, &dfops);
> +	if (error)
> +		goto out_bmap_cancel;
> +
>  	error = xfs_defer_finish(&tp, &dfops, NULL);
>  	if (error)
> -		goto out_trans_cancel;
> +		goto out_bmap_cancel;
>  
>  	error = xfs_trans_commit(tp);
> +	if (error)
> +		goto out_unlock;
>  
>  	*offset_fsb = imap.br_startoff + imap.br_blockcount;
> +
>  out_unlock:
> -	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +	xfs_iunlock(ip, lockmode);
>  	return error;
> -out_trans_cancel:
> +
> +out_bmap_cancel:
>  	xfs_defer_cancel(&dfops);
> +	xfs_trans_unreserve_quota_nblks(tp, ip, (long)resblks, 0,
> +			XFS_QMOPT_RES_REGBLKS);
> +out_trans_cancel:
>  	xfs_trans_cancel(tp);
>  	goto out_unlock;
>  }
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Christoph Hellwig]

On Wed, Dec 07, 2016 at 02:00:09PM -0500, Brian Foster wrote:
> > -	if (ap->flags & XFS_BMAPI_COWFORK)
> > -		align = xfs_get_cowextsz_hint(ap->ip);
> > -	else if (xfs_alloc_is_userdata(ap->datatype))
> > +	if (xfs_alloc_is_userdata(ap->datatype))
> 
> Doesn't this defeat the purpose (i.e., fragmentation avoidance via wider
> allocations) of the cowextszhint for direct I/O? I think it would be
> better to be consistent with the approach for traditional I/O + extsz
> and incorporate the alignment into the reservation. Perhaps the hunk of
> code that already does just that in xfs_iomap_write_direct() could be
> converted to a small helper and reused..?

We're already doing the alignment to the cowextsize hint in
__xfs_reflink_allocate_cow so that we can take the cowextsize into
account.

> > +	resblks = XFS_DIOSTRAT_SPACE_RES(mp, end_fsb - *offset_fsb);
> > +
> > +	error = xfs_qm_dqattach(ip, 0);
> 
> This is already in the (only) caller.

Yes, it can be dropped, although a superflous xfs_qm_dqattach is
totally harmless anyway.

Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Brian Foster]

On Wed, Dec 07, 2016 at 08:37:09PM +0100, Christoph Hellwig wrote:
> On Wed, Dec 07, 2016 at 02:00:09PM -0500, Brian Foster wrote:
> > > -	if (ap->flags & XFS_BMAPI_COWFORK)
> > > -		align = xfs_get_cowextsz_hint(ap->ip);
> > > -	else if (xfs_alloc_is_userdata(ap->datatype))
> > > +	if (xfs_alloc_is_userdata(ap->datatype))
> > 
> > Doesn't this defeat the purpose (i.e., fragmentation avoidance via wider
> > allocations) of the cowextszhint for direct I/O? I think it would be
> > better to be consistent with the approach for traditional I/O + extsz
> > and incorporate the alignment into the reservation. Perhaps the hunk of
> > code that already does just that in xfs_iomap_write_direct() could be
> > converted to a small helper and reused..?
> 
> We're already doing the alignment to the cowextsize hint in
> __xfs_reflink_allocate_cow so that we can take the cowextsize into
> account.
> 

Only for end_fsb... xfs_bmap_btalloc() calls xfs_bmap_extsize_align()
with the alignment, which rounds out the start and end offsets.

This same end_fsb code has already been removed from
xfs_reflink_reserve_cow() for similar reasons. It should probably be
removed from xfs_reflink_allocate_cow() as well.

Brian

> > > +	resblks = XFS_DIOSTRAT_SPACE_RES(mp, end_fsb - *offset_fsb);
> > > +
> > > +	error = xfs_qm_dqattach(ip, 0);
> > 
> > This is already in the (only) caller.
> 
> Yes, it can be dropped, although a superflous xfs_qm_dqattach is
> totally harmless anyway.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Darrick J. Wong]

On Wed, Dec 07, 2016 at 02:46:34PM -0500, Brian Foster wrote:
> On Wed, Dec 07, 2016 at 08:37:09PM +0100, Christoph Hellwig wrote:
> > On Wed, Dec 07, 2016 at 02:00:09PM -0500, Brian Foster wrote:
> > > > -	if (ap->flags & XFS_BMAPI_COWFORK)
> > > > -		align = xfs_get_cowextsz_hint(ap->ip);
> > > > -	else if (xfs_alloc_is_userdata(ap->datatype))
> > > > +	if (xfs_alloc_is_userdata(ap->datatype))
> > > 
> > > Doesn't this defeat the purpose (i.e., fragmentation avoidance via wider
> > > allocations) of the cowextszhint for direct I/O? I think it would be
> > > better to be consistent with the approach for traditional I/O + extsz
> > > and incorporate the alignment into the reservation. Perhaps the hunk of
> > > code that already does just that in xfs_iomap_write_direct() could be
> > > converted to a small helper and reused..?
> > 
> > We're already doing the alignment to the cowextsize hint in
> > __xfs_reflink_allocate_cow so that we can take the cowextsize into
> > account.
> > 
> 
> Only for end_fsb... xfs_bmap_btalloc() calls xfs_bmap_extsize_align()
> with the alignment, which rounds out the start and end offsets.

Seconded; I finally got a chance to build a kernel with these three
patches and ran xfs/214 with a tracepointed kernel; it doesn't round the
start down to the previous cowextsize alignment like 4.9 does... and
thus the quota accounting is off.

--D

> 
> This same end_fsb code has already been removed from
> xfs_reflink_reserve_cow() for similar reasons. It should probably be
> removed from xfs_reflink_allocate_cow() as well.
> 
> Brian
> 
> > > > +	resblks = XFS_DIOSTRAT_SPACE_RES(mp, end_fsb - *offset_fsb);
> > > > +
> > > > +	error = xfs_qm_dqattach(ip, 0);
> > > 
> > > This is already in the (only) caller.
> > 
> > Yes, it can be dropped, although a superflous xfs_qm_dqattach is
> > totally harmless anyway.
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Christoph Hellwig]

On Wed, Dec 07, 2016 at 02:46:34PM -0500, Brian Foster wrote:
> Only for end_fsb... xfs_bmap_btalloc() calls xfs_bmap_extsize_align()
> with the alignment, which rounds out the start and end offsets.

... and corrupts data in the direct I/O case.

The problem is that the down-alignment in xfs_bmap_extsize_align will
now create a real extent that spans before the extent that we have
to COW in this write_begin call.  But the area before might have been
a hole before the dio write that had just before been filled with
an allocation in the data fork.  And due to the direct I/O end_io
interface that only covers the range of the whole write we don't
know at that point where exactly the COW operation started and will
happily splice back our front pad into the data fork, replacing
the just written data with garbage.  xfs/228 and sometimes generic/199
reproduce this nicely.

Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Brian Foster]

On Tue, Jan 24, 2017 at 09:37:32AM +0100, Christoph Hellwig wrote:
> On Wed, Dec 07, 2016 at 02:46:34PM -0500, Brian Foster wrote:
> > Only for end_fsb... xfs_bmap_btalloc() calls xfs_bmap_extsize_align()
> > with the alignment, which rounds out the start and end offsets.
> 
> ... and corrupts data in the direct I/O case.
> 
> The problem is that the down-alignment in xfs_bmap_extsize_align will
> now create a real extent that spans before the extent that we have
> to COW in this write_begin call.  But the area before might have been
> a hole before the dio write that had just before been filled with
> an allocation in the data fork.  And due to the direct I/O end_io
> interface that only covers the range of the whole write we don't
> know at that point where exactly the COW operation started and will
> happily splice back our front pad into the data fork, replacing
> the just written data with garbage.  xfs/228 and sometimes generic/199
> reproduce this nicely.

Is this reproducible on the current tree or only with this patch series?

Also, shouldn't the end_io handler only remap the range of the write,
regardless of whether the initial allocation ended up preallocating over
holes or purely a shared range?

Perhaps what you are saying here is that we have a single dio write that
spans wider than a shared data fork extent..? In that case, we iterate
the range in xfs_reflink_reserve_cow(). We'd skip the start of the range
that is a hole in the data fork, but as you say, the
xfs_bmapi_reserve_delalloc() call for the part of the I/O on the shared
extent can widen the COW fork allocation to before the extent in the
data fork, possibly to before the start of the I/O. (Thus we end up
allocating COW blocks over the hole anyways...).

>From there we are going to drop into iomap_dio_rw(), which looks like
it's going to check the COW fork for blocks and if found, write to those
blocks (as opposed to doing a data fork allocation). AFAICT, that means
the extent size hint shouldn't be a problem. What am I missing?

Brian

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Christoph Hellwig]

On Tue, Jan 24, 2017 at 08:50:45AM -0500, Brian Foster wrote:
> Is this reproducible on the current tree or only with this patch series?

It's only reproducible with the series modified to your review comments.

> Also, shouldn't the end_io handler only remap the range of the write,
> regardless of whether the initial allocation ended up preallocating over
> holes or purely a shared range?

The end_io handler is caller for the whole size of the write.  That's
mostly because we don't have an object corresponding to a write_begin
call.

> Perhaps what you are saying here is that we have a single dio write that
> spans wider than a shared data fork extent..?

Yes.

> In that case, we iterate
> the range in xfs_reflink_reserve_cow(). We'd skip the start of the range
> that is a hole in the data fork, but as you say, the
> xfs_bmapi_reserve_delalloc() call for the part of the I/O on the shared
> extent can widen the COW fork allocation to before the extent in the
> data fork, possibly to before the start of the I/O. (Thus we end up
> allocating COW blocks over the hole anyways...).

The problem is the following.

We have a file with the following layout


HHHHHHHHHHHHDDDDDDDDDDDDDD

where H is hole and D is data.  The H to D boundary is not aligned
to the cowextsize.

The direct I/O code now does a first pass allocating an extent for
H and copies data to it.  Then in the next step it goes on to D
and unshares it.  It then enlarges the extent into the end of the
previously H range. It does however not copy data into H again,
as the iomap iterator is past it.  The ->end_io routine however
is called for the hole range, and will move the just allocated
rounding before H back into the data fork, replacing the valid data
writtent just before.

Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Brian Foster]

On Tue, Jan 24, 2017 at 02:59:37PM +0100, Christoph Hellwig wrote:
> On Tue, Jan 24, 2017 at 08:50:45AM -0500, Brian Foster wrote:
> > Is this reproducible on the current tree or only with this patch series?
> 
> It's only reproducible with the series modified to your review comments.
> 

Ok, well then I'm probably not going to be able to follow the details
well enough to try and provide constructive feedback without seeing the
code. Looking back, my comments were generally about the tradeoff of
bypassing the extent size hint mechanism that has been built into
reflink to avoid cow fragmention.

> > Also, shouldn't the end_io handler only remap the range of the write,
> > regardless of whether the initial allocation ended up preallocating over
> > holes or purely a shared range?
> 
> The end_io handler is caller for the whole size of the write.  That's
> mostly because we don't have an object corresponding to a write_begin
> call.
> 
> > Perhaps what you are saying here is that we have a single dio write that
> > spans wider than a shared data fork extent..?
> 
> Yes.
> 
> > In that case, we iterate
> > the range in xfs_reflink_reserve_cow(). We'd skip the start of the range
> > that is a hole in the data fork, but as you say, the
> > xfs_bmapi_reserve_delalloc() call for the part of the I/O on the shared
> > extent can widen the COW fork allocation to before the extent in the
> > data fork, possibly to before the start of the I/O. (Thus we end up
> > allocating COW blocks over the hole anyways...).
> 
> The problem is the following.
> 
> We have a file with the following layout
> 
> 
> HHHHHHHHHHHHDDDDDDDDDDDDDD
> 
> where H is hole and D is data.  The H to D boundary is not aligned
> to the cowextsize.
> 
> The direct I/O code now does a first pass allocating an extent for
> H and copies data to it.  Then in the next step it goes on to D
> and unshares it.  It then enlarges the extent into the end of the
> previously H range. It does however not copy data into H again,
> as the iomap iterator is past it.  The ->end_io routine however
> is called for the hole range, and will move the just allocated
> rounding before H back into the data fork, replacing the valid data
> writtent just before.
> 

Without seeing the code, perhaps we need to pull up the cow extent size
hint mechanism from the bmapi layer to something similar to how
xfs_iomap_direct_write() handles the traditional extent size hints..?
That may allow us to more intelligently consider the current state
across the data and cow forks in such cases (to not preallocate over
existing blocks, for example, without having to kill off the extent size
hint entirely).

Brian

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Christoph Hellwig]

On Tue, Jan 24, 2017 at 10:02:22AM -0500, Brian Foster wrote:
> Ok, well then I'm probably not going to be able to follow the details
> well enough to try and provide constructive feedback without seeing the
> code. Looking back, my comments were generally about the tradeoff of
> bypassing the extent size hint mechanism that has been built into
> reflink to avoid cow fragmention.

Ok - really very little change here - only the call to the extsize
alignment in xfs_bmap_btalloc reinstated and the according reservation
changes in the caller.

Note that with the previously posted series there is no change in
handling the cowextsize hint for real on-disk allocations.  While
the current code rounds down based on the cowextsize for creating
the delayed extent we will never convert the alignment before the
write start to a real extent - it will just get cleaned up later
at inode eviction time or using the timer.

> Without seeing the code, perhaps we need to pull up the cow extent size
> hint mechanism from the bmapi layer to something similar to how
> xfs_iomap_direct_write() handles the traditional extent size hints..?
> That may allow us to more intelligently consider the current state
> across the data and cow forks in such cases (to not preallocate over
> existing blocks, for example, without having to kill off the extent size
> hint entirely).

We could.  On the other hand I'd love to get the current series in
first as the only thing it change in behavior is not allocating
additional delayed extent space that never gets used, and not writing
data twice if it's sub-blocksize, all of which seem like a clear
improvement.  And it's also the base for my pending DAX reflink support.

Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Brian Foster]

On Tue, Jan 24, 2017 at 04:09:59PM +0100, Christoph Hellwig wrote:
> On Tue, Jan 24, 2017 at 10:02:22AM -0500, Brian Foster wrote:
> > Ok, well then I'm probably not going to be able to follow the details
> > well enough to try and provide constructive feedback without seeing the
> > code. Looking back, my comments were generally about the tradeoff of
> > bypassing the extent size hint mechanism that has been built into
> > reflink to avoid cow fragmention.
> 
> Ok - really very little change here - only the call to the extsize
> alignment in xfs_bmap_btalloc reinstated and the according reservation
> changes in the caller.
> 
> Note that with the previously posted series there is no change in
> handling the cowextsize hint for real on-disk allocations.  While
> the current code rounds down based on the cowextsize for creating
> the delayed extent we will never convert the alignment before the
> write start to a real extent - it will just get cleaned up later
> at inode eviction time or using the timer.
> 

I thought that while not necessarily guaranteed, generally the entire
extent gets converted from delalloc to real blocks. IIRC, that's what
I've seen in the past when looking into the cow fork with bmap. After
all, isn't that the point of the extent size hint? Allocate wider than
the write to accommodate potential subsequent writes into a more
contiguous range.

> > Without seeing the code, perhaps we need to pull up the cow extent size
> > hint mechanism from the bmapi layer to something similar to how
> > xfs_iomap_direct_write() handles the traditional extent size hints..?
> > That may allow us to more intelligently consider the current state
> > across the data and cow forks in such cases (to not preallocate over
> > existing blocks, for example, without having to kill off the extent size
> > hint entirely).
> 
> We could.  On the other hand I'd love to get the current series in
> first as the only thing it change in behavior is not allocating
> additional delayed extent space that never gets used, and not writing
> data twice if it's sub-blocksize, all of which seem like a clear
> improvement.  And it's also the base for my pending DAX reflink support.

Fair enough, that's a different discussion. Personally, I'd prefer to
fix up the stuff we know that needs fixing before piling more stuff on
top. Particularly since something like cow I/O to a reflinked vm image
file (taking full advantage of cowextszhint) might be a more common use
case than DAX reflink at the moment.

All of this stuff is "experimental," however, so I don't feel strongly
about the order so long as we agree the regression can/should be fixed
up. So I'll defer to the maintainer on that one. ;)

Brian

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Christoph Hellwig]

On Tue, Jan 24, 2017 at 11:17:20AM -0500, Brian Foster wrote:
> I thought that while not necessarily guaranteed, generally the entire
> extent gets converted from delalloc to real blocks.

For buffered I/O that's the case.  See the discussion on my recent
xfs_bmapi_write patch.

> IIRC, that's what
> I've seen in the past when looking into the cow fork with bmap. After
> all, isn't that the point of the extent size hint? Allocate wider than
> the write to accommodate potential subsequent writes into a more
> contiguous range.

Well, for direct I/O that's not what the current code does.  Implementing
it might be useful, but I'm not sure how much the front alignment is
going to help in usual worksloads - you'd need a backwards write or
random writes that happen to look almost backwards for it to make
a difference.  I suspect most of them time we'd just allocate blocks to
reclaim them again a little later.

Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Brian Foster]

On Tue, Jan 24, 2017 at 05:21:56PM +0100, Christoph Hellwig wrote:
> On Tue, Jan 24, 2017 at 11:17:20AM -0500, Brian Foster wrote:
> > I thought that while not necessarily guaranteed, generally the entire
> > extent gets converted from delalloc to real blocks.
> 
> For buffered I/O that's the case.  See the discussion on my recent
> xfs_bmapi_write patch.
> 

Yeah, that's the behavior I would have expected...

> > IIRC, that's what
> > I've seen in the past when looking into the cow fork with bmap. After
> > all, isn't that the point of the extent size hint? Allocate wider than
> > the write to accommodate potential subsequent writes into a more
> > contiguous range.
> 
> Well, for direct I/O that's not what the current code does.  Implementing
> it might be useful, but I'm not sure how much the front alignment is
> going to help in usual worksloads - you'd need a backwards write or
> random writes that happen to look almost backwards for it to make
> a difference.  I suspect most of them time we'd just allocate blocks to
> reclaim them again a little later.

Hmm, that's not what I'm seeing (not that it really matters, but I'm
curious if I'm missing something):

# xfs_io -d -c "bmap -v" -c "pwrite 8k 4k" -c "bmap -v" -c "bmap -cv" ./file
./file:
 EXT: FILE-OFFSET      BLOCK-RANGE      AG AG-OFFSET        TOTAL
   0: [0..15]:         80..95            0 (80..95)            16 100000
   1: [16..2047]:      160..2191         0 (160..2191)       2032 100000
wrote 4096/4096 bytes at offset 8192
4 KiB, 1 ops; 0.0000 sec (12.440 MiB/sec and 3184.7134 ops/sec)
./file:
 EXT: FILE-OFFSET      BLOCK-RANGE      AG AG-OFFSET        TOTAL
   0: [0..15]:         80..95            0 (80..95)            16 100000
   1: [16..23]:        2208..2215        0 (2208..2215)         8
   2: [24..2047]:      168..2191         0 (168..2191)       2024 100000
./file:
 EXT: FILE-OFFSET      BLOCK-RANGE      AG AG-OFFSET        TOTAL
   0: [0..15]:         2192..2207        0 (2192..2207)        16
   1: [16..23]:        hole                                     8
   2: [24..255]:       2216..2447        0 (2216..2447)       232
   3: [256..2047]:     hole                                  1792

That's on a fairly recent for-next on a fully reflinked file. It looks
to me that the cow fork extent is fully allocated immediately
(otherwise, I'm not sure what else would have converted it).

Brian

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Christoph Hellwig]

On Tue, Jan 24, 2017 at 12:43:18PM -0500, Brian Foster wrote:
> Hmm, that's not what I'm seeing (not that it really matters, but I'm
> curious if I'm missing something):

Yeah, I can reproduce this on mainline.  Turns out the it was done
by the align call in xfs_bmap_btalloc that even my before run had
removed.

Took me some time to spin my head around this.

Btw, I think we have a nasty race in the current DIO code that might
expose stale data, but this is just the same kind of head spinning
exercise for now:

Thread 1 writes a range from B to c

                    B --------- C

a little later thread 2 writes from A to B

        A --------- B

but the code preallocates beyond B into the range where thread
1 has just written, but ->end_io hasn't been called yet.
But once ->end_io is called thread 2 has already allocated
up to the extent size hint into the write range of thread 1,
so the end_io handler will splice the unintialized blocks from
that preallocation back into the file right after B.

Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Christoph Hellwig]

On Tue, Jan 24, 2017 at 09:08:55PM +0100, Christoph Hellwig wrote:
> On Tue, Jan 24, 2017 at 12:43:18PM -0500, Brian Foster wrote:
> > Hmm, that's not what I'm seeing (not that it really matters, but I'm
> > curious if I'm missing something):
> 
> Yeah, I can reproduce this on mainline.  Turns out the it was done

s/can/can't/, sorry.  It's getting late for the day.

Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Darrick J. Wong]

On Tue, Jan 24, 2017 at 09:08:55PM +0100, Christoph Hellwig wrote:
> On Tue, Jan 24, 2017 at 12:43:18PM -0500, Brian Foster wrote:
> > Hmm, that's not what I'm seeing (not that it really matters, but I'm
> > curious if I'm missing something):
> 
> Yeah, I can reproduce this on mainline.  Turns out the it was done
> by the align call in xfs_bmap_btalloc that even my before run had
> removed.
> 
> Took me some time to spin my head around this.
> 
> Btw, I think we have a nasty race in the current DIO code that might
> expose stale data, but this is just the same kind of head spinning
> exercise for now:
> 
> Thread 1 writes a range from B to c
> 
>                     B --------- C

          A --------- B --------- C
               ^           ^
               d           e

I'm assuming B-C has no shared blocks, d-B has shared blocks, and that
both d & e are cowextsize boundaries.

> a little later thread 2 writes from A to B
> 
>         A --------- B
> but the code preallocates beyond B into the range where thread
> 1 has just written, but ->end_io hasn't been called yet.
> But once ->end_io is called thread 2 has already allocated
> up to the extent size hint into the write range of thread 1,
> so the end_io handler will splice the unintialized blocks from
> that preallocation back into the file right after B.

I think you're right about there being a dio race here.  I'm not sure
what the solution here is -- certainly we could disregard the cowextsize
hint, though that has a fragmentation cost that we already know about.

We could also change the dio write setup to extend the range that it
checks for shared blocks up and down to the nearest cowextsize boundary
and set up the delalloc reservations in the cow fork as necessary.  If
our thread2 comes along then it'll find the reservations already set
up for a cow so that we avoid the situation where B-C changes between
iomap_begin and dio_write_end_io does the wrong thing.  That's more in
the spirit of cowextsize since we'd promote future writes to CoW.
However it's more wasteful of blocks since we have no idea if those
future writes are ever actually going to happen, and it also adds more
bmap records if we don't use all of the reservation.

Ugh, my head hurts, I'm going to go for a walk to ponder this some more,
and to try to work out whether the buffered path is all right.

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Darrick J. Wong]

On Tue, Jan 24, 2017 at 04:09:34PM -0800, Darrick J. Wong wrote:
> On Tue, Jan 24, 2017 at 09:08:55PM +0100, Christoph Hellwig wrote:
> > On Tue, Jan 24, 2017 at 12:43:18PM -0500, Brian Foster wrote:
> > > Hmm, that's not what I'm seeing (not that it really matters, but I'm
> > > curious if I'm missing something):
> > 
> > Yeah, I can reproduce this on mainline.  Turns out the it was done
> > by the align call in xfs_bmap_btalloc that even my before run had
> > removed.
> > 
> > Took me some time to spin my head around this.
> > 
> > Btw, I think we have a nasty race in the current DIO code that might
> > expose stale data, but this is just the same kind of head spinning
> > exercise for now:
> > 
> > Thread 1 writes a range from B to c
> > 
> >                     B --------- C
> 
>           A --------- B --------- C
>                ^           ^
>                d           e
> 
> I'm assuming B-C has no shared blocks, d-B has shared blocks, and that
> both d & e are cowextsize boundaries.
> 
> > a little later thread 2 writes from A to B
> > 
> >         A --------- B
> > but the code preallocates beyond B into the range where thread
> > 1 has just written, but ->end_io hasn't been called yet.
> > But once ->end_io is called thread 2 has already allocated
> > up to the extent size hint into the write range of thread 1,
> > so the end_io handler will splice the unintialized blocks from
> > that preallocation back into the file right after B.
> 
> I think you're right about there being a dio race here.  I'm not sure
> what the solution here is -- certainly we could disregard the cowextsize
> hint, though that has a fragmentation cost that we already know about.
> 
> We could also change the dio write setup to extend the range that it
> checks for shared blocks up and down to the nearest cowextsize boundary
> and set up the delalloc reservations in the cow fork as necessary.  If
> our thread2 comes along then it'll find the reservations already set
> up for a cow so that we avoid the situation where B-C changes between
> iomap_begin and dio_write_end_io does the wrong thing.  That's more in
> the spirit of cowextsize since we'd promote future writes to CoW.
> However it's more wasteful of blocks since we have no idea if those
> future writes are ever actually going to happen, and it also adds more
> bmap records if we don't use all of the reservation.
> 
> Ugh, my head hurts, I'm going to go for a walk to ponder this some more,
> and to try to work out whether the buffered path is all right.

I think I came up with a better idea overnight: take advantage of the
unwritten bit.  Currently, all extents in the cow fork are either
delalloc or normal extents.  When we allocate the blocks to fill a cow
extent, we'll flag the extent as unwritten.  When we go to write the
data to disk, we convert the unwritten extent to a real extent, and the
post-write remap (since it only takes a file offset and length) will
be changed to remap only real, written extents into the data fork.

--D

> 
> --D
> 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

[Christoph Hellwig]

On Fri, Jan 27, 2017 at 09:44:47AM -0800, Darrick J. Wong wrote:
> I think I came up with a better idea overnight: take advantage of the
> unwritten bit.  Currently, all extents in the cow fork are either
> delalloc or normal extents.  When we allocate the blocks to fill a cow
> extent, we'll flag the extent as unwritten.  When we go to write the
> data to disk, we convert the unwritten extent to a real extent, and the
> post-write remap (since it only takes a file offset and length) will
> be changed to remap only real, written extents into the data fork.

That sounds like a possibility.  My other idea was to force COW all
direct I/O writes, that way we can't have a mismatch between extents
in the data and the COW fork.  But so far this is just an idea..