delalloc and reflink fixes & tweaks V2

delalloc and reflink fixes & tweaks V2

[Christoph Hellwig]

Hi all,

this series contains the basic reflink and delalloc fixups intended
for 4.20.  All the bits that need more work are left out for now.

Changes since v1:
  - drop various patches
  - print the inode number for dangling delalloc extents

[PATCH 1/7] xfs: remove XFS_IO_INVALID

[Christoph Hellwig]

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 <hch@lst.de>
---
 fs/xfs/xfs_aops.c |  4 ++--
 fs/xfs/xfs_aops.h | 14 ++++++--------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 49f5f5896a43..338b9d9984e0 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -917,7 +917,7 @@ xfs_vm_writepage(
 	struct writeback_control *wbc)
 {
 	struct xfs_writepage_ctx wpc = {
-		.io_type = XFS_IO_INVALID,
+		.io_type = XFS_IO_HOLE,
 	};
 	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;
 
diff --git a/fs/xfs/xfs_aops.h b/fs/xfs/xfs_aops.h
index 9af867951a10..494b4338446e 100644
--- a/fs/xfs/xfs_aops.h
+++ b/fs/xfs/xfs_aops.h
@@ -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.
-- 
2.19.0

[PATCH 2/7] xfs: always allocate blocks as unwritten for file data

[Christoph Hellwig]

XFS historically had a small race that could lead to exposing
uninitialized data in case of a crash.  If we are filling holes using
buffered I/O we convert the delayed allocation to a real allocation
before writing out the data.  If we crash after the blocks were
allocated, but before the data was written this could lead to reading
uninitialized blocks (or leaked data from a previous allocation that was
reused).  Now that we have the CIL logging extent format changes is
cheap, so we can switch to always allocating blocks as unwritten.
Note that this is not be strictly necessary for writes that append
beyond i_size, but given that we have to log a transaction in that
case anyway we might as well give all block allocations a uniform
treatment.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_aops.c  | 3 +--
 fs/xfs/xfs_aops.h  | 2 --
 fs/xfs/xfs_iomap.c | 4 ++--
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 338b9d9984e0..775cdcfe70c2 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -437,8 +437,7 @@ xfs_map_blocks(
 			imap.br_blockcount = cow_fsb - imap.br_startoff;
 
 		if (isnullstartblock(imap.br_startblock)) {
-			/* got a delalloc extent */
-			wpc->io_type = XFS_IO_DELALLOC;
+			wpc->io_type = XFS_IO_UNWRITTEN;
 			goto allocate_blocks;
 		}
 
diff --git a/fs/xfs/xfs_aops.h b/fs/xfs/xfs_aops.h
index 494b4338446e..f0710c54cf68 100644
--- a/fs/xfs/xfs_aops.h
+++ b/fs/xfs/xfs_aops.h
@@ -13,7 +13,6 @@ extern struct bio_set xfs_ioend_bioset;
  */
 enum {
 	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 */
@@ -21,7 +20,6 @@ enum {
 
 #define XFS_IO_TYPES \
 	{ XFS_IO_HOLE,			"hole" },	\
-	{ XFS_IO_DELALLOC,		"delalloc" },	\
 	{ XFS_IO_UNWRITTEN,		"unwritten" },	\
 	{ XFS_IO_OVERWRITE,		"overwrite" },	\
 	{ XFS_IO_COW,			"CoW" }
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 6320aca39f39..10fc93cebc42 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -662,11 +662,11 @@ xfs_iomap_write_allocate(
 	xfs_trans_t	*tp;
 	int		nimaps;
 	int		error = 0;
-	int		flags = XFS_BMAPI_DELALLOC;
+	int		flags = XFS_BMAPI_DELALLOC | XFS_BMAPI_PREALLOC;
 	int		nres;
 
 	if (whichfork == XFS_COW_FORK)
-		flags |= XFS_BMAPI_COWFORK | XFS_BMAPI_PREALLOC;
+		flags |= XFS_BMAPI_COWFORK;
 
 	/*
 	 * Make sure that the dquots are there.
-- 
2.19.0

[PATCH 3/7] xfs: handle zeroing in xfs_file_iomap_begin_delay

[Christoph Hellwig]

We only need to allocate blocks for zeroing for reflink inodes,
and for we currently have a special case for reflink files in
the otherwise direct I/O path that I'd like to get rid of.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_iomap.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 10fc93cebc42..7cbebcf61fa7 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -62,6 +62,21 @@ xfs_bmbt_to_iomap(
 	iomap->dax_dev = xfs_find_daxdev_for_inode(VFS_I(ip));
 }
 
+static void
+xfs_hole_to_iomap(
+	struct xfs_inode	*ip,
+	struct iomap		*iomap,
+	xfs_fileoff_t		offset_fsb,
+	xfs_fileoff_t		end_fsb)
+{
+	iomap->addr = IOMAP_NULL_ADDR;
+	iomap->type = IOMAP_HOLE;
+	iomap->offset = XFS_FSB_TO_B(ip->i_mount, offset_fsb);
+	iomap->length = XFS_FSB_TO_B(ip->i_mount, end_fsb - offset_fsb);
+	iomap->bdev = xfs_find_bdev_for_inode(VFS_I(ip));
+	iomap->dax_dev = xfs_find_daxdev_for_inode(VFS_I(ip));
+}
+
 xfs_extlen_t
 xfs_eof_alignment(
 	struct xfs_inode	*ip,
@@ -502,6 +517,7 @@ xfs_file_iomap_begin_delay(
 	struct inode		*inode,
 	loff_t			offset,
 	loff_t			count,
+	unsigned		flags,
 	struct iomap		*iomap)
 {
 	struct xfs_inode	*ip = XFS_I(inode);
@@ -539,8 +555,12 @@ xfs_file_iomap_begin_delay(
 	}
 
 	eof = !xfs_iext_lookup_extent(ip, ifp, offset_fsb, &icur, &got);
-	if (!eof && got.br_startoff <= offset_fsb) {
-		if (xfs_is_reflink_inode(ip)) {
+	if (eof)
+		got.br_startoff = maxbytes_fsb;
+	if (got.br_startoff <= offset_fsb) {
+		if (xfs_is_reflink_inode(ip) &&
+		    ((flags & IOMAP_WRITE) ||
+		     got.br_state != XFS_EXT_UNWRITTEN)) {
 			bool		shared;
 
 			end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
@@ -555,6 +575,11 @@ xfs_file_iomap_begin_delay(
 		goto done;
 	}
 
+	if (flags & IOMAP_ZERO) {
+		xfs_hole_to_iomap(ip, iomap, offset_fsb, got.br_startoff);
+		goto out_unlock;
+	}
+
 	error = xfs_qm_dqattach_locked(ip, false);
 	if (error)
 		goto out_unlock;
@@ -1009,10 +1034,11 @@ xfs_file_iomap_begin(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
 
-	if (((flags & (IOMAP_WRITE | IOMAP_DIRECT)) == IOMAP_WRITE) &&
+	if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && !(flags & IOMAP_DIRECT) &&
 			!IS_DAX(inode) && !xfs_get_extsz_hint(ip)) {
 		/* Reserve delalloc blocks for regular writeback. */
-		return xfs_file_iomap_begin_delay(inode, offset, length, iomap);
+		return xfs_file_iomap_begin_delay(inode, offset, length, flags,
+				iomap);
 	}
 
 	/*
-- 
2.19.0

[PATCH 4/7] xfs: remove the unused shared argument to xfs_reflink_reserve_cow

[Christoph Hellwig]

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

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 7cbebcf61fa7..8ba97e67b474 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -561,12 +561,10 @@ xfs_file_iomap_begin_delay(
 		if (xfs_is_reflink_inode(ip) &&
 		    ((flags & IOMAP_WRITE) ||
 		     got.br_state != XFS_EXT_UNWRITTEN)) {
-			bool		shared;
-
 			end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
 					maxbytes_fsb);
 			xfs_trim_extent(&got, offset_fsb, end_fsb - offset_fsb);
-			error = xfs_reflink_reserve_cow(ip, &got, &shared);
+			error = xfs_reflink_reserve_cow(ip, &got);
 			if (error)
 				goto out_unlock;
 		}
@@ -1091,7 +1089,7 @@ xfs_file_iomap_begin(
 			if (error)
 				goto out_unlock;
 		} else {
-			error = xfs_reflink_reserve_cow(ip, &imap, &shared);
+			error = xfs_reflink_reserve_cow(ip, &imap);
 			if (error)
 				goto out_unlock;
 		}
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 5289e22cb081..06e38e88ddee 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -241,7 +241,7 @@ xfs_reflink_trim_around_shared(
 /*
  * Trim the passed in imap to the next shared/unshared extent boundary, and
  * if imap->br_startoff points to a shared extent reserve space for it in the
- * COW fork.  In this case *shared is set to true, else to false.
+ * COW fork.
  *
  * Note that imap will always contain the block numbers for the existing blocks
  * in the data fork, as the upper layers need them for read-modify-write
@@ -250,14 +250,14 @@ xfs_reflink_trim_around_shared(
 int
 xfs_reflink_reserve_cow(
 	struct xfs_inode	*ip,
-	struct xfs_bmbt_irec	*imap,
-	bool			*shared)
+	struct xfs_bmbt_irec	*imap)
 {
 	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
 	struct xfs_bmbt_irec	got;
 	int			error = 0;
 	bool			eof = false, trimmed;
 	struct xfs_iext_cursor	icur;
+	bool			shared;
 
 	/*
 	 * Search the COW fork extent list first.  This serves two purposes:
@@ -273,18 +273,16 @@ xfs_reflink_reserve_cow(
 	if (!eof && got.br_startoff <= imap->br_startoff) {
 		trace_xfs_reflink_cow_found(ip, imap);
 		xfs_trim_extent(imap, got.br_startoff, got.br_blockcount);
-
-		*shared = true;
 		return 0;
 	}
 
 	/* Trim the mapping to the nearest shared extent boundary. */
-	error = xfs_reflink_trim_around_shared(ip, imap, shared, &trimmed);
+	error = xfs_reflink_trim_around_shared(ip, imap, &shared, &trimmed);
 	if (error)
 		return error;
 
 	/* Not shared?  Just report the (potentially capped) extent. */
-	if (!*shared)
+	if (!shared)
 		return 0;
 
 	/*
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index c585ad9552b2..b77f4079022a 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -13,7 +13,7 @@ extern int xfs_reflink_trim_around_shared(struct xfs_inode *ip,
 		struct xfs_bmbt_irec *irec, bool *shared, bool *trimmed);
 
 extern int xfs_reflink_reserve_cow(struct xfs_inode *ip,
-		struct xfs_bmbt_irec *imap, bool *shared);
+		struct xfs_bmbt_irec *imap);
 extern int xfs_reflink_allocate_cow(struct xfs_inode *ip,
 		struct xfs_bmbt_irec *imap, bool *shared, uint *lockmode);
 extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset,
-- 
2.19.0

[PATCH 5/7] xfs: remove the unused trimmed argument from xfs_reflink_trim_around_shared

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_bmap_util.c |  4 ++--
 fs/xfs/xfs_iomap.c     |  5 ++---
 fs/xfs/xfs_reflink.c   | 15 +++++----------
 fs/xfs/xfs_reflink.h   |  2 +-
 4 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 6de8d90041ff..8d2579847076 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -406,10 +406,10 @@ xfs_getbmap_report_one(
 	struct xfs_bmbt_irec	*got)
 {
 	struct kgetbmap		*p = out + bmv->bmv_entries;
-	bool			shared = false, trimmed = false;
+	bool			shared = false;
 	int			error;
 
-	error = xfs_reflink_trim_around_shared(ip, got, &shared, &trimmed);
+	error = xfs_reflink_trim_around_shared(ip, got, &shared);
 	if (error)
 		return error;
 
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 8ba97e67b474..1224eced1ee4 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1026,7 +1026,7 @@ xfs_file_iomap_begin(
 	struct xfs_bmbt_irec	imap;
 	xfs_fileoff_t		offset_fsb, end_fsb;
 	int			nimaps = 1, error = 0;
-	bool			shared = false, trimmed = false;
+	bool			shared = false;
 	unsigned		lockmode;
 
 	if (XFS_FORCED_SHUTDOWN(mp))
@@ -1062,8 +1062,7 @@ xfs_file_iomap_begin(
 
 	if (flags & IOMAP_REPORT) {
 		/* Trim the mapping to the nearest shared extent boundary. */
-		error = xfs_reflink_trim_around_shared(ip, &imap, &shared,
-				&trimmed);
+		error = xfs_reflink_trim_around_shared(ip, &imap, &shared);
 		if (error)
 			goto out_unlock;
 	}
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 06e38e88ddee..1e39a7d21c7e 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -182,8 +182,7 @@ int
 xfs_reflink_trim_around_shared(
 	struct xfs_inode	*ip,
 	struct xfs_bmbt_irec	*irec,
-	bool			*shared,
-	bool			*trimmed)
+	bool			*shared)
 {
 	xfs_agnumber_t		agno;
 	xfs_agblock_t		agbno;
@@ -209,7 +208,7 @@ xfs_reflink_trim_around_shared(
 	if (error)
 		return error;
 
-	*shared = *trimmed = false;
+	*shared = false;
 	if (fbno == NULLAGBLOCK) {
 		/* No shared blocks at all. */
 		return 0;
@@ -222,8 +221,6 @@ xfs_reflink_trim_around_shared(
 		 */
 		irec->br_blockcount = flen;
 		*shared = true;
-		if (flen != aglen)
-			*trimmed = true;
 		return 0;
 	} else {
 		/*
@@ -233,7 +230,6 @@ xfs_reflink_trim_around_shared(
 		 * start of the shared region.
 		 */
 		irec->br_blockcount = fbno - agbno;
-		*trimmed = true;
 		return 0;
 	}
 }
@@ -255,7 +251,7 @@ xfs_reflink_reserve_cow(
 	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
 	struct xfs_bmbt_irec	got;
 	int			error = 0;
-	bool			eof = false, trimmed;
+	bool			eof = false;
 	struct xfs_iext_cursor	icur;
 	bool			shared;
 
@@ -277,7 +273,7 @@ xfs_reflink_reserve_cow(
 	}
 
 	/* Trim the mapping to the nearest shared extent boundary. */
-	error = xfs_reflink_trim_around_shared(ip, imap, &shared, &trimmed);
+	error = xfs_reflink_trim_around_shared(ip, imap, &shared);
 	if (error)
 		return error;
 
@@ -366,7 +362,6 @@ xfs_find_trim_cow_extent(
 	xfs_filblks_t		count_fsb = imap->br_blockcount;
 	struct xfs_iext_cursor	icur;
 	struct xfs_bmbt_irec	got;
-	bool			trimmed;
 
 	*found = false;
 
@@ -376,7 +371,7 @@ xfs_find_trim_cow_extent(
 	 */
 	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);
+		return xfs_reflink_trim_around_shared(ip, imap, shared);
 
 	*shared = true;
 	if (isnullstartblock(got.br_startblock)) {
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index b77f4079022a..7f47202b5639 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -10,7 +10,7 @@ extern int xfs_reflink_find_shared(struct xfs_mount *mp, struct xfs_trans *tp,
 		xfs_agnumber_t agno, xfs_agblock_t agbno, xfs_extlen_t aglen,
 		xfs_agblock_t *fbno, xfs_extlen_t *flen, bool find_maximal);
 extern int xfs_reflink_trim_around_shared(struct xfs_inode *ip,
-		struct xfs_bmbt_irec *irec, bool *shared, bool *trimmed);
+		struct xfs_bmbt_irec *irec, bool *shared);
 
 extern int xfs_reflink_reserve_cow(struct xfs_inode *ip,
 		struct xfs_bmbt_irec *imap);
-- 
2.19.0

[PATCH 6/7] xfs: fix fork selection in xfs_find_trim_cow_extent

[Christoph Hellwig]

We should want to write directly into the data fork for blocks that don't
have an extent in the COW fork covering them yet.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_reflink.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 1e39a7d21c7e..ead35209ffae 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -369,9 +369,13 @@ xfs_find_trim_cow_extent(
 	 * 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)
+	if (!xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, &icur, &got))
+		got.br_startoff = offset_fsb + count_fsb;
+	if (got.br_startoff > offset_fsb) {
+		xfs_trim_extent(imap, imap->br_startoff,
+				got.br_startoff - imap->br_startoff);
 		return xfs_reflink_trim_around_shared(ip, imap, shared);
+	}
 
 	*shared = true;
 	if (isnullstartblock(got.br_startblock)) {
-- 
2.19.0

[PATCH 7/7] xfs: print dangling delalloc extents

[Christoph Hellwig]

Instead of just asserting that we have no delalloc space dangling
in an inode that gets freed print the actual offenders for debug
mode.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_super.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 207ee302b1bb..99250bcb65a7 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -933,6 +933,32 @@ xfs_fs_alloc_inode(
 	return NULL;
 }
 
+#ifdef DEBUG
+static void
+xfs_check_delalloc(
+	struct xfs_inode	*ip,
+	int			whichfork)
+{
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
+	struct xfs_bmbt_irec	got;
+	struct xfs_iext_cursor	icur;
+
+	if (!ifp || !xfs_iext_lookup_extent(ip, ifp, 0, &icur, &got))
+		return;
+	do {
+		if (isnullstartblock(got.br_startblock)) {
+			xfs_warn(ip->i_mount,
+	"ino %llx %s fork has delalloc extent at [0x%llx:0x%llx]",
+				ip->i_ino,
+				whichfork == XFS_DATA_FORK ? "data" : "cow",
+				got.br_startoff, got.br_blockcount);
+		}
+	} while (xfs_iext_next_extent(ifp, &icur, &got));
+}
+#else
+#define xfs_check_delalloc(ip, whichfork)	do { } while (0)
+#endif
+
 /*
  * Now that the generic code is guaranteed not to be accessing
  * the linux inode, we can inactivate and reclaim the inode.
@@ -951,7 +977,12 @@ xfs_fs_destroy_inode(
 
 	xfs_inactive(ip);
 
-	ASSERT(XFS_FORCED_SHUTDOWN(ip->i_mount) || ip->i_delayed_blks == 0);
+	if (!XFS_FORCED_SHUTDOWN(ip->i_mount) && ip->i_delayed_blks) {
+		xfs_check_delalloc(ip, XFS_DATA_FORK);
+		xfs_check_delalloc(ip, XFS_COW_FORK);
+		ASSERT(0);
+	}
+
 	XFS_STATS_INC(ip->i_mount, vn_reclaim);
 
 	/*
-- 
2.19.0

Re: [PATCH 1/7] xfs: remove XFS_IO_INVALID

[Brian Foster]

On Mon, Oct 01, 2018 at 05:37:35AM -0700, Christoph Hellwig wrote:
> 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 <hch@lst.de>
> ---

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

>  fs/xfs/xfs_aops.c |  4 ++--
>  fs/xfs/xfs_aops.h | 14 ++++++--------
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 49f5f5896a43..338b9d9984e0 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -917,7 +917,7 @@ xfs_vm_writepage(
>  	struct writeback_control *wbc)
>  {
>  	struct xfs_writepage_ctx wpc = {
> -		.io_type = XFS_IO_INVALID,
> +		.io_type = XFS_IO_HOLE,
>  	};
>  	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;
>  
> diff --git a/fs/xfs/xfs_aops.h b/fs/xfs/xfs_aops.h
> index 9af867951a10..494b4338446e 100644
> --- a/fs/xfs/xfs_aops.h
> +++ b/fs/xfs/xfs_aops.h
> @@ -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.
> -- 
> 2.19.0
> 

Re: [PATCH 2/7] xfs: always allocate blocks as unwritten for file data

[Brian Foster]

On Mon, Oct 01, 2018 at 05:37:36AM -0700, Christoph Hellwig wrote:
> XFS historically had a small race that could lead to exposing
> uninitialized data in case of a crash.  If we are filling holes using
> buffered I/O we convert the delayed allocation to a real allocation
> before writing out the data.  If we crash after the blocks were
> allocated, but before the data was written this could lead to reading
> uninitialized blocks (or leaked data from a previous allocation that was
> reused).  Now that we have the CIL logging extent format changes is
> cheap, so we can switch to always allocating blocks as unwritten.
> Note that this is not be strictly necessary for writes that append
> beyond i_size, but given that we have to log a transaction in that
> case anyway we might as well give all block allocations a uniform
> treatment.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

It's great that we can finally fix this, particularly with such a simple
change. IIRC, the only real thing standing in the way was the buffer
head delalloc state management mess.

>  fs/xfs/xfs_aops.c  | 3 +--
>  fs/xfs/xfs_aops.h  | 2 --
>  fs/xfs/xfs_iomap.c | 4 ++--
>  3 files changed, 3 insertions(+), 6 deletions(-)
> 
...
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 6320aca39f39..10fc93cebc42 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -662,11 +662,11 @@ xfs_iomap_write_allocate(
>  	xfs_trans_t	*tp;
>  	int		nimaps;
>  	int		error = 0;
> -	int		flags = XFS_BMAPI_DELALLOC;
> +	int		flags = XFS_BMAPI_DELALLOC | XFS_BMAPI_PREALLOC;

... though I don't quite think this is sufficient. xfs_bmapi_allocate()
has this snippet of code:

        if ((!bma->wasdel || (bma->flags & XFS_BMAPI_COWFORK)) &&
            (bma->flags & XFS_BMAPI_PREALLOC) &&
            xfs_sb_version_hasextflgbit(&mp->m_sb))
                bma->got.br_state = XFS_EXT_UNWRITTEN;

... which looks like it explicitly bypasses the PREALLOC flag for
delalloc extents. I figured this would just be an inefficiency since
prealloc conversion comes later, but if you look at
xfs_bmapi_convert_unwritten():

        /* check if we need to do real->unwritten conversion */
        if (mval->br_state == XFS_EXT_NORM &&
            (flags & (XFS_BMAPI_PREALLOC | XFS_BMAPI_CONVERT)) !=
                        (XFS_BMAPI_PREALLOC | XFS_BMAPI_CONVERT))
                return 0;

... it sees this as an existing extent and so doesn't change the state
unless the CONVERT flag is also passed. A quick test to shut down
immediately after the xfs_iomap_write_allocate() transaction commits
seems to confirm this behavior:

# xfs_io -c "fiemap -v" /mnt/file
/mnt/file:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..7]:          72..79               8   0x1

I think the right fix here is to remove the referenced logic from
xfs_bmapi_allocate(). I also think this demonstrates the need for an
xfstest. ;) Expected behavior should be easy to confirm with a new error
tag, for example.

Brian

>  	int		nres;
>  
>  	if (whichfork == XFS_COW_FORK)
> -		flags |= XFS_BMAPI_COWFORK | XFS_BMAPI_PREALLOC;
> +		flags |= XFS_BMAPI_COWFORK;
>  
>  	/*
>  	 * Make sure that the dquots are there.
> -- 
> 2.19.0
> 

Re: [PATCH 3/7] xfs: handle zeroing in xfs_file_iomap_begin_delay

[Brian Foster]

On Mon, Oct 01, 2018 at 05:37:37AM -0700, Christoph Hellwig wrote:
> We only need to allocate blocks for zeroing for reflink inodes,
> and for we currently have a special case for reflink files in
> the otherwise direct I/O path that I'd like to get rid of.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_iomap.c | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 10fc93cebc42..7cbebcf61fa7 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -62,6 +62,21 @@ xfs_bmbt_to_iomap(
>  	iomap->dax_dev = xfs_find_daxdev_for_inode(VFS_I(ip));
>  }
>  
> +static void
> +xfs_hole_to_iomap(
> +	struct xfs_inode	*ip,
> +	struct iomap		*iomap,
> +	xfs_fileoff_t		offset_fsb,
> +	xfs_fileoff_t		end_fsb)
> +{
> +	iomap->addr = IOMAP_NULL_ADDR;
> +	iomap->type = IOMAP_HOLE;
> +	iomap->offset = XFS_FSB_TO_B(ip->i_mount, offset_fsb);
> +	iomap->length = XFS_FSB_TO_B(ip->i_mount, end_fsb - offset_fsb);
> +	iomap->bdev = xfs_find_bdev_for_inode(VFS_I(ip));
> +	iomap->dax_dev = xfs_find_daxdev_for_inode(VFS_I(ip));
> +}
> +
>  xfs_extlen_t
>  xfs_eof_alignment(
>  	struct xfs_inode	*ip,
> @@ -502,6 +517,7 @@ xfs_file_iomap_begin_delay(
>  	struct inode		*inode,
>  	loff_t			offset,
>  	loff_t			count,
> +	unsigned		flags,
>  	struct iomap		*iomap)
>  {
>  	struct xfs_inode	*ip = XFS_I(inode);
> @@ -539,8 +555,12 @@ xfs_file_iomap_begin_delay(
>  	}
>  
>  	eof = !xfs_iext_lookup_extent(ip, ifp, offset_fsb, &icur, &got);
> -	if (!eof && got.br_startoff <= offset_fsb) {
> -		if (xfs_is_reflink_inode(ip)) {
> +	if (eof)
> +		got.br_startoff = maxbytes_fsb;

What's the purpose of this? Can't we just continue to use eof in the
logic below and report holes up through the requested range (offset +
length) just like the other branch does (via xfs_bmapi_read())?

> +	if (got.br_startoff <= offset_fsb) {
> +		if (xfs_is_reflink_inode(ip) &&
> +		    ((flags & IOMAP_WRITE) ||
> +		     got.br_state != XFS_EXT_UNWRITTEN)) {

I think a small comment is useful here due to the implicit logic. For
example:

/* reservation is required for writes and zeroing over normal extents */

Otherwise this seems fine.

Brian

>  			bool		shared;
>  
>  			end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
> @@ -555,6 +575,11 @@ xfs_file_iomap_begin_delay(
>  		goto done;
>  	}
>  
> +	if (flags & IOMAP_ZERO) {
> +		xfs_hole_to_iomap(ip, iomap, offset_fsb, got.br_startoff);
> +		goto out_unlock;
> +	}
> +
>  	error = xfs_qm_dqattach_locked(ip, false);
>  	if (error)
>  		goto out_unlock;
> @@ -1009,10 +1034,11 @@ xfs_file_iomap_begin(
>  	if (XFS_FORCED_SHUTDOWN(mp))
>  		return -EIO;
>  
> -	if (((flags & (IOMAP_WRITE | IOMAP_DIRECT)) == IOMAP_WRITE) &&
> +	if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && !(flags & IOMAP_DIRECT) &&
>  			!IS_DAX(inode) && !xfs_get_extsz_hint(ip)) {
>  		/* Reserve delalloc blocks for regular writeback. */
> -		return xfs_file_iomap_begin_delay(inode, offset, length, iomap);
> +		return xfs_file_iomap_begin_delay(inode, offset, length, flags,
> +				iomap);
>  	}
>  
>  	/*
> -- 
> 2.19.0
> 

Re: [PATCH 4/7] xfs: remove the unused shared argument to xfs_reflink_reserve_cow

[Brian Foster]

On Mon, Oct 01, 2018 at 05:37:38AM -0700, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_iomap.c   |  6 ++----
>  fs/xfs/xfs_reflink.c | 12 +++++-------
>  fs/xfs/xfs_reflink.h |  2 +-
>  3 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 7cbebcf61fa7..8ba97e67b474 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -561,12 +561,10 @@ xfs_file_iomap_begin_delay(
>  		if (xfs_is_reflink_inode(ip) &&
>  		    ((flags & IOMAP_WRITE) ||
>  		     got.br_state != XFS_EXT_UNWRITTEN)) {
> -			bool		shared;
> -
>  			end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
>  					maxbytes_fsb);
>  			xfs_trim_extent(&got, offset_fsb, end_fsb - offset_fsb);
> -			error = xfs_reflink_reserve_cow(ip, &got, &shared);
> +			error = xfs_reflink_reserve_cow(ip, &got);
>  			if (error)
>  				goto out_unlock;
>  		}
> @@ -1091,7 +1089,7 @@ xfs_file_iomap_begin(
>  			if (error)
>  				goto out_unlock;
>  		} else {
> -			error = xfs_reflink_reserve_cow(ip, &imap, &shared);
> +			error = xfs_reflink_reserve_cow(ip, &imap);
>  			if (error)
>  				goto out_unlock;
>  		}
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 5289e22cb081..06e38e88ddee 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -241,7 +241,7 @@ xfs_reflink_trim_around_shared(
>  /*
>   * Trim the passed in imap to the next shared/unshared extent boundary, and
>   * if imap->br_startoff points to a shared extent reserve space for it in the
> - * COW fork.  In this case *shared is set to true, else to false.
> + * COW fork.
>   *
>   * Note that imap will always contain the block numbers for the existing blocks
>   * in the data fork, as the upper layers need them for read-modify-write
> @@ -250,14 +250,14 @@ xfs_reflink_trim_around_shared(
>  int
>  xfs_reflink_reserve_cow(
>  	struct xfs_inode	*ip,
> -	struct xfs_bmbt_irec	*imap,
> -	bool			*shared)
> +	struct xfs_bmbt_irec	*imap)
>  {
>  	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
>  	struct xfs_bmbt_irec	got;
>  	int			error = 0;
>  	bool			eof = false, trimmed;
>  	struct xfs_iext_cursor	icur;
> +	bool			shared;
>  
>  	/*
>  	 * Search the COW fork extent list first.  This serves two purposes:
> @@ -273,18 +273,16 @@ xfs_reflink_reserve_cow(
>  	if (!eof && got.br_startoff <= imap->br_startoff) {
>  		trace_xfs_reflink_cow_found(ip, imap);
>  		xfs_trim_extent(imap, got.br_startoff, got.br_blockcount);
> -
> -		*shared = true;
>  		return 0;
>  	}
>  
>  	/* Trim the mapping to the nearest shared extent boundary. */
> -	error = xfs_reflink_trim_around_shared(ip, imap, shared, &trimmed);
> +	error = xfs_reflink_trim_around_shared(ip, imap, &shared, &trimmed);
>  	if (error)
>  		return error;
>  
>  	/* Not shared?  Just report the (potentially capped) extent. */
> -	if (!*shared)
> +	if (!shared)
>  		return 0;
>  
>  	/*
> diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
> index c585ad9552b2..b77f4079022a 100644
> --- a/fs/xfs/xfs_reflink.h
> +++ b/fs/xfs/xfs_reflink.h
> @@ -13,7 +13,7 @@ extern int xfs_reflink_trim_around_shared(struct xfs_inode *ip,
>  		struct xfs_bmbt_irec *irec, bool *shared, bool *trimmed);
>  
>  extern int xfs_reflink_reserve_cow(struct xfs_inode *ip,
> -		struct xfs_bmbt_irec *imap, bool *shared);
> +		struct xfs_bmbt_irec *imap);
>  extern int xfs_reflink_allocate_cow(struct xfs_inode *ip,
>  		struct xfs_bmbt_irec *imap, bool *shared, uint *lockmode);
>  extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset,
> -- 
> 2.19.0
> 

Re: [PATCH 5/7] xfs: remove the unused trimmed argument from xfs_reflink_trim_around_shared

[Brian Foster]

On Mon, Oct 01, 2018 at 05:37:39AM -0700, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_bmap_util.c |  4 ++--
>  fs/xfs/xfs_iomap.c     |  5 ++---
>  fs/xfs/xfs_reflink.c   | 15 +++++----------
>  fs/xfs/xfs_reflink.h   |  2 +-
>  4 files changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 6de8d90041ff..8d2579847076 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -406,10 +406,10 @@ xfs_getbmap_report_one(
>  	struct xfs_bmbt_irec	*got)
>  {
>  	struct kgetbmap		*p = out + bmv->bmv_entries;
> -	bool			shared = false, trimmed = false;
> +	bool			shared = false;
>  	int			error;
>  
> -	error = xfs_reflink_trim_around_shared(ip, got, &shared, &trimmed);
> +	error = xfs_reflink_trim_around_shared(ip, got, &shared);
>  	if (error)
>  		return error;
>  
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 8ba97e67b474..1224eced1ee4 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -1026,7 +1026,7 @@ xfs_file_iomap_begin(
>  	struct xfs_bmbt_irec	imap;
>  	xfs_fileoff_t		offset_fsb, end_fsb;
>  	int			nimaps = 1, error = 0;
> -	bool			shared = false, trimmed = false;
> +	bool			shared = false;
>  	unsigned		lockmode;
>  
>  	if (XFS_FORCED_SHUTDOWN(mp))
> @@ -1062,8 +1062,7 @@ xfs_file_iomap_begin(
>  
>  	if (flags & IOMAP_REPORT) {
>  		/* Trim the mapping to the nearest shared extent boundary. */
> -		error = xfs_reflink_trim_around_shared(ip, &imap, &shared,
> -				&trimmed);
> +		error = xfs_reflink_trim_around_shared(ip, &imap, &shared);
>  		if (error)
>  			goto out_unlock;
>  	}
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 06e38e88ddee..1e39a7d21c7e 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -182,8 +182,7 @@ int
>  xfs_reflink_trim_around_shared(
>  	struct xfs_inode	*ip,
>  	struct xfs_bmbt_irec	*irec,
> -	bool			*shared,
> -	bool			*trimmed)
> +	bool			*shared)
>  {
>  	xfs_agnumber_t		agno;
>  	xfs_agblock_t		agbno;
> @@ -209,7 +208,7 @@ xfs_reflink_trim_around_shared(
>  	if (error)
>  		return error;
>  
> -	*shared = *trimmed = false;
> +	*shared = false;
>  	if (fbno == NULLAGBLOCK) {
>  		/* No shared blocks at all. */
>  		return 0;
> @@ -222,8 +221,6 @@ xfs_reflink_trim_around_shared(
>  		 */
>  		irec->br_blockcount = flen;
>  		*shared = true;
> -		if (flen != aglen)
> -			*trimmed = true;
>  		return 0;
>  	} else {
>  		/*
> @@ -233,7 +230,6 @@ xfs_reflink_trim_around_shared(
>  		 * start of the shared region.
>  		 */
>  		irec->br_blockcount = fbno - agbno;
> -		*trimmed = true;
>  		return 0;
>  	}
>  }
> @@ -255,7 +251,7 @@ xfs_reflink_reserve_cow(
>  	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
>  	struct xfs_bmbt_irec	got;
>  	int			error = 0;
> -	bool			eof = false, trimmed;
> +	bool			eof = false;
>  	struct xfs_iext_cursor	icur;
>  	bool			shared;
>  
> @@ -277,7 +273,7 @@ xfs_reflink_reserve_cow(
>  	}
>  
>  	/* Trim the mapping to the nearest shared extent boundary. */
> -	error = xfs_reflink_trim_around_shared(ip, imap, &shared, &trimmed);
> +	error = xfs_reflink_trim_around_shared(ip, imap, &shared);
>  	if (error)
>  		return error;
>  
> @@ -366,7 +362,6 @@ xfs_find_trim_cow_extent(
>  	xfs_filblks_t		count_fsb = imap->br_blockcount;
>  	struct xfs_iext_cursor	icur;
>  	struct xfs_bmbt_irec	got;
> -	bool			trimmed;
>  
>  	*found = false;
>  
> @@ -376,7 +371,7 @@ xfs_find_trim_cow_extent(
>  	 */
>  	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);
> +		return xfs_reflink_trim_around_shared(ip, imap, shared);
>  
>  	*shared = true;
>  	if (isnullstartblock(got.br_startblock)) {
> diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
> index b77f4079022a..7f47202b5639 100644
> --- a/fs/xfs/xfs_reflink.h
> +++ b/fs/xfs/xfs_reflink.h
> @@ -10,7 +10,7 @@ extern int xfs_reflink_find_shared(struct xfs_mount *mp, struct xfs_trans *tp,
>  		xfs_agnumber_t agno, xfs_agblock_t agbno, xfs_extlen_t aglen,
>  		xfs_agblock_t *fbno, xfs_extlen_t *flen, bool find_maximal);
>  extern int xfs_reflink_trim_around_shared(struct xfs_inode *ip,
> -		struct xfs_bmbt_irec *irec, bool *shared, bool *trimmed);
> +		struct xfs_bmbt_irec *irec, bool *shared);
>  
>  extern int xfs_reflink_reserve_cow(struct xfs_inode *ip,
>  		struct xfs_bmbt_irec *imap);
> -- 
> 2.19.0
> 

Re: [PATCH 6/7] xfs: fix fork selection in xfs_find_trim_cow_extent

[Brian Foster]

On Mon, Oct 01, 2018 at 05:37:40AM -0700, Christoph Hellwig wrote:
> We should want to write directly into the data fork for blocks that don't
> have an extent in the COW fork covering them yet.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_reflink.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 1e39a7d21c7e..ead35209ffae 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -369,9 +369,13 @@ xfs_find_trim_cow_extent(
>  	 * 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)
> +	if (!xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, &icur, &got))
> +		got.br_startoff = offset_fsb + count_fsb;
> +	if (got.br_startoff > offset_fsb) {
> +		xfs_trim_extent(imap, imap->br_startoff,
> +				got.br_startoff - imap->br_startoff);
>  		return xfs_reflink_trim_around_shared(ip, imap, shared);
> +	}
>  
>  	*shared = true;
>  	if (isnullstartblock(got.br_startblock)) {
> -- 
> 2.19.0
> 

Re: [PATCH 7/7] xfs: print dangling delalloc extents

[Brian Foster]

On Mon, Oct 01, 2018 at 05:37:41AM -0700, Christoph Hellwig wrote:
> Instead of just asserting that we have no delalloc space dangling
> in an inode that gets freed print the actual offenders for debug
> mode.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Thanks for the tweak:

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

>  fs/xfs/xfs_super.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 207ee302b1bb..99250bcb65a7 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -933,6 +933,32 @@ xfs_fs_alloc_inode(
>  	return NULL;
>  }
>  
> +#ifdef DEBUG
> +static void
> +xfs_check_delalloc(
> +	struct xfs_inode	*ip,
> +	int			whichfork)
> +{
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
> +	struct xfs_bmbt_irec	got;
> +	struct xfs_iext_cursor	icur;
> +
> +	if (!ifp || !xfs_iext_lookup_extent(ip, ifp, 0, &icur, &got))
> +		return;
> +	do {
> +		if (isnullstartblock(got.br_startblock)) {
> +			xfs_warn(ip->i_mount,
> +	"ino %llx %s fork has delalloc extent at [0x%llx:0x%llx]",
> +				ip->i_ino,
> +				whichfork == XFS_DATA_FORK ? "data" : "cow",
> +				got.br_startoff, got.br_blockcount);
> +		}
> +	} while (xfs_iext_next_extent(ifp, &icur, &got));
> +}
> +#else
> +#define xfs_check_delalloc(ip, whichfork)	do { } while (0)
> +#endif
> +
>  /*
>   * Now that the generic code is guaranteed not to be accessing
>   * the linux inode, we can inactivate and reclaim the inode.
> @@ -951,7 +977,12 @@ xfs_fs_destroy_inode(
>  
>  	xfs_inactive(ip);
>  
> -	ASSERT(XFS_FORCED_SHUTDOWN(ip->i_mount) || ip->i_delayed_blks == 0);
> +	if (!XFS_FORCED_SHUTDOWN(ip->i_mount) && ip->i_delayed_blks) {
> +		xfs_check_delalloc(ip, XFS_DATA_FORK);
> +		xfs_check_delalloc(ip, XFS_COW_FORK);
> +		ASSERT(0);
> +	}
> +
>  	XFS_STATS_INC(ip->i_mount, vn_reclaim);
>  
>  	/*
> -- 
> 2.19.0
> 

Re: [PATCH 3/7] xfs: handle zeroing in xfs_file_iomap_begin_delay

[Christoph Hellwig]

On Mon, Oct 01, 2018 at 10:20:05AM -0400, Brian Foster wrote:
> >  	eof = !xfs_iext_lookup_extent(ip, ifp, offset_fsb, &icur, &got);
> > -	if (!eof && got.br_startoff <= offset_fsb) {
> > -		if (xfs_is_reflink_inode(ip)) {
> > +	if (eof)
> > +		got.br_startoff = maxbytes_fsb;
> 
> What's the purpose of this? Can't we just continue to use eof in the
> logic below and report holes up through the requested range (offset +
> length) just like the other branch does (via xfs_bmapi_read())?

Setting the startblock makes the code a lot simpler.  But yes,
we could limit the hole to the reporting range.

> 
> > +	if (got.br_startoff <= offset_fsb) {
> > +		if (xfs_is_reflink_inode(ip) &&
> > +		    ((flags & IOMAP_WRITE) ||
> > +		     got.br_state != XFS_EXT_UNWRITTEN)) {
> 
> I think a small comment is useful here due to the implicit logic. For
> example:
> 
> /* reservation is required for writes and zeroing over normal extents */

Ok.