linux-kernel - Re: [PATCH v4 4/4] f2fs: Optimize f2fs_truncate_data_blocks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <8a8dbb83-134b-4545-9b37-9c0b5b2be972@kernel.org>
Date: Tue, 14 Jan 2025 12:28:52 +0800
From: Chao Yu <chao@...nel.org>
To: Yi Sun <yi.sun@...soc.com>, jaegeuk@...nel.org
Cc: chao@...nel.org, sunyibuaa@...il.com,
 linux-f2fs-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
 niuzhiguo84@...il.com, Hao_hao.Wang@...soc.com, ke.wang@...soc.com
Subject: Re: [PATCH v4 4/4] f2fs: Optimize f2fs_truncate_data_blocks_range()

On 12/23/24 16:10, Yi Sun wrote:
> Function f2fs_invalidate_blocks() can process continuous
> blocks at a time, so f2fs_truncate_data_blocks_range() is
> optimized to use the new functionality of
> f2fs_invalidate_blocks().
> 
> Signed-off-by: Yi Sun <yi.sun@...soc.com>
> ---
>   fs/f2fs/file.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 68 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 81764b10840b..9980d17ef9f5 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -612,6 +612,15 @@ static int f2fs_file_open(struct inode *inode, struct file *filp)
>   	return finish_preallocate_blocks(inode);
>   }
>   
> +static bool check_curr_block_is_consecutive(struct f2fs_sb_info *sbi,
> +					block_t curr, block_t end)

static inline bool is_consecutive_blkaddrs(block_t cur, block_t end)
{
	return cur == end || cur == end + 1;
}

But maybe we don't need to add this function, see below comments.

> +{
> +	if (curr - end == 1 || curr == end)
> +		return true;
> +	else
> +		return false;
> +}
> +
>   void f2fs_truncate_data_blocks_range(struct dnode_of_data *dn, int count)
>   {
>   	struct f2fs_sb_info *sbi = F2FS_I_SB(dn->inode);
> @@ -621,8 +630,27 @@ void f2fs_truncate_data_blocks_range(struct dnode_of_data *dn, int count)
>   	int cluster_index = 0, valid_blocks = 0;
>   	int cluster_size = F2FS_I(dn->inode)->i_cluster_size;
>   	bool released = !atomic_read(&F2FS_I(dn->inode)->i_compr_blocks);
> +	/*
> +	 * Temporary record location.
> +	 * When the current  @blkaddr and @blkaddr_end can be processed
> +	 * together, update the value of @blkaddr_end.
> +	 * When it is detected that current @blkaddr is not continues with
> +	 * @blkaddr_end, it is necessary to process continues blocks

I prefer not adding these comments into function, what about describing the 
details in commit message, instead, thoughts?

> +	 * range [blkaddr_start, blkaddr_end].
> +	 */
> +	block_t blkaddr_start, blkaddr_end;
> +	/*.
> +	 * To avoid processing various invalid data blocks.
> +	 * Because @blkaddr_start and @blkaddr_end may be assigned
> +	 * NULL_ADDR or invalid data blocks, @last_valid is used to
> +	 * record this situation.
> +	 */

Ditto,

What about using blkstart & blklen to record last consecutive block addresses, 
and using blklen to identify whether we need to call f2fs_invalidate_blocks() or
not?

> +	bool last_valid = false;
> +	/* Process the last @blkaddr separately? */
> +	bool last_one = true;
>   
>   	addr = get_dnode_addr(dn->inode, dn->node_page) + ofs;
> +	blkaddr_start = blkaddr_end = le32_to_cpu(*addr);
>   
>   	/* Assumption: truncation starts with cluster */
>   	for (; count > 0; count--, addr++, dn->ofs_in_node++, cluster_index++) {
> @@ -638,24 +666,60 @@ void f2fs_truncate_data_blocks_range(struct dnode_of_data *dn, int count)
>   		}
>   
>   		if (blkaddr == NULL_ADDR)
> -			continue;
> +			goto next;
>   
>   		f2fs_set_data_blkaddr(dn, NULL_ADDR);
>   
>   		if (__is_valid_data_blkaddr(blkaddr)) {
>   			if (time_to_inject(sbi, FAULT_BLKADDR_CONSISTENCE))
> -				continue;
> +				goto next;
>   			if (!f2fs_is_valid_blkaddr_raw(sbi, blkaddr,
>   						DATA_GENERIC_ENHANCE))
> -				continue;
> +				goto next;
>   			if (compressed_cluster)
>   				valid_blocks++;
>   		}
>   
> -		f2fs_invalidate_blocks(sbi, blkaddr, 1);

How about this? Can you change based on it?

		if (blkstart + blklen == blkaddr) {
			blklen++;
		} else {
			f2fs_invalidate_blocks(sbi, blkstart, blklen);
			blkstart = blkaddr;
			blklen = 1;
		}

> +
> +		if (check_curr_block_is_consecutive(sbi, blkaddr, blkaddr_end)) {
> +			/*
> +			 * The current block @blkaddr is continuous with
> +			 * @blkaddr_end, so @blkaddr_end is updated.
> +			 * And the f2fs_invalidate_blocks() is skipped
> +			 * until @blkaddr that cannot be processed
> +			 * together is encountered.
> +			 */
> +			blkaddr_end = blkaddr;
> +			if (count == 1)
> +				last_one = false;
> +			else
> +				goto skip_invalid;
> +		}
> +
> +		f2fs_invalidate_blocks(sbi, blkaddr_start,
> +					blkaddr_end - blkaddr_start + 1);
> +		blkaddr_start = blkaddr_end = blkaddr;
> +
> +		if (count == 1 && last_one)
> +			f2fs_invalidate_blocks(sbi, blkaddr, 1);
> +
> +skip_invalid:
> +		last_valid = true;
>   
>   		if (!released || blkaddr != COMPRESS_ADDR)
>   			nr_free++;
> +
> +		continue;
> +
> +next:

next:

		if (blklen) {
			f2fs_invalidate_blocks(sbi, blkstart, blklen);
			blkstart = blkaddr;
			blklen = 1;
		}

> +		/* If consecutive blocks have been recorded, we need to process them. */
> +		if (last_valid == true)
> +			f2fs_invalidate_blocks(sbi, blkaddr_start,
> +					blkaddr_end - blkaddr_start + 1);
> +
> +		blkaddr_start = blkaddr_end = le32_to_cpu(*(addr + 1));
> +		last_valid = false;
> +
>   	}

	if (blklen)
		f2fs_invalidate_blocks(sbi, blkstart, blklen);

Thanks,

>   
>   	if (compressed_cluster)