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 EA7F71C54BB for ; Fri, 20 Dec 2024 21:22:10 +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=1734729731; cv=none; b=q0qV/07PoPDo3i0QF0OSnvGFglnEoLHlAzdSZOPTJ/sT4muwPzTTNaIIYU6F1Mfcw7CpcM2L09oPR7dgJ/gFOwbjxAmQqH34+uKNOwENlEiRnAa2Yk29ieqezM175CcvxesR/+HKA0/Joh3WagD7UR4GlikqaWEUafbekR/mZZw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734729731; c=relaxed/simple; bh=LGgD27lhXi53yEACx74DJaWK5dxFAJH+qe2RZEy4PuA=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Em6Wic4ynxWNyFbAzdahvZPBl4CO9cTGySDy7uFpwoP47RcDphaA/YiT75po1abBNgC3xOYk8Yljdg4CQ37ASiuI8owjjwlukI+Jdp58q4z391aYMzDk1fqFRA6JvPcIcq80mmoLwTNhTAAbX3xZfmaNF41hAGOJx0CB45ifElI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=tZ9U7b3I; 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="tZ9U7b3I" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 13465C4CECD; Fri, 20 Dec 2024 21:22:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1734729730; bh=LGgD27lhXi53yEACx74DJaWK5dxFAJH+qe2RZEy4PuA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=tZ9U7b3IwwEdIhqSRazEeApVi6jmgt/YU4R5i985K+AixDM4Z3CWCuBLSzYcl+c0I ho+oOsn06CDesy1THHvOgQQhP27iDcLGDD0hPLx78Uszy4XASeeF7gIPdY39wXfk/F +kmNuP65YBq9JgZmP5WICYTRDoUgbGHweUD+iae19qsxsDvWycSg4CQNV2/+SLuali rlEbd4SqiT9n1Rfkdp0/bp87EnFzhVpr3guUcNhTELJwOQA9xlwWQjNRzK/Q8ZPGtB 6b8TrY6NF8PJ3V9rrQ6CgtBna+d6NZM2jGjzaxeC8lDM5AlE/0yZHewoxwWJ5iGlXq tC3VbGqxJMESQ== Date: Fri, 20 Dec 2024 21:22:08 +0000 From: Jaegeuk Kim To: Yi Sun 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 v3 3/5] f2fs: introduce update_sit_entry_for_release() Message-ID: References: <20241104034545.497907-1-yi.sun@unisoc.com> <20241104034545.497907-4-yi.sun@unisoc.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20241104034545.497907-4-yi.sun@unisoc.com> This makes the code being inconsistent. Can you refactor first and add the loop later separately? For example, 1) add two functions, update_sit_entry_for_alloc() and update_sit_entry_for_release() 2) add a loop in update_sit_entry_for_release() Thanks, On 11/04, Yi Sun wrote: > This function can process some consecutive blocks at a time. > > When using update_sit_entry() to release consecutive blocks, > ensure that the consecutive blocks belong to the same segment. > Because after update_sit_entry_for_realese(), @segno is still > in use in update_sit_entry(). > > Signed-off-by: Yi Sun > --- > fs/f2fs/segment.c | 103 ++++++++++++++++++++++++++++++---------------- > 1 file changed, 68 insertions(+), 35 deletions(-) > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > index 5386ae18d808..843171ce414b 100644 > --- a/fs/f2fs/segment.c > +++ b/fs/f2fs/segment.c > @@ -2424,6 +2424,70 @@ static void update_segment_mtime(struct f2fs_sb_info *sbi, block_t blkaddr, > SIT_I(sbi)->max_mtime = ctime; > } > > +/* > + * NOTE: when updating multiple blocks at the same time, please ensure > + * that the consecutive input blocks belong to the same segment. > + */ > + > +static int update_sit_entry_for_release(struct f2fs_sb_info *sbi, struct seg_entry *se, > + block_t blkaddr, unsigned int offset, int del) > +{ > + bool exist; > +#ifdef CONFIG_F2FS_CHECK_FS > + bool mir_exist; > +#endif > + int i; > + int del_count = -del; > + > + f2fs_bug_on(sbi, GET_SEGNO(sbi, blkaddr) != GET_SEGNO(sbi, blkaddr + del_count - 1)); > + > + for (i = 0; i < del_count; i++) { > + exist = f2fs_test_and_clear_bit(offset + i, se->cur_valid_map); > +#ifdef CONFIG_F2FS_CHECK_FS > + mir_exist = f2fs_test_and_clear_bit(offset + i, > + se->cur_valid_map_mir); > + if (unlikely(exist != mir_exist)) { > + f2fs_err(sbi, "Inconsistent error when clearing bitmap, blk:%u, old bit:%d", > + blkaddr + i, exist); > + f2fs_bug_on(sbi, 1); > + } > +#endif > + if (unlikely(!exist)) { > + f2fs_err(sbi, "Bitmap was wrongly cleared, blk:%u", > + blkaddr + i); > + f2fs_bug_on(sbi, 1); > + se->valid_blocks++; > + del += 1; > + } else if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) { > + /* > + * If checkpoints are off, we must not reuse data that > + * was used in the previous checkpoint. If it was used > + * before, we must track that to know how much space we > + * really have. > + */ > + if (f2fs_test_bit(offset + i, se->ckpt_valid_map)) { > + spin_lock(&sbi->stat_lock); > + sbi->unusable_block_count++; > + spin_unlock(&sbi->stat_lock); > + } > + } > + > + if (f2fs_block_unit_discard(sbi) && > + f2fs_test_and_clear_bit(offset + i, se->discard_map)) > + sbi->discard_blks++; > + > + if (!f2fs_test_bit(offset + i, se->ckpt_valid_map)) > + se->ckpt_valid_blocks -= 1; > + } > + > + return del; > +} > + > +/* > + * If releasing blocks, this function supports updating multiple consecutive blocks > + * at one time, but please note that these consecutive blocks need to belong to the > + * same segment. > + */ > static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del) > { > struct seg_entry *se; > @@ -2479,43 +2543,12 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del) > if (!f2fs_test_and_set_bit(offset, se->ckpt_valid_map)) > se->ckpt_valid_blocks++; > } > - } else { > - exist = f2fs_test_and_clear_bit(offset, se->cur_valid_map); > -#ifdef CONFIG_F2FS_CHECK_FS > - mir_exist = f2fs_test_and_clear_bit(offset, > - se->cur_valid_map_mir); > - if (unlikely(exist != mir_exist)) { > - f2fs_err(sbi, "Inconsistent error when clearing bitmap, blk:%u, old bit:%d", > - blkaddr, exist); > - f2fs_bug_on(sbi, 1); > - } > -#endif > - if (unlikely(!exist)) { > - f2fs_err(sbi, "Bitmap was wrongly cleared, blk:%u", > - blkaddr); > - f2fs_bug_on(sbi, 1); > - se->valid_blocks++; > - del = 0; > - } else if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) { > - /* > - * If checkpoints are off, we must not reuse data that > - * was used in the previous checkpoint. If it was used > - * before, we must track that to know how much space we > - * really have. > - */ > - if (f2fs_test_bit(offset, se->ckpt_valid_map)) { > - spin_lock(&sbi->stat_lock); > - sbi->unusable_block_count++; > - spin_unlock(&sbi->stat_lock); > - } > - } > > - if (f2fs_block_unit_discard(sbi) && > - f2fs_test_and_clear_bit(offset, se->discard_map)) > - sbi->discard_blks++; > + if (!f2fs_test_bit(offset, se->ckpt_valid_map)) > + se->ckpt_valid_blocks += del; > + } else { > + del = update_sit_entry_for_release(sbi, se, blkaddr, offset, del); > } > - if (!f2fs_test_bit(offset, se->ckpt_valid_map)) > - se->ckpt_valid_blocks += del; > > __mark_sit_entry_dirty(sbi, segno); > > -- > 2.25.1