| Message ID | 20240816040703.736887-1-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 43aec4d01bd2ce961817a777b3846f8318f398e4 |
| Headers | show |
| Series | [f2fs-dev,v2] f2fs: check discard support for conventional zones | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On 2024/8/16 12:07, 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 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. It always returns > zero and sets NULL to the given 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: stable@vger.kernel.org > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Reviewed-by: Damien Le Moal <dlemoal@kernel.org> Reviewed-by: Chao Yu <chao@kernel.org> Thanks,
Hello: This patch was applied to jaegeuk/f2fs.git (dev) by Jaegeuk Kim <jaegeuk@kernel.org>: On Fri, 16 Aug 2024 13:07:03 +0900 you 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 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. > > [...] Here is the summary with links: - [f2fs-dev,v2] f2fs: check discard support for conventional zones https://git.kernel.org/jaegeuk/f2fs/c/43aec4d01bd2 You are awesome, thank you!
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index 78c3198a6308..f49cc404b2f2 100644 --- a/fs/f2fs/segment.c +++ b/fs/f2fs/segment.c @@ -1282,6 +1282,13 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi, wait_list, issued); return 0; } + + /* + * Issue discard for conventional zones only if the device + * supports discard. + */ + if (!bdev_max_discard_sectors(bdev)) + return -EOPNOTSUPP; } #endif
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 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. It always returns zero and sets NULL to the given 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: stable@vger.kernel.org Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> Reviewed-by: Damien Le Moal <dlemoal@kernel.org> --- Changes from v1: * Removed the else, added the comment and improved commit message wording fs/f2fs/segment.c | 7 +++++++ 1 file changed, 7 insertions(+)