Re: [PATCH 1/1] xfs: enable FITRIM on the realtime device

[Christoph Hellwig <hch@xxxxxxxxxxxxx>]

> +	if (!bdev_max_discard_sectors(mp->m_ddev_targp->bt_bdev))
> +		goto rt_discard;
> +

I think this would looks much better if we split the ddev trimming
into a separate helper, just like this patch does for the RT device:

diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index 3ad5b0505848b0..4eb71edf732c48 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -595,6 +595,48 @@ xfs_trim_rtdev_extents(
 # define xfs_trim_rtdev_extents(m,s,e,n,b)	(-EOPNOTSUPP)
 #endif /* CONFIG_XFS_RT */
 
+static int
+xfs_trim_dddev_extents(
+	struct xfs_mount	*mp,
+	xfs_daddr_t		start,
+	xfs_daddr_t		end,
+	xfs_extlen_t		minlen,
+	uint64_t		*blocks_trimmed)
+{
+	xfs_agnumber_t		start_agno, end_agno;
+	xfs_agblock_t		start_agbno, end_agbno;
+	xfs_daddr_t		ddev_end;
+	struct xfs_perag	*pag;
+	int			last_error = 0, error;
+
+	ddev_end = min_t(xfs_daddr_t, end,
+			 XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks) - 1);
+
+	start_agno = xfs_daddr_to_agno(mp, start);
+	start_agbno = xfs_daddr_to_agbno(mp, start);
+	end_agno = xfs_daddr_to_agno(mp, ddev_end);
+	end_agbno = xfs_daddr_to_agbno(mp, ddev_end);
+
+	for_each_perag_range(mp, start_agno, end_agno, pag) {
+		xfs_agblock_t	agend = pag->block_count;
+
+		if (start_agno == end_agno)
+			agend = end_agbno;
+		error = xfs_trim_extents(pag, start_agbno, agend, minlen,
+				blocks_trimmed);
+		if (error)
+			last_error = error;
+
+		if (xfs_trim_should_stop()) {
+			xfs_perag_rele(pag);
+			break;
+		}
+		start_agbno = 0;
+	}
+
+	return last_error;
+}
+
 /*
  * trim a range of the filesystem.
  *
@@ -612,15 +654,12 @@ xfs_ioc_trim(
 	struct xfs_mount		*mp,
 	struct fstrim_range __user	*urange)
 {
-	struct xfs_perag	*pag;
 	unsigned int		granularity =
 		bdev_discard_granularity(mp->m_ddev_targp->bt_bdev);
 	struct block_device	*rt_bdev = NULL;
 	struct fstrim_range	range;
-	xfs_daddr_t		start, end, ddev_end;
+	xfs_daddr_t		start, end;
 	xfs_extlen_t		minlen;
-	xfs_agnumber_t		start_agno, end_agno;
-	xfs_agblock_t		start_agbno, end_agbno;
 	xfs_rfsblock_t		max_blocks;
 	uint64_t		blocks_trimmed = 0;
 	int			error, last_error = 0;
@@ -666,35 +705,13 @@ xfs_ioc_trim(
 	start = BTOBB(range.start);
 	end = start + BTOBBT(range.len) - 1;
 
-	if (!bdev_max_discard_sectors(mp->m_ddev_targp->bt_bdev))
-		goto rt_discard;
-
-	ddev_end = min_t(xfs_daddr_t, end,
-			 XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks) - 1);
-
-	start_agno = xfs_daddr_to_agno(mp, start);
-	start_agbno = xfs_daddr_to_agbno(mp, start);
-	end_agno = xfs_daddr_to_agno(mp, ddev_end);
-	end_agbno = xfs_daddr_to_agbno(mp, ddev_end);
-
-	for_each_perag_range(mp, start_agno, end_agno, pag) {
-		xfs_agblock_t	agend = pag->block_count;
-
-		if (start_agno == end_agno)
-			agend = end_agbno;
-		error = xfs_trim_extents(pag, start_agbno, agend, minlen,
+	if (bdev_max_discard_sectors(mp->m_ddev_targp->bt_bdev)) {
+		error = xfs_trim_dddev_extents(mp, start, end, minlen,
 				&blocks_trimmed);
 		if (error)
 			last_error = error;
-
-		if (xfs_trim_should_stop()) {
-			xfs_perag_rele(pag);
-			break;
-		}
-		start_agbno = 0;
 	}
 
-rt_discard:
 	if (rt_bdev && bdev_max_discard_sectors(rt_bdev)) {
 		error = xfs_trim_rtdev_extents(mp, start, end, minlen,
 				&blocks_trimmed);