From: Jaegeuk Kim <jaegeuk@kernel.org> To: Chao Yu <yuchao0@huawei.com> Cc: linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, chao@kernel.org Subject: Re: [PATCH] f2fs: compress: remove unneed check condition Date: Sat, 24 Apr 2021 17:47:36 -0700 [thread overview] Message-ID: <YIS8KHf9VPxZl85b@google.com> (raw) In-Reply-To: <2c6f17e6-ef23-f313-5df2-6bd63d7df2b1@huawei.com> On 04/22, Chao Yu wrote: > On 2021/4/22 12:04, Jaegeuk Kim wrote: > > On 04/21, Chao Yu wrote: > > > In only call path of __cluster_may_compress(), __f2fs_write_data_pages() > > > has checked SBI_POR_DOING condition, and also cluster_may_compress() > > > has checked CP_ERROR_FLAG condition, so remove redundant check condition > > > in __cluster_may_compress() for cleanup. > > > > I think cp_error can get any time without synchronization. Is it safe to say > > it's redundant? > > Yes, > > But no matter how late we check cp_error, cp_error can happen after our > check points, it won't cause regression if we remove cp_error check there, > because for compress write, it uses OPU, it won't overwrite any existed data > in device. > > Seems it will be more appropriate to check cp_error in > f2fs_write_compressed_pages() like we did in f2fs_write_single_data_page() > rather than in __cluster_may_compress(). > > BTW, shouldn't we rename __cluster_may_compress() to > cluster_beyond_filesize() for better readability? f2fs_cluster_has_data()? > > Thanks, > > > > > > > > > Signed-off-by: Chao Yu <yuchao0@huawei.com> > > > --- > > > fs/f2fs/compress.c | 5 ----- > > > 1 file changed, 5 deletions(-) > > > > > > diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c > > > index 3c9d797dbdd6..532c311e3a89 100644 > > > --- a/fs/f2fs/compress.c > > > +++ b/fs/f2fs/compress.c > > > @@ -906,11 +906,6 @@ static bool __cluster_may_compress(struct compress_ctx *cc) > > > f2fs_bug_on(sbi, !page); > > > - if (unlikely(f2fs_cp_error(sbi))) > > > - return false; > > > - if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) > > > - return false; > > > - > > > /* beyond EOF */ > > > if (page->index >= nr_pages) > > > return false; > > > -- > > > 2.29.2 > > . > >
WARNING: multiple messages have this Message-ID (diff)
From: Jaegeuk Kim <jaegeuk@kernel.org> To: Chao Yu <yuchao0@huawei.com> Cc: linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net Subject: Re: [f2fs-dev] [PATCH] f2fs: compress: remove unneed check condition Date: Sat, 24 Apr 2021 17:47:36 -0700 [thread overview] Message-ID: <YIS8KHf9VPxZl85b@google.com> (raw) In-Reply-To: <2c6f17e6-ef23-f313-5df2-6bd63d7df2b1@huawei.com> On 04/22, Chao Yu wrote: > On 2021/4/22 12:04, Jaegeuk Kim wrote: > > On 04/21, Chao Yu wrote: > > > In only call path of __cluster_may_compress(), __f2fs_write_data_pages() > > > has checked SBI_POR_DOING condition, and also cluster_may_compress() > > > has checked CP_ERROR_FLAG condition, so remove redundant check condition > > > in __cluster_may_compress() for cleanup. > > > > I think cp_error can get any time without synchronization. Is it safe to say > > it's redundant? > > Yes, > > But no matter how late we check cp_error, cp_error can happen after our > check points, it won't cause regression if we remove cp_error check there, > because for compress write, it uses OPU, it won't overwrite any existed data > in device. > > Seems it will be more appropriate to check cp_error in > f2fs_write_compressed_pages() like we did in f2fs_write_single_data_page() > rather than in __cluster_may_compress(). > > BTW, shouldn't we rename __cluster_may_compress() to > cluster_beyond_filesize() for better readability? f2fs_cluster_has_data()? > > Thanks, > > > > > > > > > Signed-off-by: Chao Yu <yuchao0@huawei.com> > > > --- > > > fs/f2fs/compress.c | 5 ----- > > > 1 file changed, 5 deletions(-) > > > > > > diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c > > > index 3c9d797dbdd6..532c311e3a89 100644 > > > --- a/fs/f2fs/compress.c > > > +++ b/fs/f2fs/compress.c > > > @@ -906,11 +906,6 @@ static bool __cluster_may_compress(struct compress_ctx *cc) > > > f2fs_bug_on(sbi, !page); > > > - if (unlikely(f2fs_cp_error(sbi))) > > > - return false; > > > - if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) > > > - return false; > > > - > > > /* beyond EOF */ > > > if (page->index >= nr_pages) > > > return false; > > > -- > > > 2.29.2 > > . > > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
next prev parent reply other threads:[~2021-04-25 0:47 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-21 8:39 [PATCH] f2fs: compress: remove unneed check condition Chao Yu 2021-04-21 8:39 ` [f2fs-dev] " Chao Yu 2021-04-22 4:04 ` Jaegeuk Kim 2021-04-22 4:04 ` [f2fs-dev] " Jaegeuk Kim 2021-04-22 6:51 ` Chao Yu 2021-04-22 6:51 ` [f2fs-dev] " Chao Yu 2021-04-25 0:47 ` Jaegeuk Kim [this message] 2021-04-25 0:47 ` Jaegeuk Kim 2021-04-25 1:28 ` Chao Yu 2021-04-25 1:28 ` [f2fs-dev] " Chao Yu 2021-04-26 17:05 ` Jaegeuk Kim 2021-04-26 17:05 ` [f2fs-dev] " Jaegeuk Kim 2021-04-27 2:43 ` Chao Yu 2021-04-27 2:43 ` [f2fs-dev] " Chao Yu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YIS8KHf9VPxZl85b@google.com \ --to=jaegeuk@kernel.org \ --cc=chao@kernel.org \ --cc=linux-f2fs-devel@lists.sourceforge.net \ --cc=linux-kernel@vger.kernel.org \ --cc=yuchao0@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.