On 11/26/25 11:37, Chaitanya Kulkarni wrote: > On 11/25/25 18:47, Chao Yu wrote: >> On 11/25/25 07:48, Chaitanya Kulkarni wrote: >>> __blkdev_issue_discard() always returns 0, making the error assignment >>> in __submit_discard_cmd() dead code. >>> >>> Initialize err to 0 and remove the error assignment from the >>> __blkdev_issue_discard() call to err. Move fault injection code into >>> already present if branch where err is set to -EIO. >>> >>> This preserves the fault injection behavior while removing dead error >>> handling. >>> >>> Reviewed-by: Martin K. Petersen <[email protected]> >>> Reviewed-by: Johannes Thumshirn <[email protected]> >>> Reviewed-by: Christoph Hellwig <[email protected]> >>> Signed-off-by: Chaitanya Kulkarni <[email protected]> >>> --- >>> fs/f2fs/segment.c | 10 +++------- >>> 1 file changed, 3 insertions(+), 7 deletions(-) >>> >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>> index b45eace879d7..22b736ec9c51 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,8 @@ 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); >> Oh, wait, bio can be NULL? Then below f2fs_bug_on() will trigger panic or >> warning. >> >> Thanks, > > That will happen without this patch also or not ? > > Since __blkdev_issue_discard() is always returning 0 irrespective of bio > is null or not. > > The following condition in original code will only execute when err is set to > -EIO and that will only happen when time_to_inject() -> true. > Original call to __blkdev_issue_discard() without this patch will always > return 0 even for bio == NULL after __blkdev_issue_discard(). > > This is what we are trying to fix so caller should not rely on > __blkdev_issue_discard() return value :- > > 354 if (err) { > 1355 spin_lock_irqsave(&dc->lock, flags); > 1356 if (dc->state == D_PARTIAL) > 1357 dc->state = D_SUBMIT; > 1358 spin_unlock_irqrestore(&dc->lock, flags); > 1359 > 1360 break; > 1361 } > > which will lead f2fs_bug_on() for bio == NULL even without this patch. > > This patch is not changing exiting behavior, correct me if I'm wrong.
Yes, I think you're right, thanks for the explanation. So it's fine to leave this cleanup patch as it is, and let's fix this bug in a separated patch. Thanks, > > >> >>> f2fs_bug_on(sbi, !bio); >>> >>> /* > > -ck > > _______________________________________________ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
