diff mbox series

[f2fs-dev] f2fs: clean up w/ sbi->log_sectors_per_block

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

Commit Message

Chao Yu May 23, 2023, 12:35 p.m. UTC
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(-)

Comments

patchwork-bot+f2fs@kernel.org May 30, 2023, 11:40 p.m. UTC | #1
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!
Shinichiro Kawasaki Aug. 4, 2023, 9:43 a.m. UTC | #2
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/
Jaegeuk Kim Aug. 4, 2023, 7:21 p.m. UTC | #3
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/
Chao Yu Aug. 7, 2023, 2:07 a.m. UTC | #4
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/
Chao Yu Aug. 7, 2023, 2:10 a.m. UTC | #5
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/
Jaegeuk Kim Aug. 7, 2023, 7:55 p.m. UTC | #6
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 mbox series

Patch

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);