[PATCH V3 00/10] Bail out if transaction can cause extent count to overflow

[PATCH V3 00/10] Bail out if transaction can cause extent count to overflow

[Chandan Babu R]

XFS does not check for possible overflow of per-inode extent counter
fields when adding extents to either data or attr fork.

For e.g.
1. Insert 5 million xattrs (each having a value size of 255 bytes) and
   then delete 50% of them in an alternating manner.

2. On a 4k block sized XFS filesystem instance, the above causes 98511
   extents to be created in the attr fork of the inode.

   xfsaild/loop0  2008 [003]  1475.127209: probe:xfs_inode_to_disk: (ffffffffa43fb6b0) if_nextents=98511 i_ino=131

3. The incore inode fork extent counter is a signed 32-bit
   quantity. However, the on-disk extent counter is an unsigned 16-bit
   quantity and hence cannot hold 98511 extents.

4. The following incorrect value is stored in the xattr extent counter,
   # xfs_db -f -c 'inode 131' -c 'print core.naextents' /dev/loop0
   core.naextents = -32561

This patchset adds a new helper function
(i.e. xfs_iext_count_may_overflow()) to check for overflow of the
per-inode data and xattr extent counters and invokes it before
starting an fs operation (e.g. creating a new directory entry). With
this patchset applied, XFS detects counter overflows and returns with
an error rather than causing a silent corruption.

The patchset has been tested by executing xfstests with the following
mkfs.xfs options,
1. -m crc=0 -b size=1k
2. -m crc=0 -b size=4k
3. -m crc=0 -b size=512
4. -m rmapbt=1,reflink=1 -b size=1k
5. -m rmapbt=1,reflink=1 -b size=4k

The patches can also be obtained from
https://github.com/chandanr/linux.git at branch xfs-reserve-extent-count-v3.

Changelog:
V2 -> V3:
  1. Move the definition of xfs_iext_count_may_overflow() from
     libxfs/xfs_trans_resv.c to libxfs/xfs_inode_fork.c. Also, I tried
     to make xfs_iext_count_may_overflow() an inline function by
     placing the definition in libxfs/xfs_inode_fork.h. However this
     required that the definition of 'struct xfs_inode' be available,
     since xfs_iext_count_may_overflow() uses a 'struct xfs_inode *'
     type variable.
  2. Handle XFS_COW_FORK within xfs_iext_count_may_overflow() by
     returning a success value.
  3. Rename XFS_IEXT_ADD_CNT to XFS_IEXT_ADD_NOSPLIT_CNT. Thanks to
     Darrick for the suggesting the new name.
  4. Expand comments to make use of 80 columns.

V1 -> V2:
  1. Rename helper function from xfs_trans_resv_ext_cnt() to
     xfs_iext_count_may_overflow().
  2. Define and use macros to represent fs operations and the
     corresponding increase in extent count.
  3. Split the patches based on the fs operation being performed.

Chandan Babu R (10):
  xfs: Add helper for checking per-inode extent count overflow
  xfs: Check for extent overflow when trivally adding a new extent
  xfs: Check for extent overflow when deleting an extent
  xfs: Check for extent overflow when adding/removing xattrs
  xfs: Check for extent overflow when adding/removing dir entries
  xfs: Check for extent overflow when writing to unwritten extent
  xfs: Check for extent overflow when inserting a hole
  xfs: Check for extent overflow when moving extent from cow to data
    fork
  xfs: Check for extent overflow when remapping an extent
  xfs: Check for extent overflow when swapping extents

 fs/xfs/libxfs/xfs_attr.c       | 13 ++++++
 fs/xfs/libxfs/xfs_bmap.c       |  6 +++
 fs/xfs/libxfs/xfs_inode_fork.c | 23 +++++++++++
 fs/xfs/libxfs/xfs_inode_fork.h | 75 ++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_bmap_item.c         |  4 ++
 fs/xfs/xfs_bmap_util.c         | 30 ++++++++++++++
 fs/xfs/xfs_dquot.c             |  8 +++-
 fs/xfs/xfs_inode.c             | 27 ++++++++++++
 fs/xfs/xfs_iomap.c             | 10 +++++
 fs/xfs/xfs_reflink.c           | 10 +++++
 fs/xfs/xfs_rtalloc.c           |  5 +++
 fs/xfs/xfs_symlink.c           |  5 +++
 12 files changed, 215 insertions(+), 1 deletion(-)

-- 
2.28.0

[PATCH V3 01/10] xfs: Add helper for checking per-inode extent count overflow

[Chandan Babu R]

XFS does not check for possible overflow of per-inode extent counter
fields when adding extents to either data or attr fork.

For e.g.
1. Insert 5 million xattrs (each having a value size of 255 bytes) and
   then delete 50% of them in an alternating manner.

2. On a 4k block sized XFS filesystem instance, the above causes 98511
   extents to be created in the attr fork of the inode.

   xfsaild/loop0  2008 [003]  1475.127209: probe:xfs_inode_to_disk: (ffffffffa43fb6b0) if_nextents=98511 i_ino=131

3. The incore inode fork extent counter is a signed 32-bit
   quantity. However the on-disk extent counter is an unsigned 16-bit
   quantity and hence cannot hold 98511 extents.

4. The following incorrect value is stored in the attr extent counter,
   # xfs_db -f -c 'inode 131' -c 'print core.naextents' /dev/loop0
   core.naextents = -32561

This commit adds a new helper function (i.e.
xfs_iext_count_may_overflow()) to check for overflow of the per-inode
data and xattr extent counters. Future patches will use this function to
make sure that an FS operation won't cause the extent counter to
overflow.

Suggested-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_inode_fork.c | 23 +++++++++++++++++++++++
 fs/xfs/libxfs/xfs_inode_fork.h |  2 ++
 2 files changed, 25 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 0cf853d42d62..3a084aea8f85 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -23,6 +23,7 @@
 #include "xfs_da_btree.h"
 #include "xfs_dir2_priv.h"
 #include "xfs_attr_leaf.h"
+#include "xfs_types.h"
 
 kmem_zone_t *xfs_ifork_zone;
 
@@ -728,3 +729,25 @@ xfs_ifork_verify_local_attr(
 
 	return 0;
 }
+
+int
+xfs_iext_count_may_overflow(
+	struct xfs_inode	*ip,
+	int			whichfork,
+	int			nr_to_add)
+{
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
+	uint64_t		max_exts;
+	uint64_t		nr_exts;
+
+	if (whichfork == XFS_COW_FORK)
+		return 0;
+
+	max_exts = (whichfork == XFS_ATTR_FORK) ? MAXAEXTNUM : MAXEXTNUM;
+
+	nr_exts = ifp->if_nextents + nr_to_add;
+	if (nr_exts < ifp->if_nextents || nr_exts > max_exts)
+		return -EFBIG;
+
+	return 0;
+}
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index a4953e95c4f3..0beb8e2a00be 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -172,5 +172,7 @@ extern void xfs_ifork_init_cow(struct xfs_inode *ip);
 
 int xfs_ifork_verify_local_data(struct xfs_inode *ip);
 int xfs_ifork_verify_local_attr(struct xfs_inode *ip);
+int xfs_iext_count_may_overflow(struct xfs_inode *ip, int whichfork,
+		int nr_to_add);
 
 #endif	/* __XFS_INODE_FORK_H__ */
-- 
2.28.0

[PATCH V3 02/10] xfs: Check for extent overflow when trivally adding a new extent

[Chandan Babu R]

When adding a new data extent (without modifying an inode's existing
extents) the extent count increases only by 1. This commit checks for
extent count overflow in such cases.

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_bmap.c       | 6 ++++++
 fs/xfs/libxfs/xfs_inode_fork.h | 6 ++++++
 fs/xfs/xfs_bmap_util.c         | 5 +++++
 fs/xfs/xfs_dquot.c             | 8 +++++++-
 fs/xfs/xfs_iomap.c             | 5 +++++
 fs/xfs/xfs_rtalloc.c           | 5 +++++
 6 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 9c40d5971035..dcc8eeecd571 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4527,6 +4527,12 @@ xfs_bmapi_convert_delalloc(
 		return error;
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
+
+	error = xfs_iext_count_may_overflow(ip, whichfork,
+			XFS_IEXT_ADD_NOSPLIT_CNT);
+	if (error)
+		goto out_trans_cancel;
+
 	xfs_trans_ijoin(tp, ip, 0);
 
 	if (!xfs_iext_lookup_extent(ip, ifp, offset_fsb, &bma.icur, &bma.got) ||
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 0beb8e2a00be..7fc2b129a2e7 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -34,6 +34,12 @@ struct xfs_ifork {
 #define	XFS_IFEXTENTS	0x02	/* All extent pointers are read in */
 #define	XFS_IFBROOT	0x04	/* i_broot points to the bmap b-tree root */
 
+/*
+ * Worst-case increase in the fork extent count when we're adding a single
+ * extent to a fork and there's no possibility of splitting an existing mapping.
+ */
+#define XFS_IEXT_ADD_NOSPLIT_CNT	(1)
+
 /*
  * Fork handling.
  */
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index afdc7f8e0e70..7b76a48b0885 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -822,6 +822,11 @@ xfs_alloc_file_space(
 		if (error)
 			goto error1;
 
+		error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
+				XFS_IEXT_ADD_NOSPLIT_CNT);
+		if (error)
+			goto error0;
+
 		xfs_trans_ijoin(tp, ip, 0);
 
 		error = xfs_bmapi_write(tp, ip, startoffset_fsb,
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 04dc2be19c3a..59ea9485ebda 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -290,8 +290,14 @@ xfs_dquot_disk_alloc(
 		return -ESRCH;
 	}
 
-	/* Create the block mapping. */
 	xfs_trans_ijoin(tp, quotip, XFS_ILOCK_EXCL);
+
+	error = xfs_iext_count_may_overflow(quotip, XFS_DATA_FORK,
+			XFS_IEXT_ADD_NOSPLIT_CNT);
+	if (error)
+		return error;
+
+	/* Create the block mapping. */
 	error = xfs_bmapi_write(tp, quotip, dqp->q_fileoffset,
 			XFS_DQUOT_CLUSTER_SIZE_FSB, XFS_BMAPI_METADATA, 0, &map,
 			&nmaps);
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 0e3f62cde375..37b0c743c116 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -250,6 +250,11 @@ xfs_iomap_write_direct(
 	if (error)
 		goto out_trans_cancel;
 
+	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
+			XFS_IEXT_ADD_NOSPLIT_CNT);
+	if (error)
+		goto out_trans_cancel;
+
 	xfs_trans_ijoin(tp, ip, 0);
 
 	/*
diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 6209e7b6b895..9508ab00a00b 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -787,6 +787,11 @@ xfs_growfs_rt_alloc(
 		xfs_ilock(ip, XFS_ILOCK_EXCL);
 		xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
 
+		error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
+				XFS_IEXT_ADD_NOSPLIT_CNT);
+		if (error)
+			goto out_trans_cancel;
+
 		/*
 		 * Allocate blocks to the bitmap file.
 		 */
-- 
2.28.0

[PATCH V3 03/10] xfs: Check for extent overflow when deleting an extent

[Chandan Babu R]

Deleting a file range from the middle of an existing extent can cause
the per-inode extent count to increase by 1. This commit checks for
extent count overflow in such cases.

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_inode_fork.h | 6 ++++++
 fs/xfs/xfs_bmap_item.c         | 4 ++++
 fs/xfs/xfs_bmap_util.c         | 5 +++++
 3 files changed, 15 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 7fc2b129a2e7..2642e4847ee0 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -39,6 +39,12 @@ struct xfs_ifork {
  * extent to a fork and there's no possibility of splitting an existing mapping.
  */
 #define XFS_IEXT_ADD_NOSPLIT_CNT	(1)
+/*
+ * Removing an extent from the middle of an existing extent can cause the extent
+ * count to increase by 1.
+ * i.e. | Old extent | Hole | Old extent |
+ */
+#define XFS_IEXT_REMOVE_CNT		(1)
 
 /*
  * Fork handling.
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index ec3691372e7c..b9c35fb10de4 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -519,6 +519,10 @@ xfs_bui_item_recover(
 	}
 	xfs_trans_ijoin(tp, ip, 0);
 
+	error = xfs_iext_count_may_overflow(ip, whichfork, XFS_IEXT_REMOVE_CNT);
+	if (error)
+		goto err_inode;
+
 	count = bmap->me_len;
 	error = xfs_trans_log_finish_bmap_update(tp, budp, type, ip, whichfork,
 			bmap->me_startoff, bmap->me_startblock, &count, state);
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 7b76a48b0885..59d4da38aadf 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -891,6 +891,11 @@ xfs_unmap_extent(
 
 	xfs_trans_ijoin(tp, ip, 0);
 
+	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
+			XFS_IEXT_REMOVE_CNT);
+	if (error)
+		goto out_trans_cancel;
+
 	error = xfs_bunmapi(tp, ip, startoffset_fsb, len_fsb, 0, 2, done);
 	if (error)
 		goto out_trans_cancel;
-- 
2.28.0

[PATCH V3 04/10] xfs: Check for extent overflow when adding/removing xattrs

[Chandan Babu R]

Adding/removing an xattr can cause XFS_DA_NODE_MAXDEPTH extents to be
added. One extra extent for dabtree in case a local attr is large enough
to cause a double split.  It can also cause extent count to increase
proportional to the size of a remote xattr's value.

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_attr.c       | 13 +++++++++++++
 fs/xfs/libxfs/xfs_inode_fork.h |  9 +++++++++
 2 files changed, 22 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index d4583a0d1b3f..c481389da40f 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -396,6 +396,7 @@ xfs_attr_set(
 	struct xfs_trans_res	tres;
 	bool			rsvd = (args->attr_filter & XFS_ATTR_ROOT);
 	int			error, local;
+	int			rmt_blks = 0;
 	unsigned int		total;
 
 	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
@@ -442,11 +443,15 @@ xfs_attr_set(
 		tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
 		tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
 		total = args->total;
+
+		if (!local)
+			rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
 	} else {
 		XFS_STATS_INC(mp, xs_attr_remove);
 
 		tres = M_RES(mp)->tr_attrrm;
 		total = XFS_ATTRRM_SPACE_RES(mp);
+		rmt_blks = xfs_attr3_rmt_blocks(mp, XFS_XATTR_SIZE_MAX);
 	}
 
 	/*
@@ -460,6 +465,14 @@ xfs_attr_set(
 
 	xfs_ilock(dp, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(args->trans, dp, 0);
+
+	if (args->value || xfs_inode_hasattr(dp)) {
+		error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK,
+				XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
+		if (error)
+			goto out_trans_cancel;
+	}
+
 	if (args->value) {
 		unsigned int	quota_flags = XFS_QMOPT_RES_REGBLKS;
 
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 2642e4847ee0..aae8e6e80b71 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -45,6 +45,15 @@ struct xfs_ifork {
  * i.e. | Old extent | Hole | Old extent |
  */
 #define XFS_IEXT_REMOVE_CNT		(1)
+/*
+ * Adding/removing an xattr can cause XFS_DA_NODE_MAXDEPTH extents to
+ * be added. One extra extent for dabtree in case a local attr is
+ * large enough to cause a double split.  It can also cause extent
+ * count to increase proportional to the size of a remote xattr's
+ * value.
+ */
+#define XFS_IEXT_ATTR_MANIP_CNT(rmt_blks) \
+	(XFS_DA_NODE_MAXDEPTH + max(1, rmt_blks))
 
 /*
  * Fork handling.
-- 
2.28.0

[PATCH V3 05/10] xfs: Check for extent overflow when adding/removing dir entries

[Chandan Babu R]

Directory entry addition/removal can cause the following,
1. Data block can be added/removed.
   A new extent can cause extent count to increase by 1.
2. Free disk block can be added/removed.
   Same behaviour as described above for Data block.
3. Dabtree blocks.
   XFS_DA_NODE_MAXDEPTH blocks can be added. Each of these
   can be new extents. Hence extent count can increase by
   XFS_DA_NODE_MAXDEPTH.

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_inode_fork.h | 12 ++++++++++++
 fs/xfs/xfs_inode.c             | 27 +++++++++++++++++++++++++++
 fs/xfs/xfs_symlink.c           |  5 +++++
 3 files changed, 44 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index aae8e6e80b71..f686c7418d2b 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -54,6 +54,18 @@ struct xfs_ifork {
  */
 #define XFS_IEXT_ATTR_MANIP_CNT(rmt_blks) \
 	(XFS_DA_NODE_MAXDEPTH + max(1, rmt_blks))
+/*
+ * Directory entry addition/removal can cause the following,
+ * 1. Data block can be added/removed.
+ *    A new extent can cause extent count to increase by 1.
+ * 2. Free disk block can be added/removed.
+ *    Same behaviour as described above for Data block.
+ * 3. Dabtree blocks.
+ *    XFS_DA_NODE_MAXDEPTH blocks can be added. Each of these can be new
+ *    extents. Hence extent count can increase by XFS_DA_NODE_MAXDEPTH.
+ */
+#define XFS_IEXT_DIR_MANIP_CNT(mp) \
+	((XFS_DA_NODE_MAXDEPTH + 1 + 1) * (mp)->m_dir_geo->fsbcount)
 
 /*
  * Fork handling.
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 407d6299606d..8d195b6ef326 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1175,6 +1175,11 @@ xfs_create(
 	if (error)
 		goto out_trans_cancel;
 
+	error = xfs_iext_count_may_overflow(dp, XFS_DATA_FORK,
+			XFS_IEXT_DIR_MANIP_CNT(mp));
+	if (error)
+		goto out_trans_cancel;
+
 	/*
 	 * A newly created regular or special file just has one directory
 	 * entry pointing to them, but a directory also the "." entry
@@ -1391,6 +1396,11 @@ xfs_link(
 	xfs_trans_ijoin(tp, sip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, tdp, XFS_ILOCK_EXCL);
 
+	error = xfs_iext_count_may_overflow(tdp, XFS_DATA_FORK,
+			XFS_IEXT_DIR_MANIP_CNT(mp));
+	if (error)
+		goto error_return;
+
 	/*
 	 * If we are using project inheritance, we only allow hard link
 	 * creation in our tree when the project IDs are the same; else
@@ -2861,6 +2871,11 @@ xfs_remove(
 	xfs_trans_ijoin(tp, dp, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
 
+	error = xfs_iext_count_may_overflow(dp, XFS_DATA_FORK,
+			XFS_IEXT_DIR_MANIP_CNT(mp));
+	if (error)
+		goto out_trans_cancel;
+
 	/*
 	 * If we're removing a directory perform some additional validation.
 	 */
@@ -3221,6 +3236,18 @@ xfs_rename(
 	if (wip)
 		xfs_trans_ijoin(tp, wip, XFS_ILOCK_EXCL);
 
+	error = xfs_iext_count_may_overflow(src_dp, XFS_DATA_FORK,
+			XFS_IEXT_DIR_MANIP_CNT(mp));
+	if (error)
+		goto out_trans_cancel;
+
+	if (target_ip == NULL) {
+		error = xfs_iext_count_may_overflow(target_dp, XFS_DATA_FORK,
+				XFS_IEXT_DIR_MANIP_CNT(mp));
+		if (error)
+			goto out_trans_cancel;
+	}
+
 	/*
 	 * If we are using project inheritance, we only allow renames
 	 * into our tree when the project IDs are the same; else the
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index 8e88a7ca387e..581a4032a817 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -220,6 +220,11 @@ xfs_symlink(
 	if (error)
 		goto out_trans_cancel;
 
+	error = xfs_iext_count_may_overflow(dp, XFS_DATA_FORK,
+			XFS_IEXT_DIR_MANIP_CNT(mp));
+	if (error)
+		goto out_trans_cancel;
+
 	/*
 	 * Allocate an inode for the symlink.
 	 */
-- 
2.28.0

[PATCH V3 06/10] xfs: Check for extent overflow when writing to unwritten extent

[Chandan Babu R]

A write to a sub-interval of an existing unwritten extent causes
the original extent to be split into 3 extents
i.e. | Unwritten | Real | Unwritten |
Hence extent count can increase by 2.

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_inode_fork.h | 7 +++++++
 fs/xfs/xfs_iomap.c             | 5 +++++
 2 files changed, 12 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index f686c7418d2b..83ff90e2a5fe 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -66,6 +66,13 @@ struct xfs_ifork {
  */
 #define XFS_IEXT_DIR_MANIP_CNT(mp) \
 	((XFS_DA_NODE_MAXDEPTH + 1 + 1) * (mp)->m_dir_geo->fsbcount)
+/*
+ * A write to a sub-interval of an existing unwritten extent causes the original
+ * extent to be split into 3 extents
+ * i.e. | Unwritten | Real | Unwritten |
+ * Hence extent count can increase by 2.
+ */
+#define XFS_IEXT_WRITE_UNWRITTEN_CNT	(2)
 
 /*
  * Fork handling.
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 37b0c743c116..694b25fbb4a3 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -566,6 +566,11 @@ xfs_iomap_write_unwritten(
 		if (error)
 			goto error_on_bmapi_transaction;
 
+		error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
+				XFS_IEXT_WRITE_UNWRITTEN_CNT);
+		if (error)
+			goto error_on_bmapi_transaction;
+
 		/*
 		 * Modify the unwritten extent state of the buffer.
 		 */
-- 
2.28.0

[PATCH V3 07/10] xfs: Check for extent overflow when inserting a hole

[Chandan Babu R]

The extent mapping the file offset at which a hole has to be
inserted will be split into two extents causing extent count to
increase by 1.

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_inode_fork.h | 6 ++++++
 fs/xfs/xfs_bmap_util.c         | 9 +++++++++
 2 files changed, 15 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 83ff90e2a5fe..d0e49b015b62 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -73,6 +73,12 @@ struct xfs_ifork {
  * Hence extent count can increase by 2.
  */
 #define XFS_IEXT_WRITE_UNWRITTEN_CNT	(2)
+/*
+ * The extent mapping the file offset at which a hole has to be inserted will be
+ * split into two extents causing extent count to increase by 1.
+ */
+#define XFS_IEXT_INSERT_HOLE_CNT	(1)
+
 
 /*
  * Fork handling.
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 59d4da38aadf..e682eecebb1f 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1165,6 +1165,15 @@ xfs_insert_file_space(
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, 0);
 
+	/*
+	 * Splitting the extent mapping containing stop_fsb will cause
+	 * extent count to increase by 1.
+	 */
+	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
+			XFS_IEXT_INSERT_HOLE_CNT);
+	if (error)
+		goto out_trans_cancel;
+
 	/*
 	 * The extent shifting code works on extent granularity. So, if stop_fsb
 	 * is not the starting block of extent, we need to split the extent at
-- 
2.28.0

[PATCH V3 08/10] xfs: Check for extent overflow when moving extent from cow to data fork

[Chandan Babu R]

Moving an extent to data fork can cause a sub-interval of an existing
extent to be unmapped. This will increase extent count by 1. Mapping in
the new extent can increase the extent count by 1 again i.e.
 | Old extent | New extent | Old extent |
Hence number of extents increases by 2.

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_inode_fork.h | 9 ++++++++-
 fs/xfs/xfs_reflink.c           | 5 +++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index d0e49b015b62..850d53162545 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -78,7 +78,14 @@ struct xfs_ifork {
  * split into two extents causing extent count to increase by 1.
  */
 #define XFS_IEXT_INSERT_HOLE_CNT	(1)
-
+/*
+ * Moving an extent to data fork can cause a sub-interval of an existing extent
+ * to be unmapped. This will increase extent count by 1. Mapping in the new
+ * extent can increase the extent count by 1 again i.e.
+ * | Old extent | New extent | Old extent |
+ * Hence number of extents increases by 2.
+ */
+#define XFS_IEXT_REFLINK_END_COW_CNT	(2)
 
 /*
  * Fork handling.
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index aac83f9d6107..c1d2a741e1af 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -628,6 +628,11 @@ xfs_reflink_end_cow_extent(
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, 0);
 
+	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
+			XFS_IEXT_REFLINK_END_COW_CNT);
+	if (error)
+		goto out_cancel;
+
 	/*
 	 * 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
-- 
2.28.0

[PATCH V3 09/10] xfs: Check for extent overflow when remapping an extent

[Chandan Babu R]

Remapping an extent involves unmapping the existing extent and mapping
in the new extent. When unmapping, an extent containing the entire unmap
range can be split into two extents,
i.e. | Old extent | hole | Old extent |
Hence extent count increases by 1.

Mapping in the new extent into the destination file can increase the
extent count by 1.

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_inode_fork.h | 14 ++++++++++++++
 fs/xfs/xfs_reflink.c           |  5 +++++
 2 files changed, 19 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 850d53162545..d1c675cf803a 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -86,6 +86,20 @@ struct xfs_ifork {
  * Hence number of extents increases by 2.
  */
 #define XFS_IEXT_REFLINK_END_COW_CNT	(2)
+/*
+ * Remapping an extent involves unmapping the existing extent and mapping in the
+ * new extent.
+ *
+ * When unmapping, an extent containing the entire unmap range can be split into
+ * two extents,
+ * i.e. | Old extent | hole | Old extent |
+ * Hence extent count increases by 1.
+ *
+ * Mapping in the new extent into the destination file can increase the extent
+ * count by 1.
+ */
+#define XFS_IEXT_REFLINK_REMAP_CNT(smap_real, dmap_written) \
+	(((smap_real) ? 1 : 0) + ((dmap_written) ? 1 : 0))
 
 /*
  * Fork handling.
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index c1d2a741e1af..9884fd51efee 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1099,6 +1099,11 @@ xfs_reflink_remap_extent(
 			goto out_cancel;
 	}
 
+	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
+			XFS_IEXT_REFLINK_REMAP_CNT(smap_real, dmap_written));
+	if (error)
+		goto out_cancel;
+
 	if (smap_real) {
 		/*
 		 * If the extent we're unmapping is backed by storage (written
-- 
2.28.0

[PATCH V3 10/10] xfs: Check for extent overflow when swapping extents

[Chandan Babu R]

Removing an initial range of source/donor file's extent and adding a new
extent (from donor/source file) in its place will cause extent count to
increase by 1.

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_inode_fork.h |  6 ++++++
 fs/xfs/xfs_bmap_util.c         | 11 +++++++++++
 2 files changed, 17 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index d1c675cf803a..4219b01f1034 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -100,6 +100,12 @@ struct xfs_ifork {
  */
 #define XFS_IEXT_REFLINK_REMAP_CNT(smap_real, dmap_written) \
 	(((smap_real) ? 1 : 0) + ((dmap_written) ? 1 : 0))
+/*
+ * Removing an initial range of source/donor file's extent and adding a new
+ * extent (from donor/source file) in its place will cause extent count to
+ * increase by 1.
+ */
+#define XFS_IEXT_SWAP_RMAP_CNT		(1)
 
 /*
  * Fork handling.
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index e682eecebb1f..7105525dadd5 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1375,6 +1375,17 @@ xfs_swap_extent_rmap(
 		/* Unmap the old blocks in the source file. */
 		while (tirec.br_blockcount) {
 			ASSERT(tp->t_firstblock == NULLFSBLOCK);
+
+			error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
+					XFS_IEXT_SWAP_RMAP_CNT);
+			if (error)
+				goto out;
+
+			error = xfs_iext_count_may_overflow(tip, XFS_DATA_FORK,
+					XFS_IEXT_SWAP_RMAP_CNT);
+			if (error)
+				goto out;
+
 			trace_xfs_swap_extent_rmap_remap_piece(tip, &tirec);
 
 			/* Read extent from the source file */
-- 
2.28.0

Re: [PATCH V3 01/10] xfs: Add helper for checking per-inode extent count overflow

[Darrick J. Wong]

On Thu, Aug 20, 2020 at 11:13:40AM +0530, Chandan Babu R wrote:
> XFS does not check for possible overflow of per-inode extent counter
> fields when adding extents to either data or attr fork.
> 
> For e.g.
> 1. Insert 5 million xattrs (each having a value size of 255 bytes) and
>    then delete 50% of them in an alternating manner.
> 
> 2. On a 4k block sized XFS filesystem instance, the above causes 98511
>    extents to be created in the attr fork of the inode.
> 
>    xfsaild/loop0  2008 [003]  1475.127209: probe:xfs_inode_to_disk: (ffffffffa43fb6b0) if_nextents=98511 i_ino=131
> 
> 3. The incore inode fork extent counter is a signed 32-bit
>    quantity. However the on-disk extent counter is an unsigned 16-bit
>    quantity and hence cannot hold 98511 extents.
> 
> 4. The following incorrect value is stored in the attr extent counter,
>    # xfs_db -f -c 'inode 131' -c 'print core.naextents' /dev/loop0
>    core.naextents = -32561
> 
> This commit adds a new helper function (i.e.
> xfs_iext_count_may_overflow()) to check for overflow of the per-inode
> data and xattr extent counters. Future patches will use this function to
> make sure that an FS operation won't cause the extent counter to
> overflow.
> 
> Suggested-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>

Seems reasonable so far...

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

--D

> ---
>  fs/xfs/libxfs/xfs_inode_fork.c | 23 +++++++++++++++++++++++
>  fs/xfs/libxfs/xfs_inode_fork.h |  2 ++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 0cf853d42d62..3a084aea8f85 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -23,6 +23,7 @@
>  #include "xfs_da_btree.h"
>  #include "xfs_dir2_priv.h"
>  #include "xfs_attr_leaf.h"
> +#include "xfs_types.h"
>  
>  kmem_zone_t *xfs_ifork_zone;
>  
> @@ -728,3 +729,25 @@ xfs_ifork_verify_local_attr(
>  
>  	return 0;
>  }
> +
> +int
> +xfs_iext_count_may_overflow(
> +	struct xfs_inode	*ip,
> +	int			whichfork,
> +	int			nr_to_add)
> +{
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
> +	uint64_t		max_exts;
> +	uint64_t		nr_exts;
> +
> +	if (whichfork == XFS_COW_FORK)
> +		return 0;
> +
> +	max_exts = (whichfork == XFS_ATTR_FORK) ? MAXAEXTNUM : MAXEXTNUM;
> +
> +	nr_exts = ifp->if_nextents + nr_to_add;
> +	if (nr_exts < ifp->if_nextents || nr_exts > max_exts)
> +		return -EFBIG;
> +
> +	return 0;
> +}
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index a4953e95c4f3..0beb8e2a00be 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -172,5 +172,7 @@ extern void xfs_ifork_init_cow(struct xfs_inode *ip);
>  
>  int xfs_ifork_verify_local_data(struct xfs_inode *ip);
>  int xfs_ifork_verify_local_attr(struct xfs_inode *ip);
> +int xfs_iext_count_may_overflow(struct xfs_inode *ip, int whichfork,
> +		int nr_to_add);
>  
>  #endif	/* __XFS_INODE_FORK_H__ */
> -- 
> 2.28.0
> 

Re: [PATCH V3 02/10] xfs: Check for extent overflow when trivally adding a new extent

[Darrick J. Wong]

On Thu, Aug 20, 2020 at 11:13:41AM +0530, Chandan Babu R wrote:
> When adding a new data extent (without modifying an inode's existing
> extents) the extent count increases only by 1. This commit checks for
> extent count overflow in such cases.
> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>

Seems fine to me...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_bmap.c       | 6 ++++++
>  fs/xfs/libxfs/xfs_inode_fork.h | 6 ++++++
>  fs/xfs/xfs_bmap_util.c         | 5 +++++
>  fs/xfs/xfs_dquot.c             | 8 +++++++-
>  fs/xfs/xfs_iomap.c             | 5 +++++
>  fs/xfs/xfs_rtalloc.c           | 5 +++++
>  6 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 9c40d5971035..dcc8eeecd571 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4527,6 +4527,12 @@ xfs_bmapi_convert_delalloc(
>  		return error;
>  
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +
> +	error = xfs_iext_count_may_overflow(ip, whichfork,
> +			XFS_IEXT_ADD_NOSPLIT_CNT);
> +	if (error)
> +		goto out_trans_cancel;
> +
>  	xfs_trans_ijoin(tp, ip, 0);
>  
>  	if (!xfs_iext_lookup_extent(ip, ifp, offset_fsb, &bma.icur, &bma.got) ||
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 0beb8e2a00be..7fc2b129a2e7 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -34,6 +34,12 @@ struct xfs_ifork {
>  #define	XFS_IFEXTENTS	0x02	/* All extent pointers are read in */
>  #define	XFS_IFBROOT	0x04	/* i_broot points to the bmap b-tree root */
>  
> +/*
> + * Worst-case increase in the fork extent count when we're adding a single
> + * extent to a fork and there's no possibility of splitting an existing mapping.
> + */
> +#define XFS_IEXT_ADD_NOSPLIT_CNT	(1)
> +
>  /*
>   * Fork handling.
>   */
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index afdc7f8e0e70..7b76a48b0885 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -822,6 +822,11 @@ xfs_alloc_file_space(
>  		if (error)
>  			goto error1;
>  
> +		error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
> +				XFS_IEXT_ADD_NOSPLIT_CNT);
> +		if (error)
> +			goto error0;
> +
>  		xfs_trans_ijoin(tp, ip, 0);
>  
>  		error = xfs_bmapi_write(tp, ip, startoffset_fsb,
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 04dc2be19c3a..59ea9485ebda 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -290,8 +290,14 @@ xfs_dquot_disk_alloc(
>  		return -ESRCH;
>  	}
>  
> -	/* Create the block mapping. */
>  	xfs_trans_ijoin(tp, quotip, XFS_ILOCK_EXCL);
> +
> +	error = xfs_iext_count_may_overflow(quotip, XFS_DATA_FORK,
> +			XFS_IEXT_ADD_NOSPLIT_CNT);
> +	if (error)
> +		return error;
> +
> +	/* Create the block mapping. */
>  	error = xfs_bmapi_write(tp, quotip, dqp->q_fileoffset,
>  			XFS_DQUOT_CLUSTER_SIZE_FSB, XFS_BMAPI_METADATA, 0, &map,
>  			&nmaps);
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 0e3f62cde375..37b0c743c116 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -250,6 +250,11 @@ xfs_iomap_write_direct(
>  	if (error)
>  		goto out_trans_cancel;
>  
> +	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
> +			XFS_IEXT_ADD_NOSPLIT_CNT);
> +	if (error)
> +		goto out_trans_cancel;
> +
>  	xfs_trans_ijoin(tp, ip, 0);
>  
>  	/*
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 6209e7b6b895..9508ab00a00b 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -787,6 +787,11 @@ xfs_growfs_rt_alloc(
>  		xfs_ilock(ip, XFS_ILOCK_EXCL);
>  		xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>  
> +		error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
> +				XFS_IEXT_ADD_NOSPLIT_CNT);
> +		if (error)
> +			goto out_trans_cancel;
> +
>  		/*
>  		 * Allocate blocks to the bitmap file.
>  		 */
> -- 
> 2.28.0
> 

Re: [PATCH V3 10/10] xfs: Check for extent overflow when swapping extents

[Darrick J. Wong]

On Thu, Aug 20, 2020 at 11:13:49AM +0530, Chandan Babu R wrote:
> Removing an initial range of source/donor file's extent and adding a new
> extent (from donor/source file) in its place will cause extent count to
> increase by 1.
> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_inode_fork.h |  6 ++++++
>  fs/xfs/xfs_bmap_util.c         | 11 +++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index d1c675cf803a..4219b01f1034 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -100,6 +100,12 @@ struct xfs_ifork {
>   */
>  #define XFS_IEXT_REFLINK_REMAP_CNT(smap_real, dmap_written) \
>  	(((smap_real) ? 1 : 0) + ((dmap_written) ? 1 : 0))
> +/*
> + * Removing an initial range of source/donor file's extent and adding a new
> + * extent (from donor/source file) in its place will cause extent count to
> + * increase by 1.
> + */
> +#define XFS_IEXT_SWAP_RMAP_CNT		(1)
>  
>  /*
>   * Fork handling.
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index e682eecebb1f..7105525dadd5 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1375,6 +1375,17 @@ xfs_swap_extent_rmap(
>  		/* Unmap the old blocks in the source file. */
>  		while (tirec.br_blockcount) {
>  			ASSERT(tp->t_firstblock == NULLFSBLOCK);
> +
> +			error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
> +					XFS_IEXT_SWAP_RMAP_CNT);
> +			if (error)
> +				goto out;
> +
> +			error = xfs_iext_count_may_overflow(tip, XFS_DATA_FORK,
> +					XFS_IEXT_SWAP_RMAP_CNT);

Heh, the old swapext code is very gritty.  Two questions--

If either of irec and uirec describe a hole, why do we need to check for
an extent count overflow?

Second, is the transaction clean at the point where we could goto out?
I'm pretty sure it is, but if there's a chance we could end up bailing
out with a dirty transaction, then we need to do this check elsewhere
such that we don't shut down the filesystem.

(I'm pretty sure the answer to #2 is "yes", but I thought I'd better
ask.)

--D

> +			if (error)
> +				goto out;
> +
>  			trace_xfs_swap_extent_rmap_remap_piece(tip, &tirec);
>  
>  			/* Read extent from the source file */
> -- 
> 2.28.0
> 

Re: [PATCH V3 09/10] xfs: Check for extent overflow when remapping an extent

[Darrick J. Wong]

On Thu, Aug 20, 2020 at 11:13:48AM +0530, Chandan Babu R wrote:
> Remapping an extent involves unmapping the existing extent and mapping
> in the new extent. When unmapping, an extent containing the entire unmap
> range can be split into two extents,
> i.e. | Old extent | hole | Old extent |
> Hence extent count increases by 1.
> 
> Mapping in the new extent into the destination file can increase the
> extent count by 1.
> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_inode_fork.h | 14 ++++++++++++++
>  fs/xfs/xfs_reflink.c           |  5 +++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 850d53162545..d1c675cf803a 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -86,6 +86,20 @@ struct xfs_ifork {
>   * Hence number of extents increases by 2.
>   */
>  #define XFS_IEXT_REFLINK_END_COW_CNT	(2)
> +/*

It's usually considered good style to put a blank line between the
previous definition and the new comment.

With that fixed,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> + * Remapping an extent involves unmapping the existing extent and mapping in the
> + * new extent.
> + *
> + * When unmapping, an extent containing the entire unmap range can be split into
> + * two extents,
> + * i.e. | Old extent | hole | Old extent |
> + * Hence extent count increases by 1.
> + *
> + * Mapping in the new extent into the destination file can increase the extent
> + * count by 1.
> + */
> +#define XFS_IEXT_REFLINK_REMAP_CNT(smap_real, dmap_written) \
> +	(((smap_real) ? 1 : 0) + ((dmap_written) ? 1 : 0))
>  
>  /*
>   * Fork handling.
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index c1d2a741e1af..9884fd51efee 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -1099,6 +1099,11 @@ xfs_reflink_remap_extent(
>  			goto out_cancel;
>  	}
>  
> +	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
> +			XFS_IEXT_REFLINK_REMAP_CNT(smap_real, dmap_written));
> +	if (error)
> +		goto out_cancel;
> +
>  	if (smap_real) {
>  		/*
>  		 * If the extent we're unmapping is backed by storage (written
> -- 
> 2.28.0
> 

Re: [PATCH V3 08/10] xfs: Check for extent overflow when moving extent from cow to data fork

[Darrick J. Wong]

On Thu, Aug 20, 2020 at 11:13:47AM +0530, Chandan Babu R wrote:
> Moving an extent to data fork can cause a sub-interval of an existing
> extent to be unmapped. This will increase extent count by 1. Mapping in
> the new extent can increase the extent count by 1 again i.e.
>  | Old extent | New extent | Old extent |
> Hence number of extents increases by 2.
> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_inode_fork.h | 9 ++++++++-
>  fs/xfs/xfs_reflink.c           | 5 +++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index d0e49b015b62..850d53162545 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -78,7 +78,14 @@ struct xfs_ifork {
>   * split into two extents causing extent count to increase by 1.
>   */
>  #define XFS_IEXT_INSERT_HOLE_CNT	(1)
> -
> +/*
> + * Moving an extent to data fork can cause a sub-interval of an existing extent
> + * to be unmapped. This will increase extent count by 1. Mapping in the new
> + * extent can increase the extent count by 1 again i.e.
> + * | Old extent | New extent | Old extent |
> + * Hence number of extents increases by 2.
> + */
> +#define XFS_IEXT_REFLINK_END_COW_CNT	(2)
>  
>  /*
>   * Fork handling.
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index aac83f9d6107..c1d2a741e1af 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -628,6 +628,11 @@ xfs_reflink_end_cow_extent(
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(tp, ip, 0);
>  
> +	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
> +			XFS_IEXT_REFLINK_END_COW_CNT);
> +	if (error)
> +		goto out_cancel;

What happens if we fail here?  I think for buffered writes this means
that writeback fails and we store an EIO in the address space for
eventual return via fsync()?   And for a direct write this means that
EIO gets sent back to the caller, right?

Assuming I understood that correctly, I think this is a reasonable
enough place to check for overflows, and hence

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

It would be nicer to check this kind of thing at write() time to put all
the EFBIG errors up front, but I don't think you can do that without
tracking extent count "reservations" incore.

--D

> +
>  	/*
>  	 * 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
> -- 
> 2.28.0
> 

Re: [PATCH V3 03/10] xfs: Check for extent overflow when deleting an extent

[Darrick J. Wong]

On Thu, Aug 20, 2020 at 11:13:42AM +0530, Chandan Babu R wrote:
> Deleting a file range from the middle of an existing extent can cause
> the per-inode extent count to increase by 1. This commit checks for
> extent count overflow in such cases.
> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_inode_fork.h | 6 ++++++
>  fs/xfs/xfs_bmap_item.c         | 4 ++++
>  fs/xfs/xfs_bmap_util.c         | 5 +++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 7fc2b129a2e7..2642e4847ee0 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -39,6 +39,12 @@ struct xfs_ifork {
>   * extent to a fork and there's no possibility of splitting an existing mapping.
>   */
>  #define XFS_IEXT_ADD_NOSPLIT_CNT	(1)
> +/*
> + * Removing an extent from the middle of an existing extent can cause the extent
> + * count to increase by 1.
> + * i.e. | Old extent | Hole | Old extent |
> + */
> +#define XFS_IEXT_REMOVE_CNT		(1)

The first thought that popped into my head after reading the subject
line was "UH-oh, is this going to result in undeletable files when the
extent counts hit max and the user tries to rm?"

Then I realized that "when deleting an extent" actually refers to
punching holes in the middle of files, not truncating them.

So I think at the very least the subject line should be changed to
say that we're talking about hole punching, not general file deletion;
and the constant probably ought to be called XFS_IEXT_PUNCH_CNT to make
that clearer.

Aside from that the logic seems ok to me.

(Also PS I'm not reviewing these patches in order...)

--D

>  
>  /*
>   * Fork handling.
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index ec3691372e7c..b9c35fb10de4 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -519,6 +519,10 @@ xfs_bui_item_recover(
>  	}
>  	xfs_trans_ijoin(tp, ip, 0);
>  
> +	error = xfs_iext_count_may_overflow(ip, whichfork, XFS_IEXT_REMOVE_CNT);
> +	if (error)
> +		goto err_inode;
> +
>  	count = bmap->me_len;
>  	error = xfs_trans_log_finish_bmap_update(tp, budp, type, ip, whichfork,
>  			bmap->me_startoff, bmap->me_startblock, &count, state);
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 7b76a48b0885..59d4da38aadf 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -891,6 +891,11 @@ xfs_unmap_extent(
>  
>  	xfs_trans_ijoin(tp, ip, 0);
>  
> +	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
> +			XFS_IEXT_REMOVE_CNT);
> +	if (error)
> +		goto out_trans_cancel;
> +
>  	error = xfs_bunmapi(tp, ip, startoffset_fsb, len_fsb, 0, 2, done);
>  	if (error)
>  		goto out_trans_cancel;
> -- 
> 2.28.0
> 

Re: [PATCH V3 04/10] xfs: Check for extent overflow when adding/removing xattrs

[Darrick J. Wong]

On Thu, Aug 20, 2020 at 11:13:43AM +0530, Chandan Babu R wrote:
> Adding/removing an xattr can cause XFS_DA_NODE_MAXDEPTH extents to be
> added. One extra extent for dabtree in case a local attr is large enough
> to cause a double split.  It can also cause extent count to increase
> proportional to the size of a remote xattr's value.
> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c       | 13 +++++++++++++
>  fs/xfs/libxfs/xfs_inode_fork.h |  9 +++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index d4583a0d1b3f..c481389da40f 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -396,6 +396,7 @@ xfs_attr_set(
>  	struct xfs_trans_res	tres;
>  	bool			rsvd = (args->attr_filter & XFS_ATTR_ROOT);
>  	int			error, local;
> +	int			rmt_blks = 0;
>  	unsigned int		total;
>  
>  	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
> @@ -442,11 +443,15 @@ xfs_attr_set(
>  		tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
>  		tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
>  		total = args->total;
> +
> +		if (!local)
> +			rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
>  	} else {
>  		XFS_STATS_INC(mp, xs_attr_remove);
>  
>  		tres = M_RES(mp)->tr_attrrm;
>  		total = XFS_ATTRRM_SPACE_RES(mp);
> +		rmt_blks = xfs_attr3_rmt_blocks(mp, XFS_XATTR_SIZE_MAX);
>  	}
>  
>  	/*
> @@ -460,6 +465,14 @@ xfs_attr_set(
>  
>  	xfs_ilock(dp, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(args->trans, dp, 0);
> +
> +	if (args->value || xfs_inode_hasattr(dp)) {
> +		error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK,
> +				XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));

What happens if the free space is fragmented and each of these rmt
blocks results in a separate allocation?

I'm also not sure why we'd need to account for the remote blocks if
we're removing an attr?  Those mappings simply go away, right?

--D

> +		if (error)
> +			goto out_trans_cancel;
> +	}
> +
>  	if (args->value) {
>  		unsigned int	quota_flags = XFS_QMOPT_RES_REGBLKS;
>  
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 2642e4847ee0..aae8e6e80b71 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -45,6 +45,15 @@ struct xfs_ifork {
>   * i.e. | Old extent | Hole | Old extent |
>   */
>  #define XFS_IEXT_REMOVE_CNT		(1)
> +/*
> + * Adding/removing an xattr can cause XFS_DA_NODE_MAXDEPTH extents to
> + * be added. One extra extent for dabtree in case a local attr is
> + * large enough to cause a double split.  It can also cause extent
> + * count to increase proportional to the size of a remote xattr's
> + * value.
> + */
> +#define XFS_IEXT_ATTR_MANIP_CNT(rmt_blks) \
> +	(XFS_DA_NODE_MAXDEPTH + max(1, rmt_blks))
>  
>  /*
>   * Fork handling.
> -- 
> 2.28.0
> 

Re: [PATCH V3 05/10] xfs: Check for extent overflow when adding/removing dir entries

[Darrick J. Wong]

On Thu, Aug 20, 2020 at 11:13:44AM +0530, Chandan Babu R wrote:
> Directory entry addition/removal can cause the following,
> 1. Data block can be added/removed.
>    A new extent can cause extent count to increase by 1.
> 2. Free disk block can be added/removed.
>    Same behaviour as described above for Data block.
> 3. Dabtree blocks.
>    XFS_DA_NODE_MAXDEPTH blocks can be added. Each of these
>    can be new extents. Hence extent count can increase by
>    XFS_DA_NODE_MAXDEPTH.
> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>

I wonder how long until someone's going to ask us to make
XFS_IEXT_DIR_MANIP_CNT return a number that's more tightly tied to the
actual directory inode in question, but 4 billion extents would already
be pretty egregious, so maybe we don't care...

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

--D

> ---
>  fs/xfs/libxfs/xfs_inode_fork.h | 12 ++++++++++++
>  fs/xfs/xfs_inode.c             | 27 +++++++++++++++++++++++++++
>  fs/xfs/xfs_symlink.c           |  5 +++++
>  3 files changed, 44 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index aae8e6e80b71..f686c7418d2b 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -54,6 +54,18 @@ struct xfs_ifork {
>   */
>  #define XFS_IEXT_ATTR_MANIP_CNT(rmt_blks) \
>  	(XFS_DA_NODE_MAXDEPTH + max(1, rmt_blks))
> +/*
> + * Directory entry addition/removal can cause the following,
> + * 1. Data block can be added/removed.
> + *    A new extent can cause extent count to increase by 1.
> + * 2. Free disk block can be added/removed.
> + *    Same behaviour as described above for Data block.
> + * 3. Dabtree blocks.
> + *    XFS_DA_NODE_MAXDEPTH blocks can be added. Each of these can be new
> + *    extents. Hence extent count can increase by XFS_DA_NODE_MAXDEPTH.
> + */
> +#define XFS_IEXT_DIR_MANIP_CNT(mp) \
> +	((XFS_DA_NODE_MAXDEPTH + 1 + 1) * (mp)->m_dir_geo->fsbcount)
>  
>  /*
>   * Fork handling.
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 407d6299606d..8d195b6ef326 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1175,6 +1175,11 @@ xfs_create(
>  	if (error)
>  		goto out_trans_cancel;
>  
> +	error = xfs_iext_count_may_overflow(dp, XFS_DATA_FORK,
> +			XFS_IEXT_DIR_MANIP_CNT(mp));
> +	if (error)
> +		goto out_trans_cancel;
> +
>  	/*
>  	 * A newly created regular or special file just has one directory
>  	 * entry pointing to them, but a directory also the "." entry
> @@ -1391,6 +1396,11 @@ xfs_link(
>  	xfs_trans_ijoin(tp, sip, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(tp, tdp, XFS_ILOCK_EXCL);
>  
> +	error = xfs_iext_count_may_overflow(tdp, XFS_DATA_FORK,
> +			XFS_IEXT_DIR_MANIP_CNT(mp));
> +	if (error)
> +		goto error_return;
> +
>  	/*
>  	 * If we are using project inheritance, we only allow hard link
>  	 * creation in our tree when the project IDs are the same; else
> @@ -2861,6 +2871,11 @@ xfs_remove(
>  	xfs_trans_ijoin(tp, dp, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>  
> +	error = xfs_iext_count_may_overflow(dp, XFS_DATA_FORK,
> +			XFS_IEXT_DIR_MANIP_CNT(mp));
> +	if (error)
> +		goto out_trans_cancel;
> +
>  	/*
>  	 * If we're removing a directory perform some additional validation.
>  	 */
> @@ -3221,6 +3236,18 @@ xfs_rename(
>  	if (wip)
>  		xfs_trans_ijoin(tp, wip, XFS_ILOCK_EXCL);
>  
> +	error = xfs_iext_count_may_overflow(src_dp, XFS_DATA_FORK,
> +			XFS_IEXT_DIR_MANIP_CNT(mp));
> +	if (error)
> +		goto out_trans_cancel;
> +
> +	if (target_ip == NULL) {
> +		error = xfs_iext_count_may_overflow(target_dp, XFS_DATA_FORK,
> +				XFS_IEXT_DIR_MANIP_CNT(mp));
> +		if (error)
> +			goto out_trans_cancel;
> +	}
> +
>  	/*
>  	 * If we are using project inheritance, we only allow renames
>  	 * into our tree when the project IDs are the same; else the
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index 8e88a7ca387e..581a4032a817 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -220,6 +220,11 @@ xfs_symlink(
>  	if (error)
>  		goto out_trans_cancel;
>  
> +	error = xfs_iext_count_may_overflow(dp, XFS_DATA_FORK,
> +			XFS_IEXT_DIR_MANIP_CNT(mp));
> +	if (error)
> +		goto out_trans_cancel;
> +
>  	/*
>  	 * Allocate an inode for the symlink.
>  	 */
> -- 
> 2.28.0
> 

Re: [PATCH V3 01/10] xfs: Add helper for checking per-inode extent count overflow

[Darrick J. Wong]

On Mon, Aug 31, 2020 at 09:08:23AM -0700, Darrick J. Wong wrote:
> On Thu, Aug 20, 2020 at 11:13:40AM +0530, Chandan Babu R wrote:
> > XFS does not check for possible overflow of per-inode extent counter
> > fields when adding extents to either data or attr fork.
> > 
> > For e.g.
> > 1. Insert 5 million xattrs (each having a value size of 255 bytes) and
> >    then delete 50% of them in an alternating manner.
> > 
> > 2. On a 4k block sized XFS filesystem instance, the above causes 98511
> >    extents to be created in the attr fork of the inode.
> > 
> >    xfsaild/loop0  2008 [003]  1475.127209: probe:xfs_inode_to_disk: (ffffffffa43fb6b0) if_nextents=98511 i_ino=131
> > 
> > 3. The incore inode fork extent counter is a signed 32-bit
> >    quantity. However the on-disk extent counter is an unsigned 16-bit
> >    quantity and hence cannot hold 98511 extents.
> > 
> > 4. The following incorrect value is stored in the attr extent counter,
> >    # xfs_db -f -c 'inode 131' -c 'print core.naextents' /dev/loop0
> >    core.naextents = -32561
> > 
> > This commit adds a new helper function (i.e.
> > xfs_iext_count_may_overflow()) to check for overflow of the per-inode
> > data and xattr extent counters. Future patches will use this function to
> > make sure that an FS operation won't cause the extent counter to
> > overflow.
> > 
> > Suggested-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> 
> Seems reasonable so far...
> 
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> --D
> 
> > ---
> >  fs/xfs/libxfs/xfs_inode_fork.c | 23 +++++++++++++++++++++++
> >  fs/xfs/libxfs/xfs_inode_fork.h |  2 ++
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> > index 0cf853d42d62..3a084aea8f85 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.c
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> > @@ -23,6 +23,7 @@
> >  #include "xfs_da_btree.h"
> >  #include "xfs_dir2_priv.h"
> >  #include "xfs_attr_leaf.h"
> > +#include "xfs_types.h"
> >  
> >  kmem_zone_t *xfs_ifork_zone;
> >  
> > @@ -728,3 +729,25 @@ xfs_ifork_verify_local_attr(
> >  
> >  	return 0;
> >  }
> > +
> > +int
> > +xfs_iext_count_may_overflow(
> > +	struct xfs_inode	*ip,
> > +	int			whichfork,
> > +	int			nr_to_add)
> > +{
> > +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
> > +	uint64_t		max_exts;
> > +	uint64_t		nr_exts;
> > +
> > +	if (whichfork == XFS_COW_FORK)
> > +		return 0;
> > +
> > +	max_exts = (whichfork == XFS_ATTR_FORK) ? MAXAEXTNUM : MAXEXTNUM;
> > +
> > +	nr_exts = ifp->if_nextents + nr_to_add;
> > +	if (nr_exts < ifp->if_nextents || nr_exts > max_exts)
> > +		return -EFBIG;

Something I thought of after the fact -- can you add a new fault
injection point to lower the max extent count?  That way we can
facilitate the construction of fstests cases to check the operation of
the new predicate without having to spend lots of time constructing huge
fragmented files.

(There /are/ test cases somewhere, riiight? ;))

No need to add it here, you can tack it onto the end of the series as a
new patch.

--D

> > +
> > +	return 0;
> > +}
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > index a4953e95c4f3..0beb8e2a00be 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -172,5 +172,7 @@ extern void xfs_ifork_init_cow(struct xfs_inode *ip);
> >  
> >  int xfs_ifork_verify_local_data(struct xfs_inode *ip);
> >  int xfs_ifork_verify_local_attr(struct xfs_inode *ip);
> > +int xfs_iext_count_may_overflow(struct xfs_inode *ip, int whichfork,
> > +		int nr_to_add);
> >  
> >  #endif	/* __XFS_INODE_FORK_H__ */
> > -- 
> > 2.28.0
> > 

Re: [PATCH V3 06/10] xfs: Check for extent overflow when writing to unwritten extent

[Darrick J. Wong]

On Thu, Aug 20, 2020 at 11:13:45AM +0530, Chandan Babu R wrote:
> A write to a sub-interval of an existing unwritten extent causes
> the original extent to be split into 3 extents
> i.e. | Unwritten | Real | Unwritten |
> Hence extent count can increase by 2.
> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>

Seems fine to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_inode_fork.h | 7 +++++++
>  fs/xfs/xfs_iomap.c             | 5 +++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index f686c7418d2b..83ff90e2a5fe 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -66,6 +66,13 @@ struct xfs_ifork {
>   */
>  #define XFS_IEXT_DIR_MANIP_CNT(mp) \
>  	((XFS_DA_NODE_MAXDEPTH + 1 + 1) * (mp)->m_dir_geo->fsbcount)
> +/*
> + * A write to a sub-interval of an existing unwritten extent causes the original
> + * extent to be split into 3 extents
> + * i.e. | Unwritten | Real | Unwritten |
> + * Hence extent count can increase by 2.
> + */
> +#define XFS_IEXT_WRITE_UNWRITTEN_CNT	(2)
>  
>  /*
>   * Fork handling.
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 37b0c743c116..694b25fbb4a3 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -566,6 +566,11 @@ xfs_iomap_write_unwritten(
>  		if (error)
>  			goto error_on_bmapi_transaction;
>  
> +		error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
> +				XFS_IEXT_WRITE_UNWRITTEN_CNT);
> +		if (error)
> +			goto error_on_bmapi_transaction;
> +
>  		/*
>  		 * Modify the unwritten extent state of the buffer.
>  		 */
> -- 
> 2.28.0
> 

Re: [PATCH V3 07/10] xfs: Check for extent overflow when inserting a hole

[Darrick J. Wong]

On Thu, Aug 20, 2020 at 11:13:46AM +0530, Chandan Babu R wrote:
> The extent mapping the file offset at which a hole has to be
> inserted will be split into two extents causing extent count to
> increase by 1.
> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_inode_fork.h | 6 ++++++
>  fs/xfs/xfs_bmap_util.c         | 9 +++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 83ff90e2a5fe..d0e49b015b62 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -73,6 +73,12 @@ struct xfs_ifork {
>   * Hence extent count can increase by 2.
>   */
>  #define XFS_IEXT_WRITE_UNWRITTEN_CNT	(2)
> +/*
> + * The extent mapping the file offset at which a hole has to be inserted will be
> + * split into two extents causing extent count to increase by 1.
> + */
> +#define XFS_IEXT_INSERT_HOLE_CNT	(1)

Given my earlier comments about how patch 3 is really only testing the
hole punching case, maybe it should be folded into this one?  They're
both inserting holes in the extent map.

Otherwise this patch looks good to me.

--D

> +
>  
>  /*
>   * Fork handling.
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 59d4da38aadf..e682eecebb1f 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1165,6 +1165,15 @@ xfs_insert_file_space(
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(tp, ip, 0);
>  
> +	/*
> +	 * Splitting the extent mapping containing stop_fsb will cause
> +	 * extent count to increase by 1.
> +	 */
> +	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
> +			XFS_IEXT_INSERT_HOLE_CNT);
> +	if (error)
> +		goto out_trans_cancel;
> +
>  	/*
>  	 * The extent shifting code works on extent granularity. So, if stop_fsb
>  	 * is not the starting block of extent, we need to split the extent at
> -- 
> 2.28.0
> 

Re: [PATCH V3 01/10] xfs: Add helper for checking per-inode extent count overflow

[Chandan Babu R]

On Monday 31 August 2020 10:14:35 PM IST Darrick J. Wong wrote:
> On Mon, Aug 31, 2020 at 09:08:23AM -0700, Darrick J. Wong wrote:
> > On Thu, Aug 20, 2020 at 11:13:40AM +0530, Chandan Babu R wrote:
> > > XFS does not check for possible overflow of per-inode extent counter
> > > fields when adding extents to either data or attr fork.
> > > 
> > > For e.g.
> > > 1. Insert 5 million xattrs (each having a value size of 255 bytes) and
> > >    then delete 50% of them in an alternating manner.
> > > 
> > > 2. On a 4k block sized XFS filesystem instance, the above causes 98511
> > >    extents to be created in the attr fork of the inode.
> > > 
> > >    xfsaild/loop0  2008 [003]  1475.127209: probe:xfs_inode_to_disk: (ffffffffa43fb6b0) if_nextents=98511 i_ino=131
> > > 
> > > 3. The incore inode fork extent counter is a signed 32-bit
> > >    quantity. However the on-disk extent counter is an unsigned 16-bit
> > >    quantity and hence cannot hold 98511 extents.
> > > 
> > > 4. The following incorrect value is stored in the attr extent counter,
> > >    # xfs_db -f -c 'inode 131' -c 'print core.naextents' /dev/loop0
> > >    core.naextents = -32561
> > > 
> > > This commit adds a new helper function (i.e.
> > > xfs_iext_count_may_overflow()) to check for overflow of the per-inode
> > > data and xattr extent counters. Future patches will use this function to
> > > make sure that an FS operation won't cause the extent counter to
> > > overflow.
> > > 
> > > Suggested-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > 
> > Seems reasonable so far...
> > 
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > --D
> > 
> > > ---
> > >  fs/xfs/libxfs/xfs_inode_fork.c | 23 +++++++++++++++++++++++
> > >  fs/xfs/libxfs/xfs_inode_fork.h |  2 ++
> > >  2 files changed, 25 insertions(+)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> > > index 0cf853d42d62..3a084aea8f85 100644
> > > --- a/fs/xfs/libxfs/xfs_inode_fork.c
> > > +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> > > @@ -23,6 +23,7 @@
> > >  #include "xfs_da_btree.h"
> > >  #include "xfs_dir2_priv.h"
> > >  #include "xfs_attr_leaf.h"
> > > +#include "xfs_types.h"
> > >  
> > >  kmem_zone_t *xfs_ifork_zone;
> > >  
> > > @@ -728,3 +729,25 @@ xfs_ifork_verify_local_attr(
> > >  
> > >  	return 0;
> > >  }
> > > +
> > > +int
> > > +xfs_iext_count_may_overflow(
> > > +	struct xfs_inode	*ip,
> > > +	int			whichfork,
> > > +	int			nr_to_add)
> > > +{
> > > +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
> > > +	uint64_t		max_exts;
> > > +	uint64_t		nr_exts;
> > > +
> > > +	if (whichfork == XFS_COW_FORK)
> > > +		return 0;
> > > +
> > > +	max_exts = (whichfork == XFS_ATTR_FORK) ? MAXAEXTNUM : MAXEXTNUM;
> > > +
> > > +	nr_exts = ifp->if_nextents + nr_to_add;
> > > +	if (nr_exts < ifp->if_nextents || nr_exts > max_exts)
> > > +		return -EFBIG;
> 
> Something I thought of after the fact -- can you add a new fault
> injection point to lower the max extent count?  That way we can
> facilitate the construction of fstests cases to check the operation of
> the new predicate without having to spend lots of time constructing huge
> fragmented files.

Sure, I will do that.

> 
> (There /are/ test cases somewhere, riiight? ;))

Apart from executing xfstests, I had tested the patchset with the use case
described in the commit message of this patch. But with an error injection
facility available, it should be easier to add tests to fstests. I will work
on that. Thanks for the suggestion.

> 
> No need to add it here, you can tack it onto the end of the series as a
> new patch.
> 
> --D
> 
> > > +
> > > +	return 0;
> > > +}
> > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > > index a4953e95c4f3..0beb8e2a00be 100644
> > > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > > @@ -172,5 +172,7 @@ extern void xfs_ifork_init_cow(struct xfs_inode *ip);
> > >  
> > >  int xfs_ifork_verify_local_data(struct xfs_inode *ip);
> > >  int xfs_ifork_verify_local_attr(struct xfs_inode *ip);
> > > +int xfs_iext_count_may_overflow(struct xfs_inode *ip, int whichfork,
> > > +		int nr_to_add);
> > >  
> > >  #endif	/* __XFS_INODE_FORK_H__ */
> 


-- 
chandan

Re: [PATCH V3 03/10] xfs: Check for extent overflow when deleting an extent

[Chandan Babu R]

On Monday 31 August 2020 10:04:51 PM IST Darrick J. Wong wrote:
> On Thu, Aug 20, 2020 at 11:13:42AM +0530, Chandan Babu R wrote:
> > Deleting a file range from the middle of an existing extent can cause
> > the per-inode extent count to increase by 1. This commit checks for
> > extent count overflow in such cases.
> > 
> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > ---
> >  fs/xfs/libxfs/xfs_inode_fork.h | 6 ++++++
> >  fs/xfs/xfs_bmap_item.c         | 4 ++++
> >  fs/xfs/xfs_bmap_util.c         | 5 +++++
> >  3 files changed, 15 insertions(+)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > index 7fc2b129a2e7..2642e4847ee0 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -39,6 +39,12 @@ struct xfs_ifork {
> >   * extent to a fork and there's no possibility of splitting an existing mapping.
> >   */
> >  #define XFS_IEXT_ADD_NOSPLIT_CNT	(1)
> > +/*
> > + * Removing an extent from the middle of an existing extent can cause the extent
> > + * count to increase by 1.
> > + * i.e. | Old extent | Hole | Old extent |
> > + */
> > +#define XFS_IEXT_REMOVE_CNT		(1)
> 
> The first thought that popped into my head after reading the subject
> line was "UH-oh, is this going to result in undeletable files when the
> extent counts hit max and the user tries to rm?"
> 
> Then I realized that "when deleting an extent" actually refers to
> punching holes in the middle of files, not truncating them.
> 
> So I think at the very least the subject line should be changed to
> say that we're talking about hole punching, not general file deletion;
> and the constant probably ought to be called XFS_IEXT_PUNCH_CNT to make
> that clearer.
>

Sure, I will change the commit message and the name of the constant.

> Aside from that the logic seems ok to me.
> 
> (Also PS I'm not reviewing these patches in order...)
> 
> --D
> 
> >  
> >  /*
> >   * Fork handling.
> > diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> > index ec3691372e7c..b9c35fb10de4 100644
> > --- a/fs/xfs/xfs_bmap_item.c
> > +++ b/fs/xfs/xfs_bmap_item.c
> > @@ -519,6 +519,10 @@ xfs_bui_item_recover(
> >  	}
> >  	xfs_trans_ijoin(tp, ip, 0);
> >  
> > +	error = xfs_iext_count_may_overflow(ip, whichfork, XFS_IEXT_REMOVE_CNT);
> > +	if (error)
> > +		goto err_inode;
> > +
> >  	count = bmap->me_len;
> >  	error = xfs_trans_log_finish_bmap_update(tp, budp, type, ip, whichfork,
> >  			bmap->me_startoff, bmap->me_startblock, &count, state);
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index 7b76a48b0885..59d4da38aadf 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -891,6 +891,11 @@ xfs_unmap_extent(
> >  
> >  	xfs_trans_ijoin(tp, ip, 0);
> >  
> > +	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
> > +			XFS_IEXT_REMOVE_CNT);
> > +	if (error)
> > +		goto out_trans_cancel;
> > +
> >  	error = xfs_bunmapi(tp, ip, startoffset_fsb, len_fsb, 0, 2, done);
> >  	if (error)
> >  		goto out_trans_cancel;
> 

-- 
chandan

Re: [PATCH V3 04/10] xfs: Check for extent overflow when adding/removing xattrs

[Chandan Babu R]

On Monday 31 August 2020 10:07:59 PM IST Darrick J. Wong wrote:
> On Thu, Aug 20, 2020 at 11:13:43AM +0530, Chandan Babu R wrote:
> > Adding/removing an xattr can cause XFS_DA_NODE_MAXDEPTH extents to be
> > added. One extra extent for dabtree in case a local attr is large enough
> > to cause a double split.  It can also cause extent count to increase
> > proportional to the size of a remote xattr's value.
> > 
> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > ---
> >  fs/xfs/libxfs/xfs_attr.c       | 13 +++++++++++++
> >  fs/xfs/libxfs/xfs_inode_fork.h |  9 +++++++++
> >  2 files changed, 22 insertions(+)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index d4583a0d1b3f..c481389da40f 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -396,6 +396,7 @@ xfs_attr_set(
> >  	struct xfs_trans_res	tres;
> >  	bool			rsvd = (args->attr_filter & XFS_ATTR_ROOT);
> >  	int			error, local;
> > +	int			rmt_blks = 0;
> >  	unsigned int		total;
> >  
> >  	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
> > @@ -442,11 +443,15 @@ xfs_attr_set(
> >  		tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
> >  		tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> >  		total = args->total;
> > +
> > +		if (!local)
> > +			rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
> >  	} else {
> >  		XFS_STATS_INC(mp, xs_attr_remove);
> >  
> >  		tres = M_RES(mp)->tr_attrrm;
> >  		total = XFS_ATTRRM_SPACE_RES(mp);
> > +		rmt_blks = xfs_attr3_rmt_blocks(mp, XFS_XATTR_SIZE_MAX);
> >  	}
> >  
> >  	/*
> > @@ -460,6 +465,14 @@ xfs_attr_set(
> >  
> >  	xfs_ilock(dp, XFS_ILOCK_EXCL);
> >  	xfs_trans_ijoin(args->trans, dp, 0);
> > +
> > +	if (args->value || xfs_inode_hasattr(dp)) {
> > +		error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK,
> > +				XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
> 
> What happens if the free space is fragmented and each of these rmt
> blocks results in a separate allocation?

In such a case, We would have "rmt_blks" number of new extents. These are
accounted for by "max(1, rmt_blks)" in XFS_IEXT_ATTR_MANIP_CNT(rmt_blks)
macro. The two arguments to max() i.e. "1" and "rmt_blks" are mutually
exclusive.
"1" occurs when a local value is large enough to cause a double split of a
dabtree leaf.
"rmt_blks" occurs when an xattr value is large enough to be stored
non-locally.

> 
> I'm also not sure why we'd need to account for the remote blocks if
> we're removing an attr?  Those mappings simply go away, right?
>

Lets say the following extent mappings are present in the attr fork of an
inode,
 | Dabtree block | Hole | Dabtree block |

Lets say, to store an xattr's remote value we allocate blocks which cause the
following,
 | Dabtree block | Remote value | Dabtree block |

i.e the region labelled above as "Remote value" is contiguous with both its
neighbours in terms of both "file offset" (belonging to attr fork) and "disk
offset" space. In such a case, xfs_bmap_add_extent_hole_real() would mark the
entire region shown above as just one extent.

A future xattr remove opertion, will fragment this extent into two causing
extent count to increase by 1. Considering the worst case scenario where each
of blocks holding the remote value ends up belonging to such an extent, the
macro XFS_IEXT_ATTR_MANIP_CNT() adds "rmt_blks" number to possible increase in
extent count.

> --D
> 
> > +		if (error)
> > +			goto out_trans_cancel;
> > +	}
> > +
> >  	if (args->value) {
> >  		unsigned int	quota_flags = XFS_QMOPT_RES_REGBLKS;
> >  
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > index 2642e4847ee0..aae8e6e80b71 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -45,6 +45,15 @@ struct xfs_ifork {
> >   * i.e. | Old extent | Hole | Old extent |
> >   */
> >  #define XFS_IEXT_REMOVE_CNT		(1)
> > +/*
> > + * Adding/removing an xattr can cause XFS_DA_NODE_MAXDEPTH extents to
> > + * be added. One extra extent for dabtree in case a local attr is
> > + * large enough to cause a double split.  It can also cause extent
> > + * count to increase proportional to the size of a remote xattr's
> > + * value.
> > + */
> > +#define XFS_IEXT_ATTR_MANIP_CNT(rmt_blks) \
> > +	(XFS_DA_NODE_MAXDEPTH + max(1, rmt_blks))
> >  
> >  /*
> >   * Fork handling.
> 

-- 
chandan

Re: [PATCH V3 07/10] xfs: Check for extent overflow when inserting a hole

[Chandan Babu R]

On Monday 31 August 2020 10:16:35 PM IST Darrick J. Wong wrote:
> On Thu, Aug 20, 2020 at 11:13:46AM +0530, Chandan Babu R wrote:
> > The extent mapping the file offset at which a hole has to be
> > inserted will be split into two extents causing extent count to
> > increase by 1.
> > 
> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > ---
> >  fs/xfs/libxfs/xfs_inode_fork.h | 6 ++++++
> >  fs/xfs/xfs_bmap_util.c         | 9 +++++++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > index 83ff90e2a5fe..d0e49b015b62 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -73,6 +73,12 @@ struct xfs_ifork {
> >   * Hence extent count can increase by 2.
> >   */
> >  #define XFS_IEXT_WRITE_UNWRITTEN_CNT	(2)
> > +/*
> > + * The extent mapping the file offset at which a hole has to be inserted will be
> > + * split into two extents causing extent count to increase by 1.
> > + */
> > +#define XFS_IEXT_INSERT_HOLE_CNT	(1)
> 
> Given my earlier comments about how patch 3 is really only testing the
> hole punching case, maybe it should be folded into this one?  They're
> both inserting holes in the extent map.
>
I agree. I will make the required changes.

-- 
chandan

Re: [PATCH V3 08/10] xfs: Check for extent overflow when moving extent from cow to data fork

[Chandan Babu R]

On Monday 31 August 2020 9:59:08 PM IST Darrick J. Wong wrote:
> On Thu, Aug 20, 2020 at 11:13:47AM +0530, Chandan Babu R wrote:
> > Moving an extent to data fork can cause a sub-interval of an existing
> > extent to be unmapped. This will increase extent count by 1. Mapping in
> > the new extent can increase the extent count by 1 again i.e.
> >  | Old extent | New extent | Old extent |
> > Hence number of extents increases by 2.
> > 
> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > ---
> >  fs/xfs/libxfs/xfs_inode_fork.h | 9 ++++++++-
> >  fs/xfs/xfs_reflink.c           | 5 +++++
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > index d0e49b015b62..850d53162545 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -78,7 +78,14 @@ struct xfs_ifork {
> >   * split into two extents causing extent count to increase by 1.
> >   */
> >  #define XFS_IEXT_INSERT_HOLE_CNT	(1)
> > -
> > +/*
> > + * Moving an extent to data fork can cause a sub-interval of an existing extent
> > + * to be unmapped. This will increase extent count by 1. Mapping in the new
> > + * extent can increase the extent count by 1 again i.e.
> > + * | Old extent | New extent | Old extent |
> > + * Hence number of extents increases by 2.
> > + */
> > +#define XFS_IEXT_REFLINK_END_COW_CNT	(2)
> >  
> >  /*
> >   * Fork handling.
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index aac83f9d6107..c1d2a741e1af 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -628,6 +628,11 @@ xfs_reflink_end_cow_extent(
> >  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> >  	xfs_trans_ijoin(tp, ip, 0);
> >  
> > +	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
> > +			XFS_IEXT_REFLINK_END_COW_CNT);
> > +	if (error)
> > +		goto out_cancel;
> 
> What happens if we fail here?  I think for buffered writes this means
> that writeback fails and we store an EIO in the address space for
> eventual return via fsync()?   And for a direct write this means that
> EIO gets sent back to the caller, right?
>

Yes, you are right about that.

> Assuming I understood that correctly, I think this is a reasonable
> enough place to check for overflows, and hence
> 
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> It would be nicer to check this kind of thing at write() time to put all
> the EFBIG errors up front, but I don't think you can do that without
> tracking extent count "reservations" incore.
> 
> --D
> 
> > +
> >  	/*
> >  	 * 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
> 

-- 
chandan

Re: [PATCH V3 09/10] xfs: Check for extent overflow when remapping an extent

[Chandan Babu R]

On Monday 31 August 2020 9:53:35 PM IST Darrick J. Wong wrote:
> On Thu, Aug 20, 2020 at 11:13:48AM +0530, Chandan Babu R wrote:
> > Remapping an extent involves unmapping the existing extent and mapping
> > in the new extent. When unmapping, an extent containing the entire unmap
> > range can be split into two extents,
> > i.e. | Old extent | hole | Old extent |
> > Hence extent count increases by 1.
> > 
> > Mapping in the new extent into the destination file can increase the
> > extent count by 1.
> > 
> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > ---
> >  fs/xfs/libxfs/xfs_inode_fork.h | 14 ++++++++++++++
> >  fs/xfs/xfs_reflink.c           |  5 +++++
> >  2 files changed, 19 insertions(+)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > index 850d53162545..d1c675cf803a 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -86,6 +86,20 @@ struct xfs_ifork {
> >   * Hence number of extents increases by 2.
> >   */
> >  #define XFS_IEXT_REFLINK_END_COW_CNT	(2)
> > +/*
> 
> It's usually considered good style to put a blank line between the
> previous definition and the new comment.
>

Ok. I will fix that.

> With that fixed,
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 

-- 
chandan

Re: [PATCH V3 10/10] xfs: Check for extent overflow when swapping extents

[Chandan Babu R]

On Monday 31 August 2020 9:50:39 PM IST Darrick J. Wong wrote:
> On Thu, Aug 20, 2020 at 11:13:49AM +0530, Chandan Babu R wrote:
> > Removing an initial range of source/donor file's extent and adding a new
> > extent (from donor/source file) in its place will cause extent count to
> > increase by 1.
> > 
> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > ---
> >  fs/xfs/libxfs/xfs_inode_fork.h |  6 ++++++
> >  fs/xfs/xfs_bmap_util.c         | 11 +++++++++++
> >  2 files changed, 17 insertions(+)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > index d1c675cf803a..4219b01f1034 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -100,6 +100,12 @@ struct xfs_ifork {
> >   */
> >  #define XFS_IEXT_REFLINK_REMAP_CNT(smap_real, dmap_written) \
> >  	(((smap_real) ? 1 : 0) + ((dmap_written) ? 1 : 0))
> > +/*
> > + * Removing an initial range of source/donor file's extent and adding a new
> > + * extent (from donor/source file) in its place will cause extent count to
> > + * increase by 1.
> > + */
> > +#define XFS_IEXT_SWAP_RMAP_CNT		(1)
> >  
> >  /*
> >   * Fork handling.
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index e682eecebb1f..7105525dadd5 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -1375,6 +1375,17 @@ xfs_swap_extent_rmap(
> >  		/* Unmap the old blocks in the source file. */
> >  		while (tirec.br_blockcount) {
> >  			ASSERT(tp->t_firstblock == NULLFSBLOCK);
> > +
> > +			error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
> > +					XFS_IEXT_SWAP_RMAP_CNT);
> > +			if (error)
> > +				goto out;
> > +
> > +			error = xfs_iext_count_may_overflow(tip, XFS_DATA_FORK,
> > +					XFS_IEXT_SWAP_RMAP_CNT);
> 
> Heh, the old swapext code is very gritty.  Two questions--
> 
> If either of irec and uirec describe a hole, why do we need to check for
> an extent count overflow?

Thanks for pointing this out. I missed this. The check for overflow should be
moved later in the code i.e. after reading up the extent mappings. Also, the
overflow check should be made on source file only if the donor file extent
has a valid on-disk mapping and vice versa.

> 
> Second, is the transaction clean at the point where we could goto out?
> I'm pretty sure it is, but if there's a chance we could end up bailing
> out with a dirty transaction, then we need to do this check elsewhere
> such that we don't shut down the filesystem.
> 
> (I'm pretty sure the answer to #2 is "yes", but I thought I'd better
> ask.)

In xfs_swap_extents(), the code between allocating a new transaction and
invoking xfs_swap_extent_rmap() does not peform any operation that can dirty
the transaction.

Inside xfs_swap_extent_rmap(), we invoke xfs_defer_finish() every time we
register deferred operations to exchange extents between two inodes'
forks. xfs_defer_finish() will always return with a clean transaction. So, we
can safely return an error code on detecting an overflow.
> 
> --D
> 
> > +			if (error)
> > +				goto out;
> > +
> >  			trace_xfs_swap_extent_rmap_remap_piece(tip, &tirec);
> >  
> >  			/* Read extent from the source file */
> 

-- 
chandan