Hi Jaegeuk,
> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaeg...@kernel.org]
> Sent: Saturday, January 24, 2015 3:43 AM
> To: Chao Yu
> Cc: 'Changman Lee'; linux-f2fs-de...@lists.sourceforge.net;
> linux-kernel@vger.kernel.org
> Subject: Re: [f2fs-dev][RFC PATCH 06/10] f2fs: add core functions for rb-tree
> extent cache
>
> On Fri, Jan 23, 2015 at 02:15:56PM +0800, Chao Yu wrote:
> > Hi Jaegeuk,
> >
> > > -----Original Message-----
> > > From: Jaegeuk Kim [mailto:jaeg...@kernel.org]
> > > Sent: Friday, January 23, 2015 9:48 AM
> > > To: Chao Yu
> > > Cc: Changman Lee; linux-f2fs-de...@lists.sourceforge.net;
> > > linux-kernel@vger.kernel.org
> > > Subject: Re: [f2fs-dev][RFC PATCH 06/10] f2fs: add core functions for
> > > rb-tree extent cache
> > >
> > > On Mon, Jan 12, 2015 at 03:14:48PM +0800, Chao Yu wrote:
> > > > This patch adds core functions including slab cache init function and
> > > > init/lookup/update/shrink/destroy function for rb-tree based extent
> > > > cache.
> > > >
> > > > Thank Jaegeuk Kim and Changman Lee as they gave much suggestion about
> > > > detail
> > > > design and implementation of extent cache.
> > > >
> > > > Todo:
> > > > * add a cached_ei into struct extent_tree for a quick recent cache.
> > > > * register rb-based extent cache shrink with mm shrink interface.
> > > > * disable dir inode's extent cache.
> > > >
>
[snip]
> > > > +static void f2fs_update_extent_tree(struct inode *inode, pgoff_t fofs,
> > > > + block_t blkaddr)
> > > > +{
> > > > + struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > > > + nid_t ino = inode->i_ino;
> > > > + struct extent_tree *et;
> > > > + struct extent_node *en = NULL, *en1 = NULL, *en2 = NULL, *en3 =
> > > > NULL;
> > > > + struct extent_node *den = NULL;
> > > > + struct extent_info *pei;
> > > > + struct extent_info ei;
> > > > + unsigned int endofs;
> > > > +
> > > > + if (is_inode_flag_set(F2FS_I(inode), FI_NO_EXTENT))
> > > > + return;
> > > > +
> > > > +retry:
> > > > + down_write(&sbi->extent_tree_lock);
> > > > + et = radix_tree_lookup(&sbi->extent_tree_root, ino);
> > > > + if (!et) {
> > > > + et = kmem_cache_alloc(extent_tree_slab, GFP_ATOMIC);
> > >
> > > et = f2fs_kmem_cache_alloc(.., GFP_ATOMIC);
> >
> > How about modifying as below to avoid holding extent_tree_lock for long
> > time?
>
> Hmm. I don't think it doesn't take such the long time.
> It's GFP_ATOMIC.
>
> Moreover, for radix_tree, I prefer to use f2fs_radix_tree_insert with
> GFP_NOIO.
> Since, we actually don't need to call kmem_cache_free.
How about use following code?
f2fs_kmem_cache_alloc(extent_tree_slab, GFP_NOFS);
f2fs_radix_tree_insert(&sbi->extent_tree_root, ino, et);
in init_extent_cache_info()
INIT_RADIX_TREE(&sbi->extent_tree_root, GFP_NOIO);
>
> >
> > et = kmem_cache_alloc(extent_tree_slab, GFP_ATOMIC);
> > if (!et) {
> > up_write(&sbi->extent_tree_lock);
> > cond_resched();
> > goto retry;
> > }
> >
> > >
> > > > + if (!et) {
> > > > + up_write(&sbi->extent_tree_lock);
> > > > + goto retry;
> > > > + }
> > > > + if (radix_tree_insert(&sbi->extent_tree_root, ino, et))
> > > > {
> > > > + up_write(&sbi->extent_tree_lock);
> > > > + kmem_cache_free(extent_tree_slab, et);
> >
> > cond_resched()?
>
> I'm not sure why this should be needed.
> There is rw_semaphore, so we have already a rescheduling point.
>
[snip]
> > > Can we just remove en1 and en2 in __insert_extent_tree?
> >
> > en1 and en2 is newly added, in __attach_extent_node we do not add en1 and
> > en2 into
> > lru list, so if you do not want to keep split part in lru list, let's just
> > remove
> > the below codes.
>
> What I meant was, how about avoiding attaching en1 and en2, if they are
> splits whose
> lens are less than F2FS_MIN_EXTENT_LEN in advance?
Oh, it's better, I will do it.
[snip]
> > > No use of tree_cnt.
> >
> > This is used by trace function in RFC PATCH 10/10.
>
> Well, then, it needs to add tree_cnt in Patch #10. :)
OK, let's move it. :)
Thanks,
>
> Thanks,
>
> >
> > Thanks for your review! :)
> >
> > Regards,
> > Yu
> >
> > >
> > > Thanks,
> >
> > [snip]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
