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

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

[Jia Zhu]

Commit 7bc0f46d418a ("f2fs: fix out-place-update DIO write")
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;
		}

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)) {
 		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) {
 			if (unlikely(f2fs_cp_error(sbi))) {
 				err = -EIO;
 				goto sync_out;
-- 
2.10.1

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

[Chao Yu]

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

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

[Jia Zhu]

Previously, we added a parameter @map.m_may_create to trigger OPU
allocation and call f2fs_balance_fs() correctly.

But in get_more_blocks(), @create has been overwritten by below code.
So the function f2fs_map_blocks() will not allocate new block address
but directly go out. Meanwile,there are several functions calling
f2fs_map_blocks() directly and @map.m_may_create not initialized.
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;
	}

This patch fixes it.

Signed-off-by: Jia Zhu <zhujia13@huawei.com>
---
 fs/f2fs/data.c | 5 +++++
 fs/f2fs/file.c | 4 +++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index aa8843a..7226300 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1052,6 +1052,10 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
 	end = pgofs + maxblocks;
 
 	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;
@@ -1261,6 +1265,7 @@ bool f2fs_overwrite_io(struct inode *inode, loff_t pos, size_t len)
 	map.m_next_pgofs = NULL;
 	map.m_next_extent = NULL;
 	map.m_seg_type = NO_CHECK_TYPE;
+	map.m_may_create = false;
 	last_lblk = F2FS_BLK_ALIGN(pos + len);
 
 	while (map.m_lblk < last_lblk) {
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 3271830..ff82350 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2201,7 +2201,8 @@ static int f2fs_defragment_range(struct f2fs_sb_info *sbi,
 {
 	struct inode *inode = file_inode(filp);
 	struct f2fs_map_blocks map = { .m_next_extent = NULL,
-					.m_seg_type = NO_CHECK_TYPE };
+					.m_seg_type = NO_CHECK_TYPE ,
+					.m_may_create = false };
 	struct extent_info ei = {0, 0, 0};
 	pgoff_t pg_start, pg_end, next_pgofs;
 	unsigned int blk_per_seg = sbi->blocks_per_seg;
@@ -2935,6 +2936,7 @@ int f2fs_precache_extents(struct inode *inode)
 	map.m_next_pgofs = NULL;
 	map.m_next_extent = &m_next_extent;
 	map.m_seg_type = NO_CHECK_TYPE;
+	map.m_may_create = false;
 	end = F2FS_I_SB(inode)->max_file_blocks;
 
 	while (map.m_lblk < end) {
-- 
2.10.1

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

[Chao Yu]

On 2018-11-20 4:29, Jia Zhu wrote:
> Previously, we added a parameter @map.m_may_create to trigger OPU
> allocation and call f2fs_balance_fs() correctly.
> 
> But in get_more_blocks(), @create has been overwritten by below code.
> So the function f2fs_map_blocks() will not allocate new block address
> but directly go out. Meanwile,there are several functions calling
> f2fs_map_blocks() directly and @map.m_may_create not initialized.

Oh, I missed to check all f2fs_map_blocks structure referrers, sorry.

> 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;
> 	}
> 
> This patch fixes it.
> 
> Signed-off-by: Jia Zhu <zhujia13@huawei.com>> ---

It will be better to add simple change logs here to indicate how you modify your
patch comparing to previous one, please keep that rule for your next patch. ;)

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

>  fs/f2fs/data.c | 5 +++++
>  fs/f2fs/file.c | 4 +++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index aa8843a..7226300 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1052,6 +1052,10 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
>  	end = pgofs + maxblocks;
>  
>  	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;
> @@ -1261,6 +1265,7 @@ bool f2fs_overwrite_io(struct inode *inode, loff_t pos, size_t len)
>  	map.m_next_pgofs = NULL;
>  	map.m_next_extent = NULL;
>  	map.m_seg_type = NO_CHECK_TYPE;
> +	map.m_may_create = false;
>  	last_lblk = F2FS_BLK_ALIGN(pos + len);
>  
>  	while (map.m_lblk < last_lblk) {
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 3271830..ff82350 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2201,7 +2201,8 @@ static int f2fs_defragment_range(struct f2fs_sb_info *sbi,
>  {
>  	struct inode *inode = file_inode(filp);
>  	struct f2fs_map_blocks map = { .m_next_extent = NULL,
> -					.m_seg_type = NO_CHECK_TYPE };
> +					.m_seg_type = NO_CHECK_TYPE ,
> +					.m_may_create = false };
>  	struct extent_info ei = {0, 0, 0};
>  	pgoff_t pg_start, pg_end, next_pgofs;
>  	unsigned int blk_per_seg = sbi->blocks_per_seg;
> @@ -2935,6 +2936,7 @@ int f2fs_precache_extents(struct inode *inode)
>  	map.m_next_pgofs = NULL;
>  	map.m_next_extent = &m_next_extent;
>  	map.m_seg_type = NO_CHECK_TYPE;
> +	map.m_may_create = false;
>  	end = F2FS_I_SB(inode)->max_file_blocks;
>  
>  	while (map.m_lblk < end) {
> 

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

[Chao Yu]

Ping,

On 2018/11/20 19:58, Chao Yu wrote:
> On 2018-11-20 4:29, Jia Zhu wrote:
>> Previously, we added a parameter @map.m_may_create to trigger OPU
>> allocation and call f2fs_balance_fs() correctly.
>>
>> But in get_more_blocks(), @create has been overwritten by below code.
>> So the function f2fs_map_blocks() will not allocate new block address
>> but directly go out. Meanwile,there are several functions calling
>> f2fs_map_blocks() directly and @map.m_may_create not initialized.
> 
> Oh, I missed to check all f2fs_map_blocks structure referrers, sorry.
> 
>> 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;
>> 	}
>>
>> This patch fixes it.
>>
>> Signed-off-by: Jia Zhu <zhujia13@huawei.com>> ---
> 
> It will be better to add simple change logs here to indicate how you modify your
> patch comparing to previous one, please keep that rule for your next patch. ;)
> 
> Reviewed-by: Chao Yu <yuchao0@huawei.com>
> 
> Thanks,
> 
>>  fs/f2fs/data.c | 5 +++++
>>  fs/f2fs/file.c | 4 +++-
>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index aa8843a..7226300 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -1052,6 +1052,10 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
>>  	end = pgofs + maxblocks;
>>  
>>  	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;
>> @@ -1261,6 +1265,7 @@ bool f2fs_overwrite_io(struct inode *inode, loff_t pos, size_t len)
>>  	map.m_next_pgofs = NULL;
>>  	map.m_next_extent = NULL;
>>  	map.m_seg_type = NO_CHECK_TYPE;
>> +	map.m_may_create = false;
>>  	last_lblk = F2FS_BLK_ALIGN(pos + len);
>>  
>>  	while (map.m_lblk < last_lblk) {
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 3271830..ff82350 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -2201,7 +2201,8 @@ static int f2fs_defragment_range(struct f2fs_sb_info *sbi,
>>  {
>>  	struct inode *inode = file_inode(filp);
>>  	struct f2fs_map_blocks map = { .m_next_extent = NULL,
>> -					.m_seg_type = NO_CHECK_TYPE };
>> +					.m_seg_type = NO_CHECK_TYPE ,
>> +					.m_may_create = false };
>>  	struct extent_info ei = {0, 0, 0};
>>  	pgoff_t pg_start, pg_end, next_pgofs;
>>  	unsigned int blk_per_seg = sbi->blocks_per_seg;
>> @@ -2935,6 +2936,7 @@ int f2fs_precache_extents(struct inode *inode)
>>  	map.m_next_pgofs = NULL;
>>  	map.m_next_extent = &m_next_extent;
>>  	map.m_seg_type = NO_CHECK_TYPE;
>> +	map.m_may_create = false;
>>  	end = F2FS_I_SB(inode)->max_file_blocks;
>>  
>>  	while (map.m_lblk < end) {
>>
> 
> .
>