Re: [f2fs-dev] [PATCH] f2fs: multidev: fix to recognize valid zero block address

[Chao Yu <chao@kernel.org>]

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


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