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

[Chao Yu <chao@kernel.org>]

On 2021/10/16 16:01, fengnan chang wrote:
> Chao Yu <chao@kernel.org> 于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 <changfengnan@vivo.com>
>>> ---
>>>    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.

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