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