RE:(2) (2) [PATCH] f2fs: avoid unused block when dio write in LFS mode

[Daejun Park]

Hi Chao Yu,

> 
>--------- Original Message ---------
>Sender : Chao Yu <chao@xxxxxxxxxx>
>Date : 2024-08-16 10:56 (GMT+9)
>Title : Re: (2) [PATCH] f2fs: avoid unused block when dio write in LFS mode
> 
>On 2024/8/16 8:17, Daejun Park wrote:
>> Hi Chao Yu,
>>> 
>>> --------- Original Message ---------
>>> Sender : Chao Yu <chao@xxxxxxxxxx>
>>> Date : 2024-08-14 18:50 (GMT+9)
>>> Title : Re: [PATCH] f2fs: avoid unused block when dio write in LFS mode
>>> 
>>> On 2024/8/1 15:47, Daejun Park wrote:
>>>> This patch addresses the problem that when using LFS mode, unused blocks
>>>> may occur in f2fs_map_blocks() during block allocation for dio writes.
>>>>
>>>> If a new section is allocated during block allocation, it will not be
>>>> included in the map struct by map_is_mergeable() if the LBA of the
>>>
>>> I didn't get it, why below condition in map_is_mergeable() can not catch this
>>> case? Can you please explain more?
>>>
>>>         if (map->m_pblk != NEW_ADDR && blkaddr == (map->m_pblk + ofs))
>>>                 return true;
>>>
>>
>> Thank you for your review.
>> map_is_mergeable() returns true when the last block in the section is merged.
>> The problem is the next block allocation, which happens consecutively.
>> Since it will be allocated a new section, its block will be the first
>> block in the section.
>> If the newly allocated section is not contiguous with the previous section,
>> map_is_mergeable() will return false.
>> So the block is allocated but unused.
>> However, it is not freed, so the block is unusable.
>> If my explanation was not clear enough, please feel free to ask questions.
>
>It's clear to me now, thanks for your explanation.
>
>>
>> Thanks
>> Daejun
>>
>>> Thanks,
>>>
>>>> allocated block is not contiguous. However, the block already allocated
>>>> in this process will remain unused due to the LFS mode.
>>>>
>>>> This patch avoids the possibility of unused blocks by escaping
>>>> f2fs_map_blocks() when allocating the last block in a section.
>>>>
>>>> Signed-off-by: Daejun Park <daejun7.park@xxxxxxxxxxx>
>>>> ---
>>>>    fs/f2fs/data.c 13 +++++++++++++
>>>>    1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>> index b6dcb3bcaef7..b27a3f448f32 100644
>>>> --- a/fs/f2fs/data.c
>>>> +++ b/fs/f2fs/data.c
>>>> @@ -1711,6 +1711,19 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map, int flag)
>>>>                     dn.ofs_in_node = end_offset;
>>>>             }
>>>> 
>>>> +        if (flag == F2FS_GET_BLOCK_DIO && f2fs_lfs_mode(sbi)) {
>
>The condition should check map->m_may_create as well, otherwise f2fs_map_blocks()
>from read path will break here?

I will add checking map->m_may_create.

>
>>>> +                int segno = GET_SEGNO(sbi, blkaddr);
>>>> +                bool last_seg, last_blk;
>>>> +
>>>> +                last_seg = !((segno + 1) % SEGS_PER_SEC(sbi));
>
>Should consider the case that segno #(SEGS_PER_SEC - 1) is not valid?
>e.g. SEGS_PER_SEC is 4, CAP_SEGS_PER_SEC is 2?
>
>>>> +                last_blk = (f2fs_usable_blks_in_seg(sbi, segno) - 1) ==
>>>> +                                GET_BLKOFF_FROM_SEG0(sbi, blkaddr);
>
>if (GET_SEGOFF_FROM_SEG0() % BLKS_PER_SEC() == CAP_BLKS_PER_SEC() - 1)
>       goto sync_out;

I will use the suggested checking code.

Thanks,
Daejun

>
>Thanks,
>
>>>> +
>>>> +                /* LBA of the next block to be allocated may not be contiguous. */
>>>> +                if (last_seg && last_blk)
>>>> +                        goto sync_out;
>>>> +        }
>>>> +
>>>>             if (pgofs >= end)
>>>>                     goto sync_out;
>>>>             else if (dn.ofs_in_node < end_offset)