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