From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751125AbdK3HPg (ORCPT ); Thu, 30 Nov 2017 02:15:36 -0500 Received: from szxga05-in.huawei.com ([45.249.212.191]:11483 "EHLO szxga05-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750727AbdK3HPf (ORCPT ); Thu, 30 Nov 2017 02:15:35 -0500 Subject: Re: [PATCH] f2fs: avoid false positive of free secs check To: Yunlong Song , Chao Yu , , CC: , , , , References: <1511765699-47553-1-git-send-email-yunlong.song@huawei.com> <277e385f-650f-fe29-ac41-ff847a1b7858@huawei.com> From: Chao Yu Message-ID: <3cbc03ab-5918-d8b9-fced-18f1c674f2da@huawei.com> Date: Thu, 30 Nov 2017 15:15:11 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <277e385f-650f-fe29-ac41-ff847a1b7858@huawei.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.134.22.195] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090203.5A1FB009.005C,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 4255b062cef6dd87eeb12f56c356183d Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2017/11/30 10:42, Yunlong Song wrote: > SSR can make hot/warm/cold nodes written together, so why should we account > them different? Current segment which is using ssr allocation has only one valid type, so we can not write data/node with different type into current segment which already has fixed type, right? Thanks, > > On 2017/11/29 19:56, Chao Yu wrote: >> On 2017/11/27 14:54, Yunlong Song wrote: >>> Sometimes f2fs_gc is called with no target victim (e.g. xfstest >>> generic/027, ndirty_node:545 ndiry_dent:1 ndirty_imeta:513 rsvd_segs:21 >>> free_segs:27, has_not_enough_free_secs will return true). This patch >>> first merges pages and then converts into sections. >> I don't think this could be right, IMO, instead, it would be better to >> account dirty hot/warm/cold nodes or imeta separately, as actually, they >> will use different section, but currently, our calculation way is based >> on that they could be written to same section. >> >> Thanks, >> >>> Signed-off-by: Yunlong Song >>> --- >>> fs/f2fs/f2fs.h | 9 --------- >>> fs/f2fs/segment.c | 12 +++++++----- >>> fs/f2fs/segment.h | 13 +++++++++---- >>> 3 files changed, 16 insertions(+), 18 deletions(-) >>> >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>> index ca6b0c9..e89cff7 100644 >>> --- a/fs/f2fs/f2fs.h >>> +++ b/fs/f2fs/f2fs.h >>> @@ -1675,15 +1675,6 @@ static inline int get_dirty_pages(struct inode *inode) >>> return atomic_read(&F2FS_I(inode)->dirty_pages); >>> } >>> >>> -static inline int get_blocktype_secs(struct f2fs_sb_info *sbi, int block_type) >>> -{ >>> - unsigned int pages_per_sec = sbi->segs_per_sec * sbi->blocks_per_seg; >>> - unsigned int segs = (get_pages(sbi, block_type) + pages_per_sec - 1) >> >>> - sbi->log_blocks_per_seg; >>> - >>> - return segs / sbi->segs_per_sec; >>> -} >>> - >>> static inline block_t valid_user_blocks(struct f2fs_sb_info *sbi) >>> { >>> return sbi->total_valid_block_count; >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>> index c117e09..603f805 100644 >>> --- a/fs/f2fs/segment.c >>> +++ b/fs/f2fs/segment.c >>> @@ -171,17 +171,19 @@ static unsigned long __find_rev_next_zero_bit(const unsigned long *addr, >>> >>> bool need_SSR(struct f2fs_sb_info *sbi) >>> { >>> - int node_secs = get_blocktype_secs(sbi, F2FS_DIRTY_NODES); >>> - int dent_secs = get_blocktype_secs(sbi, F2FS_DIRTY_DENTS); >>> - int imeta_secs = get_blocktype_secs(sbi, F2FS_DIRTY_IMETA); >>> + s64 node_pages = get_pages(sbi, F2FS_DIRTY_NODES); >>> + s64 dent_pages = get_pages(sbi, F2FS_DIRTY_DENTS); >>> + s64 imeta_pages = get_pages(sbi, F2FS_DIRTY_IMETA); >>> >>> if (test_opt(sbi, LFS)) >>> return false; >>> if (sbi->gc_thread && sbi->gc_thread->gc_urgent) >>> return true; >>> >>> - return free_sections(sbi) <= (node_secs + 2 * dent_secs + imeta_secs + >>> - SM_I(sbi)->min_ssr_sections + reserved_sections(sbi)); >>> + return free_sections(sbi) <= >>> + (PAGE2SEC(sbi, node_pages + imeta_pages) + >>> + PAGE2SEC(sbi, 2 * dent_pages) + >>> + SM_I(sbi)->min_ssr_sections + reserved_sections(sbi)); >>> } >>> >>> void register_inmem_page(struct inode *inode, struct page *page) >>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h >>> index d1d394c..723d79e 100644 >>> --- a/fs/f2fs/segment.h >>> +++ b/fs/f2fs/segment.h >>> @@ -115,6 +115,10 @@ >>> #define SECTOR_TO_BLOCK(sectors) \ >>> ((sectors) >> F2FS_LOG_SECTORS_PER_BLOCK) >>> >>> +#define PAGE2SEC(sbi, pages) \ >>> + ((((pages) + BLKS_PER_SEC(sbi) - 1) \ >>> + >> sbi->log_blocks_per_seg) / sbi->segs_per_sec) >>> + >>> /* >>> * indicate a block allocation direction: RIGHT and LEFT. >>> * RIGHT means allocating new sections towards the end of volume. >>> @@ -527,9 +531,9 @@ static inline bool has_curseg_enough_space(struct f2fs_sb_info *sbi) >>> static inline bool has_not_enough_free_secs(struct f2fs_sb_info *sbi, >>> int freed, int needed) >>> { >>> - int node_secs = get_blocktype_secs(sbi, F2FS_DIRTY_NODES); >>> - int dent_secs = get_blocktype_secs(sbi, F2FS_DIRTY_DENTS); >>> - int imeta_secs = get_blocktype_secs(sbi, F2FS_DIRTY_IMETA); >>> + s64 node_pages = get_pages(sbi, F2FS_DIRTY_NODES); >>> + s64 dent_pages = get_pages(sbi, F2FS_DIRTY_DENTS); >>> + s64 imeta_pages = get_pages(sbi, F2FS_DIRTY_IMETA); >>> >>> if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) >>> return false; >>> @@ -538,7 +542,8 @@ static inline bool has_not_enough_free_secs(struct f2fs_sb_info *sbi, >>> has_curseg_enough_space(sbi)) >>> return false; >>> return (free_sections(sbi) + freed) <= >>> - (node_secs + 2 * dent_secs + imeta_secs + >>> + (PAGE2SEC(sbi, node_pages + imeta_pages) + >>> + PAGE2SEC(sbi, 2 * dent_pages) + >>> reserved_sections(sbi) + needed); >>> } >>> >>> >> . >> > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chao Yu Subject: Re: [PATCH] f2fs: avoid false positive of free secs check Date: Thu, 30 Nov 2017 15:15:11 +0800 Message-ID: <3cbc03ab-5918-d8b9-fced-18f1c674f2da@huawei.com> References: <1511765699-47553-1-git-send-email-yunlong.song@huawei.com> <277e385f-650f-fe29-ac41-ff847a1b7858@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <277e385f-650f-fe29-ac41-ff847a1b7858@huawei.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Yunlong Song , Chao Yu , jaegeuk@kernel.org, yunlong.song@icloud.com Cc: miaoxie@huawei.com, bintian.wang@huawei.com, linux-fsdevel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org List-Id: linux-f2fs-devel.lists.sourceforge.net On 2017/11/30 10:42, Yunlong Song wrote: > SSR can make hot/warm/cold nodes written together, so why should we account > them different? Current segment which is using ssr allocation has only one valid type, so we can not write data/node with different type into current segment which already has fixed type, right? Thanks, > > On 2017/11/29 19:56, Chao Yu wrote: >> On 2017/11/27 14:54, Yunlong Song wrote: >>> Sometimes f2fs_gc is called with no target victim (e.g. xfstest >>> generic/027, ndirty_node:545 ndiry_dent:1 ndirty_imeta:513 rsvd_segs:21 >>> free_segs:27, has_not_enough_free_secs will return true). This patch >>> first merges pages and then converts into sections. >> I don't think this could be right, IMO, instead, it would be better to >> account dirty hot/warm/cold nodes or imeta separately, as actually, they >> will use different section, but currently, our calculation way is based >> on that they could be written to same section. >> >> Thanks, >> >>> Signed-off-by: Yunlong Song >>> --- >>> fs/f2fs/f2fs.h | 9 --------- >>> fs/f2fs/segment.c | 12 +++++++----- >>> fs/f2fs/segment.h | 13 +++++++++---- >>> 3 files changed, 16 insertions(+), 18 deletions(-) >>> >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>> index ca6b0c9..e89cff7 100644 >>> --- a/fs/f2fs/f2fs.h >>> +++ b/fs/f2fs/f2fs.h >>> @@ -1675,15 +1675,6 @@ static inline int get_dirty_pages(struct inode *inode) >>> return atomic_read(&F2FS_I(inode)->dirty_pages); >>> } >>> >>> -static inline int get_blocktype_secs(struct f2fs_sb_info *sbi, int block_type) >>> -{ >>> - unsigned int pages_per_sec = sbi->segs_per_sec * sbi->blocks_per_seg; >>> - unsigned int segs = (get_pages(sbi, block_type) + pages_per_sec - 1) >> >>> - sbi->log_blocks_per_seg; >>> - >>> - return segs / sbi->segs_per_sec; >>> -} >>> - >>> static inline block_t valid_user_blocks(struct f2fs_sb_info *sbi) >>> { >>> return sbi->total_valid_block_count; >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>> index c117e09..603f805 100644 >>> --- a/fs/f2fs/segment.c >>> +++ b/fs/f2fs/segment.c >>> @@ -171,17 +171,19 @@ static unsigned long __find_rev_next_zero_bit(const unsigned long *addr, >>> >>> bool need_SSR(struct f2fs_sb_info *sbi) >>> { >>> - int node_secs = get_blocktype_secs(sbi, F2FS_DIRTY_NODES); >>> - int dent_secs = get_blocktype_secs(sbi, F2FS_DIRTY_DENTS); >>> - int imeta_secs = get_blocktype_secs(sbi, F2FS_DIRTY_IMETA); >>> + s64 node_pages = get_pages(sbi, F2FS_DIRTY_NODES); >>> + s64 dent_pages = get_pages(sbi, F2FS_DIRTY_DENTS); >>> + s64 imeta_pages = get_pages(sbi, F2FS_DIRTY_IMETA); >>> >>> if (test_opt(sbi, LFS)) >>> return false; >>> if (sbi->gc_thread && sbi->gc_thread->gc_urgent) >>> return true; >>> >>> - return free_sections(sbi) <= (node_secs + 2 * dent_secs + imeta_secs + >>> - SM_I(sbi)->min_ssr_sections + reserved_sections(sbi)); >>> + return free_sections(sbi) <= >>> + (PAGE2SEC(sbi, node_pages + imeta_pages) + >>> + PAGE2SEC(sbi, 2 * dent_pages) + >>> + SM_I(sbi)->min_ssr_sections + reserved_sections(sbi)); >>> } >>> >>> void register_inmem_page(struct inode *inode, struct page *page) >>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h >>> index d1d394c..723d79e 100644 >>> --- a/fs/f2fs/segment.h >>> +++ b/fs/f2fs/segment.h >>> @@ -115,6 +115,10 @@ >>> #define SECTOR_TO_BLOCK(sectors) \ >>> ((sectors) >> F2FS_LOG_SECTORS_PER_BLOCK) >>> >>> +#define PAGE2SEC(sbi, pages) \ >>> + ((((pages) + BLKS_PER_SEC(sbi) - 1) \ >>> + >> sbi->log_blocks_per_seg) / sbi->segs_per_sec) >>> + >>> /* >>> * indicate a block allocation direction: RIGHT and LEFT. >>> * RIGHT means allocating new sections towards the end of volume. >>> @@ -527,9 +531,9 @@ static inline bool has_curseg_enough_space(struct f2fs_sb_info *sbi) >>> static inline bool has_not_enough_free_secs(struct f2fs_sb_info *sbi, >>> int freed, int needed) >>> { >>> - int node_secs = get_blocktype_secs(sbi, F2FS_DIRTY_NODES); >>> - int dent_secs = get_blocktype_secs(sbi, F2FS_DIRTY_DENTS); >>> - int imeta_secs = get_blocktype_secs(sbi, F2FS_DIRTY_IMETA); >>> + s64 node_pages = get_pages(sbi, F2FS_DIRTY_NODES); >>> + s64 dent_pages = get_pages(sbi, F2FS_DIRTY_DENTS); >>> + s64 imeta_pages = get_pages(sbi, F2FS_DIRTY_IMETA); >>> >>> if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING))) >>> return false; >>> @@ -538,7 +542,8 @@ static inline bool has_not_enough_free_secs(struct f2fs_sb_info *sbi, >>> has_curseg_enough_space(sbi)) >>> return false; >>> return (free_sections(sbi) + freed) <= >>> - (node_secs + 2 * dent_secs + imeta_secs + >>> + (PAGE2SEC(sbi, node_pages + imeta_pages) + >>> + PAGE2SEC(sbi, 2 * dent_pages) + >>> reserved_sections(sbi) + needed); >>> } >>> >>> >> . >> >