| Message ID | 20230523123521.67656-1-chao@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 584212446615e26bb78a9d9c4665e252b6563f7e |
| Headers | show |
| Series | [f2fs-dev] f2fs: clean up w/ sbi->log_sectors_per_block | expand |
Hello: This patch was applied to jaegeuk/f2fs.git (dev) by Jaegeuk Kim <jaegeuk@kernel.org>: On Tue, 23 May 2023 20:35:21 +0800 you wrote: > Use sbi->log_sectors_per_block to clean up below calculated one: > > unsigned int log_sectors_per_block = sbi->log_blocksize - SECTOR_SHIFT; > > Signed-off-by: Chao Yu <chao@kernel.org> > --- > fs/f2fs/segment.c | 23 +++++++++++------------ > 1 file changed, 11 insertions(+), 12 deletions(-) Here is the summary with links: - [f2fs-dev] f2fs: clean up w/ sbi->log_sectors_per_block https://git.kernel.org/jaegeuk/f2fs/c/584212446615 You are awesome, thank you!
On May 23, 2023 / 20:35, Chao Yu wrote: > Use sbi->log_sectors_per_block to clean up below calculated one: > > unsigned int log_sectors_per_block = sbi->log_blocksize - SECTOR_SHIFT; Hello Chao, When I ran workloads on f2fs using v6.5-rcX with fixes [1][2] and a zoned block devices with 4kb logical block size, I observe mount failure as follows. When I revert this commit, the failure goes away. [ 167.781975][ T1555] F2FS-fs (dm-0): IO Block Size: 4 KB [ 167.890728][ T1555] F2FS-fs (dm-0): Found nat_bits in checkpoint [ 171.482588][ T1555] F2FS-fs (dm-0): Zone without valid block has non-zero write pointer. Reset the write pointer: wp[0x1300,0x8] [ 171.496000][ T1555] F2FS-fs (dm-0): (0) : Unaligned zone reset attempted (block 280000 + 80000) [ 171.505037][ T1555] F2FS-fs (dm-0): Discard zone failed: (errno=-5) The patch replaced "sbi->log_blocksize - SECTOR_SHIFT" with "sbi->log_sectors_per_block". However, I think these two are not equal when the device has 4k logical block size. The former uses Linux kernel sector size 512 byte. The latter use 512b sector size or 4kb sector size depending on the device. mkfs.f2fs obtains logical block size via BLKSSZGET ioctl from the device and reflects it to the value sbi->log_sector_size_per_block. This causes unexpected write pointer calculations in check_zone_write_pointer(). This resulted in unexpected zone reset and the mount failure. I think this patch needs revert. What do you think? [1] https://lkml.kernel.org/linux-f2fs-devel/20230711050101.GA19128@lst.de/ [2] https://lore.kernel.org/linux-f2fs-devel/20230804091556.2372567-1-shinichiro.kawasaki@wdc.com/
On 08/04, Shinichiro Kawasaki wrote: > On May 23, 2023 / 20:35, Chao Yu wrote: > > Use sbi->log_sectors_per_block to clean up below calculated one: > > > > unsigned int log_sectors_per_block = sbi->log_blocksize - SECTOR_SHIFT; > > Hello Chao, > > When I ran workloads on f2fs using v6.5-rcX with fixes [1][2] and a zoned block > devices with 4kb logical block size, I observe mount failure as follows. When > I revert this commit, the failure goes away. > > [ 167.781975][ T1555] F2FS-fs (dm-0): IO Block Size: 4 KB > [ 167.890728][ T1555] F2FS-fs (dm-0): Found nat_bits in checkpoint > [ 171.482588][ T1555] F2FS-fs (dm-0): Zone without valid block has non-zero write pointer. Reset the write pointer: wp[0x1300,0x8] > [ 171.496000][ T1555] F2FS-fs (dm-0): (0) : Unaligned zone reset attempted (block 280000 + 80000) > [ 171.505037][ T1555] F2FS-fs (dm-0): Discard zone failed: (errno=-5) > > The patch replaced "sbi->log_blocksize - SECTOR_SHIFT" with > "sbi->log_sectors_per_block". However, I think these two are not equal when the > device has 4k logical block size. The former uses Linux kernel sector size 512 > byte. The latter use 512b sector size or 4kb sector size depending on the > device. mkfs.f2fs obtains logical block size via BLKSSZGET ioctl from the device > and reflects it to the value sbi->log_sector_size_per_block. This causes > unexpected write pointer calculations in check_zone_write_pointer(). This > resulted in unexpected zone reset and the mount failure. > > I think this patch needs revert. What do you think? Yeah, applied the revert. > > [1] https://lkml.kernel.org/linux-f2fs-devel/20230711050101.GA19128@lst.de/ > [2] https://lore.kernel.org/linux-f2fs-devel/20230804091556.2372567-1-shinichiro.kawasaki@wdc.com/
On 2023/8/4 17:43, Shinichiro Kawasaki wrote: > On May 23, 2023 / 20:35, Chao Yu wrote: >> Use sbi->log_sectors_per_block to clean up below calculated one: >> >> unsigned int log_sectors_per_block = sbi->log_blocksize - SECTOR_SHIFT; > > Hello Chao, > > When I ran workloads on f2fs using v6.5-rcX with fixes [1][2] and a zoned block > devices with 4kb logical block size, I observe mount failure as follows. When > I revert this commit, the failure goes away. > > [ 167.781975][ T1555] F2FS-fs (dm-0): IO Block Size: 4 KB > [ 167.890728][ T1555] F2FS-fs (dm-0): Found nat_bits in checkpoint > [ 171.482588][ T1555] F2FS-fs (dm-0): Zone without valid block has non-zero write pointer. Reset the write pointer: wp[0x1300,0x8] > [ 171.496000][ T1555] F2FS-fs (dm-0): (0) : Unaligned zone reset attempted (block 280000 + 80000) > [ 171.505037][ T1555] F2FS-fs (dm-0): Discard zone failed: (errno=-5) > > The patch replaced "sbi->log_blocksize - SECTOR_SHIFT" with > "sbi->log_sectors_per_block". However, I think these two are not equal when the > device has 4k logical block size. The former uses Linux kernel sector size 512 > byte. The latter use 512b sector size or 4kb sector size depending on the > device. mkfs.f2fs obtains logical block size via BLKSSZGET ioctl from the device > and reflects it to the value sbi->log_sector_size_per_block. This causes > unexpected write pointer calculations in check_zone_write_pointer(). This > resulted in unexpected zone reset and the mount failure. > > I think this patch needs revert. What do you think? Hi Shinichiro, Oh, my bad, I think we need to revert that patch. Thanks for the report! :) Thanks, > > [1] https://lkml.kernel.org/linux-f2fs-devel/20230711050101.GA19128@lst.de/ > [2] https://lore.kernel.org/linux-f2fs-devel/20230804091556.2372567-1-shinichiro.kawasaki@wdc.com/
On 2023/8/5 3:21, Jaegeuk Kim wrote: > On 08/04, Shinichiro Kawasaki wrote: >> On May 23, 2023 / 20:35, Chao Yu wrote: >>> Use sbi->log_sectors_per_block to clean up below calculated one: >>> >>> unsigned int log_sectors_per_block = sbi->log_blocksize - SECTOR_SHIFT; >> >> Hello Chao, >> >> When I ran workloads on f2fs using v6.5-rcX with fixes [1][2] and a zoned block >> devices with 4kb logical block size, I observe mount failure as follows. When >> I revert this commit, the failure goes away. >> >> [ 167.781975][ T1555] F2FS-fs (dm-0): IO Block Size: 4 KB >> [ 167.890728][ T1555] F2FS-fs (dm-0): Found nat_bits in checkpoint >> [ 171.482588][ T1555] F2FS-fs (dm-0): Zone without valid block has non-zero write pointer. Reset the write pointer: wp[0x1300,0x8] >> [ 171.496000][ T1555] F2FS-fs (dm-0): (0) : Unaligned zone reset attempted (block 280000 + 80000) >> [ 171.505037][ T1555] F2FS-fs (dm-0): Discard zone failed: (errno=-5) >> >> The patch replaced "sbi->log_blocksize - SECTOR_SHIFT" with >> "sbi->log_sectors_per_block". However, I think these two are not equal when the >> device has 4k logical block size. The former uses Linux kernel sector size 512 >> byte. The latter use 512b sector size or 4kb sector size depending on the >> device. mkfs.f2fs obtains logical block size via BLKSSZGET ioctl from the device >> and reflects it to the value sbi->log_sector_size_per_block. This causes >> unexpected write pointer calculations in check_zone_write_pointer(). This >> resulted in unexpected zone reset and the mount failure. >> >> I think this patch needs revert. What do you think? > > Yeah, applied the revert. Jaegeuk, could you please send the patch to mailing list? Thanks, > >> >> [1] https://lkml.kernel.org/linux-f2fs-devel/20230711050101.GA19128@lst.de/ >> [2] https://lore.kernel.org/linux-f2fs-devel/20230804091556.2372567-1-shinichiro.kawasaki@wdc.com/
On 08/07, Chao Yu wrote: > On 2023/8/5 3:21, Jaegeuk Kim wrote: > > On 08/04, Shinichiro Kawasaki wrote: > > > On May 23, 2023 / 20:35, Chao Yu wrote: > > > > Use sbi->log_sectors_per_block to clean up below calculated one: > > > > > > > > unsigned int log_sectors_per_block = sbi->log_blocksize - SECTOR_SHIFT; > > > > > > Hello Chao, > > > > > > When I ran workloads on f2fs using v6.5-rcX with fixes [1][2] and a zoned block > > > devices with 4kb logical block size, I observe mount failure as follows. When > > > I revert this commit, the failure goes away. > > > > > > [ 167.781975][ T1555] F2FS-fs (dm-0): IO Block Size: 4 KB > > > [ 167.890728][ T1555] F2FS-fs (dm-0): Found nat_bits in checkpoint > > > [ 171.482588][ T1555] F2FS-fs (dm-0): Zone without valid block has non-zero write pointer. Reset the write pointer: wp[0x1300,0x8] > > > [ 171.496000][ T1555] F2FS-fs (dm-0): (0) : Unaligned zone reset attempted (block 280000 + 80000) > > > [ 171.505037][ T1555] F2FS-fs (dm-0): Discard zone failed: (errno=-5) > > > > > > The patch replaced "sbi->log_blocksize - SECTOR_SHIFT" with > > > "sbi->log_sectors_per_block". However, I think these two are not equal when the > > > device has 4k logical block size. The former uses Linux kernel sector size 512 > > > byte. The latter use 512b sector size or 4kb sector size depending on the > > > device. mkfs.f2fs obtains logical block size via BLKSSZGET ioctl from the device > > > and reflects it to the value sbi->log_sector_size_per_block. This causes > > > unexpected write pointer calculations in check_zone_write_pointer(). This > > > resulted in unexpected zone reset and the mount failure. > > > > > > I think this patch needs revert. What do you think? > > > > Yeah, applied the revert. > > Jaegeuk, could you please send the patch to mailing list? I sent. Thanks. > > Thanks, > > > > > > > > > [1] https://lkml.kernel.org/linux-f2fs-devel/20230711050101.GA19128@lst.de/ > > > [2] https://lore.kernel.org/linux-f2fs-devel/20230804091556.2372567-1-shinichiro.kawasaki@wdc.com/
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index 43d537d29b52..9282399cc810 100644 --- a/fs/f2fs/segment.c +++ b/fs/f2fs/segment.c @@ -4768,17 +4768,17 @@ static int check_zone_write_pointer(struct f2fs_sb_info *sbi, { unsigned int wp_segno, wp_blkoff, zone_secno, zone_segno, segno; block_t zone_block, wp_block, last_valid_block; - unsigned int log_sectors_per_block = sbi->log_blocksize - SECTOR_SHIFT; int i, s, b, ret; struct seg_entry *se; if (zone->type != BLK_ZONE_TYPE_SEQWRITE_REQ) return 0; - wp_block = fdev->start_blk + (zone->wp >> log_sectors_per_block); + wp_block = fdev->start_blk + (zone->wp >> sbi->log_sectors_per_block); wp_segno = GET_SEGNO(sbi, wp_block); wp_blkoff = wp_block - START_BLOCK(sbi, wp_segno); - zone_block = fdev->start_blk + (zone->start >> log_sectors_per_block); + zone_block = fdev->start_blk + (zone->start >> + sbi->log_sectors_per_block); zone_segno = GET_SEGNO(sbi, zone_block); zone_secno = GET_SEC_FROM_SEG(sbi, zone_segno); @@ -4824,7 +4824,7 @@ static int check_zone_write_pointer(struct f2fs_sb_info *sbi, "pointer. Reset the write pointer: wp[0x%x,0x%x]", wp_segno, wp_blkoff); ret = __f2fs_issue_discard_zone(sbi, fdev->bdev, zone_block, - zone->len >> log_sectors_per_block); + zone->len >> sbi->log_sectors_per_block); if (ret) f2fs_err(sbi, "Discard zone failed: %s (errno=%d)", fdev->path, ret); @@ -4885,7 +4885,6 @@ static int fix_curseg_write_pointer(struct f2fs_sb_info *sbi, int type) struct blk_zone zone; unsigned int cs_section, wp_segno, wp_blkoff, wp_sector_off; block_t cs_zone_block, wp_block; - unsigned int log_sectors_per_block = sbi->log_blocksize - SECTOR_SHIFT; sector_t zone_sector; int err; @@ -4897,8 +4896,8 @@ static int fix_curseg_write_pointer(struct f2fs_sb_info *sbi, int type) return 0; /* report zone for the sector the curseg points to */ - zone_sector = (sector_t)(cs_zone_block - zbd->start_blk) - << log_sectors_per_block; + zone_sector = (sector_t)(cs_zone_block - zbd->start_blk) << + sbi->log_sectors_per_block; err = blkdev_report_zones(zbd->bdev, zone_sector, 1, report_one_zone_cb, &zone); if (err != 1) { @@ -4910,10 +4909,10 @@ static int fix_curseg_write_pointer(struct f2fs_sb_info *sbi, int type) if (zone.type != BLK_ZONE_TYPE_SEQWRITE_REQ) return 0; - wp_block = zbd->start_blk + (zone.wp >> log_sectors_per_block); + wp_block = zbd->start_blk + (zone.wp >> sbi->log_sectors_per_block); wp_segno = GET_SEGNO(sbi, wp_block); wp_blkoff = wp_block - START_BLOCK(sbi, wp_segno); - wp_sector_off = zone.wp & GENMASK(log_sectors_per_block - 1, 0); + wp_sector_off = zone.wp & GENMASK(sbi->log_sectors_per_block - 1, 0); if (cs->segno == wp_segno && cs->next_blkoff == wp_blkoff && wp_sector_off == 0) @@ -4940,8 +4939,8 @@ static int fix_curseg_write_pointer(struct f2fs_sb_info *sbi, int type) if (!zbd) return 0; - zone_sector = (sector_t)(cs_zone_block - zbd->start_blk) - << log_sectors_per_block; + zone_sector = (sector_t)(cs_zone_block - zbd->start_blk) << + sbi->log_sectors_per_block; err = blkdev_report_zones(zbd->bdev, zone_sector, 1, report_one_zone_cb, &zone); if (err != 1) { @@ -4959,7 +4958,7 @@ static int fix_curseg_write_pointer(struct f2fs_sb_info *sbi, int type) "Reset the zone: curseg[0x%x,0x%x]", type, cs->segno, cs->next_blkoff); err = __f2fs_issue_discard_zone(sbi, zbd->bdev, cs_zone_block, - zone.len >> log_sectors_per_block); + zone.len >> sbi->log_sectors_per_block); if (err) { f2fs_err(sbi, "Discard zone failed: %s (errno=%d)", zbd->path, err);
Use sbi->log_sectors_per_block to clean up below calculated one: unsigned int log_sectors_per_block = sbi->log_blocksize - SECTOR_SHIFT; Signed-off-by: Chao Yu <chao@kernel.org> --- fs/f2fs/segment.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)