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

[PATCH V2 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  2035 [003]  9643.390490: probe:xfs_iflush_int: (ffffffffac6225c0) if_nextents=98511 inode=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-v2.

Changelog:
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       |  8 ++++
 fs/xfs/libxfs/xfs_inode_fork.h | 72 ++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_trans_resv.c | 33 ++++++++++++++++
 fs/xfs/libxfs/xfs_trans_resv.h |  2 +
 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           | 11 ++++++
 fs/xfs/xfs_rtalloc.c           |  5 +++
 fs/xfs/xfs_symlink.c           |  5 +++
 13 files changed, 227 insertions(+), 1 deletion(-)

-- 
2.28.0

[PATCH V2 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  2035 [003]  9643.390490: probe:xfs_iflush_int: (ffffffffac6225c0) if_nextents=98511 inode=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 will not 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_trans_resv.c | 33 +++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_trans_resv.h |  2 ++
 2 files changed, 35 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index d1a0848cb52e..d21990d9df7a 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -832,6 +832,39 @@ xfs_calc_sb_reservation(
 	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
 }
 
+int
+xfs_iext_count_may_overflow(
+	struct xfs_inode	*ip,
+	int			whichfork,
+	int			nr_to_add)
+{
+	struct xfs_ifork	*ifp;
+	uint64_t		max_exts = 0;
+	uint64_t		nr_exts;
+
+	switch (whichfork) {
+	case XFS_DATA_FORK:
+		max_exts = MAXEXTNUM;
+		break;
+
+	case XFS_ATTR_FORK:
+		max_exts = MAXAEXTNUM;
+		break;
+
+	default:
+		ASSERT(0);
+		break;
+	}
+
+	ifp = XFS_IFORK_PTR(ip, whichfork);
+	nr_exts = ifp->if_nextents + nr_to_add;
+
+	if (nr_exts > max_exts)
+		return -EFBIG;
+
+	return 0;
+}
+
 void
 xfs_trans_resv_calc(
 	struct xfs_mount	*mp,
diff --git a/fs/xfs/libxfs/xfs_trans_resv.h b/fs/xfs/libxfs/xfs_trans_resv.h
index 7241ab28cf84..9d71b51990ac 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.h
+++ b/fs/xfs/libxfs/xfs_trans_resv.h
@@ -93,5 +93,7 @@ struct xfs_trans_resv {
 
 void xfs_trans_resv_calc(struct xfs_mount *mp, struct xfs_trans_resv *resp);
 uint xfs_allocfree_log_count(struct xfs_mount *mp, uint num_ops);
+int xfs_iext_count_may_overflow(struct xfs_inode *ip, int whichfork,
+		int nr_exts);
 
 #endif	/* __XFS_TRANS_RESV_H__ */
-- 
2.28.0

[PATCH V2 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       | 8 ++++++++
 fs/xfs/libxfs/xfs_inode_fork.h | 2 ++
 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, 32 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 9c40d5971035..e64f645415b1 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4527,6 +4527,14 @@ xfs_bmapi_convert_delalloc(
 		return error;
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
+
+	if (whichfork == XFS_DATA_FORK) {
+		error = xfs_iext_count_may_overflow(ip, whichfork,
+				XFS_IEXT_ADD_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 a4953e95c4f3..3e7e4b980d49 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -34,6 +34,8 @@ 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 */
 
+#define XFS_IEXT_ADD_CNT 1
+
 /*
  * Fork handling.
  */
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index afdc7f8e0e70..c470f2cd6e66 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_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..1293e7a752c8 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_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..0af679ef9a33 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_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..c2bca6ad2533 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_CNT);
+		if (error)
+			goto out_trans_cancel;
+
 		/*
 		 * Allocate blocks to the bitmap file.
 		 */
-- 
2.28.0

[PATCH V2 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 3e7e4b980d49..228359cf9738 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -35,6 +35,12 @@ struct xfs_ifork {
 #define	XFS_IFBROOT	0x04	/* i_broot points to the bmap b-tree root */
 
 #define XFS_IEXT_ADD_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 c470f2cd6e66..94abdb547c7f 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 V2 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 228359cf9738..72a9daf5df16 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -41,6 +41,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 V2 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 | 13 +++++++++++++
 fs/xfs/xfs_inode.c             | 27 +++++++++++++++++++++++++++
 fs/xfs/xfs_symlink.c           |  5 +++++
 3 files changed, 45 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 72a9daf5df16..e39ce09a824f 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -50,6 +50,19 @@ 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 V2 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 e39ce09a824f..a929fa094cf6 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -63,6 +63,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 0af679ef9a33..81cccd4abcc6 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 V2 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 a929fa094cf6..63f83a13e0a8 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -70,6 +70,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 94abdb547c7f..f6352b5e5552 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 V2 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 | 10 +++++++++-
 fs/xfs/xfs_reflink.c           |  6 ++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 63f83a13e0a8..d750bdff17c9 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -76,7 +76,15 @@ struct xfs_ifork {
  * 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..04a7754ee681 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -29,6 +29,7 @@
 #include "xfs_iomap.h"
 #include "xfs_sb.h"
 #include "xfs_ag_resv.h"
+#include "xfs_trans_resv.h"
 
 /*
  * Copy on Write of Shared Blocks
@@ -628,6 +629,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 V2 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 | 15 +++++++++++++++
 fs/xfs/xfs_reflink.c           |  5 +++++
 2 files changed, 20 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index d750bdff17c9..afff20703270 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -85,6 +85,21 @@ 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 04a7754ee681..134d49f8c941 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1100,6 +1100,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 V2 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 afff20703270..82b26536f218 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -98,6 +98,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
 
 
 /*
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index f6352b5e5552..8159306a8c41 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 V2 01/10] xfs: Add helper for checking per-inode extent count overflow

[Christoph Hellwig]

> +int
> +xfs_iext_count_may_overflow(
> +	struct xfs_inode	*ip,
> +	int			whichfork,
> +	int			nr_to_add)
> +{
> +	struct xfs_ifork	*ifp;
> +	uint64_t		max_exts = 0;
> +	uint64_t		nr_exts;
> +
> +	switch (whichfork) {
> +	case XFS_DATA_FORK:
> +		max_exts = MAXEXTNUM;
> +		break;
> +
> +	case XFS_ATTR_FORK:
> +		max_exts = MAXAEXTNUM;
> +		break;
> +
> +	default:
> +		ASSERT(0);
> +		break;
> +	}
> +
> +	ifp = XFS_IFORK_PTR(ip, whichfork);
> +	nr_exts = ifp->if_nextents + nr_to_add;
> +
> +	if (nr_exts > max_exts)
> +		return -EFBIG;
> +
> +	return 0;
> +}

Maybe it's just me, but I would structure this very different (just
cosmetic differences, though).  First add a:

static inline uint32_t xfs_max_extents(int whichfork)
{
	return XFS_ATTR_FORK ? MAXAEXTNUM : MAXEXTNUM;
}

to have a single place that determines the max number of extents.

And the simplify the helper down to:

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 = xfs_max_extents(whichfork);
	uint64_t		nr_exts;

	if (check_add_overflow(ifp->if_nextents, nr_to_add, &nr_exts) ||
	    nr_exts > max_exts))
		return -EFBIG;
	return 0;
}

which actually might be small enough for an inline function now.

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

[Christoph Hellwig]

On Fri, Aug 14, 2020 at 01:38:25PM +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>
> ---
>  fs/xfs/libxfs/xfs_bmap.c       | 8 ++++++++
>  fs/xfs/libxfs/xfs_inode_fork.h | 2 ++
>  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, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 9c40d5971035..e64f645415b1 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4527,6 +4527,14 @@ xfs_bmapi_convert_delalloc(
>  		return error;
>  
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +
> +	if (whichfork == XFS_DATA_FORK) {

Should we add COW fork special casing to xfs_iext_count_may_overflow
instead?

> +		error = xfs_iext_count_may_overflow(ip, whichfork,
> +				XFS_IEXT_ADD_CNT);

I find the XFS_IEXT_ADD_CNT define very confusing.  An explicit 1 passed
for a counter parameter makes a lot more sense to me.

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

[Chandan Babu R]

On Monday 17 August 2020 12:21:23 PM IST Christoph Hellwig wrote:
> > +int
> > +xfs_iext_count_may_overflow(
> > +	struct xfs_inode	*ip,
> > +	int			whichfork,
> > +	int			nr_to_add)
> > +{
> > +	struct xfs_ifork	*ifp;
> > +	uint64_t		max_exts = 0;
> > +	uint64_t		nr_exts;
> > +
> > +	switch (whichfork) {
> > +	case XFS_DATA_FORK:
> > +		max_exts = MAXEXTNUM;
> > +		break;
> > +
> > +	case XFS_ATTR_FORK:
> > +		max_exts = MAXAEXTNUM;
> > +		break;
> > +
> > +	default:
> > +		ASSERT(0);
> > +		break;
> > +	}
> > +
> > +	ifp = XFS_IFORK_PTR(ip, whichfork);
> > +	nr_exts = ifp->if_nextents + nr_to_add;
> > +
> > +	if (nr_exts > max_exts)
> > +		return -EFBIG;
> > +
> > +	return 0;
> > +}
> 
> Maybe it's just me, but I would structure this very different (just
> cosmetic differences, though).  First add a:
> 
> static inline uint32_t xfs_max_extents(int whichfork)
> {
> 	return XFS_ATTR_FORK ? MAXAEXTNUM : MAXEXTNUM;
> }
> 
> to have a single place that determines the max number of extents.
> 
> And the simplify the helper down to:
> 
> 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 = xfs_max_extents(whichfork);
> 	uint64_t		nr_exts;
> 
> 	if (check_add_overflow(ifp->if_nextents, nr_to_add, &nr_exts) ||
> 	    nr_exts > max_exts))
> 		return -EFBIG;
> 	return 0;
> }
> 
> which actually might be small enough for an inline function now.
> 

I agree. I will make the suggested changes in the next version.

-- 
chandan

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

[Chandan Babu R]

On Monday 17 August 2020 12:23:07 PM IST Christoph Hellwig wrote:
> On Fri, Aug 14, 2020 at 01:38:25PM +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>
> > ---
> >  fs/xfs/libxfs/xfs_bmap.c       | 8 ++++++++
> >  fs/xfs/libxfs/xfs_inode_fork.h | 2 ++
> >  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, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 9c40d5971035..e64f645415b1 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -4527,6 +4527,14 @@ xfs_bmapi_convert_delalloc(
> >  		return error;
> >  
> >  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> > +
> > +	if (whichfork == XFS_DATA_FORK) {
> 
> Should we add COW fork special casing to xfs_iext_count_may_overflow
> instead?

I agree. Making xfs_iext_count_may_overflow() to always return success in the
case of CoW fork would mean that the if condition can be removed and hence
makes the code more readable.

> 
> > +		error = xfs_iext_count_may_overflow(ip, whichfork,
> > +				XFS_IEXT_ADD_CNT);
> 
> I find the XFS_IEXT_ADD_CNT define very confusing.  An explicit 1 passed
> for a counter parameter makes a lot more sense to me.

The reason to do this was to consolidate the comment descriptions at one
place. For e.g. the comment for XFS_IEXT_DIR_MANIP_CNT (from "[PATCH V2 05/10]
xfs: Check for extent overflow when adding/removing dir entries") is slightly
larger. Using constants (instead of macros) would mean that the same comment
has to be replicated across the 6 locations it is being used.

-- 
chandan

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

[Dave Chinner]

On Mon, Aug 17, 2020 at 07:53:07AM +0100, Christoph Hellwig wrote:
> On Fri, Aug 14, 2020 at 01:38:25PM +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>
> > ---
> >  fs/xfs/libxfs/xfs_bmap.c       | 8 ++++++++
> >  fs/xfs/libxfs/xfs_inode_fork.h | 2 ++
> >  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, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 9c40d5971035..e64f645415b1 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -4527,6 +4527,14 @@ xfs_bmapi_convert_delalloc(
> >  		return error;
> >  
> >  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> > +
> > +	if (whichfork == XFS_DATA_FORK) {
> 
> Should we add COW fork special casing to xfs_iext_count_may_overflow
> instead?
> 
> > +		error = xfs_iext_count_may_overflow(ip, whichfork,
> > +				XFS_IEXT_ADD_CNT);
> 
> I find the XFS_IEXT_ADD_CNT define very confusing.  An explicit 1 passed
> for a counter parameter makes a lot more sense to me.

I explicitly asked Chandan to convert all the magic numbers
sprinkled in the previous patch to defined values. It was impossible
to know whether the intended value was correct when it's just an
open coded number because we don't know what the number actually
stands for. And, in future, if we change the behaviour of a specific
operation, then we only have to change a single value rather than
having to track down and determine if every magic "1" is for an
extent add operation or something different.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Darrick J. Wong]

On Wed, Aug 19, 2020 at 07:49:33AM +1000, Dave Chinner wrote:
> On Mon, Aug 17, 2020 at 07:53:07AM +0100, Christoph Hellwig wrote:
> > On Fri, Aug 14, 2020 at 01:38:25PM +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>
> > > ---
> > >  fs/xfs/libxfs/xfs_bmap.c       | 8 ++++++++
> > >  fs/xfs/libxfs/xfs_inode_fork.h | 2 ++
> > >  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, 32 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > > index 9c40d5971035..e64f645415b1 100644
> > > --- a/fs/xfs/libxfs/xfs_bmap.c
> > > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > > @@ -4527,6 +4527,14 @@ xfs_bmapi_convert_delalloc(
> > >  		return error;
> > >  
> > >  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> > > +
> > > +	if (whichfork == XFS_DATA_FORK) {
> > 
> > Should we add COW fork special casing to xfs_iext_count_may_overflow
> > instead?

That seems like a reasonable idea.

> > 
> > > +		error = xfs_iext_count_may_overflow(ip, whichfork,
> > > +				XFS_IEXT_ADD_CNT);
> > 
> > I find the XFS_IEXT_ADD_CNT define very confusing.  An explicit 1 passed
> > for a counter parameter makes a lot more sense to me.
> 
> I explicitly asked Chandan to convert all the magic numbers
> sprinkled in the previous patch to defined values. It was impossible
> to know whether the intended value was correct when it's just an
> open coded number because we don't know what the number actually
> stands for. And, in future, if we change the behaviour of a specific
> operation, then we only have to change a single value rather than
> having to track down and determine if every magic "1" is for an
> extent add operation or something different.

I prefer named flags over magic numbers too, though this named constant
doesn't have a comment describing what it does, and "ADD_CNT" doesn't
really tell me much.  The subsequent patches have comments, so maybe
this should just become:

/*
 * 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	(1)

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

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

[Darrick J. Wong]

On Fri, Aug 14, 2020 at 01:38:31PM +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 | 10 +++++++++-
>  fs/xfs/xfs_reflink.c           |  6 ++++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 63f83a13e0a8..d750bdff17c9 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -76,7 +76,15 @@ struct xfs_ifork {
>   * 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 |

This comment is a little oddly formatted, mostly because my brain
thought that the line starting with "1. Mapping" was a numbered bullet
list.  If you reflow the comment further outward, you can get it to look
like this:

/*
 * Moving an extent to data fork can cause a sub-interval of an existing
 * extent to be unmapped, increasing extent count by 1. Mapping in the
 * new extent can also increase the extent count by 1:
 * | Old extent | New extent | Old extent |
 * Hence number of extents increases by 2.
 */
#define XFS_IEXT_REFLINK_END_COW_CNT 2

--D

> + * 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..04a7754ee681 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -29,6 +29,7 @@
>  #include "xfs_iomap.h"
>  #include "xfs_sb.h"
>  #include "xfs_ag_resv.h"
> +#include "xfs_trans_resv.h"
>  
>  /*
>   * Copy on Write of Shared Blocks
> @@ -628,6 +629,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
> 

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

[Chandan Babu R]

On Wednesday 19 August 2020 3:27:46 AM IST Darrick J. Wong wrote:
> On Wed, Aug 19, 2020 at 07:49:33AM +1000, Dave Chinner wrote:
> > On Mon, Aug 17, 2020 at 07:53:07AM +0100, Christoph Hellwig wrote:
> > > On Fri, Aug 14, 2020 at 01:38:25PM +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>
> > > > ---
> > > >  fs/xfs/libxfs/xfs_bmap.c       | 8 ++++++++
> > > >  fs/xfs/libxfs/xfs_inode_fork.h | 2 ++
> > > >  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, 32 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > > > index 9c40d5971035..e64f645415b1 100644
> > > > --- a/fs/xfs/libxfs/xfs_bmap.c
> > > > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > > > @@ -4527,6 +4527,14 @@ xfs_bmapi_convert_delalloc(
> > > >  		return error;
> > > >  
> > > >  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> > > > +
> > > > +	if (whichfork == XFS_DATA_FORK) {
> > > 
> > > Should we add COW fork special casing to xfs_iext_count_may_overflow
> > > instead?
> 
> That seems like a reasonable idea.
> 
> > > 
> > > > +		error = xfs_iext_count_may_overflow(ip, whichfork,
> > > > +				XFS_IEXT_ADD_CNT);
> > > 
> > > I find the XFS_IEXT_ADD_CNT define very confusing.  An explicit 1 passed
> > > for a counter parameter makes a lot more sense to me.
> > 
> > I explicitly asked Chandan to convert all the magic numbers
> > sprinkled in the previous patch to defined values. It was impossible
> > to know whether the intended value was correct when it's just an
> > open coded number because we don't know what the number actually
> > stands for. And, in future, if we change the behaviour of a specific
> > operation, then we only have to change a single value rather than
> > having to track down and determine if every magic "1" is for an
> > extent add operation or something different.
> 
> I prefer named flags over magic numbers too, though this named constant
> doesn't have a comment describing what it does, and "ADD_CNT" doesn't
> really tell me much.  The subsequent patches have comments, so maybe
> this should just become:
> 
> /*
>  * 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	(1)
>

That is perfect. Thanks for the suggestion. I will add that in the next
version of this patchset.

-- 
chandan

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

[Chandan Babu R]

On Wednesday 19 August 2020 3:33:07 AM IST Darrick J. Wong wrote:
> On Fri, Aug 14, 2020 at 01:38:31PM +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 | 10 +++++++++-
> >  fs/xfs/xfs_reflink.c           |  6 ++++++
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > index 63f83a13e0a8..d750bdff17c9 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -76,7 +76,15 @@ struct xfs_ifork {
> >   * 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 |
> 
> This comment is a little oddly formatted, mostly because my brain
> thought that the line starting with "1. Mapping" was a numbered bullet
> list.  If you reflow the comment further outward, you can get it to look
> like this:
> 
> /*
>  * Moving an extent to data fork can cause a sub-interval of an existing
>  * extent to be unmapped, increasing extent count by 1. Mapping in the
>  * new extent can also increase the extent count by 1:
>  * | Old extent | New extent | Old extent |
>  * Hence number of extents increases by 2.
>  */
> #define XFS_IEXT_REFLINK_END_COW_CNT 2
>

Sure. I will fix up all the comments in the patchset to extend upto 80
columns.

-- 
chandan

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

[Christoph Hellwig]

On Mon, Aug 17, 2020 at 01:14:16PM +0530, Chandan Babu R wrote:
> > > +		error = xfs_iext_count_may_overflow(ip, whichfork,
> > > +				XFS_IEXT_ADD_CNT);
> > 
> > I find the XFS_IEXT_ADD_CNT define very confusing.  An explicit 1 passed
> > for a counter parameter makes a lot more sense to me.
> 
> The reason to do this was to consolidate the comment descriptions at one
> place. For e.g. the comment for XFS_IEXT_DIR_MANIP_CNT (from "[PATCH V2 05/10]
> xfs: Check for extent overflow when adding/removing dir entries") is slightly
> larger. Using constants (instead of macros) would mean that the same comment
> has to be replicated across the 6 locations it is being used.

I agree with a constant if we have a complex computed value.  But a
constant for 1 where it is obvious from the context that one means
the number one as in adding a single items is just silly and really
hurts when reading the code.

> 
> -- 
> chandan
> 
> 
> 
---end quoted text---

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

[Chandan Babu R]

On Thursday 27 August 2020 1:39:03 PM IST Christoph Hellwig wrote:
> On Mon, Aug 17, 2020 at 01:14:16PM +0530, Chandan Babu R wrote:
> > > > +		error = xfs_iext_count_may_overflow(ip, whichfork,
> > > > +				XFS_IEXT_ADD_CNT);
> > > 
> > > I find the XFS_IEXT_ADD_CNT define very confusing.  An explicit 1 passed
> > > for a counter parameter makes a lot more sense to me.
> > 
> > The reason to do this was to consolidate the comment descriptions at one
> > place. For e.g. the comment for XFS_IEXT_DIR_MANIP_CNT (from "[PATCH V2 05/10]
> > xfs: Check for extent overflow when adding/removing dir entries") is slightly
> > larger. Using constants (instead of macros) would mean that the same comment
> > has to be replicated across the 6 locations it is being used.
> 
> I agree with a constant if we have a complex computed value.  But a
> constant for 1 where it is obvious from the context that one means
> the number one as in adding a single items is just silly and really
> hurts when reading the code.

I think we should retain the macros because there are nine macros out of which
three macros do trivial amounts of computation rather than having literal
integers as values. Having macros for these three while using literal integers
for other six cases would IMHO make the code non-uniform. If we end up
removing the macros completely we would have two problems,
1. Redundant code comments sprinkled across the code base explaining the logic
   behind computing the "extent delta".
2. In the future, if the "extent delta" value has to be changed for an
   operation, it has to be changed across all the relevant invocations of
   xfs_iext_count_may_overflow().

-- 
chandan