Re: [PATCH] f2fs: fix m_may_create to make OPU DIO write correctly

[Chao Yu <yuchao0@huawei.com>]

Hi Jia,

On 2018/11/17 3:41, Jia Zhu wrote:
> Commit 7bc0f46d418a ("f2fs: fix out-place-update DIO write")

Commit 7bc0f46d418a was just in dev-test branch, so the commit id will
change after being merged into linus' tree.

> added a parameter map.m_may_create to replace @create for
> triggering OPU allocation correctly.
> 
> But we find it is still in-place-update when used AndroBench
> to test this feature.In f2fs_map_blocks(), @create has been
> overwritten by the code below. So the function can not allocate
> new block address and directly go out.
> code:
> create = dio->op == REQ_OP_WRITE;
> if (dio->flags & DIO_SKIP_HOLES) {
> 	if (fs_startblk <= ((i_size_read(dio->inode) -1) >>i_blkbits))
> 		create = 0;
> 		}

For DIO_SKIP_HOLES flag's semantics, it needs local filesystem to skip
allocating physical data block if we encounter hole inside filesize during
DIO write.

How about keeping this semantics since we are still using it in
f2fs_direct_IO().

	err = __blockdev_direct_IO(iocb, inode, inode->i_sb->s_bdev,
			iter, rw == WRITE ? get_data_block_dio_write :
			get_data_block_dio, NULL, f2fs_dio_submit_bio,
			DIO_LOCKING | DIO_SKIP_HOLES);
				      ^^^^^^^^^^^^^^^

> 
> This patch use @map.m_may_create to replace @create to avoid
> this problem.
> 
> Signed-off-by: Jia Zhu <zhujia13@huawei.com>
> ---
>  fs/f2fs/data.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 8780f3d..9b61cba 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1034,7 +1034,7 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
>  	pgofs =	(pgoff_t)map->m_lblk;
>  	end = pgofs + maxblocks;
>  
> -	if (!create && f2fs_lookup_extent_cache(inode, pgofs, &ei)) {
> +	if (!map->m_may_create && f2fs_lookup_extent_cache(inode, pgofs, &ei)) {

So here, how about just skip returning existed block address during aligned
DIO writing in LFS-mode?

	if (!create && f2fs_lookup_extent_cache(inode, pgofs, &ei)) {
		if (test_opt(sbi, LFS) && flag == F2FS_GET_BLOCK_DIO &&
							map->m_may_create)
			goto next_dnode;

>  		map->m_pblk = ei.blk + pgofs - ei.fofs;
>  		map->m_len = min((pgoff_t)maxblocks, ei.fofs + ei.len - pgofs);
>  		map->m_flags = F2FS_MAP_MAPPED;
> @@ -1093,7 +1093,7 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
>  				set_inode_flag(inode, FI_APPEND_WRITE);
>  		}
>  	} else {
> -		if (create) {
> +		if (map->m_may_create) {

We don't need to change this condition due to DIO_SKIP_HOLES, right?

Thanks,

>  			if (unlikely(f2fs_cp_error(sbi))) {
>  				err = -EIO;
>  				goto sync_out;
>