> -----Original Message-----
> From: [email protected] <[email protected]> On Behalf Of
> Chao Yu
> Sent: Wednesday, October 27, 2021 11:31 AM
> To: fengnan chang <[email protected]>
> Cc: 常凤楠 <[email protected]>; Jaegeuk Kim <[email protected]>;
> [email protected]
> Subject: Re: [f2fs-dev] [PATCH v5 2/2] f2fs: fix missing inplace count in
> overwrite with direct io
>
> On 2021/10/16 16:01, fengnan chang wrote:
> > Chao Yu <[email protected]> 于2021年10月13日周三 下午11:19写道：
> >>
> >> On 2021/10/9 19:27, Fengnan Chang wrote:
> >>> For now, overwrite file with direct io use inplace policy, but not
> >>> counted, fix it. And use stat_add_inplace_blocks(sbi, 1, ) instead
> >>> of stat_inc_inplace_blocks(sb, ).
> >>>
> >>> Signed-off-by: Fengnan Chang <[email protected]>
> >>> ---
> >>> fs/f2fs/data.c | 4 +++-
> >>> fs/f2fs/f2fs.h | 8 ++++----
> >>> fs/f2fs/segment.c | 2 +-
> >>> 3 files changed, 8 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
> >>> c1490b9a1345..7798f7236376 100644
> >>> --- a/fs/f2fs/data.c
> >>> +++ b/fs/f2fs/data.c
> >>> @@ -1553,7 +1553,9 @@ int f2fs_map_blocks(struct inode *inode,
> struct f2fs_map_blocks *map,
> >>> goto sync_out;
> >>> blkaddr = dn.data_blkaddr;
> >>> set_inode_flag(inode, FI_APPEND_WRITE);
> >>> - }
> >>> + } else if (!f2fs_lfs_mode(sbi) && flag ==
> F2FS_GET_BLOCK_PRE_DIO &&
> >>> + map->m_may_create && create)
> >>> + stat_add_inplace_blocks(sbi, 1, true);
> >>
> >> What about this case?
> >>
> >> - f2fs_preallocate_blocks
> >> - f2fs_map_blocks
> >> - stat_add_inplace_blocks
> >> map.m_len > 0 && err == -ENOSPC
> >> err = 0;
> >> - __generic_file_write_iter
> >> - generic_file_direct_write
> >> - f2fs_direct_IO
> >> - get_data_block_dio_write
> >> - __allocate_data_block
> >> - stat_inc_block_count
> >>
> >> DIO blocks will be accounted into different type? IIUC.
> > Yes, it will be accounted into different type, IPU and LFS, but it
> > will not accounted into both in same time for one block.
>
> Not sure this is right, since all writes should be accounted into LFS.
Sorry, I didn't get your point, why all writes should be accounted into LFS ?
even overwrite with direct io ?
>
> >
> > root@kvm-xfstests:/mnt# cat /sys/kernel/debug/f2fs/status |grep SSR -C 2
> > buffer direct segments
> > IPU: 16 32 N/A
> > SSR: 0 0 0
> > LFS: 38 48 0
> > root@kvm-xfstests:/mnt# dd if=/dev/zero of=./1 bs=32K count=1
> > oflag=direct root@kvm-xfstests:/mnt# cat /sys/kernel/debug/f2fs/status
> |grep SSR -C 2
> > buffer direct segments
> > IPU: 16 32 N/A
> > SSR: 0 0 0
> > LFS: 38 56 0
> >
> > root@kvm-xfstests:/mnt# dd if=/dev/zero of=./1 bs=32K count=1
> > oflag=direct conv=notrunc root@kvm-xfstests:/mnt# cat
> > /sys/kernel/debug/f2fs/status |grep SSR -C 2
> > buffer direct segments
> > IPU: 16 40 N/A
> > SSR: 0 0 0
> > LFS: 38 56 0
> >
> > root@kvm-xfstests:/mnt# dd if=/dev/zero of=./1 bs=32K count=2
> > oflag=direct conv=notrunc root@kvm-xfstests:/mnt# cat
> > /sys/kernel/debug/f2fs/status |grep SSR -C 2
> > buffer direct segments
> > IPU: 16 48 N/A
> > SSR: 0 0 0
> > LFS: 41 64 0
> >
> >
> >>
> >>> } else {
> >>> if (create) {
> >>> if (unlikely(f2fs_cp_error(sbi))) { diff
> >>> --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index
> >>> bf2e73e46304..a7da836ca64f 100644
> >>> --- a/fs/f2fs/f2fs.h
> >>> +++ b/fs/f2fs/f2fs.h
> >>> @@ -3785,12 +3785,12 @@ static inline struct f2fs_stat_info
> *F2FS_STAT(struct f2fs_sb_info *sbi)
> >>> else
> \
> >>>
> ((sbi)->block_count[1][(curseg)->alloc_type]++); \
> >>> } while (0)
> >>> -#define stat_inc_inplace_blocks(sbi, direct_io)
> \
> >>> +#define stat_add_inplace_blocks(sbi, count, direct_io)
> \
> >>> do
> { \
> >>> if (direct_io)
> \
> >>> - (atomic_inc(&(sbi)->inplace_count[0]));
> \
> >>> + (atomic_add(count, &(sbi)->inplace_count[0]));
> >>> + \
> >>> else
> \
> >>> - (atomic_inc(&(sbi)->inplace_count[1]));
> \
> >>> + (atomic_add(count, &(sbi)->inplace_count[1]));
> >>> + \
> >>
> >> If count always be one, we can just keep to use atomic_inc() here?
> >>
> > I suggest not, we may use this in later patch, not ready for now.
>
> I don't thinks this is the right way, why not including above change in your
> later patch?
>
> Thanks,
>
> >
> >> Thanks,
> >>
> >>> } while (0)
> >>> #define stat_update_max_atomic_write(inode)
> \
> >>> do
> { \
> >>> @@ -3877,7 +3877,7 @@ void f2fs_update_sit_info(struct f2fs_sb_info
> *sbi);
> >>> #define stat_inc_meta_count(sbi, blkaddr) do { } while (0)
> >>> #define stat_inc_seg_type(sbi, curseg) do { }
> while (0)
> >>> #define stat_inc_block_count(sbi, curseg, direct_io) do { }
> while (0)
> >>> -#define stat_inc_inplace_blocks(sbi, direct_io) do { } while
> (0)
> >>> +#define stat_add_inplace_blocks(sbi, count, direct_io) do { }
> while (0)
> >>> #define stat_inc_seg_count(sbi, type, gc_type) do { }
> while (0)
> >>> #define stat_inc_tot_blk_count(si, blks) do { } while (0)
> >>> #define stat_inc_data_blk_count(sbi, blks, gc_type) do { } while
> >>> (0) diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index
> >>> ded744e880d0..c542c4b687ca 100644
> >>> --- a/fs/f2fs/segment.c
> >>> +++ b/fs/f2fs/segment.c
> >>> @@ -3611,7 +3611,7 @@ int f2fs_inplace_write_data(struct f2fs_io_info
> *fio)
> >>> goto drop_bio;
> >>> }
> >>>
> >>> - stat_inc_inplace_blocks(fio->sbi, false);
> >>> + stat_add_inplace_blocks(sbi, 1, false);
> >>>
> >>> if (fio->bio && !(SM_I(sbi)->ipu_policy & (1 <<
> F2FS_IPU_NOCACHE)))
> >>> err = f2fs_merge_page_bio(fio);
> >>>
> >>
> >>
> >> _______________________________________________
> >> Linux-f2fs-devel mailing list
> >> [email protected]
> >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
