lkml.org 
[lkml]   [2015]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [f2fs-dev][RFC PATCH 06/10] f2fs: add core functions for rb-tree extent cache
Date
Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Saturday, January 24, 2015 3:43 AM
> To: Chao Yu
> Cc: 'Changman Lee'; linux-f2fs-devel@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:jaegeuk@kernel.org]
> > > Sent: Friday, January 23, 2015 9:48 AM
> > > To: Chao Yu
> > > Cc: Changman Lee; linux-f2fs-devel@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]



\
 
 \ /
  Last update: 2015-01-26 07:21    [W:0.078 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site