Re: [PATCH v2] f2fs: fix to account total free nid correctly

From: Chao Yu
Date: Mon Nov 14 2016 - 20:28:39 EST


On 2016/11/15 9:19, 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

Forgot to adapt above comments, sorry, I will send v3.

Thanks,

> and do nid allocation atomically during node creation.
>
> Signed-off-by: Chao Yu <yuchao0@xxxxxxxxxx>
> ---
> fs/f2fs/f2fs.h | 2 +-
> fs/f2fs/node.c | 21 ++++++++++++++++-----
> 2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 6de1fbf..0bba5d4 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -533,7 +533,7 @@ enum nid_list {
> struct f2fs_nm_info {
> block_t nat_blkaddr; /* base disk address of NAT */
> nid_t max_nid; /* maximum possible node ids */
> - nid_t available_nids; /* maximum available node ids */
> + nid_t available_nids; /* # of available node ids */
> nid_t next_scan_nid; /* the next nid to be scanned */
> unsigned int ram_thresh; /* control the memory footprint */
> unsigned int ra_nid_pages; /* # of nid pages to be readaheaded */
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index d58438f..1b5b31a 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->available_nids == 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->available_nids--;
> 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->available_nids++;
> +
> 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)->available_nids++;
> + spin_unlock(&NM_I(sbi)->nid_list_lock);
> + }
> }
>
> if (to_journal)
> @@ -2298,7 +2308,8 @@ static int init_node_manager(struct f2fs_sb_info *sbi)
> nm_i->max_nid = NAT_ENTRY_PER_BLOCK * nat_blocks;
>
> /* not used nids: 0, node, meta, (and root counted as valid node) */
> - nm_i->available_nids = nm_i->max_nid - F2FS_RESERVED_NODE_NUM;
> + nm_i->available_nids = nm_i->max_nid - sbi->total_valid_node_count -
> + F2FS_RESERVED_NODE_NUM;
> nm_i->nid_cnt[FREE_NID_LIST] = 0;
> nm_i->nid_cnt[ALLOC_NID_LIST] = 0;
> nm_i->nat_cnt = 0;
>