[RFC PATCH] block: change __blkdev_issue_discard() return type to void

[RFC PATCH] block: change __blkdev_issue_discard() return type to void

[Chaitanya Kulkarni]

__blkdev_issue_discard() always returns 0, making all error checking
at call sites dead code. The function simply stops allocating bios 
and returns 0.

Discard operations are advisory/optimization, not critical. Some callers
have dead error checking code expecting wrong return codes such as
-ENOTSUPP when 0 is only returned. 

This patch changes __blkdev_issue_discard() return type to void and
removes dead error checking code from all call sites:

* Block layer:
  blk-lib.c: Remove return value, update blkdev_issue_discard() caller

* Device mapper:
  dm-thin.c: Change issue_discard() to void, update both callers
  md.c: Simplify conditional to just check for NULL bio

* NVMe target:
  io-cmd-bdev.c: Remove dead error handling and error_slba assignment

* Filesystems:
  f2fs/segment.c: Preserve fault injection
  xfs/xfs_discard.c: Update both xfs_discard_extents() and
  xfs_discard_rtdev_extents() to remove dead error checks

Signed-off-by: Chaitanya Kulkarni <ckulkarnilinux@gmail.com>
---
Hi,

Due to involvement of all the subsystem making it as an RFC, ideally
it shuoldn't be an RFC.

-ck
---
 block/blk-lib.c                   |  7 +++----
 drivers/md/dm-thin.c              | 12 +++++-------
 drivers/md/md.c                   |  4 ++--
 drivers/nvme/target/io-cmd-bdev.c |  7 +------
 fs/f2fs/segment.c                 |  2 +-
 fs/xfs/xfs_discard.c              | 17 +++--------------
 include/linux/blkdev.h            |  2 +-
 7 files changed, 16 insertions(+), 35 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 3030a772d3aa..ca78ec6b5326 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -60,7 +60,7 @@ struct bio *blk_alloc_discard_bio(struct block_device *bdev,
 	return bio;
 }
 
-int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
+void __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		sector_t nr_sects, gfp_t gfp_mask, struct bio **biop)
 {
 	struct bio *bio;
@@ -68,7 +68,6 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	while ((bio = blk_alloc_discard_bio(bdev, &sector, &nr_sects,
 			gfp_mask)))
 		*biop = bio_chain_and_submit(*biop, bio);
-	return 0;
 }
 EXPORT_SYMBOL(__blkdev_issue_discard);
 
@@ -90,8 +89,8 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	int ret;
 
 	blk_start_plug(&plug);
-	ret = __blkdev_issue_discard(bdev, sector, nr_sects, gfp_mask, &bio);
-	if (!ret && bio) {
+	__blkdev_issue_discard(bdev, sector, nr_sects, gfp_mask, &bio);
+	if (bio) {
 		ret = submit_bio_wait(bio);
 		if (ret == -EOPNOTSUPP)
 			ret = 0;
diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index c84149ba4e38..77c76f75c85f 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -395,13 +395,13 @@ static void begin_discard(struct discard_op *op, struct thin_c *tc, struct bio *
 	op->bio = NULL;
 }
 
-static int issue_discard(struct discard_op *op, dm_block_t data_b, dm_block_t data_e)
+static void issue_discard(struct discard_op *op, dm_block_t data_b, dm_block_t data_e)
 {
 	struct thin_c *tc = op->tc;
 	sector_t s = block_to_sectors(tc->pool, data_b);
 	sector_t len = block_to_sectors(tc->pool, data_e - data_b);
 
-	return __blkdev_issue_discard(tc->pool_dev->bdev, s, len, GFP_NOIO, &op->bio);
+	__blkdev_issue_discard(tc->pool_dev->bdev, s, len, GFP_NOIO, &op->bio);
 }
 
 static void end_discard(struct discard_op *op, int r)
@@ -1113,9 +1113,7 @@ static void passdown_double_checking_shared_status(struct dm_thin_new_mapping *m
 				break;
 		}
 
-		r = issue_discard(&op, b, e);
-		if (r)
-			goto out;
+		issue_discard(&op, b, e);
 
 		b = e;
 	}
@@ -1188,8 +1186,8 @@ static void process_prepared_discard_passdown_pt1(struct dm_thin_new_mapping *m)
 		struct discard_op op;
 
 		begin_discard(&op, tc, discard_parent);
-		r = issue_discard(&op, m->data_block, data_end);
-		end_discard(&op, r);
+		issue_discard(&op, m->data_block, data_end);
+		end_discard(&op, 0);
 	}
 }
 
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 41c476b40c7a..7fc0bb7a3814 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9041,8 +9041,8 @@ void md_submit_discard_bio(struct mddev *mddev, struct md_rdev *rdev,
 {
 	struct bio *discard_bio = NULL;
 
-	if (__blkdev_issue_discard(rdev->bdev, start, size, GFP_NOIO,
-			&discard_bio) || !discard_bio)
+	__blkdev_issue_discard(rdev->bdev, start, size, GFP_NOIO, &discard_bio);
+	if (!discard_bio)
 		return;
 
 	bio_chain(discard_bio, bio);
diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
index 8d246b8ca604..f26010c46c33 100644
--- a/drivers/nvme/target/io-cmd-bdev.c
+++ b/drivers/nvme/target/io-cmd-bdev.c
@@ -366,16 +366,11 @@ static u16 nvmet_bdev_discard_range(struct nvmet_req *req,
 		struct nvme_dsm_range *range, struct bio **bio)
 {
 	struct nvmet_ns *ns = req->ns;
-	int ret;
 
-	ret = __blkdev_issue_discard(ns->bdev,
+	__blkdev_issue_discard(ns->bdev,
 			nvmet_lba_to_sect(ns, range->slba),
 			le32_to_cpu(range->nlb) << (ns->blksize_shift - 9),
 			GFP_KERNEL, bio);
-	if (ret && ret != -EOPNOTSUPP) {
-		req->error_slba = le64_to_cpu(range->slba);
-		return errno_to_nvme_status(req, ret);
-	}
 	return NVME_SC_SUCCESS;
 }
 
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index b45eace879d7..e6078176f733 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1346,7 +1346,7 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
 		if (time_to_inject(sbi, FAULT_DISCARD)) {
 			err = -EIO;
 		} else {
-			err = __blkdev_issue_discard(bdev,
+			__blkdev_issue_discard(bdev,
 					SECTOR_FROM_BLOCK(start),
 					SECTOR_FROM_BLOCK(len),
 					GFP_NOFS, &bio);
diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index ee49f20875af..f82cc07806df 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -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 (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) {
@@ -149,7 +140,7 @@ xfs_discard_extents(
 	}
 	blk_finish_plug(&plug);
 
-	return error;
+	return 0;
 }
 
 /*
@@ -496,12 +487,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);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index f0ab02e0a673..b05c37d20b09 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1258,7 +1258,7 @@ extern void blk_io_schedule(void);
 
 int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		sector_t nr_sects, gfp_t gfp_mask);
-int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
+void __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		sector_t nr_sects, gfp_t gfp_mask, struct bio **biop);
 int blkdev_issue_secure_erase(struct block_device *bdev, sector_t sector,
 		sector_t nr_sects, gfp_t gfp);
-- 
2.40.0

Re: [RFC PATCH] block: change __blkdev_issue_discard() return type to void

[Christoph Hellwig]

On Mon, Nov 17, 2025 at 11:42:43PM -0800, Chaitanya Kulkarni wrote:
> Due to involvement of all the subsystem making it as an RFC, ideally
> it shuoldn't be an RFC.

I think best would be a series that drops error checking first,
and then changes the return type.  That way we can maybe get all
the callers fixed up in this merge window and then drop the return
value after -rc1.
>  			gfp_mask)))
>  		*biop = bio_chain_and_submit(*biop, bio);
> -	return 0;
>  }
>  EXPORT_SYMBOL(__blkdev_issue_discard);
>  
> @@ -90,8 +89,8 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
>  	int ret;
>  
>  	blk_start_plug(&plug);
> -	ret = __blkdev_issue_discard(bdev, sector, nr_sects, gfp_mask, &bio);
> -	if (!ret && bio) {
> +	__blkdev_issue_discard(bdev, sector, nr_sects, gfp_mask, &bio);

ret now needs to be initialized to 0 above.

> index 8d246b8ca604..f26010c46c33 100644
> --- a/drivers/nvme/target/io-cmd-bdev.c
> +++ b/drivers/nvme/target/io-cmd-bdev.c
> @@ -366,16 +366,11 @@ static u16 nvmet_bdev_discard_range(struct nvmet_req *req,
>  		struct nvme_dsm_range *range, struct bio **bio)
>  {
>  	struct nvmet_ns *ns = req->ns;
> -	int ret;
>  
> -	ret = __blkdev_issue_discard(ns->bdev,
> +	__blkdev_issue_discard(ns->bdev,
>  			nvmet_lba_to_sect(ns, range->slba),
>  			le32_to_cpu(range->nlb) << (ns->blksize_shift - 9),
>  			GFP_KERNEL, bio);
> -	if (ret && ret != -EOPNOTSUPP) {
> -		req->error_slba = le64_to_cpu(range->slba);
> -		return errno_to_nvme_status(req, ret);
> -	}
>  	return NVME_SC_SUCCESS;

nvmet_bdev_discard_range can return void now.

> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index b45eace879d7..e6078176f733 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1346,7 +1346,7 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
>  		if (time_to_inject(sbi, FAULT_DISCARD)) {
>  			err = -EIO;
>  		} else {
> -			err = __blkdev_issue_discard(bdev,
> +			__blkdev_issue_discard(bdev,
>  					SECTOR_FROM_BLOCK(start),
>  					SECTOR_FROM_BLOCK(len),
>  					GFP_NOFS, &bio);

Please fold the following 'if (err)' block directly into the injection
one, and either initialize err to 0, or use a direct return from that
block to skip the last branch in the function checking err.

>  	blk_finish_plug(&plug);
>  
> -	return error;
> +	return 0;

Please drop the error return for xfs_discard_extents entirely.

Re: [RFC PATCH] block: change __blkdev_issue_discard() return type to void

[Chaitanya Kulkarni]

On 11/18/25 00:04, Christoph Hellwig wrote:
> On Mon, Nov 17, 2025 at 11:42:43PM -0800, Chaitanya Kulkarni wrote:
>> Due to involvement of all the subsystem making it as an RFC, ideally
>> it shuoldn't be an RFC.
> I think best would be a series that drops error checking first,
> and then changes the return type.  That way we can maybe get all
> the callers fixed up in this merge window and then drop the return
> value after -rc1.

thanks for the suggestion

>>   			gfp_mask)))
>>   		*biop = bio_chain_and_submit(*biop, bio);
>> -	return 0;
>>   }
>>   EXPORT_SYMBOL(__blkdev_issue_discard);
>>   
>> @@ -90,8 +89,8 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
>>   	int ret;
>>   
>>   	blk_start_plug(&plug);
>> -	ret = __blkdev_issue_discard(bdev, sector, nr_sects, gfp_mask, &bio);
>> -	if (!ret && bio) {
>> +	__blkdev_issue_discard(bdev, sector, nr_sects, gfp_mask, &bio);
> ret now needs to be initialized to 0 above.

done.

>
>> index 8d246b8ca604..f26010c46c33 100644
>> --- a/drivers/nvme/target/io-cmd-bdev.c
>> +++ b/drivers/nvme/target/io-cmd-bdev.c
>> @@ -366,16 +366,11 @@ static u16 nvmet_bdev_discard_range(struct nvmet_req *req,
>>   		struct nvme_dsm_range *range, struct bio **bio)
>>   {
>>   	struct nvmet_ns *ns = req->ns;
>> -	int ret;
>>   
>> -	ret = __blkdev_issue_discard(ns->bdev,
>> +	__blkdev_issue_discard(ns->bdev,
>>   			nvmet_lba_to_sect(ns, range->slba),
>>   			le32_to_cpu(range->nlb) << (ns->blksize_shift - 9),
>>   			GFP_KERNEL, bio);
>> -	if (ret && ret != -EOPNOTSUPP) {
>> -		req->error_slba = le64_to_cpu(range->slba);
>> -		return errno_to_nvme_status(req, ret);
>> -	}
>>   	return NVME_SC_SUCCESS;
> nvmet_bdev_discard_range can return void now.

done.

>
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index b45eace879d7..e6078176f733 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -1346,7 +1346,7 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
>>   		if (time_to_inject(sbi, FAULT_DISCARD)) {
>>   			err = -EIO;
>>   		} else {
>> -			err = __blkdev_issue_discard(bdev,
>> +			__blkdev_issue_discard(bdev,
>>   					SECTOR_FROM_BLOCK(start),
>>   					SECTOR_FROM_BLOCK(len),
>>   					GFP_NOFS, &bio);
> Please fold the following 'if (err)' block directly into the injection
> one, and either initialize err to 0, or use a direct return from that
> block to skip the last branch in the function checking err.

done :-

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index b45eace879d7..3dbcfb9067e9 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1343,15 +1343,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
  
                 dc->di.len += len;
  
+               err = 0;
                 if (time_to_inject(sbi, FAULT_DISCARD)) {
                         err = -EIO;
-               } else {
-                       err = __blkdev_issue_discard(bdev,
-                                       SECTOR_FROM_BLOCK(start),
-                                       SECTOR_FROM_BLOCK(len),
-                                       GFP_NOFS, &bio);
-               }
-               if (err) {
                         spin_lock_irqsave(&dc->lock, flags);
                         if (dc->state == D_PARTIAL)
                                 dc->state = D_SUBMIT;
@@ -1360,6 +1354,10 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
                         break;
                 }
  
+               __blkdev_issue_discard(bdev,
+                               SECTOR_FROM_BLOCK(start),
+                               SECTOR_FROM_BLOCK(len),
+                               GFP_NOFS, &bio);
                 f2fs_bug_on(sbi, !bio);
  
                 /*
-- 
2.40.0


>
>>   	blk_finish_plug(&plug);
>>   
>> -	return error;
>> +	return 0;
> Please drop the error return for xfs_discard_extents entirely.
>

done :-

diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index ee49f20875af..1f35c1d80cea 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 (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);
  
@@ -739,9 +724,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 */
-- 
2.40.0


will run the basic xfstest and send out a series to just remove the dead-code.

Thanks for the review comments.

-ck