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