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@xxxxxxxxxxxx> > > 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxxxx > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > > > > > > > > > > > -- Jaegeuk Kim Samsung -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html