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

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Jun 20, 2024 at 10:00:32PM -0700, Christoph Hellwig wrote:
> > +	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);

I quite like this cleanup, though I think I'll name the helper
xfs_trim_datadev_extents instead.

>  		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)) {

This needs to check !xfs_trim_should_stop() as well, so that a ^C during
the data device discard isn't followed by a rtbitmap query.

Will send a V2.

--D

>  		error = xfs_trim_rtdev_extents(mp, start, end, minlen,
>  				&blocks_trimmed);
>