Re: Re: [f2fs-dev] [PATCH] f2fs: optimize fs_lock for better performance

[Jaegeuk Kim <jaegeuk.kim@samsung.com>]

Hi,

2013-09-07 (토), 08:00 +0000, Chao Yu:
> Hi Knize,
> 
>     Thanks for your reply, I think it's actually meaningless that it's
> being named after "spin_lock",
> it's better to rename this spinlock to "round_robin_lock".
> 
>     This patch can only resolve the issue of unbalanced fs_lock usage,
> it can not fix the deadlock issue.
> can we fix deadlock issue through this method:
> 
> - vfs_create()
>  - f2fs_create() - takes an fs_lock and save current thread info into
> thread_info[NR_GLOBAL_LOCKS]
>   - f2fs_add_link()
>    - __f2fs_add_link()
>     - init_inode_metadata()
>      - f2fs_init_security()
>       - security_inode_init_security()
>        - f2fs_initxattrs()
>         - f2fs_setxattr() - get fs_lock only if there is no current
> thread info in thread_info
>         
> So it keeps one thread can only hold one fs_lock to avoid deadlock.
> Can we use this solution?

It could be.
But, I think we can avoid to grab the fs_lock at the f2fs_initxattrs()
level, since this case only happens when f2fs_initxattrs() is called.
Let's think about ut in more detail.
Thanks,

> 
>  
> 
> thanks again!
> 
>  
> 
> ------- Original Message -------
> 
> Sender : Russ Knize<Russ.Knize@motorola.com>
> 
> Date : 九月 07, 2013 04:25 (GMT+09:00)
> 
> Title : Re: [f2fs-dev] [PATCH] f2fs: optimize fs_lock for better
> performance
> 
>  
> 
> I encountered this same issue recently and solved it in much the same
> way.  Can we rename "spin_lock" to something more meaningful? 
> 
> 
> This race actually exposed a potential deadlock between f2fs_create()
> and f2fs_initxattrs(): 
> 
> 
> - vfs_create()
>  - f2fs_create() - takes an fs_lock
>   - f2fs_add_link()
>    - __f2fs_add_link()
>     - init_inode_metadata()
>      - f2fs_init_security()
>       - security_inode_init_security()
>        - f2fs_initxattrs()
>         - f2fs_setxattr() - also takes an fs_lock
> 
> 
> If another CPU happens to have the same lock that f2fs_setxattr() was
> trying to take because of the race around next_lock_num, we can get
> into a deadlock situation if the two threads are also contending over
> another resource (like bdi).
> 
> 
> Another scenario is if the above happens while another thread is in
> the middle of grabbing all of the locks via mutex_lock_all().
>  f2fs_create() is holding a lock that mutex_lock_all() is waiting for
> and mutex_lock_all() is holding a lock that f2fs_setxattr() is waiting
> for.
> 
> 
> Russ
> 
> 
> On Fri, Sep 6, 2013 at 4:48 AM, Chao Yu <chao2.yu@samsung.com> wrote:
>         Hi Kim:
>         
>              I think there is a performance problem: when all
>         sbi->fs_lock is holded, 
>         
>         then all other threads may get the same next_lock value from
>         sbi->next_lock_num in function mutex_lock_op, 
>         
>         and wait to get the same lock at position fs_lock[next_lock],
>         it unbalance the fs_lock usage. 
>         
>         It may lost performance when we do the multithread test.
>         
>          
>         
>         Here is the patch to fix this problem:
>         
>          
>         
>         Signed-off-by: Yu Chao <chao2.yu@samsung.com>
>         
>         diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>         
>         old mode 100644
>         
>         new mode 100755
>         
>         index 467d42d..983bb45
>         
>         --- a/fs/f2fs/f2fs.h
>         
>         +++ b/fs/f2fs/f2fs.h
>         
>         @@ -371,6 +371,7 @@ struct f2fs_sb_info {
>         
>                 struct mutex fs_lock[NR_GLOBAL_LOCKS];  /* blocking FS
>         operations */
>         
>                 struct mutex node_write;                /* locking
>         node writes */
>         
>                 struct mutex writepages;                /* mutex for
>         writepages() */
>         
>         +       spinlock_t spin_lock;                   /* lock for
>         next_lock_num */
>         
>                 unsigned char next_lock_num;            /* round-robin
>         global locks */
>         
>                 int por_doing;                          /* recovery is
>         doing or not */
>         
>                 int on_build_free_nids;                 /*
>         build_free_nids is doing */
>         
>         @@ -533,15 +534,19 @@ static inline void
>         mutex_unlock_all(struct f2fs_sb_info *sbi)
>         
>          
>         
>          static inline int mutex_lock_op(struct f2fs_sb_info *sbi)
>         
>          {
>         
>         -       unsigned char next_lock = sbi->next_lock_num %
>         NR_GLOBAL_LOCKS;
>         
>         +       unsigned char next_lock;
>         
>                 int i = 0;
>         
>          
>         
>                 for (; i < NR_GLOBAL_LOCKS; i++)
>         
>                         if (mutex_trylock(&sbi->fs_lock[i]))
>         
>                                 return i;
>         
>          
>         
>         -       mutex_lock(&sbi->fs_lock[next_lock]);
>         
>         +       spin_lock(&sbi->spin_lock);
>         
>         +       next_lock = sbi->next_lock_num % NR_GLOBAL_LOCKS;
>         
>                 sbi->next_lock_num++;
>         
>         +       spin_unlock(&sbi->spin_lock);
>         
>         +
>         
>         +       mutex_lock(&sbi->fs_lock[next_lock]);
>         
>                 return next_lock;
>         
>          }
>         
>          
>         
>         diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>         
>         old mode 100644
>         
>         new mode 100755
>         
>         index 75c7dc3..4f27596
>         
>         --- a/fs/f2fs/super.c
>         
>         +++ b/fs/f2fs/super.c
>         
>         @@ -657,6 +657,7 @@ static int f2fs_fill_super(struct
>         super_block *sb, void *data, int silent)
>         
>                 mutex_init(&sbi->cp_mutex);
>         
>                 for (i = 0; i < NR_GLOBAL_LOCKS; i++)
>         
>                         mutex_init(&sbi->fs_lock[i]);
>         
>         +       spin_lock_init(&sbi->spin_lock);
>         
>                 mutex_init(&sbi->node_write);
>         
>                 sbi->por_doing = 0;
>         
>                 spin_lock_init(&sbi->stat_lock);
>         
>         (END)
>         
>          
>         
>         
>         
>         
>         
>         ------------------------------------------------------------------------------
>         Learn the latest--Visual Studio 2012, SharePoint 2013, SQL
>         2012, more!
>         Discover the easy way to master current and previous Microsoft
>         technologies
>         and advance your career. Get an incredible 1,500+ hours of
>         step-by-step
>         tutorial videos with LearnDevNow. Subscribe today and save!
>         http://pubads.g.doubleclick.net/gampad/clk?id=58041391&iu=/4140/ostg.clktrk
>         _______________________________________________
>         Linux-f2fs-devel mailing list
>         Linux-f2fs-devel@lists.sourceforge.net
>         https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>         
> 
> 
>  
> 
>  
> 
>  
> 
> 
> 
> 

-- 
Jaegeuk Kim
Samsung