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