[f2fs-dev] [bug report]WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 iomap_iter+0x32b observed with blktests zbd/010

[f2fs-dev] [bug report]WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 iomap_iter+0x32b observed with blktests zbd/010

[Yi Zhang]

Hello
I reproduced this issue on the latest linux-block/for-next, please
help check it and let me know if you need more info/test, thanks.

# ./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'
for the entire message)
# uname -r
6.8.0-rc3+

[ 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
[ 4378.460718] sd 15:0:0:0: [sdf] 262144 4096-byte logical blocks:
(1.07 GB/1.00 GiB)
[ 4378.468292] sd 15:0:0:0: [sdf] Write Protect is off
[ 4378.473172] sd 15:0:0:0: [sdf] Mode Sense: 5b 00 10 08
[ 4378.473179] sd 15:0:0:0: [sdf] Write cache: enabled, read cache:
enabled, supports DPO and FUA
[ 4378.481796] sd 15:0:0:0: [sdf] Preferred minimum I/O size 4096 bytes
[ 4378.488146] sd 15:0:0:0: [sdf] Optimal transfer size 4194304 bytes
[ 4378.494366] sd 15:0:0:0: [sdf] 256 zones of 1024 logical blocks
[ 4378.500895] sd 15:0:0:0: [sdf] Attached SCSI disk
[ 4378.591696] F2FS-fs (nullb0): Mount Device [ 0]:
/dev/nullb0,      510,        0 -    3ffff
[ 4378.600903] F2FS-fs (nullb0): Mount Device [ 1]:
/dev/sdf,      512,    40000 -    7ffff (zone: Host-managed)
[ 4378.611858] F2FS-fs (nullb0): IO Block Size:        4 KB
[ 4378.617811] F2FS-fs (nullb0): Found nat_bits in checkpoint
[ 4378.630242] F2FS-fs (nullb0): Mounted with checkpoint version = 2ddce711
[ 4381.278858] ------------[ cut here ]------------
[ 4381.283484] WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51
iomap_iter+0x32b/0x350
[ 4381.291231] Modules linked in: scsi_debug null_blk f2fs
crc32_generic lz4hc_compress lz4_compress rfkill sunrpc intel_rapl_msr
vfat fat intel_rapl_common intel_uncore_frequency
intel_uncore_frequency_common isst_if_common skx_edac nfit libnvdimm
x86_pkg_temp_thermal ipmi_ssif intel_powerclamp spi_nor coretemp
kvm_intel bnxt_en kvm iTCO_wdt irqbypass intel_pmc_bxt acpi_ipmi
iTCO_vendor_support ipmi_si rapl i2c_i801 mtd mei_me tg3 intel_cstate
intel_pch_thermal dell_smbios intel_uncore ipmi_devintf spi_intel_pci
i2c_smbus dell_wmi_descriptor dcdbas spi_intel ipmi_msghandler
wmi_bmof mei pcspkr acpi_power_meter lpc_ich loop fuse nfnetlink zram
xfs crct10dif_pclmul crc32_pclmul crc32c_intel polyval_clmulni
polyval_generic nvme ghash_clmulni_intel nvme_core sha512_ssse3
mgag200 sha256_ssse3 sha1_ssse3 megaraid_sas nvme_auth i2c_algo_bit
wmi [last unloaded: null_blk]
[ 4381.367409] CPU: 22 PID: 44011 Comm: fio Not tainted 6.8.0-rc3+ #1
[ 4381.373588] Hardware name: Dell Inc. PowerEdge R740xd/0YNX56, BIOS
2.20.1 09/13/2023
[ 4381.381327] RIP: 0010:iomap_iter+0x32b/0x350
[ 4381.385599] Code: e9 c2 fe ff ff 89 d0 4c 8d 63 28 4c 8d 6b 78 85
d2 0f 8e b5 fe ff ff e9 46 fe ff ff 0f 0b e9 71 fe ff ff 0f 0b e9 77
fe ff ff <0f> 0b e9 7c fe ff ff 0f 0b e9 7f fe ff ff 0f 0b b8 fb ff ff
ff e9
[ 4381.404346] RSP: 0018:ffffb6650558fa40 EFLAGS: 00010206
[ 4381.409569] RAX: 0000000000000000 RBX: ffffb6650558fad0 RCX: fffffffffff4a000
[ 4381.416702] RDX: 00000000000b6000 RSI: 00000000000000b6 RDI: ffff9e301c126b18
[ 4381.423834] RBP: ffffffffc112e900 R08: ffff9e33872c1000 R09: 000000000003fc00
[ 4381.430969] R10: 0000000000000004 R11: 0000000000000004 R12: ffffb6650558faf8
[ 4381.438098] R13: ffffb6650558fb48 R14: ffff9e301c126b18 R15: ffffffffc112e900
[ 4381.445229] FS:  00007eff7a09b080(0000) GS:ffff9e336fec0000(0000)
knlGS:0000000000000000
[ 4381.453316] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 4381.459062] CR2: 00007eff71c37004 CR3: 000000010d1c0001 CR4: 00000000007706f0
[ 4381.466193] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 4381.473326] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 4381.480456] PKRU: 55555554
[ 4381.483170] Call Trace:
[ 4381.485623]  <TASK>
[ 4381.487729]  ? iomap_iter+0x32b/0x350
[ 4381.491393]  ? __warn+0x81/0x130
[ 4381.494639]  ? iomap_iter+0x32b/0x350
[ 4381.498311]  ? report_bug+0x16f/0x1a0
[ 4381.501984]  ? handle_bug+0x3c/0x80
[ 4381.505476]  ? exc_invalid_op+0x17/0x70
[ 4381.509314]  ? asm_exc_invalid_op+0x1a/0x20
[ 4381.513502]  ? iomap_iter+0x32b/0x350
[ 4381.517167]  __iomap_dio_rw+0x1df/0x830
[ 4381.521009]  f2fs_file_read_iter+0x156/0x3d0 [f2fs]
[ 4381.525937]  ? selinux_file_permission+0x151/0x180
[ 4381.530729]  aio_read+0x138/0x210
[ 4381.534051]  ? io_submit_one+0x188/0x8c0
[ 4381.537974]  io_submit_one+0x188/0x8c0
[ 4381.541727]  ? do_io_getevents+0x89/0xe0
[ 4381.545652]  __x64_sys_io_submit+0x8c/0x1a0
[ 4381.549838]  do_syscall_64+0x86/0x170
[ 4381.553504]  ? syscall_exit_to_user_mode+0x89/0x230
[ 4381.558383]  ? do_syscall_64+0x95/0x170
[ 4381.562222]  ? do_syscall_64+0x95/0x170
[ 4381.566061]  ? do_syscall_64+0x95/0x170
[ 4381.569897]  ? do_syscall_64+0x95/0x170
[ 4381.573738]  ? __irq_exit_rcu+0x4b/0xc0
[ 4381.577578]  entry_SYSCALL_64_after_hwframe+0x6e/0x76
[ 4381.582631] RIP: 0033:0x7eff7a1b215d
[ 4381.586209] Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e
fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24
08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8b cc 0c 00 f7 d8 64 89
01 48
[ 4381.604953] RSP: 002b:00007fff421d86c8 EFLAGS: 00000246 ORIG_RAX:
00000000000000d1
[ 4381.612519] RAX: ffffffffffffffda RBX: 00007eff7a09aff8 RCX: 00007eff7a1b215d
[ 4381.619651] RDX: 0000564e55cae8f8 RSI: 0000000000000001 RDI: 00007eff71c84000
[ 4381.626784] RBP: 00007fff421d8700 R08: 0000564e55cc2000 R09: 00007fff421d86f0
[ 4381.633914] R10: 0000000000000000 R11: 0000000000000246 R12: 00007eff71c84000
[ 4381.641046] R13: 0000000000000000 R14: 0000000000000001 R15: 0000564e55cae8f8
[ 4381.648181]  </TASK>
[ 4381.650371] ---[ end trace 0000000000000000 ]---
[ 4381.769895] sd 15:0:0:0: [sdf] Synchronizing SCSI cache

-- 
Best Regards,
  Yi Zhang



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [bug report]WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 iomap_iter+0x32b observed with blktests zbd/010

[Shinichiro Kawasaki via Linux-f2fs-devel]

On Feb 19, 2024 / 00:58, Yi Zhang wrote:
> Hello
> I reproduced this issue on the latest linux-block/for-next, please
> help check it and let me know if you need more info/test, thanks.

[...]

> [ 4381.278858] ------------[ cut here ]------------
> [ 4381.283484] WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51
> iomap_iter+0x32b/0x350

I can not recreate the WARN and the failure on my test machines. On the other
hand, it is repeatedly recreated on CKI test machines since Feb/19/2024 [1].

  [1] https://datawarehouse.cki-project.org/issue/2508

I assume that a kernel change triggered the failure.

Yi, is it possible to bisect and identify the trigger commit using CKI test
machines? The failure is observed with v6.6.17 and v6.6.18 kernel. I guess the
failure was not observed with v6.6.16 kernel, so I suggest to bisect between
v6.6.16 and v6.6.17.

_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [bug report]WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 iomap_iter+0x32b observed with blktests zbd/010

[Yi Zhang]

On Wed, Feb 28, 2024 at 7:09 PM Shinichiro Kawasaki
<shinichiro.kawasaki@wdc.com> wrote:
>
> On Feb 19, 2024 / 00:58, Yi Zhang wrote:
> > Hello
> > I reproduced this issue on the latest linux-block/for-next, please
> > help check it and let me know if you need more info/test, thanks.
>
> [...]
>
> > [ 4381.278858] ------------[ cut here ]------------
> > [ 4381.283484] WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51
> > iomap_iter+0x32b/0x350
>
> I can not recreate the WARN and the failure on my test machines. On the other
> hand, it is repeatedly recreated on CKI test machines since Feb/19/2024 [1].
>
>   [1] https://datawarehouse.cki-project.org/issue/2508
>
> I assume that a kernel change triggered the failure.
>
> Yi, is it possible to bisect and identify the trigger commit using CKI test
> machines? The failure is observed with v6.6.17 and v6.6.18 kernel. I guess the
> failure was not observed with v6.6.16 kernel, so I suggest to bisect between

Sure, will try to bisect it later this week. :)

> v6.6.16 and v6.6.17.
>


-- 
Best Regards,
  Yi Zhang



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [bug report]WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 iomap_iter+0x32b observed with blktests zbd/010

[Bart Van Assche]

On 2/29/24 23:49, Yi Zhang wrote:
> Bisect shows it was introduced with the below commit:
> 
> commit dbf8e63f48af48f3f0a069fc971c9826312dbfc1
> Author: Eunhee Rho <eunhee83.rho@samsung.com>
> Date:   Mon Aug 1 13:40:02 2022 +0900
> 
>      f2fs: remove device type check for direct IO

(+Eunhee)

Thank you Yi for having bisected this issue. I know this takes
considerable effort.

Eunhee, please take a look at this bug report:
https://lore.kernel.org/all/CAHj4cs-kfojYC9i0G73PRkYzcxCTex=-vugRFeP40g_URGvnfQ@mail.gmail.com/

Thanks,

Bart.


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [bug report]WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 iomap_iter+0x32b observed with blktests zbd/010

[Shinichiro Kawasaki via Linux-f2fs-devel]

On Mar 01, 2024 / 15:49, Yi Zhang wrote:
> On Wed, Feb 28, 2024 at 8:08 PM Yi Zhang <yi.zhang@redhat.com> wrote:
> >
> > On Wed, Feb 28, 2024 at 7:09 PM Shinichiro Kawasaki
> > <shinichiro.kawasaki@wdc.com> wrote:
> > >
> > > On Feb 19, 2024 / 00:58, Yi Zhang wrote:
> > > > Hello
> > > > I reproduced this issue on the latest linux-block/for-next, please
> > > > help check it and let me know if you need more info/test, thanks.
> > >
> > > [...]
> > >
> > > > [ 4381.278858] ------------[ cut here ]------------
> > > > [ 4381.283484] WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51
> > > > iomap_iter+0x32b/0x350
> > >
> > > I can not recreate the WARN and the failure on my test machines. On the other
> > > hand, it is repeatedly recreated on CKI test machines since Feb/19/2024 [1].
> > >
> > >   [1] https://datawarehouse.cki-project.org/issue/2508
> > >
> > > I assume that a kernel change triggered the failure.
> > >
> > > Yi, is it possible to bisect and identify the trigger commit using CKI test
> > > machines? The failure is observed with v6.6.17 and v6.6.18 kernel. I guess the
> > > failure was not observed with v6.6.16 kernel, so I suggest to bisect between
> 
> Bisect shows it was introduced with the below commit:
> 
> commit dbf8e63f48af48f3f0a069fc971c9826312dbfc1
> Author: Eunhee Rho <eunhee83.rho@samsung.com>
> Date:   Mon Aug 1 13:40:02 2022 +0900
> 
>     f2fs: remove device type check for direct IO

Yi, thanks for bisecting. I hope Eunhee has time to check.

> 
> I also attached the config file in case you want to reproduce it.

The attached config file has this line:

# CONFIG_F2FS_FS is not set

But the WARN happened after f2fs mount, so this config should not be able to
recreate the WARN. If you have time to afford, please check and share the config
file again.

_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [bug report]WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 iomap_iter+0x32b observed with blktests zbd/010

[Shinichiro Kawasaki via Linux-f2fs-devel]

On Mar 12, 2024 / 12:57, Yi Zhang wrote:
...
> Sorry, please use this one.

Thanks. I have succeeded to recreate the issue. Will take a look tomorrow.


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [bug report]WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 iomap_iter+0x32b observed with blktests zbd/010

[Shinichiro Kawasaki via Linux-f2fs-devel]

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

I created a fix candidate patch [1]. It modifies f2fs_iomap_begin() to handle
map->m_pblk as the physical block address from each device start, not the
address of whole f2fs. I confirmed it avoids the WARN.

But I'm not so sure if the fix is good enough. map->m_pblk has dual meanings.
Sometimes it holds the physical block address of each device, and sometimes
the address of the whole f2fs. I'm not sure what is the condition for
map->m_pblk to have which meaning. I guess F2FS_GET_BLOCK_DIO flag is the
condition, but f2fs_map_blocks_cached() does not ensure it.

Also, I noticed that map->m_pblk is referred to in other functions below, and
not sure if they need the similar change as I did for f2fs_iomap_begin().

  f2fs_fiemap()
  f2fs_read_single_page()
  f2fs_bmap()
  check_swap_activate()

I would like to hear advices from f2fs experts for the fix.


[1]

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 26e317696b33..5232223a69e5 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1569,6 +1569,7 @@ static bool f2fs_map_blocks_cached(struct inode *inode,
 		int bidx = f2fs_target_device_index(sbi, map->m_pblk);
 		struct f2fs_dev_info *dev = &sbi->devs[bidx];
 
+		map->m_multidev_dio = true;
 		map->m_bdev = dev->bdev;
 		map->m_pblk -= dev->start_blk;
 		map->m_len = min(map->m_len, dev->end_blk + 1 - map->m_pblk);
@@ -4211,9 +4212,11 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 			    unsigned int flags, struct iomap *iomap,
 			    struct iomap *srcmap)
 {
+	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct f2fs_map_blocks map = {};
 	pgoff_t next_pgofs = 0;
-	int err;
+	block_t pblk;
+	int err, i;
 
 	map.m_lblk = bytes_to_blks(inode, offset);
 	map.m_len = bytes_to_blks(inode, offset + length - 1) - map.m_lblk + 1;
@@ -4239,12 +4242,17 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 	 * We should never see delalloc or compressed extents here based on
 	 * prior flushing and checks.
 	 */
-	if (WARN_ON_ONCE(map.m_pblk == NEW_ADDR))
+	pblk = map.m_pblk;
+	if (map.m_multidev_dio && map.m_flags & F2FS_MAP_MAPPED)
+		for (i = 0; i < sbi->s_ndevs; i++)
+			if (FDEV(i).bdev == map.m_bdev)
+				pblk += FDEV(i).start_blk;
+	if (WARN_ON_ONCE(pblk == NEW_ADDR))
 		return -EINVAL;
-	if (WARN_ON_ONCE(map.m_pblk == COMPRESS_ADDR))
+	if (WARN_ON_ONCE(pblk == COMPRESS_ADDR))
 		return -EINVAL;
 
-	if (map.m_pblk != NULL_ADDR) {
+	if (pblk != NULL_ADDR) {
 		iomap->length = blks_to_bytes(inode, map.m_len);
 		iomap->type = IOMAP_MAPPED;
 		iomap->flags |= IOMAP_F_MERGED;

_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [bug report]WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 iomap_iter+0x32b observed with blktests zbd/010

[Daeho Jeong]

On Sun, Mar 17, 2024 at 10:49 PM Shinichiro Kawasaki via
Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net> wrote:
>
> 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
>
> I created a fix candidate patch [1]. It modifies f2fs_iomap_begin() to handle
> map->m_pblk as the physical block address from each device start, not the
> address of whole f2fs. I confirmed it avoids the WARN.
>
> But I'm not so sure if the fix is good enough. map->m_pblk has dual meanings.
> Sometimes it holds the physical block address of each device, and sometimes
> the address of the whole f2fs. I'm not sure what is the condition for
> map->m_pblk to have which meaning. I guess F2FS_GET_BLOCK_DIO flag is the
> condition, but f2fs_map_blocks_cached() does not ensure it.
>
> Also, I noticed that map->m_pblk is referred to in other functions below, and
> not sure if they need the similar change as I did for f2fs_iomap_begin().
>
>   f2fs_fiemap()
>   f2fs_read_single_page()
>   f2fs_bmap()
>   check_swap_activate()
>
> I would like to hear advices from f2fs experts for the fix.
>
>
> [1]
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 26e317696b33..5232223a69e5 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1569,6 +1569,7 @@ static bool f2fs_map_blocks_cached(struct inode *inode,
>                 int bidx = f2fs_target_device_index(sbi, map->m_pblk);
>                 struct f2fs_dev_info *dev = &sbi->devs[bidx];
>
> +               map->m_multidev_dio = true;
>                 map->m_bdev = dev->bdev;
>                 map->m_pblk -= dev->start_blk;
>                 map->m_len = min(map->m_len, dev->end_blk + 1 - map->m_pblk);
> @@ -4211,9 +4212,11 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>                             unsigned int flags, struct iomap *iomap,
>                             struct iomap *srcmap)
>  {
> +       struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>         struct f2fs_map_blocks map = {};
>         pgoff_t next_pgofs = 0;
> -       int err;
> +       block_t pblk;
> +       int err, i;
>
>         map.m_lblk = bytes_to_blks(inode, offset);
>         map.m_len = bytes_to_blks(inode, offset + length - 1) - map.m_lblk + 1;
> @@ -4239,12 +4242,17 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>          * We should never see delalloc or compressed extents here based on
>          * prior flushing and checks.
>          */
> -       if (WARN_ON_ONCE(map.m_pblk == NEW_ADDR))
> +       pblk = map.m_pblk;
> +       if (map.m_multidev_dio && map.m_flags & F2FS_MAP_MAPPED)
> +               for (i = 0; i < sbi->s_ndevs; i++)
> +                       if (FDEV(i).bdev == map.m_bdev)
> +                               pblk += FDEV(i).start_blk;
> +       if (WARN_ON_ONCE(pblk == NEW_ADDR))
>                 return -EINVAL;
> -       if (WARN_ON_ONCE(map.m_pblk == COMPRESS_ADDR))
> +       if (WARN_ON_ONCE(pblk == COMPRESS_ADDR))
>                 return -EINVAL;
>
> -       if (map.m_pblk != NULL_ADDR) {
> +       if (pblk != NULL_ADDR) {

I feel like we should check only whether the block is really mapped or
not by checking F2FS_MAP_MAPPED field without changing the pblk, since
"0" pblk for the secondary device should remain 0 if it's the correct
value.

>                 iomap->length = blks_to_bytes(inode, map.m_len);
>                 iomap->type = IOMAP_MAPPED;
>                 iomap->flags |= IOMAP_F_MERGED;
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [bug report]WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 iomap_iter+0x32b observed with blktests zbd/010

[Shinichiro Kawasaki via Linux-f2fs-devel]

On Mar 18, 2024 / 14:12, Daeho Jeong wrote:
> On Sun, Mar 17, 2024 at 10:49 PM Shinichiro Kawasaki via
> Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net> wrote:
> >
> > 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
> >
> > I created a fix candidate patch [1]. It modifies f2fs_iomap_begin() to handle
> > map->m_pblk as the physical block address from each device start, not the
> > address of whole f2fs. I confirmed it avoids the WARN.
> >
> > But I'm not so sure if the fix is good enough. map->m_pblk has dual meanings.
> > Sometimes it holds the physical block address of each device, and sometimes
> > the address of the whole f2fs. I'm not sure what is the condition for
> > map->m_pblk to have which meaning. I guess F2FS_GET_BLOCK_DIO flag is the
> > condition, but f2fs_map_blocks_cached() does not ensure it.
> >
> > Also, I noticed that map->m_pblk is referred to in other functions below, and
> > not sure if they need the similar change as I did for f2fs_iomap_begin().
> >
> >   f2fs_fiemap()
> >   f2fs_read_single_page()
> >   f2fs_bmap()
> >   check_swap_activate()
> >
> > I would like to hear advices from f2fs experts for the fix.
> >
> >
> > [1]
> >
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 26e317696b33..5232223a69e5 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -1569,6 +1569,7 @@ static bool f2fs_map_blocks_cached(struct inode *inode,
> >                 int bidx = f2fs_target_device_index(sbi, map->m_pblk);
> >                 struct f2fs_dev_info *dev = &sbi->devs[bidx];
> >
> > +               map->m_multidev_dio = true;
> >                 map->m_bdev = dev->bdev;
> >                 map->m_pblk -= dev->start_blk;
> >                 map->m_len = min(map->m_len, dev->end_blk + 1 - map->m_pblk);
> > @@ -4211,9 +4212,11 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> >                             unsigned int flags, struct iomap *iomap,
> >                             struct iomap *srcmap)
> >  {
> > +       struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> >         struct f2fs_map_blocks map = {};
> >         pgoff_t next_pgofs = 0;
> > -       int err;
> > +       block_t pblk;
> > +       int err, i;
> >
> >         map.m_lblk = bytes_to_blks(inode, offset);
> >         map.m_len = bytes_to_blks(inode, offset + length - 1) - map.m_lblk + 1;
> > @@ -4239,12 +4242,17 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> >          * We should never see delalloc or compressed extents here based on
> >          * prior flushing and checks.
> >          */
> > -       if (WARN_ON_ONCE(map.m_pblk == NEW_ADDR))
> > +       pblk = map.m_pblk;
> > +       if (map.m_multidev_dio && map.m_flags & F2FS_MAP_MAPPED)
> > +               for (i = 0; i < sbi->s_ndevs; i++)
> > +                       if (FDEV(i).bdev == map.m_bdev)
> > +                               pblk += FDEV(i).start_blk;
> > +       if (WARN_ON_ONCE(pblk == NEW_ADDR))
> >                 return -EINVAL;
> > -       if (WARN_ON_ONCE(map.m_pblk == COMPRESS_ADDR))
> > +       if (WARN_ON_ONCE(pblk == COMPRESS_ADDR))
> >                 return -EINVAL;
> >
> > -       if (map.m_pblk != NULL_ADDR) {
> > +       if (pblk != NULL_ADDR) {
> 
> I feel like we should check only whether the block is really mapped or
> not by checking F2FS_MAP_MAPPED field without changing the pblk, since
> "0" pblk for the secondary device should remain 0 if it's the correct
> value.

My intent was to keep the physical block address from the secondary device
start in map.m_pblk. So "0" for the secondeary device is kept in map.m_pblk.

Having said that, I'm not sure that this modification covers all conditions.
I think your comment above is similar as Chao's comment. Will respond to it.

> 
> >                 iomap->length = blks_to_bytes(inode, map.m_len);
> >                 iomap->type = IOMAP_MAPPED;
> >                 iomap->flags |= IOMAP_F_MERGED;
> >
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [bug report]WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 iomap_iter+0x32b observed with blktests zbd/010

[Chao Yu]

On 2024/3/18 13:47, Shinichiro Kawasaki via Linux-f2fs-devel wrote:
> 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
> 
> I created a fix candidate patch [1]. It modifies f2fs_iomap_begin() to handle
> map->m_pblk as the physical block address from each device start, not the
> address of whole f2fs. I confirmed it avoids the WARN.
> 
> But I'm not so sure if the fix is good enough. map->m_pblk has dual meanings.
> Sometimes it holds the physical block address of each device, and sometimes
> the address of the whole f2fs. I'm not sure what is the condition for
> map->m_pblk to have which meaning. I guess F2FS_GET_BLOCK_DIO flag is the
> condition, but f2fs_map_blocks_cached() does not ensure it.
> 
> Also, I noticed that map->m_pblk is referred to in other functions below, and
> not sure if they need the similar change as I did for f2fs_iomap_begin().
> 
>    f2fs_fiemap()
>    f2fs_read_single_page()
>    f2fs_bmap()
>    check_swap_activate()
> 
> I would like to hear advices from f2fs experts for the fix.
> 
> 
> [1]
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 26e317696b33..5232223a69e5 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1569,6 +1569,7 @@ static bool f2fs_map_blocks_cached(struct inode *inode,
>   		int bidx = f2fs_target_device_index(sbi, map->m_pblk);
>   		struct f2fs_dev_info *dev = &sbi->devs[bidx];
>   
> +		map->m_multidev_dio = true;
>   		map->m_bdev = dev->bdev;
>   		map->m_pblk -= dev->start_blk;
>   		map->m_len = min(map->m_len, dev->end_blk + 1 - map->m_pblk);
> @@ -4211,9 +4212,11 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>   			    unsigned int flags, struct iomap *iomap,
>   			    struct iomap *srcmap)
>   {
> +	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>   	struct f2fs_map_blocks map = {};
>   	pgoff_t next_pgofs = 0;
> -	int err;
> +	block_t pblk;
> +	int err, i;
>   
>   	map.m_lblk = bytes_to_blks(inode, offset);
>   	map.m_len = bytes_to_blks(inode, offset + length - 1) - map.m_lblk + 1;
> @@ -4239,12 +4242,17 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>   	 * We should never see delalloc or compressed extents here based on
>   	 * prior flushing and checks.
>   	 */
> -	if (WARN_ON_ONCE(map.m_pblk == NEW_ADDR))
> +	pblk = map.m_pblk;
> +	if (map.m_multidev_dio && map.m_flags & F2FS_MAP_MAPPED)
> +		for (i = 0; i < sbi->s_ndevs; i++)
> +			if (FDEV(i).bdev == map.m_bdev)
> +				pblk += FDEV(i).start_blk;
> +	if (WARN_ON_ONCE(pblk == NEW_ADDR))
>   		return -EINVAL;
> -	if (WARN_ON_ONCE(map.m_pblk == COMPRESS_ADDR))
> +	if (WARN_ON_ONCE(pblk == COMPRESS_ADDR))
>   		return -EINVAL;

Shoudn't we check NEW_ADDR and COMPRESS_ADDR before multiple-device
block address conversion?

>   
> -	if (map.m_pblk != NULL_ADDR) {
> +	if (pblk != NULL_ADDR) {

How to distinguish NULL_ADDR and valid blkaddr 0? I guess it should
check F2FS_MAP_MAPPED flag first?

Thanks,

>   		iomap->length = blks_to_bytes(inode, map.m_len);
>   		iomap->type = IOMAP_MAPPED;
>   		iomap->flags |= IOMAP_F_MERGED;
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [bug report]WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 iomap_iter+0x32b observed with blktests zbd/010

[Shinichiro Kawasaki via Linux-f2fs-devel]

On Mar 19, 2024 / 10:22, Chao Yu wrote:
> On 2024/3/18 13:47, Shinichiro Kawasaki via Linux-f2fs-devel wrote:
> > 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
> > 
> > I created a fix candidate patch [1]. It modifies f2fs_iomap_begin() to handle
> > map->m_pblk as the physical block address from each device start, not the
> > address of whole f2fs. I confirmed it avoids the WARN.
> > 
> > But I'm not so sure if the fix is good enough. map->m_pblk has dual meanings.
> > Sometimes it holds the physical block address of each device, and sometimes
> > the address of the whole f2fs. I'm not sure what is the condition for
> > map->m_pblk to have which meaning. I guess F2FS_GET_BLOCK_DIO flag is the
> > condition, but f2fs_map_blocks_cached() does not ensure it.
> > 
> > Also, I noticed that map->m_pblk is referred to in other functions below, and
> > not sure if they need the similar change as I did for f2fs_iomap_begin().
> > 
> >    f2fs_fiemap()
> >    f2fs_read_single_page()
> >    f2fs_bmap()
> >    check_swap_activate()
> > 
> > I would like to hear advices from f2fs experts for the fix.
> > 
> > 
> > [1]
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 26e317696b33..5232223a69e5 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -1569,6 +1569,7 @@ static bool f2fs_map_blocks_cached(struct inode *inode,
> >   		int bidx = f2fs_target_device_index(sbi, map->m_pblk);
> >   		struct f2fs_dev_info *dev = &sbi->devs[bidx];
> > +		map->m_multidev_dio = true;
> >   		map->m_bdev = dev->bdev;
> >   		map->m_pblk -= dev->start_blk;
> >   		map->m_len = min(map->m_len, dev->end_blk + 1 - map->m_pblk);
> > @@ -4211,9 +4212,11 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> >   			    unsigned int flags, struct iomap *iomap,
> >   			    struct iomap *srcmap)
> >   {
> > +	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> >   	struct f2fs_map_blocks map = {};
> >   	pgoff_t next_pgofs = 0;
> > -	int err;
> > +	block_t pblk;
> > +	int err, i;
> >   	map.m_lblk = bytes_to_blks(inode, offset);
> >   	map.m_len = bytes_to_blks(inode, offset + length - 1) - map.m_lblk + 1;
> > @@ -4239,12 +4242,17 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> >   	 * We should never see delalloc or compressed extents here based on
> >   	 * prior flushing and checks.
> >   	 */
> > -	if (WARN_ON_ONCE(map.m_pblk == NEW_ADDR))
> > +	pblk = map.m_pblk;
> > +	if (map.m_multidev_dio && map.m_flags & F2FS_MAP_MAPPED)
> > +		for (i = 0; i < sbi->s_ndevs; i++)
> > +			if (FDEV(i).bdev == map.m_bdev)
> > +				pblk += FDEV(i).start_blk;
> > +	if (WARN_ON_ONCE(pblk == NEW_ADDR))
> >   		return -EINVAL;
> > -	if (WARN_ON_ONCE(map.m_pblk == COMPRESS_ADDR))
> > +	if (WARN_ON_ONCE(pblk == COMPRESS_ADDR))
> >   		return -EINVAL;
> 
> Shoudn't we check NEW_ADDR and COMPRESS_ADDR before multiple-device
> block address conversion?

As far as I understand, NEW_ADDR and COMPRESS_ADDR in map.m_pblk can be
target of "map->m_pblk -= FDEV(bidx).start_blk;" in f2fs_map_blocks(),
so I guessed that the address conversion should come first.

> 
> > -	if (map.m_pblk != NULL_ADDR) {
> > +	if (pblk != NULL_ADDR) {
> 
> How to distinguish NULL_ADDR and valid blkaddr 0? I guess it should
> check F2FS_MAP_MAPPED flag first?

I guessed that physical block address for the whole f2fs (pblk) can not be 0, so
the NULL_ADDR can have zero value. As for the physical block address of each
device (map->m_pblk) can be 0. But this is still my *guess*, and I'm not sure.


The comments from you and Daeho made me rethink. It looks problematic for me
that map->m_pblk has two meanings as I had described: "1) physical block address
from each device start", and "2) physical block address of whole f2fs". So how
about to make it have only one meaning "2) physical block address address of
whole f2fs"? I created another patch below [2]. It removes the

   map->m_pblk -= FDEV(bidx).start_blk;

lines in f2fs_map_blocks_cached() and f2fs_map_blocks() so that map->m_pblk do
not have the meaning 1). Instead, the subtraction is done in f2fs_iomap_begin().
I confirmed that this patch also avoids the WARN. I can have more confidence in
this patch, and I hope it is easier to review.

P.S. If anyone has better solution idea, feel free to provide patches. I'm
     willing to test them :)


[2]

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 26e317696b33..7404b4fbcba3 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1569,8 +1569,8 @@ static bool f2fs_map_blocks_cached(struct inode *inode,
 		int bidx = f2fs_target_device_index(sbi, map->m_pblk);
 		struct f2fs_dev_info *dev = &sbi->devs[bidx];
 
+		map->m_multidev_dio = true;
 		map->m_bdev = dev->bdev;
-		map->m_pblk -= dev->start_blk;
 		map->m_len = min(map->m_len, dev->end_blk + 1 - map->m_pblk);
 	} else {
 		map->m_bdev = inode->i_sb->s_bdev;
@@ -1793,11 +1793,8 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map, int flag)
 
 		if (map->m_multidev_dio) {
 			block_t blk_addr = map->m_pblk;
-
 			bidx = f2fs_target_device_index(sbi, map->m_pblk);
-
 			map->m_bdev = FDEV(bidx).bdev;
-			map->m_pblk -= FDEV(bidx).start_blk;
 
 			if (map->m_may_create)
 				f2fs_update_device_state(sbi, inode->i_ino,
@@ -4211,9 +4208,11 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 			    unsigned int flags, struct iomap *iomap,
 			    struct iomap *srcmap)
 {
+	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct f2fs_map_blocks map = {};
 	pgoff_t next_pgofs = 0;
-	int err;
+	block_t pblk;
+	int err, bidx;
 
 	map.m_lblk = bytes_to_blks(inode, offset);
 	map.m_len = bytes_to_blks(inode, offset + length - 1) - map.m_lblk + 1;
@@ -4249,7 +4248,12 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 		iomap->type = IOMAP_MAPPED;
 		iomap->flags |= IOMAP_F_MERGED;
 		iomap->bdev = map.m_bdev;
-		iomap->addr = blks_to_bytes(inode, map.m_pblk);
+		pblk = map.m_pblk;
+		if (map.m_multidev_dio && map.m_flags & F2FS_MAP_MAPPED) {
+			bidx = f2fs_target_device_index(sbi, map.m_pblk);
+			pblk -= FDEV(bidx).start_blk;
+		}
+		iomap->addr = blks_to_bytes(inode, pblk);
 	} else {
 		if (flags & IOMAP_WRITE)
 			return -ENOTBLK;


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [bug report]WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 iomap_iter+0x32b observed with blktests zbd/010

[Chao Yu]

On 2024/3/19 19:13, Shinichiro Kawasaki wrote:
> On Mar 19, 2024 / 10:22, Chao Yu wrote:
>> On 2024/3/18 13:47, Shinichiro Kawasaki via Linux-f2fs-devel wrote:
>>> 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
>>>
>>> I created a fix candidate patch [1]. It modifies f2fs_iomap_begin() to handle
>>> map->m_pblk as the physical block address from each device start, not the
>>> address of whole f2fs. I confirmed it avoids the WARN.
>>>
>>> But I'm not so sure if the fix is good enough. map->m_pblk has dual meanings.
>>> Sometimes it holds the physical block address of each device, and sometimes
>>> the address of the whole f2fs. I'm not sure what is the condition for
>>> map->m_pblk to have which meaning. I guess F2FS_GET_BLOCK_DIO flag is the
>>> condition, but f2fs_map_blocks_cached() does not ensure it.
>>>
>>> Also, I noticed that map->m_pblk is referred to in other functions below, and
>>> not sure if they need the similar change as I did for f2fs_iomap_begin().
>>>
>>>     f2fs_fiemap()
>>>     f2fs_read_single_page()
>>>     f2fs_bmap()
>>>     check_swap_activate()
>>>
>>> I would like to hear advices from f2fs experts for the fix.
>>>
>>>
>>> [1]
>>>
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index 26e317696b33..5232223a69e5 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -1569,6 +1569,7 @@ static bool f2fs_map_blocks_cached(struct inode *inode,
>>>    		int bidx = f2fs_target_device_index(sbi, map->m_pblk);
>>>    		struct f2fs_dev_info *dev = &sbi->devs[bidx];
>>> +		map->m_multidev_dio = true;
>>>    		map->m_bdev = dev->bdev;
>>>    		map->m_pblk -= dev->start_blk;
>>>    		map->m_len = min(map->m_len, dev->end_blk + 1 - map->m_pblk);
>>> @@ -4211,9 +4212,11 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>>>    			    unsigned int flags, struct iomap *iomap,
>>>    			    struct iomap *srcmap)
>>>    {
>>> +	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>>    	struct f2fs_map_blocks map = {};
>>>    	pgoff_t next_pgofs = 0;
>>> -	int err;
>>> +	block_t pblk;
>>> +	int err, i;
>>>    	map.m_lblk = bytes_to_blks(inode, offset);
>>>    	map.m_len = bytes_to_blks(inode, offset + length - 1) - map.m_lblk + 1;
>>> @@ -4239,12 +4242,17 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>>>    	 * We should never see delalloc or compressed extents here based on
>>>    	 * prior flushing and checks.
>>>    	 */
>>> -	if (WARN_ON_ONCE(map.m_pblk == NEW_ADDR))
>>> +	pblk = map.m_pblk;
>>> +	if (map.m_multidev_dio && map.m_flags & F2FS_MAP_MAPPED)
>>> +		for (i = 0; i < sbi->s_ndevs; i++)
>>> +			if (FDEV(i).bdev == map.m_bdev)
>>> +				pblk += FDEV(i).start_blk;
>>> +	if (WARN_ON_ONCE(pblk == NEW_ADDR))
>>>    		return -EINVAL;
>>> -	if (WARN_ON_ONCE(map.m_pblk == COMPRESS_ADDR))
>>> +	if (WARN_ON_ONCE(pblk == COMPRESS_ADDR))
>>>    		return -EINVAL;
>>
>> Shoudn't we check NEW_ADDR and COMPRESS_ADDR before multiple-device
>> block address conversion?
> 
> As far as I understand, NEW_ADDR and COMPRESS_ADDR in map.m_pblk can be
> target of "map->m_pblk -= FDEV(bidx).start_blk;" in f2fs_map_blocks(),
> so I guessed that the address conversion should come first.
> 
>>
>>> -	if (map.m_pblk != NULL_ADDR) {
>>> +	if (pblk != NULL_ADDR) {
>>
>> How to distinguish NULL_ADDR and valid blkaddr 0? I guess it should
>> check F2FS_MAP_MAPPED flag first?
> 
> I guessed that physical block address for the whole f2fs (pblk) can not be 0, so
> the NULL_ADDR can have zero value. As for the physical block address of each
> device (map->m_pblk) can be 0. But this is still my *guess*, and I'm not sure.
> 
> 
> The comments from you and Daeho made me rethink. It looks problematic for me
> that map->m_pblk has two meanings as I had described: "1) physical block address
> from each device start", and "2) physical block address of whole f2fs". So how
> about to make it have only one meaning "2) physical block address address of
> whole f2fs"? I created another patch below [2]. It removes the
> 
>     map->m_pblk -= FDEV(bidx).start_blk;
> 
> lines in f2fs_map_blocks_cached() and f2fs_map_blocks() so that map->m_pblk do
> not have the meaning 1). Instead, the subtraction is done in f2fs_iomap_begin().
> I confirmed that this patch also avoids the WARN. I can have more confidence in
> this patch, and I hope it is easier to review.
> 
> P.S. If anyone has better solution idea, feel free to provide patches. I'm
>       willing to test them :)
> 
> 
> [2]
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 26e317696b33..7404b4fbcba3 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1569,8 +1569,8 @@ static bool f2fs_map_blocks_cached(struct inode *inode,
>   		int bidx = f2fs_target_device_index(sbi, map->m_pblk);
>   		struct f2fs_dev_info *dev = &sbi->devs[bidx];
>   
> +		map->m_multidev_dio = true;
>   		map->m_bdev = dev->bdev;
> -		map->m_pblk -= dev->start_blk;
>   		map->m_len = min(map->m_len, dev->end_blk + 1 - map->m_pblk);
>   	} else {
>   		map->m_bdev = inode->i_sb->s_bdev;
> @@ -1793,11 +1793,8 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map, int flag)
>   
>   		if (map->m_multidev_dio) {
>   			block_t blk_addr = map->m_pblk;
> -
>   			bidx = f2fs_target_device_index(sbi, map->m_pblk);
> -
>   			map->m_bdev = FDEV(bidx).bdev;
> -			map->m_pblk -= FDEV(bidx).start_blk;
>   
>   			if (map->m_may_create)
>   				f2fs_update_device_state(sbi, inode->i_ino,
> @@ -4211,9 +4208,11 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>   			    unsigned int flags, struct iomap *iomap,
>   			    struct iomap *srcmap)
>   {
> +	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>   	struct f2fs_map_blocks map = {};
>   	pgoff_t next_pgofs = 0;
> -	int err;
> +	block_t pblk;
> +	int err, bidx;
>   
>   	map.m_lblk = bytes_to_blks(inode, offset);
>   	map.m_len = bytes_to_blks(inode, offset + length - 1) - map.m_lblk + 1;
> @@ -4249,7 +4248,12 @@ static int f2fs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>   		iomap->type = IOMAP_MAPPED;
>   		iomap->flags |= IOMAP_F_MERGED;
>   		iomap->bdev = map.m_bdev;
> -		iomap->addr = blks_to_bytes(inode, map.m_pblk);
> +		pblk = map.m_pblk;
> +		if (map.m_multidev_dio && map.m_flags & F2FS_MAP_MAPPED) {
> +			bidx = f2fs_target_device_index(sbi, map.m_pblk);
> +			pblk -= FDEV(bidx).start_blk;
> +		}
> +		iomap->addr = blks_to_bytes(inode, pblk);
>   	} else {
>   		if (flags & IOMAP_WRITE)
>   			return -ENOTBLK;

Hi Shinichiro,

Can you please check below diff? IIUC, for the case: f2fs_map_blocks()
returns zero blkaddr in non-primary device, which is a verified valid
block address, we'd better to check m_flags & F2FS_MAP_MAPPED instead
of map.m_pblk != NULL_ADDR to decide whether tagging IOMAP_MAPPED flag
or not.

---
  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 6f66e3e4221a..41a56d4298c8 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -4203,7 +4203,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;



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [bug report]WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 iomap_iter+0x32b observed with blktests zbd/010

[Shinichiro Kawasaki via Linux-f2fs-devel]

On Mar 24, 2024 / 20:13, Chao Yu wrote:
...
> Hi Shinichiro,
> 
> Can you please check below diff? IIUC, for the case: f2fs_map_blocks()
> returns zero blkaddr in non-primary device, which is a verified valid
> block address, we'd better to check m_flags & F2FS_MAP_MAPPED instead
> of map.m_pblk != NULL_ADDR to decide whether tagging IOMAP_MAPPED flag
> or not.
> 
> ---
>  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 6f66e3e4221a..41a56d4298c8 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -4203,7 +4203,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;
> 

Thanks Chao, I confirmed that the diff above avoids the WARN and zbd/010
failure. From that point of view, it looks good.

One thing I noticed is that the commit message of 8d3c1fa3fa5ea ("f2fs:
don't rely on F2FS_MAP_* in f2fs_iomap_begin") says that f2fs_map_blocks()
might be setting F2FS_MAP_* flag on a hole, and that's why the commit
avoided the F2FS_MAP_MAPPED flag check. So I was not sure if it is the
right thing to reintroduce the flag check.

_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [bug report]WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 iomap_iter+0x32b observed with blktests zbd/010

[Chao Yu]

On 2024/3/25 10:14, Shinichiro Kawasaki wrote:
> On Mar 24, 2024 / 20:13, Chao Yu wrote:
> ...
>> Hi Shinichiro,
>>
>> Can you please check below diff? IIUC, for the case: f2fs_map_blocks()
>> returns zero blkaddr in non-primary device, which is a verified valid
>> block address, we'd better to check m_flags & F2FS_MAP_MAPPED instead
>> of map.m_pblk != NULL_ADDR to decide whether tagging IOMAP_MAPPED flag
>> or not.
>>
>> ---
>>   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 6f66e3e4221a..41a56d4298c8 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -4203,7 +4203,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;
>>
> 
> Thanks Chao, I confirmed that the diff above avoids the WARN and zbd/010
> failure. From that point of view, it looks good.

Thank you for the confirmation. :)

> 
> One thing I noticed is that the commit message of 8d3c1fa3fa5ea ("f2fs:
> don't rely on F2FS_MAP_* in f2fs_iomap_begin") says that f2fs_map_blocks()
> might be setting F2FS_MAP_* flag on a hole, and that's why the commit
> avoided the F2FS_MAP_MAPPED flag check. So I was not sure if it is the
> right thing to reintroduce the flag check.

I didn't see such logic in previous f2fs_map_blocks(, F2FS_GET_BLOCK_DIO) codebase,
I doubt it hits the same case: map.m_pblk is valid zero blkaddr which locates in
the head of secondary device? What do you think?

Quoted commit message from 8d3c1fa3fa5ea:

When testing with a mixed zoned / convention device combination, there
are regular but not 100% reproducible failures in xfstests generic/113
where the __is_valid_data_blkaddr assert hits due to finding a hole.

Previous code:

-	if (map.m_flags & (F2FS_MAP_MAPPED | F2FS_MAP_UNWRITTEN)) {
-		iomap->length = blks_to_bytes(inode, map.m_len);
-		if (map.m_flags & F2FS_MAP_MAPPED) {
-			iomap->type = IOMAP_MAPPED;
-			iomap->flags |= IOMAP_F_MERGED;
-		} else {
-			iomap->type = IOMAP_UNWRITTEN;
-		}
-		if (WARN_ON_ONCE(!__is_valid_data_blkaddr(map.m_pblk)))
-			return -EINVAL;

Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [bug report]WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 iomap_iter+0x32b observed with blktests zbd/010

[Shinichiro Kawasaki via Linux-f2fs-devel]

On Mar 25, 2024 / 11:06, Chao Yu wrote:
> On 2024/3/25 10:14, Shinichiro Kawasaki wrote:
> > On Mar 24, 2024 / 20:13, Chao Yu wrote:
> > ...
> > > Hi Shinichiro,
> > > 
> > > Can you please check below diff? IIUC, for the case: f2fs_map_blocks()
> > > returns zero blkaddr in non-primary device, which is a verified valid
> > > block address, we'd better to check m_flags & F2FS_MAP_MAPPED instead
> > > of map.m_pblk != NULL_ADDR to decide whether tagging IOMAP_MAPPED flag
> > > or not.
> > > 
> > > ---
> > >   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 6f66e3e4221a..41a56d4298c8 100644
> > > --- a/fs/f2fs/data.c
> > > +++ b/fs/f2fs/data.c
> > > @@ -4203,7 +4203,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;
> > > 
> > 
> > Thanks Chao, I confirmed that the diff above avoids the WARN and zbd/010
> > failure. From that point of view, it looks good.
> 
> Thank you for the confirmation. :)
> 
> > 
> > One thing I noticed is that the commit message of 8d3c1fa3fa5ea ("f2fs:
> > don't rely on F2FS_MAP_* in f2fs_iomap_begin") says that f2fs_map_blocks()
> > might be setting F2FS_MAP_* flag on a hole, and that's why the commit
> > avoided the F2FS_MAP_MAPPED flag check. So I was not sure if it is the
> > right thing to reintroduce the flag check.
> 
> I didn't see such logic in previous f2fs_map_blocks(, F2FS_GET_BLOCK_DIO) codebase,
> I doubt it hits the same case: map.m_pblk is valid zero blkaddr which locates in
> the head of secondary device? What do you think?
> 
> Quoted commit message from 8d3c1fa3fa5ea:
> 
> When testing with a mixed zoned / convention device combination, there
> are regular but not 100% reproducible failures in xfstests generic/113
> where the __is_valid_data_blkaddr assert hits due to finding a hole.
> 
> Previous code:
> 
> -	if (map.m_flags & (F2FS_MAP_MAPPED | F2FS_MAP_UNWRITTEN)) {
> -		iomap->length = blks_to_bytes(inode, map.m_len);
> -		if (map.m_flags & F2FS_MAP_MAPPED) {
> -			iomap->type = IOMAP_MAPPED;
> -			iomap->flags |= IOMAP_F_MERGED;
> -		} else {
> -			iomap->type = IOMAP_UNWRITTEN;
> -		}
> -		if (WARN_ON_ONCE(!__is_valid_data_blkaddr(map.m_pblk)))
> -			return -EINVAL;

Hmm, I can agree with your guess. Let me add two more points:

1) The commit message says that the generic/113 failure was not 100% recreated.
   So it was difficult to confirm that the commit avoided the failure, probably.

2) I ran zbd/010 using the kernel just before the commit 8d3c1fa3fa5ea, and
   observed the WARN in the hunk you quoted above.

     WARNING: CPU: 1 PID: 1035 at fs/f2fs/data.c:4164 f2fs_iomap_begin+0x19e/0x1b0 [f2fs]

   So, it implies that the WARN observed xfstests generic/113 has same failure
   scenario as blktests zbd/010, probably.


Based on these guesses, I think your fix diff is reasonable. If you post it as a
formal patch, feel free to add my:

Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [bug report]WARNING: CPU: 22 PID: 44011 at fs/iomap/iter.c:51 iomap_iter+0x32b observed with blktests zbd/010

[Chao Yu]

On 2024/3/25 14:56, Shinichiro Kawasaki wrote:
> On Mar 25, 2024 / 11:06, Chao Yu wrote:
>> On 2024/3/25 10:14, Shinichiro Kawasaki wrote:
>>> On Mar 24, 2024 / 20:13, Chao Yu wrote:
>>> ...
>>>> Hi Shinichiro,
>>>>
>>>> Can you please check below diff? IIUC, for the case: f2fs_map_blocks()
>>>> returns zero blkaddr in non-primary device, which is a verified valid
>>>> block address, we'd better to check m_flags & F2FS_MAP_MAPPED instead
>>>> of map.m_pblk != NULL_ADDR to decide whether tagging IOMAP_MAPPED flag
>>>> or not.
>>>>
>>>> ---
>>>>    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 6f66e3e4221a..41a56d4298c8 100644
>>>> --- a/fs/f2fs/data.c
>>>> +++ b/fs/f2fs/data.c
>>>> @@ -4203,7 +4203,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;
>>>>
>>>
>>> Thanks Chao, I confirmed that the diff above avoids the WARN and zbd/010
>>> failure. From that point of view, it looks good.
>>
>> Thank you for the confirmation. :)
>>
>>>
>>> One thing I noticed is that the commit message of 8d3c1fa3fa5ea ("f2fs:
>>> don't rely on F2FS_MAP_* in f2fs_iomap_begin") says that f2fs_map_blocks()
>>> might be setting F2FS_MAP_* flag on a hole, and that's why the commit
>>> avoided the F2FS_MAP_MAPPED flag check. So I was not sure if it is the
>>> right thing to reintroduce the flag check.
>>
>> I didn't see such logic in previous f2fs_map_blocks(, F2FS_GET_BLOCK_DIO) codebase,
>> I doubt it hits the same case: map.m_pblk is valid zero blkaddr which locates in
>> the head of secondary device? What do you think?
>>
>> Quoted commit message from 8d3c1fa3fa5ea:
>>
>> When testing with a mixed zoned / convention device combination, there
>> are regular but not 100% reproducible failures in xfstests generic/113
>> where the __is_valid_data_blkaddr assert hits due to finding a hole.
>>
>> Previous code:
>>
>> -	if (map.m_flags & (F2FS_MAP_MAPPED | F2FS_MAP_UNWRITTEN)) {
>> -		iomap->length = blks_to_bytes(inode, map.m_len);
>> -		if (map.m_flags & F2FS_MAP_MAPPED) {
>> -			iomap->type = IOMAP_MAPPED;
>> -			iomap->flags |= IOMAP_F_MERGED;
>> -		} else {
>> -			iomap->type = IOMAP_UNWRITTEN;
>> -		}
>> -		if (WARN_ON_ONCE(!__is_valid_data_blkaddr(map.m_pblk)))
>> -			return -EINVAL;
> 
> Hmm, I can agree with your guess. Let me add two more points:
> 
> 1) The commit message says that the generic/113 failure was not 100% recreated.
>     So it was difficult to confirm that the commit avoided the failure, probably.
> 
> 2) I ran zbd/010 using the kernel just before the commit 8d3c1fa3fa5ea, and
>     observed the WARN in the hunk you quoted above.
> 
>       WARNING: CPU: 1 PID: 1035 at fs/f2fs/data.c:4164 f2fs_iomap_begin+0x19e/0x1b0 [f2fs]
> 
>     So, it implies that the WARN observed xfstests generic/113 has same failure
>     scenario as blktests zbd/010, probably.

Yup,

> 
> 
> Based on these guesses, I think your fix diff is reasonable. If you post it as a
> formal patch, feel free to add my:
> 
> Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

Thank you for the test!

I've submitted a formal patch, let me know, if you have any comments on it, or want
to update it.

https://lore.kernel.org/linux-f2fs-devel/20240325152623.797099-1-chao@kernel.org/

Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel