From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932706AbdJ3Lul (ORCPT ); Mon, 30 Oct 2017 07:50:41 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:12646 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752185AbdJ3Luk (ORCPT ); Mon, 30 Oct 2017 07:50:40 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 mailout3.samsung.com 20171030115038epoutp032d8b38c9a01a2235c36f26f47380f6ec~yVsdEcys61523215232epoutp03v X-AuditID: b6c32a46-455ff700000010ca-34-59f7120e8273 From: Fan Li To: "'Jaegeuk Kim'" , "'Chao Yu'" Cc: "'Chao Yu'" , linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net In-reply-to: <20171030093026.GB26880@jaegeuk-macbookpro.roam.corp.google.com> Subject: RE: [f2fs-dev] [PATCH] f2fs: add a function to move nid Date: Mon, 30 Oct 2017 19:50:01 +0800 Message-id: <001101d35175$54987f20$fdc97d60$@samsung.com> MIME-version: 1.0 Content-type: text/plain; charset="Windows-1252" Content-transfer-encoding: 7bit X-Mailer: Microsoft Outlook 14.0 Content-language: en-us Thread-index: AQCgkanKBMnvBqEoQfN4RbkRJNQaSANbFeIqAcbz9moB3QDq4qUpxfXA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprAKsWRmVeSWpSXmKPExsWy7bCmmS6f0PdIg/WPOSxOTz3LZPFk/Sxm i0uL3C0u75rDZjH18VpWB1aPliNvWT02repk89i94DOTx+dNcgEsUVw2Kak5mWWpRfp2CVwZ H3tfsxVsMqvY+uorWwPjHq0uRk4OCQETif4z75m6GLk4hAR2MEr8+LaTDcL5zijRc+8Qaxcj B1jV4aYkiPhuRolLaw+yQjivgIruzGEHGcUmoC6xZWY3E4gtIuAscWr1NBYQm1kgS+Ls3hts IDangK/Ev1nnGUFsYQEHiZkdH8DiLAKqEld3/QKr5xWwlJi5dxkrhC0o8WPyPag5BhKvfy1h h7DlJTavecsM8YKCxI6zrxkh4uISkx48ZIe4wU2iuXkzM8ihEgIH2CS27D/MCtHgIvGmezlU s7DEq+Nb2CFsaYlnqzYyQtjrGCU+n7GAaN7OKDHv40cmiIQ10OZfUFfwSXQc/ssOCSJeiY42 IYgSD4nvU9qh5jtKbHx0hB0SWi1MEt/PfGSbwCg/C8lzs5A8NwvJc7OQPLSAkWUVo1hqQXFu emqxUYGRXnFibnFpXrpecn7uJkZwOtFy28G45JzPIUYBDkYlHl6NzG+RQqyJZcWVuYcYJTiY lUR4H90ACvGmJFZWpRblxxeV5qQWH2KU5mBREuet23YtQkggPbEkNTs1tSC1CCbLxMEp1cC4 2S6TefaVpmqH74Y3t3useuGTvfUil2BL99Rku/KtjyfyfhT8t+oqS9TG9yfOqd/gUrN+ezmj MdyivFX2xrOTASvVJ0Ys3822KlDpxi+r2RI6K7VnLeD5Lf3jbRTbV4dI46lvsj5tX7ol0y3A /FCoeeumrfWJFw46SXbunOqx9PJU/h/Ref+rlViKMxINtZiLihMBaUbObSMDAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrOLMWRmVeSWpSXmKPExsVy+t9jAV1eoe+RBke6jSxOTz3LZPFk/Sxm i0uL3C0u75rDZjH18VpWB1aPliNvWT02repk89i94DOTx+dNcgEsUVw2Kak5mWWpRfp2CVwZ H3tfsxVsMqvY+uorWwPjHq0uRg4OCQETicNNSV2MXBxCAjsZJdbN3cII4bxilNh/9ylrFyMn B5uAusSWmd1MILaIgLPEqdXTWECamQWyJE41GkHUtzFJdP9aAVbDKeAr8W/WeUYQW1jAQWJm xwc2EJtFQFXi6q5fLCA2r4ClxMy9y1ghbEGJH5PvgcWZBfQkPv65zQhhy0tsXvOWGcSWEFCQ 2HH2NVRcXGLSg4fsEPe4STQ3b2aewCg4C8moWUhGzUIyahaS9gWMLKsYJVMLinPTc4uNCozy Usv1ihNzi0vz0vWS83M3MQKDf9thrf4djI+XxB9iFOBgVOLhbcj+FinEmlhWXJl7iFGCg1lJ hPfRDaAQb0piZVVqUX58UWlOavEhRmkOFiVxXv78Y5FCAumJJanZqakFqUUwWSYOTqkGxk7J TWvLp5v2xTx9KCMyNzJjW4jOzQ55s4dL6uwypPYuNl4uc0fuIoPP28UTp10KLfTVrK3qa2pl mnL03dvZ06NPHnILWW9Rzt7SP13vj+v9P/qvlJd8vr5R6uATNamSp8sLtYIOq0QZtMrfzHQ/ X5/K+y304fIDKxJn8dQGRmqw7fd1MeoUVWIpzkg01GIuKk4EAFPtCnl6AgAA X-CMS-MailID: 20171030115037epcas2p28241454b2ed05c1ac707092dc8a2ed86 X-Msg-Generator: CA CMS-TYPE: 102P X-CMS-RootMailID: 20171028110400epcas1p2cd8fd0105884f449b5116b1ee9cf6092 X-RootMTR: 20171028110400epcas1p2cd8fd0105884f449b5116b1ee9cf6092 References: <000001d34fdc$7d461260$77d23720$@samsung.com> <70867f4c-e865-2acb-472a-8f24a7588d91@kernel.org> <20171030093026.GB26880@jaegeuk-macbookpro.roam.corp.google.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > -----Original Message----- > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > Sent: Monday, October 30, 2017 5:30 PM > To: Chao Yu > Cc: Fan Li; 'Chao Yu'; linux-kernel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net > Subject: Re: [f2fs-dev] [PATCH] f2fs: add a function to move nid > > On 10/28, Chao Yu wrote: > > On 2017/10/28 19:03, Fan Li wrote: > > > This patch add a new function to move nid from one state to another. > > > Move operation is heavily used, by adding a new function for it we > > > can cut down some branches from several flow. > > > > > > > > > Signed-off-by: Fan li > > > --- > > > fs/f2fs/node.c | 51 > > > ++++++++++++++++++++++++++++++--------------------- > > > 1 file changed, 30 insertions(+), 21 deletions(-) > > > > > > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index ac629d6..8116b50 > > > --- a/fs/f2fs/node.c > > > +++ b/fs/f2fs/node.c > > > @@ -1763,15 +1763,13 @@ static struct free_nid > > > *__lookup_free_nid_list(struct f2fs_nm_info *nm_i, } > > > > > > static int __insert_free_nid(struct f2fs_sb_info *sbi, > > > - struct free_nid *i, enum nid_state state, bool new) > > > + struct free_nid *i, enum nid_state state) > > > { > > > struct f2fs_nm_info *nm_i = NM_I(sbi); > > > > > > - if (new) { > > > - int err = radix_tree_insert(&nm_i->free_nid_root, i->nid, i); > > > - if (err) > > > - return err; > > > - } > > > + int err = radix_tree_insert(&nm_i->free_nid_root, i->nid, i); > > > + if (err) > > > + return err; > > > > > > f2fs_bug_on(sbi, state != i->state); > > > nm_i->nid_cnt[state]++; > > > @@ -1781,7 +1779,7 @@ static int __insert_free_nid(struct > > > f2fs_sb_info *sbi, } > > > > > > static void __remove_free_nid(struct f2fs_sb_info *sbi, > > > - struct free_nid *i, enum nid_state state, bool reuse) > > > + struct free_nid *i, enum nid_state state) > > > { > > > struct f2fs_nm_info *nm_i = NM_I(sbi); > > > > > > @@ -1789,8 +1787,23 @@ static void __remove_free_nid(struct f2fs_sb_info *sbi, > > > nm_i->nid_cnt[state]--; > > > if (state == FREE_NID) > > > list_del(&i->list); > > > - if (!reuse) > > > - radix_tree_delete(&nm_i->free_nid_root, i->nid); > > > + radix_tree_delete(&nm_i->free_nid_root, i->nid); } > > > + > > > +static void __move_free_nid(struct f2fs_sb_info *sbi, struct free_nid *i, > > > + enum nid_state org_state, enum nid_state dst_state) { > > > + struct f2fs_nm_info *nm_i = NM_I(sbi); > > > + > > > + f2fs_bug_on(sbi, org_state != i->state); > > > + i->state = dst_state; > > > + nm_i->nid_cnt[org_state]--; > > > + nm_i->nid_cnt[dst_state]++; > > > + > > > + if (org_state == FREE_NID) > > > > Welcome back to community, Fan. :) > > > > Minor, how about "if (dst_stat == PREALLOC_NID)" ? > > How about this? > > switch (dst_state) { > case PREALLOC_NID: > list_del(&i->list); > break; > case FREE_NID: > list_add_tail(&i->list, &nm_i->free_nid_list); > break; > default: > BUG_ON(1); > }; > I had thought about it, the reason why I finally chose the current version is because " dst_state== PREALLOC_NID" implies that all states other than PREALLOC_NID are in a list, so we need to call list_del. Of course it's true now, but in the future if we add more states which aren't in a list, it would be harder for us to track this branch to modify it since it's not related to FREE_NID explicitly. However the difference is trivial, if you think it's not a big deal, it's fine by me. > > > > Otherwise this patch looks good to me, anyway, you can add: > > > > Reviewed-by: Chao Yu > > > > Thanks, > > > > > + list_del(&i->list); > > > + else if (dst_state == FREE_NID) > > > + list_add_tail(&i->list, &nm_i->free_nid_list); > > > } > > > > > > /* return if the nid is recognized as free */ @@ -1850,7 +1863,7 @@ > > > static bool add_free_nid(struct f2fs_sb_info *sbi, nid_t nid, bool build) > > > } > > > } > > > ret = true; > > > - err = __insert_free_nid(sbi, i, FREE_NID, true); > > > + err = __insert_free_nid(sbi, i, FREE_NID); > > > err_out: > > > spin_unlock(&nm_i->nid_list_lock); > > > radix_tree_preload_end(); > > > @@ -1869,7 +1882,7 @@ static void remove_free_nid(struct f2fs_sb_info *sbi, nid_t nid) > > > spin_lock(&nm_i->nid_list_lock); > > > i = __lookup_free_nid_list(nm_i, nid); > > > if (i && i->state == FREE_NID) { > > > - __remove_free_nid(sbi, i, FREE_NID, false); > > > + __remove_free_nid(sbi, i, FREE_NID); > > > need_free = true; > > > } > > > spin_unlock(&nm_i->nid_list_lock); > > > @@ -2080,9 +2093,7 @@ bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid) > > > struct free_nid, list); > > > *nid = i->nid; > > > > > > - __remove_free_nid(sbi, i, FREE_NID, true); > > > - i->state = PREALLOC_NID; > > > - __insert_free_nid(sbi, i, PREALLOC_NID, false); > > > + __move_free_nid(sbi, i, FREE_NID, PREALLOC_NID); > > > nm_i->available_nids--; > > > > > > update_free_nid_bitmap(sbi, *nid, false, false); @@ -2108,7 > > > +2119,7 @@ void alloc_nid_done(struct f2fs_sb_info *sbi, nid_t nid) > > > spin_lock(&nm_i->nid_list_lock); > > > i = __lookup_free_nid_list(nm_i, nid); > > > f2fs_bug_on(sbi, !i); > > > - __remove_free_nid(sbi, i, PREALLOC_NID, false); > > > + __remove_free_nid(sbi, i, PREALLOC_NID); > > > spin_unlock(&nm_i->nid_list_lock); > > > > > > kmem_cache_free(free_nid_slab, i); @@ -2131,12 +2142,10 @@ void > > > alloc_nid_failed(struct f2fs_sb_info *sbi, nid_t nid) > > > f2fs_bug_on(sbi, !i); > > > > > > if (!available_free_memory(sbi, FREE_NIDS)) { > > > - __remove_free_nid(sbi, i, PREALLOC_NID, false); > > > + __remove_free_nid(sbi, i, PREALLOC_NID); > > > need_free = true; > > > } else { > > > - __remove_free_nid(sbi, i, PREALLOC_NID, true); > > > - i->state = FREE_NID; > > > - __insert_free_nid(sbi, i, FREE_NID, false); > > > + __move_free_nid(sbi, i, PREALLOC_NID, FREE_NID); > > > } > > > > > > nm_i->available_nids++; > > > @@ -2167,7 +2176,7 @@ int try_to_free_nids(struct f2fs_sb_info *sbi, int nr_shrink) > > > nm_i->nid_cnt[FREE_NID] <= MAX_FREE_NIDS) > > > break; > > > > > > - __remove_free_nid(sbi, i, FREE_NID, false); > > > + __remove_free_nid(sbi, i, FREE_NID); > > > kmem_cache_free(free_nid_slab, i); > > > nr_shrink--; > > > } > > > @@ -2746,7 +2755,7 @@ void destroy_node_manager(struct f2fs_sb_info *sbi) > > > /* destroy free nid list */ > > > spin_lock(&nm_i->nid_list_lock); > > > list_for_each_entry_safe(i, next_i, &nm_i->free_nid_list, list) { > > > - __remove_free_nid(sbi, i, FREE_NID, false); > > > + __remove_free_nid(sbi, i, FREE_NID); > > > spin_unlock(&nm_i->nid_list_lock); > > > kmem_cache_free(free_nid_slab, i); > > > spin_lock(&nm_i->nid_list_lock); > > >