On 2017/11/7 12:54, Jaegeuk Kim wrote:
> On 11/07, heyunlei wrote:
>>
>>> -----Original Message-----
>>> From: Jaegeuk Kim [mailto:[email protected]]
>>> Sent: Tuesday, November 07, 2017 10:16 AM
>>> To: heyunlei
>>> Cc: Yuchao (T); [email protected]; Wangbintian;
>>> Wangkai (Morgan)
>>> Subject: Re: [f2fs-dev][PATCH v3] f2fs: Separate nat entry mem alloc from
>>> nat_tree_locks
>>>
>>> On 11/06, Yunlei He wrote:
>>>> This path separate nat_entry mem alloc from nat_tree_lock,
>>>> which will benefit in some low mem case.
>>>>
>>>> Signed-off-by: Yunlei He <[email protected]>
>>>> ---
>>>> fs/f2fs/node.c | 48 +++++++++++++++++++++++++++++++++++-------------
>>>> 1 file changed, 35 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>>> index fef5c68..4af6277 100644
>>>> --- a/fs/f2fs/node.c
>>>> +++ b/fs/f2fs/node.c
>>>> @@ -257,22 +257,15 @@ static struct nat_entry *grab_nat_entry(struct
>>>> f2fs_nm_info *nm_i, nid_t nid,
>>>>
>>>> if (no_fail) {
>>>> new = f2fs_kmem_cache_alloc(nat_entry_slab, GFP_NOFS);
>>>> - f2fs_radix_tree_insert(&nm_i->nat_root, nid, new);
>>>> } else {
>>>> new = kmem_cache_alloc(nat_entry_slab, GFP_NOFS);
>>>> if (!new)
>>>> return NULL;
>>>> - if (radix_tree_insert(&nm_i->nat_root, nid, new)) {
>>>> - kmem_cache_free(nat_entry_slab, new);
>>>> - return NULL;
>>>> - }
>>>> }
>>>>
>>>> memset(new, 0, sizeof(struct nat_entry));
>>>> nat_set_nid(new, nid);
>>>> nat_reset_flag(new);
>>>> - list_add_tail(&new->list, &nm_i->nat_entries);
>>>> - nm_i->nat_cnt++;
>>>> return new;
>>>> }
>>>>
>>>> @@ -281,18 +274,33 @@ static void cache_nat_entry(struct f2fs_sb_info
>>>> *sbi, nid_t nid,
>>>> {
>>>> struct f2fs_nm_info *nm_i = NM_I(sbi);
>>>> struct nat_entry *e;
>>>> + struct nat_entry *new = grab_nat_entry(nm_i, nid, false);
>>>
>>> How about this?
>>>
>>> {
>>> struct nat_entry *new = grab_nat_entry(nm_i, nid, false);
>>> bool free = false;
>>>
>>> if (!new)
>>> return;
>>
>> Return here may loss nat cache check blow?
>
> We don't need to cache this entry all the time with down_write(), and the
> below
> bug_on would not be a big concern.
>
>>
>> BTW, here __lookup_nat_cache() could find an nat entry? We look up earlier
>> and
>> node page lock always held during whole get node info process.
>
> Well, we don't really have to make such the hard assumption in order to avoid
> just one condition check.
Actually, in production, file system's environment is not very well, suffering
issues caused by CPU, memory or storage oftenly, IMO, in-memory consistence
check of fs meta info is needed.
How do you think of check this under CONFIG_F2FS_CHECK_FS as below:
#ifdef CONFIG_F2FS_CHECK_FS
down_read()
e = __lookup_nat_cache()
f2fs_bug_on(sbi, e && other_condition);
up_read()
#endif
down_write()
....
Thanks,
>
> Thanks,
>
>>
>> Thanks.
>>>
>>> down_write();
>>> e = __lookup_nat_cache();
>>> if (!e) {
>>> if (!radix_tree_insert(new)) {
>>> list_add_tail(new);
>>> nm_i->nat_cnt++;
>>> node_info_from_raw_nat(&new);
>>> } else {
>>> free = true;
>>> }
>>> } else {
>>> f2fs_bug_on();
>>> free = true;
>>> }
>>> up_write();
>>> if (free)
>>> kmem_cache_free(new);
>>> }
>>>
>>>>
>>>> + down_write(&nm_i->nat_tree_lock);
>>>> e = __lookup_nat_cache(nm_i, nid);
>>>> if (!e) {
>>>> - e = grab_nat_entry(nm_i, nid, false);
>>>> - if (e)
>>>> - node_info_from_raw_nat(&e->ni, ne);
>>>> + if (!new)
>>>> + goto out;
>>>> +
>>>> + e = new;
>>>> + if (radix_tree_insert(&nm_i->nat_root, nid, e)) {
>>>> + kmem_cache_free(nat_entry_slab, e);
>>>> + goto out;
>>>> + }
>>>> +
>>>> + list_add_tail(&e->list, &nm_i->nat_entries);
>>>> + nm_i->nat_cnt++;
>>>> + node_info_from_raw_nat(&e->ni, ne);
>>>> } else {
>>>> f2fs_bug_on(sbi, nat_get_ino(e) != le32_to_cpu(ne->ino) ||
>>>> nat_get_blkaddr(e) !=
>>>> le32_to_cpu(ne->block_addr) ||
>>>> nat_get_version(e) != ne->version);
>>>> + if (new)
>>>> + kmem_cache_free(nat_entry_slab, new);
>>>> }
>>>> +out:
>>>> + up_write(&nm_i->nat_tree_lock);
>>>> }
>>>>
>>>> static void set_node_addr(struct f2fs_sb_info *sbi, struct node_info *ni,
>>>> @@ -300,11 +308,21 @@ static void set_node_addr(struct f2fs_sb_info *sbi,
>>>> struct node_info *ni,
>>>> {
>>>> struct f2fs_nm_info *nm_i = NM_I(sbi);
>>>> struct nat_entry *e;
>>>> + struct nat_entry *new = grab_nat_entry(nm_i, ni->nid, true);
>>>>
>>>> +retry:
>>>> down_write(&nm_i->nat_tree_lock);
>>>> e = __lookup_nat_cache(nm_i, ni->nid);
>>>> if (!e) {
>>>> - e = grab_nat_entry(nm_i, ni->nid, true);
>>>> + e = new;
>>>> + if (radix_tree_insert(&nm_i->nat_root, ni->nid, e)) {
>>>> + up_write(&nm_i->nat_tree_lock);
>>>> + cond_resched();
>>>> + goto retry;
>>>> + }
>>>> +
>>>> + list_add_tail(&e->list, &nm_i->nat_entries);
>>>> + nm_i->nat_cnt++;
>>>> copy_node_info(&e->ni, ni);
>>>> f2fs_bug_on(sbi, ni->blk_addr == NEW_ADDR);
>>>> } else if (new_blkaddr == NEW_ADDR) {
>>>> @@ -315,6 +333,9 @@ static void set_node_addr(struct f2fs_sb_info *sbi,
>>>> struct node_info *ni,
>>>> */
>>>> copy_node_info(&e->ni, ni);
>>>> f2fs_bug_on(sbi, ni->blk_addr != NULL_ADDR);
>>>> + kmem_cache_free(nat_entry_slab, new);
>>>> + } else {
>>>> + kmem_cache_free(nat_entry_slab, new);
>>>> }
>>>>
>>>> /* sanity check */
>>>> @@ -424,9 +445,7 @@ void get_node_info(struct f2fs_sb_info *sbi, nid_t
>>>> nid, struct node_info *ni)
>>>> f2fs_put_page(page, 1);
>>>> cache:
>>>> /* cache nat entry */
>>>> - down_write(&nm_i->nat_tree_lock);
>>>> cache_nat_entry(sbi, nid, &ne);
>>>> - up_write(&nm_i->nat_tree_lock);
>>>> }
>>>>
>>>> /*
>>>> @@ -2375,6 +2394,9 @@ static void remove_nats_in_journal(struct
>>>> f2fs_sb_info *sbi)
>>>> ne = __lookup_nat_cache(nm_i, nid);
>>>> if (!ne) {
>>>> ne = grab_nat_entry(nm_i, nid, true);
>>>> + f2fs_radix_tree_insert(&nm_i->nat_root, nid, ne);
>>>> + list_add_tail(&ne->list, &nm_i->nat_entries);
>>>> + nm_i->nat_cnt++;
>>>> node_info_from_raw_nat(&ne->ni, &raw_ne);
>>>> }
>>>>
>>>> --
>>>> 1.9.1
>
> .
>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
