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

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

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