On 8/16/24 11:23, Shin'ichiro Kawasaki wrote: > As the helper function f2fs_bdev_support_discard() shows, f2fs checks if > the target block devices support discard by calling > bdev_max_discard_sectors() and bdev_is_zoned(). This check works good
s/good/well > for most cases, but it does not work for conventional zones on zoned > block devices. F2fs assumes that zoned block devices support discard, > and calls __submit_discard_cmd(). When __submit_discard_cmd() is called > for sequential write required zones, it works fine since > __submit_discard_cmd() issues zone reset commands instead of discard > commands. However, when __submit_discard_cmd() is called for > conventional zones, __blkdev_issue_discard() is called even when the > devices do not support discard. > > The inappropriate __blkdev_issue_discard() call was not a problem before > the commit 30f1e7241422 ("block: move discard checks into the ioctl > handler") because __blkdev_issue_discard() checked if the target devices > support discard or not. If not, it returned EOPNOTSUPP. After the > commit, __blkdev_issue_discard() no longer checks it and always returns > zero and sets NULL to the give bio pointer. This NULL pointer triggers > f2fs_bug_on() in __submit_discard_cmd(). The BUG is recreated with the > commands below at the umount step, where /dev/nullb0 is a zoned null_blk > with 5GB total size, 128MB zone size and 10 conventional zones. > > $ mkfs.f2fs -f -m /dev/nullb0 > $ mount /dev/nullb0 /mnt > $ for ((i=0;i<5;i++)); do dd if=/dev/zero of=/mnt/test bs=65536 count=1600 > conv=fsync; done > $ umount /mnt > > To fix the BUG, avoid the inappropriate __blkdev_issue_discard() call. > When discard is requested for conventional zones, check if the device > supports discard or not. If not, return EOPNOTSUPP. > > Fixes: 30f1e7241422 ("block: move discard checks into the ioctl handler") > Cc: sta...@vger.kernel.org > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawas...@wdc.com> > --- > fs/f2fs/segment.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > index 78c3198a6308..e10bf3c4eed5 100644 > --- a/fs/f2fs/segment.c > +++ b/fs/f2fs/segment.c > @@ -1281,6 +1281,8 @@ static int __submit_discard_cmd(struct f2fs_sb_info > *sbi, > __submit_zone_reset_cmd(sbi, dc, flag, > wait_list, issued); > return 0; > + } else if (!bdev_max_discard_sectors(bdev)) { > + return -EOPNOTSUPP; > } You do not need the else here. And I suggest adding a comment as well: /* * Issue discard for conventional zones only if the * device supports discard. */ if (!bdev_max_discard_sectors(bdev)) return -EOPNOTSUPP; With that: Reviewed-by: Damien Le Moal <dlem...@kernel.org> > } > #endif -- Damien Le Moal Western Digital Research _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel