Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 3 Feb 2022 22:12:49 -0800
From:   Jaegeuk Kim <jaegeuk@kernel.org>
To:     Chao Yu <chao@kernel.org>
Cc:     linux-f2fs-devel@lists.sourceforge.net,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] f2fs: introduce F2FS_IPU_DEFRAG to allow file
 defragment in IPU mode
Message-ID: <YfzD4aTbDW2xKlO9@google.com>
References: <20220204010539.42672-1-chao@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220204010539.42672-1-chao@kernel.org>
Precedence: bulk

On 02/04, Chao Yu wrote:
> Once IPU policy is enabled in some cases:
> a) f2fs forces to use F2FS_IPU_FORCE in a small-sized volume
> b) user configures IPU policy via sysfs
> 
> Then we may fail to defragment file via ioctl due to IPU policy check,
> it doesn't make sense, let's introduce a new IPU policy to allow OPU
> during file defragmentation, by default use IPU_DEFRAG instead of
> IPU_FORCE in small-sized volume for android device.
> 
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
> v2:
> - handle F2FS_IPU_DEFRAG before F2FS_IPU_FORCE in check_inplace_update_policy().
>  Documentation/ABI/testing/sysfs-fs-f2fs |  3 ++-
>  fs/f2fs/data.c                          | 18 +++++++++++++-----
>  fs/f2fs/f2fs.h                          |  3 ++-
>  fs/f2fs/file.c                          | 18 +++++++++++-------
>  fs/f2fs/segment.h                       |  8 ++++----
>  fs/f2fs/super.c                         |  2 +-
>  6 files changed, 33 insertions(+), 19 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-fs-f2fs b/Documentation/ABI/testing/sysfs-fs-f2fs
> index ce8103f522cb..d8abb674ee86 100644
> --- a/Documentation/ABI/testing/sysfs-fs-f2fs
> +++ b/Documentation/ABI/testing/sysfs-fs-f2fs
> @@ -55,8 +55,9 @@ Description:	Controls the in-place-update policy.
>  		0x04  F2FS_IPU_UTIL
>  		0x08  F2FS_IPU_SSR_UTIL
>  		0x10  F2FS_IPU_FSYNC
> -		0x20  F2FS_IPU_ASYNC,
> +		0x20  F2FS_IPU_ASYNC
>  		0x40  F2FS_IPU_NOCACHE
> +		0x80  F2FS_IPU_DEFRAG

How about F2FS_IPU_HONOR_OPU_WRITE?

>  		====  =================
>  
>  		Refer segment.h for details.
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index b3c152de4bba..3c0f001e3fe1 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2460,6 +2460,9 @@ static inline bool check_inplace_update_policy(struct inode *inode,
>  	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>  	unsigned int policy = SM_I(sbi)->ipu_policy;
>  
> +	if (policy & (0x1 << F2FS_IPU_DEFRAG) &&
> +			is_inode_flag_set(inode, FI_DEFRAG_PROCESS))
> +		return false;
>  	if (policy & (0x1 << F2FS_IPU_FORCE))
>  		return true;
>  	if (policy & (0x1 << F2FS_IPU_SSR) && f2fs_need_SSR(sbi))
> @@ -2530,6 +2533,9 @@ bool f2fs_should_update_outplace(struct inode *inode, struct f2fs_io_info *fio)
>  	if (is_inode_flag_set(inode, FI_ALIGNED_WRITE))
>  		return true;
>  
> +	if (is_inode_flag_set(inode, FI_DEFRAG_PROCESS))
> +		return true;
> +
>  	if (fio) {
>  		if (page_private_gcing(fio->page))
>  			return true;
> @@ -3154,8 +3160,8 @@ static int __f2fs_write_data_pages(struct address_space *mapping,
>  			f2fs_available_free_memory(sbi, DIRTY_DENTS))
>  		goto skip_write;
>  
> -	/* skip writing during file defragment */
> -	if (is_inode_flag_set(inode, FI_DO_DEFRAG))
> +	/* skip writing in file defragment preparing stage */
> +	if (is_inode_flag_set(inode, FI_DEFRAG_PREPARE))
>  		goto skip_write;
>  
>  	trace_f2fs_writepages(mapping->host, wbc, DATA);
> @@ -3729,6 +3735,7 @@ static int f2fs_migrate_blocks(struct inode *inode, block_t start_blk,
>  	filemap_invalidate_lock(inode->i_mapping);
>  
>  	set_inode_flag(inode, FI_ALIGNED_WRITE);
> +	set_inode_flag(inode, FI_DEFRAG_PROCESS);
>  
>  	for (; secidx < end_sec; secidx++) {
>  		f2fs_down_write(&sbi->pin_sem);
> @@ -3737,7 +3744,7 @@ static int f2fs_migrate_blocks(struct inode *inode, block_t start_blk,
>  		f2fs_allocate_new_section(sbi, CURSEG_COLD_DATA_PINNED, false);
>  		f2fs_unlock_op(sbi);
>  
> -		set_inode_flag(inode, FI_DO_DEFRAG);
> +		set_inode_flag(inode, FI_DEFRAG_PREPARE);
>  
>  		for (blkofs = 0; blkofs < blk_per_sec; blkofs++) {
>  			struct page *page;
> @@ -3754,7 +3761,7 @@ static int f2fs_migrate_blocks(struct inode *inode, block_t start_blk,
>  			f2fs_put_page(page, 1);
>  		}
>  
> -		clear_inode_flag(inode, FI_DO_DEFRAG);
> +		clear_inode_flag(inode, FI_DEFRAG_PREPARE);
>  
>  		ret = filemap_fdatawrite(inode->i_mapping);
>  
> @@ -3765,7 +3772,8 @@ static int f2fs_migrate_blocks(struct inode *inode, block_t start_blk,
>  	}
>  
>  done:
> -	clear_inode_flag(inode, FI_DO_DEFRAG);
> +	clear_inode_flag(inode, FI_DEFRAG_PREPARE);
> +	clear_inode_flag(inode, FI_DEFRAG_PROCESS);
>  	clear_inode_flag(inode, FI_ALIGNED_WRITE);
>  
>  	filemap_invalidate_unlock(inode->i_mapping);
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 6ddb98ff0b7c..5af415208488 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -737,7 +737,8 @@ enum {
>  	FI_DROP_CACHE,		/* drop dirty page cache */
>  	FI_DATA_EXIST,		/* indicate data exists */
>  	FI_INLINE_DOTS,		/* indicate inline dot dentries */
> -	FI_DO_DEFRAG,		/* indicate defragment is running */
> +	FI_DEFRAG_PREPARE,	/* indicate defragment is preparing */

This is used for f2fs_migrate_blocks for swap file. Can we rename to
FI_SKIP_WRITES that makes skipping writeback pages?

> +	FI_DEFRAG_PROCESS,	/* indicate defragment is processing */

How about FI_OPU_WRITE?

>  	FI_DIRTY_FILE,		/* indicate regular/symlink has dirty pages */
>  	FI_PREALLOCATED_ALL,	/* all blocks for write were preallocated */
>  	FI_HOT_DATA,		/* indicate file is hot */
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 6ccdd6e347e2..3a573125002b 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2559,10 +2559,6 @@ static int f2fs_defragment_range(struct f2fs_sb_info *sbi,
>  	bool fragmented = false;
>  	int err;
>  
> -	/* if in-place-update policy is enabled, don't waste time here */
> -	if (f2fs_should_update_inplace(inode, NULL))
> -		return -EINVAL;
> -
>  	pg_start = range->start >> PAGE_SHIFT;
>  	pg_end = (range->start + range->len) >> PAGE_SHIFT;
>  
> @@ -2570,6 +2566,13 @@ static int f2fs_defragment_range(struct f2fs_sb_info *sbi,
>  
>  	inode_lock(inode);
>  
> +	/* if in-place-update policy is enabled, don't waste time here */
> +	set_inode_flag(inode, FI_DEFRAG_PROCESS);
> +	if (f2fs_should_update_inplace(inode, NULL)) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
>  	/* writeback all dirty pages in the range */
>  	err = filemap_write_and_wait_range(inode->i_mapping, range->start,
>  						range->start + range->len - 1);
> @@ -2651,7 +2654,7 @@ static int f2fs_defragment_range(struct f2fs_sb_info *sbi,
>  			goto check;
>  		}
>  
> -		set_inode_flag(inode, FI_DO_DEFRAG);
> +		set_inode_flag(inode, FI_DEFRAG_PREPARE);
>  
>  		idx = map.m_lblk;
>  		while (idx < map.m_lblk + map.m_len && cnt < blk_per_seg) {
> @@ -2676,15 +2679,16 @@ static int f2fs_defragment_range(struct f2fs_sb_info *sbi,
>  		if (map.m_lblk < pg_end && cnt < blk_per_seg)
>  			goto do_map;
>  
> -		clear_inode_flag(inode, FI_DO_DEFRAG);
> +		clear_inode_flag(inode, FI_DEFRAG_PREPARE);
>  
>  		err = filemap_fdatawrite(inode->i_mapping);
>  		if (err)
>  			goto out;
>  	}
>  clear_out:
> -	clear_inode_flag(inode, FI_DO_DEFRAG);
> +	clear_inode_flag(inode, FI_DEFRAG_PREPARE);
>  out:
> +	clear_inode_flag(inode, FI_DEFRAG_PROCESS);
>  	inode_unlock(inode);
>  	if (!err)
>  		range->len = (u64)total << PAGE_SHIFT;
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index 0291cd55cf09..a9373861ab1c 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -651,7 +651,8 @@ static inline int utilization(struct f2fs_sb_info *sbi)
>   *                     pages over min_fsync_blocks. (=default option)
>   * F2FS_IPU_ASYNC - do IPU given by asynchronous write requests.
>   * F2FS_IPU_NOCACHE - disable IPU bio cache.
> - * F2FS_IPUT_DISABLE - disable IPU. (=default option in LFS mode)
> + * F2FS_IPU_DEFRAG - allow OPU during file defragmentation.
> + * F2FS_IPU_DISABLE - disable IPU. (=default option in LFS mode)
>   */
>  #define DEF_MIN_IPU_UTIL	70
>  #define DEF_MIN_FSYNC_BLOCKS	8
> @@ -667,6 +668,7 @@ enum {
>  	F2FS_IPU_FSYNC,
>  	F2FS_IPU_ASYNC,
>  	F2FS_IPU_NOCACHE,
> +	F2FS_IPU_DEFRAG,
>  };
>  
>  static inline unsigned int curseg_segno(struct f2fs_sb_info *sbi,
> @@ -675,9 +677,7 @@ static inline unsigned int curseg_segno(struct f2fs_sb_info *sbi,
>  	struct curseg_info *curseg = CURSEG_I(sbi, type);
>  	return curseg->segno;
>  }
> -
> -static inline unsigned char curseg_alloc_type(struct f2fs_sb_info *sbi,
> -		int type)
> +static inline unsigned char curseg_alloc_type(struct f2fs_sb_info *sbi, int type)

Don't do this.

>  {
>  	struct curseg_info *curseg = CURSEG_I(sbi, type);
>  	return curseg->alloc_type;
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 9af6c20532ec..cc0429279b4c 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -3957,7 +3957,7 @@ static void f2fs_tuning_parameters(struct f2fs_sb_info *sbi)
>  		F2FS_OPTION(sbi).alloc_mode = ALLOC_MODE_REUSE;
>  		if (f2fs_block_unit_discard(sbi))
>  			sm_i->dcc_info->discard_granularity = 1;
> -		sm_i->ipu_policy = 1 << F2FS_IPU_FORCE;

No, I think we should have both.

> +		sm_i->ipu_policy = 1 << F2FS_IPU_DEFRAG;
>  	}
>  
>  	sbi->readdir_ra = 1;
> -- 
> 2.32.0