From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9CF3E2AE6C for ; Tue, 14 Jan 2025 04:28:57 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736828937; cv=none; b=JLgUqkl43iv22Su5GPzjlJqG8LK8Yrl6Kxrf9cD5POe6T6MaAtD/BBdOwy6L6+XZbJMDTV+/k49DitCLW4nQZGrPq85L19/oEm1nXgtLrfl+XNtWq3Q+qzNGk6xhmyYy8WPYEDtfeWrhXRbdG95MVcLhAUeVYi3c6KSasUBMbBw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736828937; c=relaxed/simple; bh=0jGIxZv0Zyyj2rLgdz0tL1vL9XOy1sV6rojo5ZNA8xQ=; h=Message-ID:Date:MIME-Version:Cc:Subject:To:References:From: In-Reply-To:Content-Type; b=hLno5LDuDsxNqXkhVm6nEaavXZWvbkcwpOkSrYibYa1tDLRc6Fv12Qv60agXmkhEox6pt0FRLESrqh6c1gHr9HkguE7PLuic5okLihQjpumpVOY7VS07tvxyAutvEg/R/hBd1VtfUB+eNxecxQ0Yn3+N0u8iIyjHqoY0U8IQsh4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=pMRwcPSj; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="pMRwcPSj" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 36094C4CEDF; Tue, 14 Jan 2025 04:28:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1736828937; bh=0jGIxZv0Zyyj2rLgdz0tL1vL9XOy1sV6rojo5ZNA8xQ=; h=Date:Cc:Subject:To:References:From:In-Reply-To:From; b=pMRwcPSjOrsKSWThTOHxCWmKyL8B95IrIsfYUbHJ4MiD748aZ0PqWgbB5TKakHwxh G95qYDBOoKcXWhB6STQ0BGOJf66QtoRglZ9diMlU2g7bvcKjG7J8OJMYIQIdo8LDRw yR67ZXOjHjKs3pmXlEFQpftpUfxhsOURNoySurBVdEw/2lVEJqLAtd0oewEAliDx3U djwKfqZjpZZBOJsPCUQWnStrzgj128ujzgJFThQbZYVheuTBnRhe2CMwPQYeQi9ycp nO0RE9JxYHv0aesvdxjOFyQuWQaJg1UFGgybV1MAi+OJl2mUAlXIX8hUkYCrTELx/+ SnttZJbXI2KvA== Message-ID: <8a8dbb83-134b-4545-9b37-9c0b5b2be972@kernel.org> Date: Tue, 14 Jan 2025 12:28:52 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Cc: chao@kernel.org, sunyibuaa@gmail.com, linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, niuzhiguo84@gmail.com, Hao_hao.Wang@unisoc.com, ke.wang@unisoc.com Subject: Re: [PATCH v4 4/4] f2fs: Optimize f2fs_truncate_data_blocks_range() To: Yi Sun , jaegeuk@kernel.org References: <20241223081044.1126291-1-yi.sun@unisoc.com> <20241223081044.1126291-5-yi.sun@unisoc.com> Content-Language: en-US From: Chao Yu In-Reply-To: <20241223081044.1126291-5-yi.sun@unisoc.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 > --- > 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)