even more extent mapping cleanups

even more extent mapping cleanups

[Christoph Hellwig]

Hi all,

This series contains yey another round of extent map related cleanups
to prepare for the new incore extent list.

The first one adds the additional asserts requested by Darrick, and
the rest are cleanups that bubbled to the front of my local queue
during a rebase over the weekend.

Because it is based on a huge number of outstanding (and reviewed)
patches it might be a good idea to pull in the git tree for testing
or detailed review:

	git://git.infradead.org/users/hch/xfs.git xfs-bmbt-cleanup.3

Gitweb:

	http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-bmbt-cleanup.3

[PATCH 1/4] xfs: add asserts for the mmap lock in xfs_{insert,collapse}_file_space

[Christoph Hellwig]

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

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 034f3429ca8c..170b74c7f2d5 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1334,6 +1334,8 @@ xfs_collapse_file_space(
 	bool			done = false;
 
 	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+	ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
+
 	trace_xfs_collapse_file_space(ip);
 
 	error = xfs_free_file_space(ip, offset, len);
@@ -1408,6 +1410,8 @@ xfs_insert_file_space(
 	bool			done = false;
 
 	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+	ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
+
 	trace_xfs_insert_file_space(ip);
 
 	error = xfs_prepare_shift(ip, offset);
-- 
2.14.2

[PATCH 2/4] xfs: remove xfs_bmbt_validate_extent

[Christoph Hellwig]

We have stop supporting file systems without unwritten extent bit support
a long time ago, so remove the debug code for it which will get in the way
of future changes.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c       |  8 +-------
 fs/xfs/libxfs/xfs_bmap_btree.h | 14 --------------
 fs/xfs/libxfs/xfs_inode_fork.c |  7 -------
 3 files changed, 1 insertion(+), 28 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 46813b71dd74..19ec8b1f99dd 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1166,8 +1166,7 @@ xfs_bmap_add_attrfork(
 /*
  * Read in the extents to if_extents.
  * All inode fields are set up by caller, we just traverse the btree
- * and copy the records in. If the file system cannot contain unwritten
- * extents, the records are checked for no "state" flags.
+ * and copy the records in.
  */
 int					/* error */
 xfs_bmap_read_extents(
@@ -1255,11 +1254,6 @@ xfs_bmap_read_extents(
 			xfs_bmbt_rec_host_t *trp = xfs_iext_get_ext(ifp, i);
 			trp->l0 = be64_to_cpu(frp->l0);
 			trp->l1 = be64_to_cpu(frp->l1);
-			if (!xfs_bmbt_validate_extent(mp, whichfork, trp)) {
-				XFS_ERROR_REPORT("xfs_bmap_read_extents(2)",
-						 XFS_ERRLEVEL_LOW, mp);
-				goto error0;
-			}
 			trace_xfs_read_extent(ip, i, state, _THIS_IP_);
 		}
 		xfs_trans_brelse(tp, bp);
diff --git a/fs/xfs/libxfs/xfs_bmap_btree.h b/fs/xfs/libxfs/xfs_bmap_btree.h
index 6f891eeb88f6..82d397de8e00 100644
--- a/fs/xfs/libxfs/xfs_bmap_btree.h
+++ b/fs/xfs/libxfs/xfs_bmap_btree.h
@@ -123,18 +123,4 @@ extern int xfs_bmbt_change_owner(struct xfs_trans *tp, struct xfs_inode *ip,
 extern struct xfs_btree_cur *xfs_bmbt_init_cursor(struct xfs_mount *,
 		struct xfs_trans *, struct xfs_inode *, int);
 
-/*
- * Check that the extent does not contain an invalid unwritten extent flag.
- */
-static inline bool xfs_bmbt_validate_extent(struct xfs_mount *mp, int whichfork,
-		struct xfs_bmbt_rec_host *ep)
-{
-	if (ep->l0 >> (64 - BMBT_EXNTFLAG_BITLEN) == 0)
-		return true;
-	if (whichfork == XFS_DATA_FORK &&
-	    xfs_sb_version_hasextflgbit(&mp->m_sb))
-		return true;
-	return false;
-}
-
 #endif	/* __XFS_BMAP_BTREE_H__ */
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index b1e69734c450..48a5dec360cd 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -373,11 +373,6 @@ xfs_iformat_extents(
 			xfs_bmbt_rec_host_t *ep = xfs_iext_get_ext(ifp, i);
 			ep->l0 = get_unaligned_be64(&dp->l0);
 			ep->l1 = get_unaligned_be64(&dp->l1);
-			if (!xfs_bmbt_validate_extent(mp, whichfork, ep)) {
-				XFS_ERROR_REPORT("xfs_iformat_extents(2)",
-						 XFS_ERRLEVEL_LOW, mp);
-				return -EFSCORRUPTED;
-			}
 			trace_xfs_read_extent(ip, i, state, _THIS_IP_);
 		}
 	}
@@ -801,8 +796,6 @@ xfs_iextents_copy(
 	for (i = 0; i < nrecs; i++) {
 		xfs_bmbt_rec_host_t *ep = xfs_iext_get_ext(ifp, i);
 
-		ASSERT(xfs_bmbt_validate_extent(ip->i_mount, whichfork, ep));
-
 		start_block = xfs_bmbt_get_startblock(ep);
 		if (isnullstartblock(start_block)) {
 			/*
-- 
2.14.2

[PATCH 3/4] xfs: merge xfs_bmap_read_extents into xfs_iread_extents

[Christoph Hellwig]

xfs_iread_extents is just a trivial wrapper, there is no good reason
to keep the two separate.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c       | 83 +++++++++++++++++++++++++-----------------
 fs/xfs/libxfs/xfs_bmap.h       |  2 -
 fs/xfs/libxfs/xfs_inode_fork.c | 37 -------------------
 3 files changed, 49 insertions(+), 73 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 19ec8b1f99dd..53ff6d92b532 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1164,32 +1164,37 @@ xfs_bmap_add_attrfork(
  */
 
 /*
- * Read in the extents to if_extents.
- * All inode fields are set up by caller, we just traverse the btree
- * and copy the records in.
+ * Read in extents from a btree-format inode.
  */
-int					/* error */
-xfs_bmap_read_extents(
-	xfs_trans_t		*tp,	/* transaction pointer */
-	xfs_inode_t		*ip,	/* incore inode */
-	int			whichfork) /* data or attr fork */
+int
+xfs_iread_extents(
+	struct xfs_trans	*tp,
+	struct xfs_inode	*ip,
+	int			whichfork)
 {
-	struct xfs_btree_block	*block;	/* current btree block */
-	xfs_fsblock_t		bno;	/* block # of "block" */
-	xfs_buf_t		*bp;	/* buffer for "block" */
-	int			error;	/* error return value */
-	xfs_extnum_t		i, j;	/* index into the extents list */
-	xfs_ifork_t		*ifp;	/* fork structure */
-	int			level;	/* btree level, for checking */
-	xfs_mount_t		*mp;	/* file system mount structure */
-	__be64			*pp;	/* pointer to block address */
-	/* REFERENCED */
-	xfs_extnum_t		room;	/* number of entries there's room for */
+	struct xfs_mount	*mp = ip->i_mount;
 	int			state = xfs_bmap_fork_to_state(whichfork);
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
+	xfs_extnum_t		nextents = XFS_IFORK_NEXTENTS(ip, whichfork);
+	struct xfs_btree_block	*block = ifp->if_broot;
+	xfs_fsblock_t		bno;
+	struct xfs_buf		*bp;
+	xfs_extnum_t		i, j;
+	int			level;
+	__be64			*pp;
+	int			error;
+
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+
+	if (unlikely(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE)) {
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
+		return -EFSCORRUPTED;
+	}
+
+	ifp->if_bytes = 0;
+	ifp->if_real_bytes = 0;
+	xfs_iext_add(ifp, 0, nextents);
 
-	mp = ip->i_mount;
-	ifp = XFS_IFORK_PTR(ip, whichfork);
-	block = ifp->if_broot;
 	/*
 	 * Root level must use BMAP_BROOT_PTR_ADDR macro to get ptr out.
 	 */
@@ -1206,21 +1211,22 @@ xfs_bmap_read_extents(
 		error = xfs_btree_read_bufl(mp, tp, bno, 0, &bp,
 				XFS_BMAP_BTREE_REF, &xfs_bmbt_buf_ops);
 		if (error)
-			return error;
+			goto out;
 		block = XFS_BUF_TO_BLOCK(bp);
 		if (level == 0)
 			break;
 		pp = XFS_BMBT_PTR_ADDR(mp, block, 1, mp->m_bmap_dmxr[1]);
 		bno = be64_to_cpu(*pp);
 		XFS_WANT_CORRUPTED_GOTO(mp,
-			XFS_FSB_SANITY_CHECK(mp, bno), error0);
+			XFS_FSB_SANITY_CHECK(mp, bno), out_brelse);
 		xfs_trans_brelse(tp, bp);
 	}
+
 	/*
 	 * Here with bp and block set to the leftmost leaf node in the tree.
 	 */
-	room = xfs_iext_count(ifp);
 	i = 0;
+
 	/*
 	 * Loop over all leaf nodes.  Copy information to the extent records.
 	 */
@@ -1230,14 +1236,15 @@ xfs_bmap_read_extents(
 		xfs_extnum_t	num_recs;
 
 		num_recs = xfs_btree_get_numrecs(block);
-		if (unlikely(i + num_recs > room)) {
-			ASSERT(i + num_recs <= room);
+		if (unlikely(i + num_recs > nextents)) {
+			ASSERT(i + num_recs <= nextents);
 			xfs_warn(ip->i_mount,
 				"corrupt dinode %Lu, (btree extents).",
 				(unsigned long long) ip->i_ino);
-			XFS_CORRUPTION_ERROR("xfs_bmap_read_extents(1)",
+			XFS_CORRUPTION_ERROR(__func__,
 				XFS_ERRLEVEL_LOW, ip->i_mount, block);
-			goto error0;
+			error = -EFSCORRUPTED;
+			goto out_brelse;
 		}
 		/*
 		 * Read-ahead the next leaf block, if any.
@@ -1266,16 +1273,24 @@ xfs_bmap_read_extents(
 		error = xfs_btree_read_bufl(mp, tp, bno, 0, &bp,
 				XFS_BMAP_BTREE_REF, &xfs_bmbt_buf_ops);
 		if (error)
-			return error;
+			goto out;
 		block = XFS_BUF_TO_BLOCK(bp);
 	}
-	if (i != XFS_IFORK_NEXTENTS(ip, whichfork))
-		return -EFSCORRUPTED;
+
+	if (i != XFS_IFORK_NEXTENTS(ip, whichfork)) {
+		error = -EFSCORRUPTED;
+		goto out;
+	}
 	ASSERT(i == xfs_iext_count(ifp));
+
+	ifp->if_flags |= XFS_IFEXTENTS;
 	return 0;
-error0:
+
+out_brelse:
 	xfs_trans_brelse(tp, bp);
-	return -EFSCORRUPTED;
+out:
+	xfs_iext_destroy(ifp);
+	return error;
 }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index 6c426cdfb758..de34bad03c46 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -198,8 +198,6 @@ int	xfs_bmap_last_before(struct xfs_trans *tp, struct xfs_inode *ip,
 int	xfs_bmap_last_offset(struct xfs_inode *ip, xfs_fileoff_t *unused,
 		int whichfork);
 int	xfs_bmap_one_block(struct xfs_inode *ip, int whichfork);
-int	xfs_bmap_read_extents(struct xfs_trans *tp, struct xfs_inode *ip,
-		int whichfork);
 int	xfs_bmapi_read(struct xfs_inode *ip, xfs_fileoff_t bno,
 		xfs_filblks_t len, struct xfs_bmbt_irec *mval,
 		int *nmap, int flags);
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 48a5dec360cd..c70f9ef07b6d 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -443,43 +443,6 @@ xfs_iformat_btree(
 	return 0;
 }
 
-/*
- * Read in extents from a btree-format inode.
- * Allocate and fill in if_extents.  Real work is done in xfs_bmap.c.
- */
-int
-xfs_iread_extents(
-	xfs_trans_t	*tp,
-	xfs_inode_t	*ip,
-	int		whichfork)
-{
-	int		error;
-	xfs_ifork_t	*ifp;
-	xfs_extnum_t	nextents;
-
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
-
-	if (unlikely(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE)) {
-		XFS_ERROR_REPORT("xfs_iread_extents", XFS_ERRLEVEL_LOW,
-				 ip->i_mount);
-		return -EFSCORRUPTED;
-	}
-	nextents = XFS_IFORK_NEXTENTS(ip, whichfork);
-	ifp = XFS_IFORK_PTR(ip, whichfork);
-
-	/*
-	 * We know that the size is valid (it's checked in iformat_btree)
-	 */
-	ifp->if_bytes = ifp->if_real_bytes = 0;
-	xfs_iext_add(ifp, 0, nextents);
-	error = xfs_bmap_read_extents(tp, ip, whichfork);
-	if (error) {
-		xfs_iext_destroy(ifp);
-		return error;
-	}
-	ifp->if_flags |= XFS_IFEXTENTS;
-	return 0;
-}
 /*
  * Reallocate the space for if_broot based on the number of records
  * being added or deleted as indicated in rec_diff.  Move the records
-- 
2.14.2

[PATCH 4/4] xfs: add a new xfs_iext_lookup_extent_before helper

[Christoph Hellwig]

This helper looks up the last extent the covers space before the passed
in block number.  This is useful for truncate and similar operations that
operate backwards over the extent list.  For xfs_bunmapi it also is
a slight optimization as we can return early if there are not extents
at or below the end of the to be truncated range.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c       | 27 +++++++--------------------
 fs/xfs/libxfs/xfs_inode_fork.c | 21 +++++++++++++++++++++
 fs/xfs/libxfs/xfs_inode_fork.h |  4 ++++
 fs/xfs/xfs_reflink.c           | 19 +++++++------------
 4 files changed, 39 insertions(+), 32 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 53ff6d92b532..b26c4ece7cbe 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1380,17 +1380,8 @@ xfs_bmap_last_before(
 			return error;
 	}
 
-	if (xfs_iext_lookup_extent(ip, ifp, *last_block - 1, &idx, &got)) {
-		if (got.br_startoff <= *last_block - 1)
-			return 0;
-	}
-
-	if (xfs_iext_get_extent(ifp, idx - 1, &got)) {
-		*last_block = got.br_startoff + got.br_blockcount;
-		return 0;
-	}
-
-	*last_block = 0;
+	if (!xfs_iext_lookup_extent_before(ip, ifp, last_block, &idx, &got))
+		*last_block = 0;
 	return 0;
 }
 
@@ -5154,17 +5145,13 @@ __xfs_bunmapi(
 	}
 	XFS_STATS_INC(mp, xs_blk_unmap);
 	isrt = (whichfork == XFS_DATA_FORK) && XFS_IS_REALTIME_INODE(ip);
-	end = start + len - 1;
+	end = start + len;
 
-	/*
-	 * Check to see if the given block number is past the end of the
-	 * file, back up to the last block if so...
-	 */
-	if (!xfs_iext_lookup_extent(ip, ifp, end, &lastx, &got)) {
-		ASSERT(lastx > 0);
-		xfs_iext_get_extent(ifp, --lastx, &got);
-		end = got.br_startoff + got.br_blockcount - 1;
+	if (!xfs_iext_lookup_extent_before(ip, ifp, &end, &lastx, &got)) {
+		*rlen = 0;
+		return 0;
 	}
+	end--;
 
 	logflags = 0;
 	if (ifp->if_flags & XFS_IFBROOT) {
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index c70f9ef07b6d..5b90f912e41a 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -1960,6 +1960,27 @@ xfs_iext_lookup_extent(
 	return true;
 }
 
+/*
+ * Returns the last extent before end, and if this extent doesn't cover
+ * end, update end to the end of the extent.
+ */
+bool
+xfs_iext_lookup_extent_before(
+	struct xfs_inode	*ip,
+	struct xfs_ifork	*ifp,
+	xfs_fileoff_t		*end,
+	xfs_extnum_t		*idxp,
+	struct xfs_bmbt_irec	*gotp)
+{
+	if (xfs_iext_lookup_extent(ip, ifp, *end - 1, idxp, gotp) &&
+	    gotp->br_startoff <= *end - 1)
+		return true;
+	if (!xfs_iext_get_extent(ifp, --*idxp, gotp))
+		return false;
+	*end = gotp->br_startoff + gotp->br_blockcount;
+	return true;
+}
+
 /*
  * Return true if there is an extent at index idx, and return the expanded
  * extent structure at idx in that case.  Else return false.
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index e0c42ea9b8d0..113fd42ec36d 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -183,6 +183,10 @@ void		xfs_iext_irec_update_extoffs(struct xfs_ifork *, int, int);
 bool		xfs_iext_lookup_extent(struct xfs_inode *ip,
 			struct xfs_ifork *ifp, xfs_fileoff_t bno,
 			xfs_extnum_t *idxp, struct xfs_bmbt_irec *gotp);
+bool		xfs_iext_lookup_extent_before(struct xfs_inode *ip,
+			struct xfs_ifork *ifp, xfs_fileoff_t *end,
+			xfs_extnum_t *idxp, struct xfs_bmbt_irec *gotp);
+
 bool		xfs_iext_get_extent(struct xfs_ifork *ifp, xfs_extnum_t idx,
 			struct xfs_bmbt_irec *gotp);
 void		xfs_iext_update_extent(struct xfs_inode *ip, int state,
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 37e603bf1591..1205747e1409 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -733,18 +733,13 @@ xfs_reflink_end_cow(
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, 0);
 
-	/* If there is a hole at end_fsb - 1 go to the previous extent */
-	if (!xfs_iext_lookup_extent(ip, ifp, end_fsb - 1, &idx, &got) ||
-	    got.br_startoff > end_fsb) {
-		/*
-		 * In case of racing, overlapping AIO writes no COW extents
-		 * might be left by the time I/O completes for the loser of
-		 * the race.  In that case we are done.
-		 */
-		if (idx <= 0)
-			goto out_cancel;
-		xfs_iext_get_extent(ifp, --idx, &got);
-	}
+	/*
+	 * In case of racing, overlapping AIO writes no COW extents might be
+	 * left by the time I/O completes for the loser of the race.  In that
+	 * case we are done.
+	 */
+	if (!xfs_iext_lookup_extent_before(ip, ifp, &end_fsb, &idx, &got))
+		goto out_cancel;
 
 	/* Walk backwards until we're out of the I/O range... */
 	while (got.br_startoff + got.br_blockcount > offset_fsb) {
-- 
2.14.2

Re: [PATCH 2/4] xfs: remove xfs_bmbt_validate_extent

[Darrick J. Wong]

On Mon, Oct 23, 2017 at 08:30:15AM +0200, Christoph Hellwig wrote:
> We have stop supporting file systems without unwritten extent bit support
> a long time ago, so remove the debug code for it which will get in the way
> of future changes.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c       |  8 +-------
>  fs/xfs/libxfs/xfs_bmap_btree.h | 14 --------------
>  fs/xfs/libxfs/xfs_inode_fork.c |  7 -------
>  3 files changed, 1 insertion(+), 28 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 46813b71dd74..19ec8b1f99dd 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -1166,8 +1166,7 @@ xfs_bmap_add_attrfork(
>  /*
>   * Read in the extents to if_extents.
>   * All inode fields are set up by caller, we just traverse the btree
> - * and copy the records in. If the file system cannot contain unwritten
> - * extents, the records are checked for no "state" flags.
> + * and copy the records in.
>   */
>  int					/* error */
>  xfs_bmap_read_extents(
> @@ -1255,11 +1254,6 @@ xfs_bmap_read_extents(
>  			xfs_bmbt_rec_host_t *trp = xfs_iext_get_ext(ifp, i);
>  			trp->l0 = be64_to_cpu(frp->l0);
>  			trp->l1 = be64_to_cpu(frp->l1);
> -			if (!xfs_bmbt_validate_extent(mp, whichfork, trp)) {
> -				XFS_ERROR_REPORT("xfs_bmap_read_extents(2)",
> -						 XFS_ERRLEVEL_LOW, mp);
> -				goto error0;
> -			}
>  			trace_xfs_read_extent(ip, i, state, _THIS_IP_);
>  		}
>  		xfs_trans_brelse(tp, bp);
> diff --git a/fs/xfs/libxfs/xfs_bmap_btree.h b/fs/xfs/libxfs/xfs_bmap_btree.h
> index 6f891eeb88f6..82d397de8e00 100644
> --- a/fs/xfs/libxfs/xfs_bmap_btree.h
> +++ b/fs/xfs/libxfs/xfs_bmap_btree.h
> @@ -123,18 +123,4 @@ extern int xfs_bmbt_change_owner(struct xfs_trans *tp, struct xfs_inode *ip,
>  extern struct xfs_btree_cur *xfs_bmbt_init_cursor(struct xfs_mount *,
>  		struct xfs_trans *, struct xfs_inode *, int);
>  
> -/*
> - * Check that the extent does not contain an invalid unwritten extent flag.
> - */
> -static inline bool xfs_bmbt_validate_extent(struct xfs_mount *mp, int whichfork,
> -		struct xfs_bmbt_rec_host *ep)
> -{
> -	if (ep->l0 >> (64 - BMBT_EXNTFLAG_BITLEN) == 0)
> -		return true;
> -	if (whichfork == XFS_DATA_FORK &&
> -	    xfs_sb_version_hasextflgbit(&mp->m_sb))
> -		return true;
> -	return false;

By removing this, I think we no longer report corruption if we encounter
'unwritten' attr fork extents, which (AFAIK) aren't allowed.

--D

> -}
> -
>  #endif	/* __XFS_BMAP_BTREE_H__ */
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index b1e69734c450..48a5dec360cd 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -373,11 +373,6 @@ xfs_iformat_extents(
>  			xfs_bmbt_rec_host_t *ep = xfs_iext_get_ext(ifp, i);
>  			ep->l0 = get_unaligned_be64(&dp->l0);
>  			ep->l1 = get_unaligned_be64(&dp->l1);
> -			if (!xfs_bmbt_validate_extent(mp, whichfork, ep)) {
> -				XFS_ERROR_REPORT("xfs_iformat_extents(2)",
> -						 XFS_ERRLEVEL_LOW, mp);
> -				return -EFSCORRUPTED;
> -			}



>  			trace_xfs_read_extent(ip, i, state, _THIS_IP_);
>  		}
>  	}
> @@ -801,8 +796,6 @@ xfs_iextents_copy(
>  	for (i = 0; i < nrecs; i++) {
>  		xfs_bmbt_rec_host_t *ep = xfs_iext_get_ext(ifp, i);
>  
> -		ASSERT(xfs_bmbt_validate_extent(ip->i_mount, whichfork, ep));
> -
>  		start_block = xfs_bmbt_get_startblock(ep);
>  		if (isnullstartblock(start_block)) {
>  			/*
> -- 
> 2.14.2
> 
> --
> 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 1/4] xfs: add asserts for the mmap lock in xfs_{insert,collapse}_file_space

[Darrick J. Wong]

On Mon, Oct 23, 2017 at 08:30:14AM +0200, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
>  fs/xfs/xfs_bmap_util.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 034f3429ca8c..170b74c7f2d5 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1334,6 +1334,8 @@ xfs_collapse_file_space(
>  	bool			done = false;
>  
>  	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> +	ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
> +
>  	trace_xfs_collapse_file_space(ip);
>  
>  	error = xfs_free_file_space(ip, offset, len);
> @@ -1408,6 +1410,8 @@ xfs_insert_file_space(
>  	bool			done = false;
>  
>  	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> +	ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
> +
>  	trace_xfs_insert_file_space(ip);
>  
>  	error = xfs_prepare_shift(ip, offset);
> -- 
> 2.14.2
> 
> --
> 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 3/4] xfs: merge xfs_bmap_read_extents into xfs_iread_extents

[Darrick J. Wong]

On Mon, Oct 23, 2017 at 08:30:16AM +0200, Christoph Hellwig wrote:
> xfs_iread_extents is just a trivial wrapper, there is no good reason
> to keep the two separate.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
>  fs/xfs/libxfs/xfs_bmap.c       | 83 +++++++++++++++++++++++++-----------------
>  fs/xfs/libxfs/xfs_bmap.h       |  2 -
>  fs/xfs/libxfs/xfs_inode_fork.c | 37 -------------------
>  3 files changed, 49 insertions(+), 73 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 19ec8b1f99dd..53ff6d92b532 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -1164,32 +1164,37 @@ xfs_bmap_add_attrfork(
>   */
>  
>  /*
> - * Read in the extents to if_extents.
> - * All inode fields are set up by caller, we just traverse the btree
> - * and copy the records in.
> + * Read in extents from a btree-format inode.
>   */
> -int					/* error */
> -xfs_bmap_read_extents(
> -	xfs_trans_t		*tp,	/* transaction pointer */
> -	xfs_inode_t		*ip,	/* incore inode */
> -	int			whichfork) /* data or attr fork */
> +int
> +xfs_iread_extents(
> +	struct xfs_trans	*tp,
> +	struct xfs_inode	*ip,
> +	int			whichfork)
>  {
> -	struct xfs_btree_block	*block;	/* current btree block */
> -	xfs_fsblock_t		bno;	/* block # of "block" */
> -	xfs_buf_t		*bp;	/* buffer for "block" */
> -	int			error;	/* error return value */
> -	xfs_extnum_t		i, j;	/* index into the extents list */
> -	xfs_ifork_t		*ifp;	/* fork structure */
> -	int			level;	/* btree level, for checking */
> -	xfs_mount_t		*mp;	/* file system mount structure */
> -	__be64			*pp;	/* pointer to block address */
> -	/* REFERENCED */
> -	xfs_extnum_t		room;	/* number of entries there's room for */
> +	struct xfs_mount	*mp = ip->i_mount;
>  	int			state = xfs_bmap_fork_to_state(whichfork);
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
> +	xfs_extnum_t		nextents = XFS_IFORK_NEXTENTS(ip, whichfork);
> +	struct xfs_btree_block	*block = ifp->if_broot;
> +	xfs_fsblock_t		bno;
> +	struct xfs_buf		*bp;
> +	xfs_extnum_t		i, j;
> +	int			level;
> +	__be64			*pp;
> +	int			error;
> +
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> +
> +	if (unlikely(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE)) {
> +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> +		return -EFSCORRUPTED;
> +	}
> +
> +	ifp->if_bytes = 0;
> +	ifp->if_real_bytes = 0;
> +	xfs_iext_add(ifp, 0, nextents);
>  
> -	mp = ip->i_mount;
> -	ifp = XFS_IFORK_PTR(ip, whichfork);
> -	block = ifp->if_broot;
>  	/*
>  	 * Root level must use BMAP_BROOT_PTR_ADDR macro to get ptr out.
>  	 */
> @@ -1206,21 +1211,22 @@ xfs_bmap_read_extents(
>  		error = xfs_btree_read_bufl(mp, tp, bno, 0, &bp,
>  				XFS_BMAP_BTREE_REF, &xfs_bmbt_buf_ops);
>  		if (error)
> -			return error;
> +			goto out;
>  		block = XFS_BUF_TO_BLOCK(bp);
>  		if (level == 0)
>  			break;
>  		pp = XFS_BMBT_PTR_ADDR(mp, block, 1, mp->m_bmap_dmxr[1]);
>  		bno = be64_to_cpu(*pp);
>  		XFS_WANT_CORRUPTED_GOTO(mp,
> -			XFS_FSB_SANITY_CHECK(mp, bno), error0);
> +			XFS_FSB_SANITY_CHECK(mp, bno), out_brelse);
>  		xfs_trans_brelse(tp, bp);
>  	}
> +
>  	/*
>  	 * Here with bp and block set to the leftmost leaf node in the tree.
>  	 */
> -	room = xfs_iext_count(ifp);
>  	i = 0;
> +
>  	/*
>  	 * Loop over all leaf nodes.  Copy information to the extent records.
>  	 */
> @@ -1230,14 +1236,15 @@ xfs_bmap_read_extents(
>  		xfs_extnum_t	num_recs;
>  
>  		num_recs = xfs_btree_get_numrecs(block);
> -		if (unlikely(i + num_recs > room)) {
> -			ASSERT(i + num_recs <= room);
> +		if (unlikely(i + num_recs > nextents)) {
> +			ASSERT(i + num_recs <= nextents);
>  			xfs_warn(ip->i_mount,
>  				"corrupt dinode %Lu, (btree extents).",
>  				(unsigned long long) ip->i_ino);
> -			XFS_CORRUPTION_ERROR("xfs_bmap_read_extents(1)",
> +			XFS_CORRUPTION_ERROR(__func__,
>  				XFS_ERRLEVEL_LOW, ip->i_mount, block);
> -			goto error0;
> +			error = -EFSCORRUPTED;
> +			goto out_brelse;
>  		}
>  		/*
>  		 * Read-ahead the next leaf block, if any.
> @@ -1266,16 +1273,24 @@ xfs_bmap_read_extents(
>  		error = xfs_btree_read_bufl(mp, tp, bno, 0, &bp,
>  				XFS_BMAP_BTREE_REF, &xfs_bmbt_buf_ops);
>  		if (error)
> -			return error;
> +			goto out;
>  		block = XFS_BUF_TO_BLOCK(bp);
>  	}
> -	if (i != XFS_IFORK_NEXTENTS(ip, whichfork))
> -		return -EFSCORRUPTED;
> +
> +	if (i != XFS_IFORK_NEXTENTS(ip, whichfork)) {
> +		error = -EFSCORRUPTED;
> +		goto out;
> +	}
>  	ASSERT(i == xfs_iext_count(ifp));
> +
> +	ifp->if_flags |= XFS_IFEXTENTS;
>  	return 0;
> -error0:
> +
> +out_brelse:
>  	xfs_trans_brelse(tp, bp);
> -	return -EFSCORRUPTED;
> +out:
> +	xfs_iext_destroy(ifp);
> +	return error;
>  }
>  
>  /*
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index 6c426cdfb758..de34bad03c46 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -198,8 +198,6 @@ int	xfs_bmap_last_before(struct xfs_trans *tp, struct xfs_inode *ip,
>  int	xfs_bmap_last_offset(struct xfs_inode *ip, xfs_fileoff_t *unused,
>  		int whichfork);
>  int	xfs_bmap_one_block(struct xfs_inode *ip, int whichfork);
> -int	xfs_bmap_read_extents(struct xfs_trans *tp, struct xfs_inode *ip,
> -		int whichfork);
>  int	xfs_bmapi_read(struct xfs_inode *ip, xfs_fileoff_t bno,
>  		xfs_filblks_t len, struct xfs_bmbt_irec *mval,
>  		int *nmap, int flags);
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 48a5dec360cd..c70f9ef07b6d 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -443,43 +443,6 @@ xfs_iformat_btree(
>  	return 0;
>  }
>  
> -/*
> - * Read in extents from a btree-format inode.
> - * Allocate and fill in if_extents.  Real work is done in xfs_bmap.c.
> - */
> -int
> -xfs_iread_extents(
> -	xfs_trans_t	*tp,
> -	xfs_inode_t	*ip,
> -	int		whichfork)
> -{
> -	int		error;
> -	xfs_ifork_t	*ifp;
> -	xfs_extnum_t	nextents;
> -
> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> -
> -	if (unlikely(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE)) {
> -		XFS_ERROR_REPORT("xfs_iread_extents", XFS_ERRLEVEL_LOW,
> -				 ip->i_mount);
> -		return -EFSCORRUPTED;
> -	}
> -	nextents = XFS_IFORK_NEXTENTS(ip, whichfork);
> -	ifp = XFS_IFORK_PTR(ip, whichfork);
> -
> -	/*
> -	 * We know that the size is valid (it's checked in iformat_btree)
> -	 */
> -	ifp->if_bytes = ifp->if_real_bytes = 0;
> -	xfs_iext_add(ifp, 0, nextents);
> -	error = xfs_bmap_read_extents(tp, ip, whichfork);
> -	if (error) {
> -		xfs_iext_destroy(ifp);
> -		return error;
> -	}
> -	ifp->if_flags |= XFS_IFEXTENTS;
> -	return 0;
> -}
>  /*
>   * Reallocate the space for if_broot based on the number of records
>   * being added or deleted as indicated in rec_diff.  Move the records
> -- 
> 2.14.2
> 
> --
> 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 4/4] xfs: add a new xfs_iext_lookup_extent_before helper

[Darrick J. Wong]

On Mon, Oct 23, 2017 at 08:30:17AM +0200, Christoph Hellwig wrote:
> This helper looks up the last extent the covers space before the passed
> in block number.  This is useful for truncate and similar operations that
> operate backwards over the extent list.  For xfs_bunmapi it also is
> a slight optimization as we can return early if there are not extents
> at or below the end of the to be truncated range.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
>  fs/xfs/libxfs/xfs_bmap.c       | 27 +++++++--------------------
>  fs/xfs/libxfs/xfs_inode_fork.c | 21 +++++++++++++++++++++
>  fs/xfs/libxfs/xfs_inode_fork.h |  4 ++++
>  fs/xfs/xfs_reflink.c           | 19 +++++++------------
>  4 files changed, 39 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 53ff6d92b532..b26c4ece7cbe 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -1380,17 +1380,8 @@ xfs_bmap_last_before(
>  			return error;
>  	}
>  
> -	if (xfs_iext_lookup_extent(ip, ifp, *last_block - 1, &idx, &got)) {
> -		if (got.br_startoff <= *last_block - 1)
> -			return 0;
> -	}
> -
> -	if (xfs_iext_get_extent(ifp, idx - 1, &got)) {
> -		*last_block = got.br_startoff + got.br_blockcount;
> -		return 0;
> -	}
> -
> -	*last_block = 0;
> +	if (!xfs_iext_lookup_extent_before(ip, ifp, last_block, &idx, &got))
> +		*last_block = 0;
>  	return 0;
>  }
>  
> @@ -5154,17 +5145,13 @@ __xfs_bunmapi(
>  	}
>  	XFS_STATS_INC(mp, xs_blk_unmap);
>  	isrt = (whichfork == XFS_DATA_FORK) && XFS_IS_REALTIME_INODE(ip);
> -	end = start + len - 1;
> +	end = start + len;
>  
> -	/*
> -	 * Check to see if the given block number is past the end of the
> -	 * file, back up to the last block if so...
> -	 */
> -	if (!xfs_iext_lookup_extent(ip, ifp, end, &lastx, &got)) {
> -		ASSERT(lastx > 0);
> -		xfs_iext_get_extent(ifp, --lastx, &got);
> -		end = got.br_startoff + got.br_blockcount - 1;
> +	if (!xfs_iext_lookup_extent_before(ip, ifp, &end, &lastx, &got)) {
> +		*rlen = 0;
> +		return 0;
>  	}
> +	end--;
>  
>  	logflags = 0;
>  	if (ifp->if_flags & XFS_IFBROOT) {
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index c70f9ef07b6d..5b90f912e41a 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -1960,6 +1960,27 @@ xfs_iext_lookup_extent(
>  	return true;
>  }
>  
> +/*
> + * Returns the last extent before end, and if this extent doesn't cover
> + * end, update end to the end of the extent.
> + */
> +bool
> +xfs_iext_lookup_extent_before(
> +	struct xfs_inode	*ip,
> +	struct xfs_ifork	*ifp,
> +	xfs_fileoff_t		*end,
> +	xfs_extnum_t		*idxp,
> +	struct xfs_bmbt_irec	*gotp)
> +{
> +	if (xfs_iext_lookup_extent(ip, ifp, *end - 1, idxp, gotp) &&
> +	    gotp->br_startoff <= *end - 1)
> +		return true;
> +	if (!xfs_iext_get_extent(ifp, --*idxp, gotp))
> +		return false;
> +	*end = gotp->br_startoff + gotp->br_blockcount;
> +	return true;
> +}
> +
>  /*
>   * Return true if there is an extent at index idx, and return the expanded
>   * extent structure at idx in that case.  Else return false.
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index e0c42ea9b8d0..113fd42ec36d 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -183,6 +183,10 @@ void		xfs_iext_irec_update_extoffs(struct xfs_ifork *, int, int);
>  bool		xfs_iext_lookup_extent(struct xfs_inode *ip,
>  			struct xfs_ifork *ifp, xfs_fileoff_t bno,
>  			xfs_extnum_t *idxp, struct xfs_bmbt_irec *gotp);
> +bool		xfs_iext_lookup_extent_before(struct xfs_inode *ip,
> +			struct xfs_ifork *ifp, xfs_fileoff_t *end,
> +			xfs_extnum_t *idxp, struct xfs_bmbt_irec *gotp);
> +
>  bool		xfs_iext_get_extent(struct xfs_ifork *ifp, xfs_extnum_t idx,
>  			struct xfs_bmbt_irec *gotp);
>  void		xfs_iext_update_extent(struct xfs_inode *ip, int state,
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 37e603bf1591..1205747e1409 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -733,18 +733,13 @@ xfs_reflink_end_cow(
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(tp, ip, 0);
>  
> -	/* If there is a hole at end_fsb - 1 go to the previous extent */
> -	if (!xfs_iext_lookup_extent(ip, ifp, end_fsb - 1, &idx, &got) ||
> -	    got.br_startoff > end_fsb) {
> -		/*
> -		 * In case of racing, overlapping AIO writes no COW extents
> -		 * might be left by the time I/O completes for the loser of
> -		 * the race.  In that case we are done.
> -		 */
> -		if (idx <= 0)
> -			goto out_cancel;
> -		xfs_iext_get_extent(ifp, --idx, &got);
> -	}
> +	/*
> +	 * In case of racing, overlapping AIO writes no COW extents might be
> +	 * left by the time I/O completes for the loser of the race.  In that
> +	 * case we are done.
> +	 */
> +	if (!xfs_iext_lookup_extent_before(ip, ifp, &end_fsb, &idx, &got))
> +		goto out_cancel;
>  
>  	/* Walk backwards until we're out of the I/O range... */
>  	while (got.br_startoff + got.br_blockcount > offset_fsb) {
> -- 
> 2.14.2
> 
> --
> 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/4] xfs: remove xfs_bmbt_validate_extent

[Christoph Hellwig]

> By removing this, I think we no longer report corruption if we encounter
> 'unwritten' attr fork extents, which (AFAIK) aren't allowed.

True.  Oh well, skip this patch for now if you apply the rest, and
I'll have to do some gymnastics to keep it.

Re: [PATCH 2/4] xfs: remove xfs_bmbt_validate_extent

[Darrick J. Wong]

On Tue, Oct 24, 2017 at 09:54:15AM +0200, Christoph Hellwig wrote:
> > By removing this, I think we no longer report corruption if we encounter
> > 'unwritten' attr fork extents, which (AFAIK) aren't allowed.
> 
> True.  Oh well, skip this patch for now if you apply the rest, and
> I'll have to do some gymnastics to keep it.

Ok.  I can try to fix it up in the patch stack tomorrow, but feel free
to beat me to it. :P

--D

Re: [PATCH 2/4] xfs: remove xfs_bmbt_validate_extent

[Christoph Hellwig]

On Tue, Oct 24, 2017 at 10:24:26PM -0700, Darrick J. Wong wrote:
> On Tue, Oct 24, 2017 at 09:54:15AM +0200, Christoph Hellwig wrote:
> > > By removing this, I think we no longer report corruption if we encounter
> > > 'unwritten' attr fork extents, which (AFAIK) aren't allowed.
> > 
> > True.  Oh well, skip this patch for now if you apply the rest, and
> > I'll have to do some gymnastics to keep it.
> 
> Ok.  I can try to fix it up in the patch stack tomorrow, but feel free
> to beat me to it. :P

Just discard all the patches but the first for now.