Re: [PATCH V2] f2fs: introduce free nid bitmap

[Jaegeuk Kim <jaegeuk@kernel.org>]

On 02/27, Chao Yu wrote:
> On 2017/2/26 3:02, Jaegeuk Kim wrote:
> > On 02/25, Chao Yu wrote:
> >> Hi Jaegeuk,
> >>
> >> I added below diff code into this patch in order to fix incorrectly nid status
> >> updating to reduce large latency of generic/251 in fstest, could you help to
> >> review code below?
> > 
> > Understand the problem, and how about this?
> > 
> > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > index 52db02396878..83b305689913 100644
> > --- a/fs/f2fs/node.c
> > +++ b/fs/f2fs/node.c
> > @@ -1765,7 +1765,7 @@ static void __remove_nid_from_list(struct f2fs_sb_info *sbi,
> >  		radix_tree_delete(&nm_i->free_nid_root, i->nid);
> >  }
> >  
> > -static int add_free_nid(struct f2fs_sb_info *sbi, nid_t nid, bool build)
> > +static bool add_free_nid(struct f2fs_sb_info *sbi, nid_t nid, bool build)
> >  {
> >  	struct f2fs_nm_info *nm_i = NM_I(sbi);
> >  	struct free_nid *i;
> > @@ -1774,14 +1774,14 @@ static int add_free_nid(struct f2fs_sb_info *sbi, nid_t nid, bool build)
> >  
> >  	/* 0 nid should not be used */
> >  	if (unlikely(nid == 0))
> > -		return 0;
> > +		return false;
> >  
> >  	if (build) {
> >  		/* do not add allocated nids */
> >  		ne = __lookup_nat_cache(nm_i, nid);
> >  		if (ne && (!get_nat_flag(ne, IS_CHECKPOINTED) ||
> >  				nat_get_blkaddr(ne) != NULL_ADDR))
> > -			return 0;
> > +			return false;
> >  	}
> >  
> >  	i = f2fs_kmem_cache_alloc(free_nid_slab, GFP_NOFS);
> > @@ -1790,7 +1790,7 @@ static int add_free_nid(struct f2fs_sb_info *sbi, nid_t nid, bool build)
> >  
> >  	if (radix_tree_preload(GFP_NOFS)) {
> >  		kmem_cache_free(free_nid_slab, i);
> > -		return 0;
> > +		return false;
> 
> If there is no memory, actually current free nid is still available, we need to
> return true (or other value -1?), then caller can set free_nid_bitmap correctly.
> 
> >  	}
> >  
> >  	spin_lock(&nm_i->nid_list_lock);
> > @@ -1799,9 +1799,9 @@ static int add_free_nid(struct f2fs_sb_info *sbi, nid_t nid, bool build)
> >  	radix_tree_preload_end();
> >  	if (err) {
> >  		kmem_cache_free(free_nid_slab, i);
> > -		return 0;
> > +		return false;
> 
> ditto.

Yup, applied.

Thanks,

> 
> Thanks,
> 
> >  	}
> > -	return 1;
> > +	return true;
> >  }
> >  
> >  static void remove_free_nid(struct f2fs_sb_info *sbi, nid_t nid)
> > @@ -1851,6 +1851,7 @@ static void scan_nat_page(struct f2fs_sb_info *sbi,
> >  	i = start_nid % NAT_ENTRY_PER_BLOCK;
> >  
> >  	for (; i < NAT_ENTRY_PER_BLOCK; i++, start_nid++) {
> > +		bool freed = false;
> >  
> >  		if (unlikely(start_nid >= nm_i->max_nid))
> >  			break;
> > @@ -1858,8 +1859,8 @@ static void scan_nat_page(struct f2fs_sb_info *sbi,
> >  		blk_addr = le32_to_cpu(nat_blk->entries[i].block_addr);
> >  		f2fs_bug_on(sbi, blk_addr == NEW_ADDR);
> >  		if (blk_addr == NULL_ADDR)
> > -			add_free_nid(sbi, start_nid, true);
> > -		update_free_nid_bitmap(sbi, start_nid, blk_addr == NULL_ADDR);
> > +			freed = add_free_nid(sbi, start_nid, true);
> > +		update_free_nid_bitmap(sbi, start_nid, freed);
> >  	}
> >  }
> >  
> >