Re: [f2fs-dev] [PATCH 1/2 v4] f2fs: support large folio for immutable non-compressed case

[Jaegeuk Kim <jaegeuk@kernel.org>]

On 01/03, Nanzhe Zhao wrote:
> Dear Kim:
> Thanks for your quick reply!
> 
> I applied the two bug fixes on my local branch and found that
> I still couldn't pass my test of generating and reading a heavily
> fragmented file.
> 
> The root cause is that current code will treat hole blocks as mapped
> blocks as well and mistakenly increment read_pages_pending, resulting
> task hung in readahead.
> 
> Inside f2fs_map_blocks():
> 
> /* DIO READ and hole case, should not map the blocks. */
> if (!(flag == F2FS_GET_BLOCK_DIO && is_hole && !map->m_may_create))
> 	map->m_flags |= F2FS_MAP_MAPPED;
> 
> it will have map->m_flags marked with F2FS_MAP_MAPPED in non-DIO and
> no blocks creation context for NULL_ADDR and NEW_ADDR, except for
> holes mapped to an unallocated dnode.
> 
> Personally, I think a better fix is to add a helper function
> f2fs_block_needs_zeroing(). The condition could be: return true if the
> current blkaddr is NULL_ADDR or NEW_ADDR.
> 
> Then we can reverse the order of the checks under the got_it: label:
> first `if (f2fs_block_needs_zeroing()) ...`, and then `else if
> (map->m_flags & F2FS_MAP_MAPPED)`, while keeping all the logic inside
> those statements unchanged.
> 
> For the parameters of f2fs_block_needs_zeroing(), I think we can pass
> `struct f2fs_map_blocks` directly, because it already contains all the
> information we need. Also, if we later want to support batching
> contiguous physical block mappings and bio additions inside the loop,
> this signature should be more extensible.
> 
> If you think this approach makes sense, I can send a patch to fix all
> three bugs. Thank you.

I think that's feasbile. Could you please post a patch to discuss further?

Thanks,

> 
> Best regards,
> Nanzhe Zhao