Message ID | 20240325152623.797099-1-chao@kernel.org (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [f2fs-dev] f2fs: multidev: fix to recognize valid zero block address | expand |
I confirmed the issue was fixed now, thanks. Tested-by: Yi Zhang <yi.zhang@redhat.com> On Mon, Mar 25, 2024 at 11:26 PM Chao Yu <chao@kernel.org> wrote: > > As reported by Yi Zhang in mailing list [1], kernel warning was catched > during zbd/010 test as below: > > ./check zbd/010 > zbd/010 (test gap zone support with F2FS) [failed] > runtime ... 3.752s > something found in dmesg: > [ 4378.146781] run blktests zbd/010 at 2024-02-18 11:31:13 > [ 4378.192349] null_blk: module loaded > [ 4378.209860] null_blk: disk nullb0 created > [ 4378.413285] scsi_debug:sdebug_driver_probe: scsi_debug: trim > poll_queues to 0. poll_q/nr_hw = (0/1) > [ 4378.422334] scsi host15: scsi_debug: version 0191 [20210520] > dev_size_mb=1024, opts=0x0, submit_queues=1, statistics=0 > [ 4378.434922] scsi 15:0:0:0: Direct-Access-ZBC Linux > scsi_debug 0191 PQ: 0 ANSI: 7 > [ 4378.443343] scsi 15:0:0:0: Power-on or device reset occurred > [ 4378.449371] sd 15:0:0:0: Attached scsi generic sg5 type 20 > [ 4378.449418] sd 15:0:0:0: [sdf] Host-managed zoned block device > ... > (See '/mnt/tests/gitlab.com/api/v4/projects/19168116/repository/archive.zip/storage/blktests/blk/blktests/results/nodev/zbd/010.dmesg' > > WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 > CPU: 22 PID: 44011 Comm: fio Not tainted 6.8.0-rc3+ #1 > RIP: 0010:iomap_iter+0x32b/0x350 > Call Trace: > <TASK> > __iomap_dio_rw+0x1df/0x830 > f2fs_file_read_iter+0x156/0x3d0 [f2fs] > aio_read+0x138/0x210 > io_submit_one+0x188/0x8c0 > __x64_sys_io_submit+0x8c/0x1a0 > do_syscall_64+0x86/0x170 > entry_SYSCALL_64_after_hwframe+0x6e/0x76 > > Shinichiro Kawasaki helps to analyse this issue and proposes a potential > fixing patch in [2]. > > Quoted from reply of Shinichiro Kawasaki: > > "I confirmed that the trigger commit is dbf8e63f48af as Yi reported. I took a > look in the commit, but it looks fine to me. So I thought the cause is not > in the commit diff. > > I found the WARN is printed when the f2fs is set up with multiple devices, > and read requests are mapped to the very first block of the second device in the > direct read path. In this case, f2fs_map_blocks() and f2fs_map_blocks_cached() > modify map->m_pblk as the physical block address from each block device. It > becomes zero when it is mapped to the first block of the device. However, > f2fs_iomap_begin() assumes that map->m_pblk is the physical block address of the > whole f2fs, across the all block devices. It compares map->m_pblk against > NULL_ADDR == 0, then go into the unexpected branch and sets the invalid > iomap->length. The WARN catches the invalid iomap->length. > > This WARN is printed even for non-zoned block devices, by following steps. > > - Create two (non-zoned) null_blk devices memory backed with 128MB size each: > nullb0 and nullb1. > # mkfs.f2fs /dev/nullb0 -c /dev/nullb1 > # mount -t f2fs /dev/nullb0 "${mount_dir}" > # dd if=/dev/zero of="${mount_dir}/test.dat" bs=1M count=192 > # dd if="${mount_dir}/test.dat" of=/dev/null bs=1M count=192 iflag=direct > > ..." > > So, the root cause of this issue is: when multi-devices feature is on, > f2fs_map_blocks() may return zero blkaddr in non-primary device, which is > a verified valid block address, however, f2fs_iomap_begin() treats it as > an invalid block address, and then it triggers the warning in iomap > framework code. > > Finally, as discussed, we decide to use a more simple and direct way that > checking (map.m_flags & F2FS_MAP_MAPPED) condition instead of > (map.m_pblk != NULL_ADDR) to fix this issue. > > Thanks a lot for the effort of Yi Zhang and Shinichiro Kawasaki on this > issue. > > [1] https://lore.kernel.org/linux-f2fs-devel/CAHj4cs-kfojYC9i0G73PRkYzcxCTex=-vugRFeP40g_URGvnfQ@mail.gmail.com/ > [2] https://lore.kernel.org/linux-f2fs-devel/gngdj77k4picagsfdtiaa7gpgnup6fsgwzsltx6milmhegmjff@iax2n4wvrqye/ > > Reported-by: Yi Zhang <yi.zhang@redhat.com> > Closes: https://lore.kernel.org/linux-f2fs-devel/CAHj4cs-kfojYC9i0G73PRkYzcxCTex=-vugRFeP40g_URGvnfQ@mail.gmail.com/ > Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Fixes: 1517c1a7a445 ("f2fs: implement iomap operations") > Fixes: 8d3c1fa3fa5e ("f2fs: don't rely on F2FS_MAP_* in f2fs_iomap_begin") > Signed-off-by: Chao Yu <chao@kernel.org> > --- > fs/f2fs/data.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index d9494b5fc7c1..5ef1874b572a 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -4185,7 +4185,7 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length, > if (WARN_ON_ONCE(map.m_pblk == COMPRESS_ADDR)) > return -EINVAL; > > - if (map.m_pblk != NULL_ADDR) { > + if (map.m_flags & F2FS_MAP_MAPPED) { > iomap->length = blks_to_bytes(inode, map.m_len); > iomap->type = IOMAP_MAPPED; > iomap->flags |= IOMAP_F_MERGED; > -- > 2.40.1 >
On 2024/3/26 21:26, Yi Zhang wrote: > I confirmed the issue was fixed now, thanks. > > Tested-by: Yi Zhang <yi.zhang@redhat.com> Yi, thanks for the test. Thanks, > > On Mon, Mar 25, 2024 at 11:26 PM Chao Yu <chao@kernel.org> wrote: >> >> As reported by Yi Zhang in mailing list [1], kernel warning was catched >> during zbd/010 test as below: >> >> ./check zbd/010 >> zbd/010 (test gap zone support with F2FS) [failed] >> runtime ... 3.752s >> something found in dmesg: >> [ 4378.146781] run blktests zbd/010 at 2024-02-18 11:31:13 >> [ 4378.192349] null_blk: module loaded >> [ 4378.209860] null_blk: disk nullb0 created >> [ 4378.413285] scsi_debug:sdebug_driver_probe: scsi_debug: trim >> poll_queues to 0. poll_q/nr_hw = (0/1) >> [ 4378.422334] scsi host15: scsi_debug: version 0191 [20210520] >> dev_size_mb=1024, opts=0x0, submit_queues=1, statistics=0 >> [ 4378.434922] scsi 15:0:0:0: Direct-Access-ZBC Linux >> scsi_debug 0191 PQ: 0 ANSI: 7 >> [ 4378.443343] scsi 15:0:0:0: Power-on or device reset occurred >> [ 4378.449371] sd 15:0:0:0: Attached scsi generic sg5 type 20 >> [ 4378.449418] sd 15:0:0:0: [sdf] Host-managed zoned block device >> ... >> (See '/mnt/tests/gitlab.com/api/v4/projects/19168116/repository/archive.zip/storage/blktests/blk/blktests/results/nodev/zbd/010.dmesg' >> >> WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 >> CPU: 22 PID: 44011 Comm: fio Not tainted 6.8.0-rc3+ #1 >> RIP: 0010:iomap_iter+0x32b/0x350 >> Call Trace: >> <TASK> >> __iomap_dio_rw+0x1df/0x830 >> f2fs_file_read_iter+0x156/0x3d0 [f2fs] >> aio_read+0x138/0x210 >> io_submit_one+0x188/0x8c0 >> __x64_sys_io_submit+0x8c/0x1a0 >> do_syscall_64+0x86/0x170 >> entry_SYSCALL_64_after_hwframe+0x6e/0x76 >> >> Shinichiro Kawasaki helps to analyse this issue and proposes a potential >> fixing patch in [2]. >> >> Quoted from reply of Shinichiro Kawasaki: >> >> "I confirmed that the trigger commit is dbf8e63f48af as Yi reported. I took a >> look in the commit, but it looks fine to me. So I thought the cause is not >> in the commit diff. >> >> I found the WARN is printed when the f2fs is set up with multiple devices, >> and read requests are mapped to the very first block of the second device in the >> direct read path. In this case, f2fs_map_blocks() and f2fs_map_blocks_cached() >> modify map->m_pblk as the physical block address from each block device. It >> becomes zero when it is mapped to the first block of the device. However, >> f2fs_iomap_begin() assumes that map->m_pblk is the physical block address of the >> whole f2fs, across the all block devices. It compares map->m_pblk against >> NULL_ADDR == 0, then go into the unexpected branch and sets the invalid >> iomap->length. The WARN catches the invalid iomap->length. >> >> This WARN is printed even for non-zoned block devices, by following steps. >> >> - Create two (non-zoned) null_blk devices memory backed with 128MB size each: >> nullb0 and nullb1. >> # mkfs.f2fs /dev/nullb0 -c /dev/nullb1 >> # mount -t f2fs /dev/nullb0 "${mount_dir}" >> # dd if=/dev/zero of="${mount_dir}/test.dat" bs=1M count=192 >> # dd if="${mount_dir}/test.dat" of=/dev/null bs=1M count=192 iflag=direct >> >> ..." >> >> So, the root cause of this issue is: when multi-devices feature is on, >> f2fs_map_blocks() may return zero blkaddr in non-primary device, which is >> a verified valid block address, however, f2fs_iomap_begin() treats it as >> an invalid block address, and then it triggers the warning in iomap >> framework code. >> >> Finally, as discussed, we decide to use a more simple and direct way that >> checking (map.m_flags & F2FS_MAP_MAPPED) condition instead of >> (map.m_pblk != NULL_ADDR) to fix this issue. >> >> Thanks a lot for the effort of Yi Zhang and Shinichiro Kawasaki on this >> issue. >> >> [1] https://lore.kernel.org/linux-f2fs-devel/CAHj4cs-kfojYC9i0G73PRkYzcxCTex=-vugRFeP40g_URGvnfQ@mail.gmail.com/ >> [2] https://lore.kernel.org/linux-f2fs-devel/gngdj77k4picagsfdtiaa7gpgnup6fsgwzsltx6milmhegmjff@iax2n4wvrqye/ >> >> Reported-by: Yi Zhang <yi.zhang@redhat.com> >> Closes: https://lore.kernel.org/linux-f2fs-devel/CAHj4cs-kfojYC9i0G73PRkYzcxCTex=-vugRFeP40g_URGvnfQ@mail.gmail.com/ >> Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> >> Fixes: 1517c1a7a445 ("f2fs: implement iomap operations") >> Fixes: 8d3c1fa3fa5e ("f2fs: don't rely on F2FS_MAP_* in f2fs_iomap_begin") >> Signed-off-by: Chao Yu <chao@kernel.org> >> --- >> fs/f2fs/data.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >> index d9494b5fc7c1..5ef1874b572a 100644 >> --- a/fs/f2fs/data.c >> +++ b/fs/f2fs/data.c >> @@ -4185,7 +4185,7 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length, >> if (WARN_ON_ONCE(map.m_pblk == COMPRESS_ADDR)) >> return -EINVAL; >> >> - if (map.m_pblk != NULL_ADDR) { >> + if (map.m_flags & F2FS_MAP_MAPPED) { >> iomap->length = blks_to_bytes(inode, map.m_len); >> iomap->type = IOMAP_MAPPED; >> iomap->flags |= IOMAP_F_MERGED; >> -- >> 2.40.1 >> > >
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index d9494b5fc7c1..5ef1874b572a 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -4185,7 +4185,7 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length, if (WARN_ON_ONCE(map.m_pblk == COMPRESS_ADDR)) return -EINVAL; - if (map.m_pblk != NULL_ADDR) { + if (map.m_flags & F2FS_MAP_MAPPED) { iomap->length = blks_to_bytes(inode, map.m_len); iomap->type = IOMAP_MAPPED; iomap->flags |= IOMAP_F_MERGED;