Re: [f2fs-dev] [PATCH V3 6/6] xfs: ignore discard return value

Yongpeng Yang
2025-11-26T02:37:17Z

On 11/25/25 07:48, Chaitanya Kulkarni wrote:
> __blkdev_issue_discard() always returns 0, making all error checking
> in XFS discard functions dead code.
>
> Change xfs_discard_extents() return type to void, remove error variable,
> error checking, and error logging for the __blkdev_issue_discard() call
> in same function.
>
> Update xfs_trim_perag_extents() and xfs_trim_rtgroup_extents() to
> ignore the xfs_discard_extents() return value and error checking
> code.
>
> Update xfs_discard_rtdev_extents() to ignore __blkdev_issue_discard()
> return value and error checking code.
>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Chaitanya Kulkarni <ckulkarnilinux@gmail.com>
> ---
> fs/xfs/xfs_discard.c | 27 +++++----------------------
> fs/xfs/xfs_discard.h | 2 +-
> 2 files changed, 6 insertions(+), 23 deletions(-)
>
> diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> index 6917de832191..b6ffe4807a11 100644
> --- a/fs/xfs/xfs_discard.c
> +++ b/fs/xfs/xfs_discard.c
> @@ -108,7 +108,7 @@ xfs_discard_endio(
> * list. We plug and chain the bios so that we only need a single completion
> * call to clear all the busy extents once the discards are complete.
> */
> -int
> +void
> xfs_discard_extents(
> struct xfs_mount *mp,
> struct xfs_busy_extents *extents)
> @@ -116,7 +116,6 @@ xfs_discard_extents(
> struct xfs_extent_busy *busyp;
> struct bio *bio = NULL;
> struct blk_plug plug;
> - int error = 0;
>
> blk_start_plug(&plug);
> list_for_each_entry(busyp, &extents->extent_list, list) {
> @@ -126,18 +125,10 @@ xfs_discard_extents(
>
> trace_xfs_discard_extent(xg, busyp->bno, busyp->length);
>
> - error = __blkdev_issue_discard(btp->bt_bdev,
> + __blkdev_issue_discard(btp->bt_bdev,
> xfs_gbno_to_daddr(xg, busyp->bno),
> XFS_FSB_TO_BB(mp, busyp->length),
> GFP_KERNEL, &bio);

If blk_alloc_discard_bio() fails to allocate a bio inside
__blkdev_issue_discard(), this may lead to an invalid loop in
list_for_each_entry{}. Instead of using __blkdev_issue_discard(), how
about allocate and submit the discard bios explicitly in
list_for_each_entry{}?

Yongpeng,

> - if (error && error != -EOPNOTSUPP) {
> - xfs_info(mp,
> - "discard failed for extent [0x%llx,%u], error %d",
> - (unsigned long long)busyp->bno,
> - busyp->length,
> - error);
> - break;
> - }
> }
>
> if (bio) {
> @@ -148,8 +139,6 @@ xfs_discard_extents(
> xfs_discard_endio_work(&extents->endio_work);
> }
> blk_finish_plug(&plug);
> -
> - return error;
> }
>
> /*
> @@ -385,9 +374,7 @@ xfs_trim_perag_extents(
> * list after this function call, as it may have been freed by
> * the time control returns to us.
> */
> - error = xfs_discard_extents(pag_mount(pag), extents);
> - if (error)
> - break;
> + xfs_discard_extents(pag_mount(pag), extents);
>
> if (xfs_trim_should_stop())
> break;
> @@ -496,12 +483,10 @@ xfs_discard_rtdev_extents(
>
> trace_xfs_discard_rtextent(mp, busyp->bno, busyp->length);
>
> - error = __blkdev_issue_discard(bdev,
> + __blkdev_issue_discard(bdev,
> xfs_rtb_to_daddr(mp, busyp->bno),
> XFS_FSB_TO_BB(mp, busyp->length),
> GFP_NOFS, &bio);
> - if (error)
> - break;
> }
> xfs_discard_free_rtdev_extents(tr);
>
> @@ -741,9 +726,7 @@ xfs_trim_rtgroup_extents(
> * list after this function call, as it may have been freed by
> * the time control returns to us.
> */
> - error = xfs_discard_extents(rtg_mount(rtg), tr.extents);
> - if (error)
> - break;
> + xfs_discard_extents(rtg_mount(rtg), tr.extents);
>
> low = tr.restart_rtx;
> } while (!xfs_trim_should_stop() && low <= high);
> diff --git a/fs/xfs/xfs_discard.h b/fs/xfs/xfs_discard.h
> index 2b1a85223a56..8c5cc4af6a07 100644
> --- a/fs/xfs/xfs_discard.h
> +++ b/fs/xfs/xfs_discard.h
> @@ -6,7 +6,7 @@ struct fstrim_range;
> struct xfs_mount;
> struct xfs_busy_extents;
>
> -int xfs_discard_extents(struct xfs_mount *mp, struct xfs_busy_extents *busy);
> +void xfs_discard_extents(struct xfs_mount *mp, struct xfs_busy_extents *busy);
> int xfs_ioc_trim(struct xfs_mount *mp, struct fstrim_range __user *fstrim);
>
> #endif /* XFS_DISCARD_H */