* Re: [PATCH] f2fs: fix to account total free nid correctly
@ 2016-11-14 20:45 ` Jaegeuk Kim
0 siblings, 0 replies; 10+ messages in thread
From: Jaegeuk Kim @ 2016-11-14 20:45 UTC (permalink / raw)
To: Chao Yu; +Cc: chao, linux-kernel, linux-f2fs-devel
On Mon, Nov 14, 2016 at 07:24:56PM +0800, Chao Yu wrote:
> Thread A Thread B Thread C
> - f2fs_create
> - f2fs_new_inode
> - f2fs_lock_op
> - alloc_nid
> alloc last nid
> - f2fs_unlock_op
> - f2fs_create
> - f2fs_new_inode
> - f2fs_lock_op
> - alloc_nid
> as node count still not
> be increased, we will
> loop in alloc_nid
> - f2fs_write_node_pages
> - f2fs_balance_fs_bg
> - f2fs_sync_fs
> - write_checkpoint
> - block_operations
> - f2fs_lock_all
> - f2fs_lock_op
>
> While creating new inode, we do not allocate and account nid atomically,
> so that when there is almost no free nids left, we may encounter deadloop
> like above stack.
>
> In order to avoid that, add nm_i::free_nid_cnt for accounting free nids
> and do nid allocation atomically during node creation.
How about using nm_i::avaiable_nids for this?
It seems that we don't need both of variables at the same time.
Thanks,
>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
> fs/f2fs/f2fs.h | 1 +
> fs/f2fs/node.c | 19 +++++++++++++++----
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 6de1fbf..9de6f20 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -551,6 +551,7 @@ struct f2fs_nm_info {
> struct radix_tree_root free_nid_root;/* root of the free_nid cache */
> struct list_head nid_list[MAX_NID_LIST];/* lists for free nids */
> unsigned int nid_cnt[MAX_NID_LIST]; /* the number of free node id */
> + unsigned int free_nid_cnt; /* the number of total free nid */
> spinlock_t nid_list_lock; /* protect nid lists ops */
> struct mutex build_lock; /* lock for build free nids */
>
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index d58438f..e412d0e 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1885,11 +1885,13 @@ bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
> return false;
> }
> #endif
> - if (unlikely(sbi->total_valid_node_count + 1 > nm_i->available_nids))
> - return false;
> -
> spin_lock(&nm_i->nid_list_lock);
>
> + if (unlikely(nm_i->free_nid_cnt == 0)) {
> + spin_unlock(&nm_i->nid_list_lock);
> + return false;
> + }
> +
> /* We should not use stale free nids created by build_free_nids */
> if (nm_i->nid_cnt[FREE_NID_LIST] && !on_build_free_nids(nm_i)) {
> f2fs_bug_on(sbi, list_empty(&nm_i->nid_list[FREE_NID_LIST]));
> @@ -1900,6 +1902,7 @@ bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
> __remove_nid_from_list(sbi, i, FREE_NID_LIST, true);
> i->state = NID_ALLOC;
> __insert_nid_to_list(sbi, i, ALLOC_NID_LIST, false);
> + nm_i->free_nid_cnt--;
> spin_unlock(&nm_i->nid_list_lock);
> return true;
> }
> @@ -1951,6 +1954,9 @@ void alloc_nid_failed(struct f2fs_sb_info *sbi, nid_t nid)
> i->state = NID_NEW;
> __insert_nid_to_list(sbi, i, FREE_NID_LIST, false);
> }
> +
> + nm_i->free_nid_cnt++;
> +
> spin_unlock(&nm_i->nid_list_lock);
>
> if (need_free)
> @@ -2222,8 +2228,12 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
> raw_nat_from_node_info(raw_ne, &ne->ni);
> nat_reset_flag(ne);
> __clear_nat_cache_dirty(NM_I(sbi), ne);
> - if (nat_get_blkaddr(ne) == NULL_ADDR)
> + if (nat_get_blkaddr(ne) == NULL_ADDR) {
> add_free_nid(sbi, nid, false);
> + spin_lock(&NM_I(sbi)->nid_list_lock);
> + NM_I(sbi)->free_nid_cnt++;
> + spin_unlock(&NM_I(sbi)->nid_list_lock);
> + }
> }
>
> if (to_journal)
> @@ -2302,6 +2312,7 @@ static int init_node_manager(struct f2fs_sb_info *sbi)
> nm_i->nid_cnt[FREE_NID_LIST] = 0;
> nm_i->nid_cnt[ALLOC_NID_LIST] = 0;
> nm_i->nat_cnt = 0;
> + nm_i->free_nid_cnt = nm_i->available_nids - sbi->total_valid_node_count;
> nm_i->ram_thresh = DEF_RAM_THRESHOLD;
> nm_i->ra_nid_pages = DEF_RA_NID_PAGES;
> nm_i->dirty_nats_ratio = DEF_DIRTY_NAT_RATIO_THRESHOLD;
> --
> 2.8.2.311.gee88674
------------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: fix to account total free nid correctly
2016-11-14 20:45 ` Jaegeuk Kim
@ 2016-11-15 1:15 ` Chao Yu
-1 siblings, 0 replies; 10+ messages in thread
From: Chao Yu @ 2016-11-15 1:15 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, chao
On 2016/11/15 4:45, Jaegeuk Kim wrote:
> On Mon, Nov 14, 2016 at 07:24:56PM +0800, Chao Yu wrote:
>> Thread A Thread B Thread C
>> - f2fs_create
>> - f2fs_new_inode
>> - f2fs_lock_op
>> - alloc_nid
>> alloc last nid
>> - f2fs_unlock_op
>> - f2fs_create
>> - f2fs_new_inode
>> - f2fs_lock_op
>> - alloc_nid
>> as node count still not
>> be increased, we will
>> loop in alloc_nid
>> - f2fs_write_node_pages
>> - f2fs_balance_fs_bg
>> - f2fs_sync_fs
>> - write_checkpoint
>> - block_operations
>> - f2fs_lock_all
>> - f2fs_lock_op
>>
>> While creating new inode, we do not allocate and account nid atomically,
>> so that when there is almost no free nids left, we may encounter deadloop
>> like above stack.
>>
>> In order to avoid that, add nm_i::free_nid_cnt for accounting free nids
>> and do nid allocation atomically during node creation.
>
> How about using nm_i::avaiable_nids for this?
> It seems that we don't need both of variables at the same time.
Yep, let me reuse nm_i::avaiable_nids in v2 patch. :)
Thanks,
>
> Thanks,
>
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>> fs/f2fs/f2fs.h | 1 +
>> fs/f2fs/node.c | 19 +++++++++++++++----
>> 2 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 6de1fbf..9de6f20 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -551,6 +551,7 @@ struct f2fs_nm_info {
>> struct radix_tree_root free_nid_root;/* root of the free_nid cache */
>> struct list_head nid_list[MAX_NID_LIST];/* lists for free nids */
>> unsigned int nid_cnt[MAX_NID_LIST]; /* the number of free node id */
>> + unsigned int free_nid_cnt; /* the number of total free nid */
>> spinlock_t nid_list_lock; /* protect nid lists ops */
>> struct mutex build_lock; /* lock for build free nids */
>>
>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>> index d58438f..e412d0e 100644
>> --- a/fs/f2fs/node.c
>> +++ b/fs/f2fs/node.c
>> @@ -1885,11 +1885,13 @@ bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
>> return false;
>> }
>> #endif
>> - if (unlikely(sbi->total_valid_node_count + 1 > nm_i->available_nids))
>> - return false;
>> -
>> spin_lock(&nm_i->nid_list_lock);
>>
>> + if (unlikely(nm_i->free_nid_cnt == 0)) {
>> + spin_unlock(&nm_i->nid_list_lock);
>> + return false;
>> + }
>> +
>> /* We should not use stale free nids created by build_free_nids */
>> if (nm_i->nid_cnt[FREE_NID_LIST] && !on_build_free_nids(nm_i)) {
>> f2fs_bug_on(sbi, list_empty(&nm_i->nid_list[FREE_NID_LIST]));
>> @@ -1900,6 +1902,7 @@ bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
>> __remove_nid_from_list(sbi, i, FREE_NID_LIST, true);
>> i->state = NID_ALLOC;
>> __insert_nid_to_list(sbi, i, ALLOC_NID_LIST, false);
>> + nm_i->free_nid_cnt--;
>> spin_unlock(&nm_i->nid_list_lock);
>> return true;
>> }
>> @@ -1951,6 +1954,9 @@ void alloc_nid_failed(struct f2fs_sb_info *sbi, nid_t nid)
>> i->state = NID_NEW;
>> __insert_nid_to_list(sbi, i, FREE_NID_LIST, false);
>> }
>> +
>> + nm_i->free_nid_cnt++;
>> +
>> spin_unlock(&nm_i->nid_list_lock);
>>
>> if (need_free)
>> @@ -2222,8 +2228,12 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>> raw_nat_from_node_info(raw_ne, &ne->ni);
>> nat_reset_flag(ne);
>> __clear_nat_cache_dirty(NM_I(sbi), ne);
>> - if (nat_get_blkaddr(ne) == NULL_ADDR)
>> + if (nat_get_blkaddr(ne) == NULL_ADDR) {
>> add_free_nid(sbi, nid, false);
>> + spin_lock(&NM_I(sbi)->nid_list_lock);
>> + NM_I(sbi)->free_nid_cnt++;
>> + spin_unlock(&NM_I(sbi)->nid_list_lock);
>> + }
>> }
>>
>> if (to_journal)
>> @@ -2302,6 +2312,7 @@ static int init_node_manager(struct f2fs_sb_info *sbi)
>> nm_i->nid_cnt[FREE_NID_LIST] = 0;
>> nm_i->nid_cnt[ALLOC_NID_LIST] = 0;
>> nm_i->nat_cnt = 0;
>> + nm_i->free_nid_cnt = nm_i->available_nids - sbi->total_valid_node_count;
>> nm_i->ram_thresh = DEF_RAM_THRESHOLD;
>> nm_i->ra_nid_pages = DEF_RA_NID_PAGES;
>> nm_i->dirty_nats_ratio = DEF_DIRTY_NAT_RATIO_THRESHOLD;
>> --
>> 2.8.2.311.gee88674
>
> .
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: fix to account total free nid correctly
@ 2016-11-15 1:15 ` Chao Yu
0 siblings, 0 replies; 10+ messages in thread
From: Chao Yu @ 2016-11-15 1:15 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: chao, linux-kernel, linux-f2fs-devel
On 2016/11/15 4:45, Jaegeuk Kim wrote:
> On Mon, Nov 14, 2016 at 07:24:56PM +0800, Chao Yu wrote:
>> Thread A Thread B Thread C
>> - f2fs_create
>> - f2fs_new_inode
>> - f2fs_lock_op
>> - alloc_nid
>> alloc last nid
>> - f2fs_unlock_op
>> - f2fs_create
>> - f2fs_new_inode
>> - f2fs_lock_op
>> - alloc_nid
>> as node count still not
>> be increased, we will
>> loop in alloc_nid
>> - f2fs_write_node_pages
>> - f2fs_balance_fs_bg
>> - f2fs_sync_fs
>> - write_checkpoint
>> - block_operations
>> - f2fs_lock_all
>> - f2fs_lock_op
>>
>> While creating new inode, we do not allocate and account nid atomically,
>> so that when there is almost no free nids left, we may encounter deadloop
>> like above stack.
>>
>> In order to avoid that, add nm_i::free_nid_cnt for accounting free nids
>> and do nid allocation atomically during node creation.
>
> How about using nm_i::avaiable_nids for this?
> It seems that we don't need both of variables at the same time.
Yep, let me reuse nm_i::avaiable_nids in v2 patch. :)
Thanks,
>
> Thanks,
>
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>> fs/f2fs/f2fs.h | 1 +
>> fs/f2fs/node.c | 19 +++++++++++++++----
>> 2 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 6de1fbf..9de6f20 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -551,6 +551,7 @@ struct f2fs_nm_info {
>> struct radix_tree_root free_nid_root;/* root of the free_nid cache */
>> struct list_head nid_list[MAX_NID_LIST];/* lists for free nids */
>> unsigned int nid_cnt[MAX_NID_LIST]; /* the number of free node id */
>> + unsigned int free_nid_cnt; /* the number of total free nid */
>> spinlock_t nid_list_lock; /* protect nid lists ops */
>> struct mutex build_lock; /* lock for build free nids */
>>
>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>> index d58438f..e412d0e 100644
>> --- a/fs/f2fs/node.c
>> +++ b/fs/f2fs/node.c
>> @@ -1885,11 +1885,13 @@ bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
>> return false;
>> }
>> #endif
>> - if (unlikely(sbi->total_valid_node_count + 1 > nm_i->available_nids))
>> - return false;
>> -
>> spin_lock(&nm_i->nid_list_lock);
>>
>> + if (unlikely(nm_i->free_nid_cnt == 0)) {
>> + spin_unlock(&nm_i->nid_list_lock);
>> + return false;
>> + }
>> +
>> /* We should not use stale free nids created by build_free_nids */
>> if (nm_i->nid_cnt[FREE_NID_LIST] && !on_build_free_nids(nm_i)) {
>> f2fs_bug_on(sbi, list_empty(&nm_i->nid_list[FREE_NID_LIST]));
>> @@ -1900,6 +1902,7 @@ bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
>> __remove_nid_from_list(sbi, i, FREE_NID_LIST, true);
>> i->state = NID_ALLOC;
>> __insert_nid_to_list(sbi, i, ALLOC_NID_LIST, false);
>> + nm_i->free_nid_cnt--;
>> spin_unlock(&nm_i->nid_list_lock);
>> return true;
>> }
>> @@ -1951,6 +1954,9 @@ void alloc_nid_failed(struct f2fs_sb_info *sbi, nid_t nid)
>> i->state = NID_NEW;
>> __insert_nid_to_list(sbi, i, FREE_NID_LIST, false);
>> }
>> +
>> + nm_i->free_nid_cnt++;
>> +
>> spin_unlock(&nm_i->nid_list_lock);
>>
>> if (need_free)
>> @@ -2222,8 +2228,12 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>> raw_nat_from_node_info(raw_ne, &ne->ni);
>> nat_reset_flag(ne);
>> __clear_nat_cache_dirty(NM_I(sbi), ne);
>> - if (nat_get_blkaddr(ne) == NULL_ADDR)
>> + if (nat_get_blkaddr(ne) == NULL_ADDR) {
>> add_free_nid(sbi, nid, false);
>> + spin_lock(&NM_I(sbi)->nid_list_lock);
>> + NM_I(sbi)->free_nid_cnt++;
>> + spin_unlock(&NM_I(sbi)->nid_list_lock);
>> + }
>> }
>>
>> if (to_journal)
>> @@ -2302,6 +2312,7 @@ static int init_node_manager(struct f2fs_sb_info *sbi)
>> nm_i->nid_cnt[FREE_NID_LIST] = 0;
>> nm_i->nid_cnt[ALLOC_NID_LIST] = 0;
>> nm_i->nat_cnt = 0;
>> + nm_i->free_nid_cnt = nm_i->available_nids - sbi->total_valid_node_count;
>> nm_i->ram_thresh = DEF_RAM_THRESHOLD;
>> nm_i->ra_nid_pages = DEF_RA_NID_PAGES;
>> nm_i->dirty_nats_ratio = DEF_DIRTY_NAT_RATIO_THRESHOLD;
>> --
>> 2.8.2.311.gee88674
>
> .
>
------------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to account total free nid correctly
2016-11-14 20:45 ` Jaegeuk Kim
@ 2016-11-17 9:42 ` heyunlei
-1 siblings, 0 replies; 10+ messages in thread
From: heyunlei @ 2016-11-17 9:42 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu; +Cc: chao, linux-kernel, linux-f2fs-devel
On 2016/11/15 4:45, Jaegeuk Kim wrote:
> On Mon, Nov 14, 2016 at 07:24:56PM +0800, Chao Yu wrote:
>> Thread A Thread B Thread C
>> - f2fs_create
>> - f2fs_new_inode
>> - f2fs_lock_op
>> - alloc_nid
>> alloc last nid
>> - f2fs_unlock_op
>> - f2fs_create
>> - f2fs_new_inode
>> - f2fs_lock_op
>> - alloc_nid
>> as node count still not
>> be increased, we will
>> loop in alloc_nid
>> - f2fs_write_node_pages
>> - f2fs_balance_fs_bg
>> - f2fs_sync_fs
>> - write_checkpoint
>> - block_operations
>> - f2fs_lock_all
>> - f2fs_lock_op
>>
>> While creating new inode, we do not allocate and account nid atomically,
>> so that when there is almost no free nids left, we may encounter deadloop
>> like above stack.
>>
>> In order to avoid that, add nm_i::free_nid_cnt for accounting free nids
>> and do nid allocation atomically during node creation.
>
> How about using nm_i::avaiable_nids for this?
> It seems that we don't need both of variables at the same time.
>
> Thanks,
>
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>> fs/f2fs/f2fs.h | 1 +
>> fs/f2fs/node.c | 19 +++++++++++++++----
>> 2 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 6de1fbf..9de6f20 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -551,6 +551,7 @@ struct f2fs_nm_info {
>> struct radix_tree_root free_nid_root;/* root of the free_nid cache */
>> struct list_head nid_list[MAX_NID_LIST];/* lists for free nids */
>> unsigned int nid_cnt[MAX_NID_LIST]; /* the number of free node id */
>> + unsigned int free_nid_cnt; /* the number of total free nid */
>> spinlock_t nid_list_lock; /* protect nid lists ops */
>> struct mutex build_lock; /* lock for build free nids */
>>
>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>> index d58438f..e412d0e 100644
>> --- a/fs/f2fs/node.c
>> +++ b/fs/f2fs/node.c
>> @@ -1885,11 +1885,13 @@ bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
>> return false;
>> }
>> #endif
>> - if (unlikely(sbi->total_valid_node_count + 1 > nm_i->available_nids))
>> - return false;
>> -
>> spin_lock(&nm_i->nid_list_lock);
>>
>> + if (unlikely(nm_i->free_nid_cnt == 0)) {
>> + spin_unlock(&nm_i->nid_list_lock);
>> + return false;
>> + }
>> +
>> /* We should not use stale free nids created by build_free_nids */
>> if (nm_i->nid_cnt[FREE_NID_LIST] && !on_build_free_nids(nm_i)) {
>> f2fs_bug_on(sbi, list_empty(&nm_i->nid_list[FREE_NID_LIST]));
>> @@ -1900,6 +1902,7 @@ bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
>> __remove_nid_from_list(sbi, i, FREE_NID_LIST, true);
>> i->state = NID_ALLOC;
>> __insert_nid_to_list(sbi, i, ALLOC_NID_LIST, false);
>> + nm_i->free_nid_cnt--;
>> spin_unlock(&nm_i->nid_list_lock);
>> return true;
>> }
>> @@ -1951,6 +1954,9 @@ void alloc_nid_failed(struct f2fs_sb_info *sbi, nid_t nid)
>> i->state = NID_NEW;
>> __insert_nid_to_list(sbi, i, FREE_NID_LIST, false);
>> }
>> +
>> + nm_i->free_nid_cnt++;
>> +
>> spin_unlock(&nm_i->nid_list_lock);
>>
>> if (need_free)
>> @@ -2222,8 +2228,12 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>> raw_nat_from_node_info(raw_ne, &ne->ni);
>> nat_reset_flag(ne);
>> __clear_nat_cache_dirty(NM_I(sbi), ne);
>> - if (nat_get_blkaddr(ne) == NULL_ADDR)
>> + if (nat_get_blkaddr(ne) == NULL_ADDR) {
>> add_free_nid(sbi, nid, false);
>> + spin_lock(&NM_I(sbi)->nid_list_lock);
>> + NM_I(sbi)->free_nid_cnt++;
Hi all,
Here, we should consider clean NULL_ADDR nat entry in journal.
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index dcfab29..b22ecb0 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -158,6 +158,13 @@ static void __set_nat_cache_dirty(struct f2fs_nm_info *nm_i,
if (get_nat_flag(ne, IS_DIRTY))
return;
+ if (ne->ni.blk_addr == NULL_ADDR) {
+ spin_lock(&nm_i->free_nid_list_lock);
+ nm_i->available_nids--;
+ spin_unlock(&nm_i->free_nid_list_lock);
+ }
+
+
Thanks.
>> + spin_unlock(&NM_I(sbi)->nid_list_lock);
>> + }
>> }
>>
>> if (to_journal)
>> @@ -2302,6 +2312,7 @@ static int init_node_manager(struct f2fs_sb_info *sbi)
>> nm_i->nid_cnt[FREE_NID_LIST] = 0;
>> nm_i->nid_cnt[ALLOC_NID_LIST] = 0;
>> nm_i->nat_cnt = 0;
>> + nm_i->free_nid_cnt = nm_i->available_nids - sbi->total_valid_node_count;
>> nm_i->ram_thresh = DEF_RAM_THRESHOLD;
>> nm_i->ra_nid_pages = DEF_RA_NID_PAGES;
>> nm_i->dirty_nats_ratio = DEF_DIRTY_NAT_RATIO_THRESHOLD;
>> --
>> 2.8.2.311.gee88674
>
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
> .
>
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: fix to account total free nid correctly
@ 2016-11-17 9:42 ` heyunlei
0 siblings, 0 replies; 10+ messages in thread
From: heyunlei @ 2016-11-17 9:42 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu; +Cc: chao, linux-kernel, linux-f2fs-devel
On 2016/11/15 4:45, Jaegeuk Kim wrote:
> On Mon, Nov 14, 2016 at 07:24:56PM +0800, Chao Yu wrote:
>> Thread A Thread B Thread C
>> - f2fs_create
>> - f2fs_new_inode
>> - f2fs_lock_op
>> - alloc_nid
>> alloc last nid
>> - f2fs_unlock_op
>> - f2fs_create
>> - f2fs_new_inode
>> - f2fs_lock_op
>> - alloc_nid
>> as node count still not
>> be increased, we will
>> loop in alloc_nid
>> - f2fs_write_node_pages
>> - f2fs_balance_fs_bg
>> - f2fs_sync_fs
>> - write_checkpoint
>> - block_operations
>> - f2fs_lock_all
>> - f2fs_lock_op
>>
>> While creating new inode, we do not allocate and account nid atomically,
>> so that when there is almost no free nids left, we may encounter deadloop
>> like above stack.
>>
>> In order to avoid that, add nm_i::free_nid_cnt for accounting free nids
>> and do nid allocation atomically during node creation.
>
> How about using nm_i::avaiable_nids for this?
> It seems that we don't need both of variables at the same time.
>
> Thanks,
>
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>> fs/f2fs/f2fs.h | 1 +
>> fs/f2fs/node.c | 19 +++++++++++++++----
>> 2 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 6de1fbf..9de6f20 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -551,6 +551,7 @@ struct f2fs_nm_info {
>> struct radix_tree_root free_nid_root;/* root of the free_nid cache */
>> struct list_head nid_list[MAX_NID_LIST];/* lists for free nids */
>> unsigned int nid_cnt[MAX_NID_LIST]; /* the number of free node id */
>> + unsigned int free_nid_cnt; /* the number of total free nid */
>> spinlock_t nid_list_lock; /* protect nid lists ops */
>> struct mutex build_lock; /* lock for build free nids */
>>
>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>> index d58438f..e412d0e 100644
>> --- a/fs/f2fs/node.c
>> +++ b/fs/f2fs/node.c
>> @@ -1885,11 +1885,13 @@ bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
>> return false;
>> }
>> #endif
>> - if (unlikely(sbi->total_valid_node_count + 1 > nm_i->available_nids))
>> - return false;
>> -
>> spin_lock(&nm_i->nid_list_lock);
>>
>> + if (unlikely(nm_i->free_nid_cnt == 0)) {
>> + spin_unlock(&nm_i->nid_list_lock);
>> + return false;
>> + }
>> +
>> /* We should not use stale free nids created by build_free_nids */
>> if (nm_i->nid_cnt[FREE_NID_LIST] && !on_build_free_nids(nm_i)) {
>> f2fs_bug_on(sbi, list_empty(&nm_i->nid_list[FREE_NID_LIST]));
>> @@ -1900,6 +1902,7 @@ bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
>> __remove_nid_from_list(sbi, i, FREE_NID_LIST, true);
>> i->state = NID_ALLOC;
>> __insert_nid_to_list(sbi, i, ALLOC_NID_LIST, false);
>> + nm_i->free_nid_cnt--;
>> spin_unlock(&nm_i->nid_list_lock);
>> return true;
>> }
>> @@ -1951,6 +1954,9 @@ void alloc_nid_failed(struct f2fs_sb_info *sbi, nid_t nid)
>> i->state = NID_NEW;
>> __insert_nid_to_list(sbi, i, FREE_NID_LIST, false);
>> }
>> +
>> + nm_i->free_nid_cnt++;
>> +
>> spin_unlock(&nm_i->nid_list_lock);
>>
>> if (need_free)
>> @@ -2222,8 +2228,12 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>> raw_nat_from_node_info(raw_ne, &ne->ni);
>> nat_reset_flag(ne);
>> __clear_nat_cache_dirty(NM_I(sbi), ne);
>> - if (nat_get_blkaddr(ne) == NULL_ADDR)
>> + if (nat_get_blkaddr(ne) == NULL_ADDR) {
>> add_free_nid(sbi, nid, false);
>> + spin_lock(&NM_I(sbi)->nid_list_lock);
>> + NM_I(sbi)->free_nid_cnt++;
Hi all,
Here, we should consider clean NULL_ADDR nat entry in journal.
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index dcfab29..b22ecb0 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -158,6 +158,13 @@ static void __set_nat_cache_dirty(struct f2fs_nm_info *nm_i,
if (get_nat_flag(ne, IS_DIRTY))
return;
+ if (ne->ni.blk_addr == NULL_ADDR) {
+ spin_lock(&nm_i->free_nid_list_lock);
+ nm_i->available_nids--;
+ spin_unlock(&nm_i->free_nid_list_lock);
+ }
+
+
Thanks.
>> + spin_unlock(&NM_I(sbi)->nid_list_lock);
>> + }
>> }
>>
>> if (to_journal)
>> @@ -2302,6 +2312,7 @@ static int init_node_manager(struct f2fs_sb_info *sbi)
>> nm_i->nid_cnt[FREE_NID_LIST] = 0;
>> nm_i->nid_cnt[ALLOC_NID_LIST] = 0;
>> nm_i->nat_cnt = 0;
>> + nm_i->free_nid_cnt = nm_i->available_nids - sbi->total_valid_node_count;
>> nm_i->ram_thresh = DEF_RAM_THRESHOLD;
>> nm_i->ra_nid_pages = DEF_RA_NID_PAGES;
>> nm_i->dirty_nats_ratio = DEF_DIRTY_NAT_RATIO_THRESHOLD;
>> --
>> 2.8.2.311.gee88674
>
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
> .
>
------------------------------------------------------------------------------
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to account total free nid correctly
2016-11-17 9:42 ` heyunlei
@ 2016-11-17 12:09 ` Chao Yu
-1 siblings, 0 replies; 10+ messages in thread
From: Chao Yu @ 2016-11-17 12:09 UTC (permalink / raw)
To: heyunlei, Jaegeuk Kim; +Cc: chao, linux-kernel, linux-f2fs-devel
Hi Yunlei,
On 2016/11/17 17:42, heyunlei wrote:
>
>
> On 2016/11/15 4:45, Jaegeuk Kim wrote:
>> On Mon, Nov 14, 2016 at 07:24:56PM +0800, Chao Yu wrote:
>>> Thread A Thread B Thread C
>>> - f2fs_create
>>> - f2fs_new_inode
>>> - f2fs_lock_op
>>> - alloc_nid
>>> alloc last nid
>>> - f2fs_unlock_op
>>> - f2fs_create
>>> - f2fs_new_inode
>>> - f2fs_lock_op
>>> - alloc_nid
>>> as node count still not
>>> be increased, we will
>>> loop in alloc_nid
>>> - f2fs_write_node_pages
>>> - f2fs_balance_fs_bg
>>> - f2fs_sync_fs
>>> - write_checkpoint
>>> - block_operations
>>> - f2fs_lock_all
>>> - f2fs_lock_op
>>>
>>> While creating new inode, we do not allocate and account nid atomically,
>>> so that when there is almost no free nids left, we may encounter deadloop
>>> like above stack.
>>>
>>> In order to avoid that, add nm_i::free_nid_cnt for accounting free nids
>>> and do nid allocation atomically during node creation.
>>
>> How about using nm_i::avaiable_nids for this?
>> It seems that we don't need both of variables at the same time.
>>
>> Thanks,
>>
>>>
>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>> ---
>>> fs/f2fs/f2fs.h | 1 +
>>> fs/f2fs/node.c | 19 +++++++++++++++----
>>> 2 files changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index 6de1fbf..9de6f20 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -551,6 +551,7 @@ struct f2fs_nm_info {
>>> struct radix_tree_root free_nid_root;/* root of the free_nid cache */
>>> struct list_head nid_list[MAX_NID_LIST];/* lists for free nids */
>>> unsigned int nid_cnt[MAX_NID_LIST]; /* the number of free node id */
>>> + unsigned int free_nid_cnt; /* the number of total free nid */
>>> spinlock_t nid_list_lock; /* protect nid lists ops */
>>> struct mutex build_lock; /* lock for build free nids */
>>>
>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>> index d58438f..e412d0e 100644
>>> --- a/fs/f2fs/node.c
>>> +++ b/fs/f2fs/node.c
>>> @@ -1885,11 +1885,13 @@ bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
>>> return false;
>>> }
>>> #endif
>>> - if (unlikely(sbi->total_valid_node_count + 1 > nm_i->available_nids))
>>> - return false;
>>> -
>>> spin_lock(&nm_i->nid_list_lock);
>>>
>>> + if (unlikely(nm_i->free_nid_cnt == 0)) {
>>> + spin_unlock(&nm_i->nid_list_lock);
>>> + return false;
>>> + }
>>> +
>>> /* We should not use stale free nids created by build_free_nids */
>>> if (nm_i->nid_cnt[FREE_NID_LIST] && !on_build_free_nids(nm_i)) {
>>> f2fs_bug_on(sbi, list_empty(&nm_i->nid_list[FREE_NID_LIST]));
>>> @@ -1900,6 +1902,7 @@ bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
>>> __remove_nid_from_list(sbi, i, FREE_NID_LIST, true);
>>> i->state = NID_ALLOC;
>>> __insert_nid_to_list(sbi, i, ALLOC_NID_LIST, false);
>>> + nm_i->free_nid_cnt--;
>>> spin_unlock(&nm_i->nid_list_lock);
>>> return true;
>>> }
>>> @@ -1951,6 +1954,9 @@ void alloc_nid_failed(struct f2fs_sb_info *sbi, nid_t nid)
>>> i->state = NID_NEW;
>>> __insert_nid_to_list(sbi, i, FREE_NID_LIST, false);
>>> }
>>> +
>>> + nm_i->free_nid_cnt++;
>>> +
>>> spin_unlock(&nm_i->nid_list_lock);
>>>
>>> if (need_free)
>>> @@ -2222,8 +2228,12 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>> raw_nat_from_node_info(raw_ne, &ne->ni);
>>> nat_reset_flag(ne);
>>> __clear_nat_cache_dirty(NM_I(sbi), ne);
>>> - if (nat_get_blkaddr(ne) == NULL_ADDR)
>>> + if (nat_get_blkaddr(ne) == NULL_ADDR) {
>>> add_free_nid(sbi, nid, false);
>>> + spin_lock(&NM_I(sbi)->nid_list_lock);
>>> + NM_I(sbi)->free_nid_cnt++;
> Hi all,
> Here, we should consider clean NULL_ADDR nat entry in journal.
>
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index dcfab29..b22ecb0 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -158,6 +158,13 @@ static void __set_nat_cache_dirty(struct f2fs_nm_info *nm_i,
> if (get_nat_flag(ne, IS_DIRTY))
> return;
>
> + if (ne->ni.blk_addr == NULL_ADDR) {
> + spin_lock(&nm_i->free_nid_list_lock);
> + nm_i->available_nids--;
> + spin_unlock(&nm_i->free_nid_list_lock);
> + }
> +
> +
Thanks for pointing this out, as we discussed, it should be moved to
remove_nats_in_journal, anyway, I will send v2.
Thanks,
>
> Thanks.
>
>>> + spin_unlock(&NM_I(sbi)->nid_list_lock);
>>> + }
>>> }
>>>
>>> if (to_journal)
>>> @@ -2302,6 +2312,7 @@ static int init_node_manager(struct f2fs_sb_info *sbi)
>>> nm_i->nid_cnt[FREE_NID_LIST] = 0;
>>> nm_i->nid_cnt[ALLOC_NID_LIST] = 0;
>>> nm_i->nat_cnt = 0;
>>> + nm_i->free_nid_cnt = nm_i->available_nids - sbi->total_valid_node_count;
>>> nm_i->ram_thresh = DEF_RAM_THRESHOLD;
>>> nm_i->ra_nid_pages = DEF_RA_NID_PAGES;
>>> nm_i->dirty_nats_ratio = DEF_DIRTY_NAT_RATIO_THRESHOLD;
>>> --
>>> 2.8.2.311.gee88674
>>
>> ------------------------------------------------------------------------------
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>
>> .
>>
>
>
> .
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: fix to account total free nid correctly
@ 2016-11-17 12:09 ` Chao Yu
0 siblings, 0 replies; 10+ messages in thread
From: Chao Yu @ 2016-11-17 12:09 UTC (permalink / raw)
To: heyunlei, Jaegeuk Kim; +Cc: chao, linux-kernel, linux-f2fs-devel
Hi Yunlei,
On 2016/11/17 17:42, heyunlei wrote:
>
>
> On 2016/11/15 4:45, Jaegeuk Kim wrote:
>> On Mon, Nov 14, 2016 at 07:24:56PM +0800, Chao Yu wrote:
>>> Thread A Thread B Thread C
>>> - f2fs_create
>>> - f2fs_new_inode
>>> - f2fs_lock_op
>>> - alloc_nid
>>> alloc last nid
>>> - f2fs_unlock_op
>>> - f2fs_create
>>> - f2fs_new_inode
>>> - f2fs_lock_op
>>> - alloc_nid
>>> as node count still not
>>> be increased, we will
>>> loop in alloc_nid
>>> - f2fs_write_node_pages
>>> - f2fs_balance_fs_bg
>>> - f2fs_sync_fs
>>> - write_checkpoint
>>> - block_operations
>>> - f2fs_lock_all
>>> - f2fs_lock_op
>>>
>>> While creating new inode, we do not allocate and account nid atomically,
>>> so that when there is almost no free nids left, we may encounter deadloop
>>> like above stack.
>>>
>>> In order to avoid that, add nm_i::free_nid_cnt for accounting free nids
>>> and do nid allocation atomically during node creation.
>>
>> How about using nm_i::avaiable_nids for this?
>> It seems that we don't need both of variables at the same time.
>>
>> Thanks,
>>
>>>
>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>> ---
>>> fs/f2fs/f2fs.h | 1 +
>>> fs/f2fs/node.c | 19 +++++++++++++++----
>>> 2 files changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index 6de1fbf..9de6f20 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -551,6 +551,7 @@ struct f2fs_nm_info {
>>> struct radix_tree_root free_nid_root;/* root of the free_nid cache */
>>> struct list_head nid_list[MAX_NID_LIST];/* lists for free nids */
>>> unsigned int nid_cnt[MAX_NID_LIST]; /* the number of free node id */
>>> + unsigned int free_nid_cnt; /* the number of total free nid */
>>> spinlock_t nid_list_lock; /* protect nid lists ops */
>>> struct mutex build_lock; /* lock for build free nids */
>>>
>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>> index d58438f..e412d0e 100644
>>> --- a/fs/f2fs/node.c
>>> +++ b/fs/f2fs/node.c
>>> @@ -1885,11 +1885,13 @@ bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
>>> return false;
>>> }
>>> #endif
>>> - if (unlikely(sbi->total_valid_node_count + 1 > nm_i->available_nids))
>>> - return false;
>>> -
>>> spin_lock(&nm_i->nid_list_lock);
>>>
>>> + if (unlikely(nm_i->free_nid_cnt == 0)) {
>>> + spin_unlock(&nm_i->nid_list_lock);
>>> + return false;
>>> + }
>>> +
>>> /* We should not use stale free nids created by build_free_nids */
>>> if (nm_i->nid_cnt[FREE_NID_LIST] && !on_build_free_nids(nm_i)) {
>>> f2fs_bug_on(sbi, list_empty(&nm_i->nid_list[FREE_NID_LIST]));
>>> @@ -1900,6 +1902,7 @@ bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
>>> __remove_nid_from_list(sbi, i, FREE_NID_LIST, true);
>>> i->state = NID_ALLOC;
>>> __insert_nid_to_list(sbi, i, ALLOC_NID_LIST, false);
>>> + nm_i->free_nid_cnt--;
>>> spin_unlock(&nm_i->nid_list_lock);
>>> return true;
>>> }
>>> @@ -1951,6 +1954,9 @@ void alloc_nid_failed(struct f2fs_sb_info *sbi, nid_t nid)
>>> i->state = NID_NEW;
>>> __insert_nid_to_list(sbi, i, FREE_NID_LIST, false);
>>> }
>>> +
>>> + nm_i->free_nid_cnt++;
>>> +
>>> spin_unlock(&nm_i->nid_list_lock);
>>>
>>> if (need_free)
>>> @@ -2222,8 +2228,12 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
>>> raw_nat_from_node_info(raw_ne, &ne->ni);
>>> nat_reset_flag(ne);
>>> __clear_nat_cache_dirty(NM_I(sbi), ne);
>>> - if (nat_get_blkaddr(ne) == NULL_ADDR)
>>> + if (nat_get_blkaddr(ne) == NULL_ADDR) {
>>> add_free_nid(sbi, nid, false);
>>> + spin_lock(&NM_I(sbi)->nid_list_lock);
>>> + NM_I(sbi)->free_nid_cnt++;
> Hi all,
> Here, we should consider clean NULL_ADDR nat entry in journal.
>
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index dcfab29..b22ecb0 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -158,6 +158,13 @@ static void __set_nat_cache_dirty(struct f2fs_nm_info *nm_i,
> if (get_nat_flag(ne, IS_DIRTY))
> return;
>
> + if (ne->ni.blk_addr == NULL_ADDR) {
> + spin_lock(&nm_i->free_nid_list_lock);
> + nm_i->available_nids--;
> + spin_unlock(&nm_i->free_nid_list_lock);
> + }
> +
> +
Thanks for pointing this out, as we discussed, it should be moved to
remove_nats_in_journal, anyway, I will send v2.
Thanks,
>
> Thanks.
>
>>> + spin_unlock(&NM_I(sbi)->nid_list_lock);
>>> + }
>>> }
>>>
>>> if (to_journal)
>>> @@ -2302,6 +2312,7 @@ static int init_node_manager(struct f2fs_sb_info *sbi)
>>> nm_i->nid_cnt[FREE_NID_LIST] = 0;
>>> nm_i->nid_cnt[ALLOC_NID_LIST] = 0;
>>> nm_i->nat_cnt = 0;
>>> + nm_i->free_nid_cnt = nm_i->available_nids - sbi->total_valid_node_count;
>>> nm_i->ram_thresh = DEF_RAM_THRESHOLD;
>>> nm_i->ra_nid_pages = DEF_RA_NID_PAGES;
>>> nm_i->dirty_nats_ratio = DEF_DIRTY_NAT_RATIO_THRESHOLD;
>>> --
>>> 2.8.2.311.gee88674
>>
>> ------------------------------------------------------------------------------
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>
>> .
>>
>
>
> .
>
------------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 10+ messages in thread