Re: [f2fs-dev] [PATCH v2 2/2] f2fs: fix missing inplace count in overwrite with direct io

[Chao Yu <chao@kernel.org>]

On 2021/9/18 14:46, 常凤楠 wrote:
> 
> 
>> -----Original Message-----
>> From: changfengnan@vivo.com <changfengnan@vivo.com> On Behalf Of
>> Chao Yu
>> Sent: Friday, September 17, 2021 9:00 PM
>> To: 常凤楠 <changfengnan@vivo.com>; jaegeuk@kernel.org
>> Cc: linux-f2fs-devel@lists.sourceforge.net
>> Subject: Re: [PATCH v2 2/2] f2fs: fix missing inplace count in overwrite with
>> direct io
>>
>> On 2021/9/17 18:19, 常凤楠 wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: 常凤楠
>>>> Sent: Thursday, September 16, 2021 8:46 PM
>>>> To: Chao Yu <chao@kernel.org>; jaegeuk@kernel.org
>>>> Cc: linux-f2fs-devel@lists.sourceforge.net
>>>> Subject: RE: [PATCH v2 2/2] f2fs: fix missing inplace count in
>>>> overwrite with direct io
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: changfengnan@vivo.com <changfengnan@vivo.com> On Behalf Of
>>>> Chao
>>>>> Yu
>>>>> Sent: Thursday, September 16, 2021 8:10 PM
>>>>> To: 常凤楠 <changfengnan@vivo.com>; jaegeuk@kernel.org
>>>>> Cc: linux-f2fs-devel@lists.sourceforge.net
>>>>> Subject: Re: [PATCH v2 2/2] f2fs: fix missing inplace count in
>>>>> overwrite with direct io
>>>>>
>>>>> On 2021/9/16 19:30, 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 <changfengnan@vivo.com>
>>>>>> ---
>>>>>>     fs/f2fs/data.c    | 7 ++++++-
>>>>>>     fs/f2fs/f2fs.h    | 8 ++++----
>>>>>>     fs/f2fs/segment.c | 2 +-
>>>>>>     3 files changed, 11 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
>>>>>> c1490b9a1345..0c5728d63c33 100644
>>>>>> --- a/fs/f2fs/data.c
>>>>>> +++ b/fs/f2fs/data.c
>>>>>> @@ -1491,6 +1491,9 @@ int f2fs_map_blocks(struct inode *inode,
>>>>>> struct
>>>>> f2fs_map_blocks *map,
>>>>>>     		if (flag == F2FS_GET_BLOCK_DIO)
>>>>>>     			f2fs_wait_on_block_writeback_range(inode,
>>>>>>     						map->m_pblk, map->m_len);
>>>>>> +		if (!f2fs_lfs_mode(sbi) && flag == F2FS_GET_BLOCK_DIO &&
>>>>>> +				map->m_may_create)
>>>>>> +			stat_add_inplace_blocks(sbi, map->m_len, true);
>>>>>>     		goto out;
>>>>>>     	}
>>>>>>
>>>>>> @@ -1553,7 +1556,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 (!create && !f2fs_lfs_mode(sbi) && flag ==
>>>>> F2FS_GET_BLOCK_DIO &&
>>>>>> +				map->m_may_create)
>>>>>
>>>>> Why not
>>>>>
>>>>> } else if {!f2fs_lfs_mode(sbi) && flag == F2FS_GET_BLOCK_DIO &&
>>>>> 					map->m_may_create)
>>>>>
>>>>
>>>> You are right, no need to check create .
>>>>
>>> There is a problem here, if remove create check, when create file and
>>> write with direct io, It will count in LFS and IPU both, because
>>> preallocate block addr first. So, We still need check create.
>>> Am I right?
>>
>> Could you please check below case w/ your original patch:
>>
>> xfs_io -f file -c "pwrite 0 8k" -c "fsync"
>> xfs_io file -c "fpunch 0 4k"
>> xfs_io  -c "open -d file" -c "pwrite -b 4k 0 8k"
>>
>> It accounts on both IPU and LFS stats.
> 
> My origin patch is need check create:
> @@ -1553,7 +1556,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 (!create && !f2fs_lfs_mode(sbi) && flag == F2FS_GET_BLOCK_DIO &&
> +				map->m_may_create)
> +			stat_add_inplace_blocks(sbi, 1, true);
> 
> And below case looks correct, So I think check create is necessary.
> 
> root@kvm-xfstests:/mnt/test# cat /sys/kernel/debug/f2fs/status |grep SSR -C 3
> 
>             buffer     direct   segments
> IPU:            0          0        N/A
> SSR:            0          0          0
> LFS:          542          0          1
> 
> BDF: 99, avg. vblocks: 488
> root@kvm-xfstests:/mnt/test# xfs_io -f file -c "pwrite 0 8k" -c "fsync"
> wrote 8192/8192 bytes at offset 0
> 8 KiB, 2 ops; 0.0078 sec (1014.070 KiB/sec and 253.5176 ops/sec)
> root@kvm-xfstests:/mnt/test# cat /sys/kernel/debug/f2fs/status |grep SSR -C 3
> 
>             buffer     direct   segments
> IPU:            0          0        N/A
> SSR:            0          0          0
> LFS:          545          0          1
> 
> BDF: 99, avg. vblocks: 488
> root@kvm-xfstests:/mnt/test# xfs_io file -c "fpunch 0 4k"
> root@kvm-xfstests:/mnt/test# cat /sys/kernel/debug/f2fs/status |grep SSR -C 3
> 
>             buffer     direct   segments
> IPU:            0          0        N/A
> SSR:            0          0          0
> LFS:          545          0          1
> 
> BDF: 99, avg. vblocks: 488
> root@kvm-xfstests:/mnt/test# xfs_io  -c "open -d file" -c "pwrite -b 4k 0 8k"
> wrote 8192/8192 bytes at offset 0
> 8 KiB, 2 ops; 0.0322 sec (248.185 KiB/sec and 62.0463 ops/sec)
> root@kvm-xfstests:/mnt/test# cat /sys/kernel/debug/f2fs/status |grep SSR -C 3
> 
>             buffer     direct   segments
> IPU:            0          2        N/A
> SSR:            0          0          0
> LFS:          545          1          1

Shouldn't this be IPU: 1 and LFS: 1? due to [0, 4k] was a hole, and [4k, 8k] mapped
to a valid blkaddr.

Thanks,

> 
> BDF: 99, avg. vblocks: 488
> 
> 
>>
>> Thanks,
>>
>>>
>>> Thanks.
>>>
>>>> Thanks.
>>>>> Thanks,
>>>>>
>>>>>> +			stat_add_inplace_blocks(sbi, 1, true);
>>>>>>     	} else {
>>>>>>     		if (create) {
>>>>>>     			if (unlikely(f2fs_cp_error(sbi))) { diff --git
>>>>>> a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 3d4ee444db27..2d81e9f0a0ee
>>>>>> 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]));	\
>>>>>>     	} 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)		do { } while (0)
>>>>>> -#define stat_inc_inplace_blocks(sbi)			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
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel