Hi Jaegeuk/Gu,
I've removed the lock and have been stress-testing with SELinux and some
additional xattr torture for 24+ hours. I have not encountered any issues
yet.
My previous suggestion about moving the lock is probably not a good idea
without some significant code rework (thanks to the f2fs_balance_fs call in
f2fs_setxattr).
Russ
On Tue, Sep 10, 2013 at 10:22 PM, Gu Zheng <[email protected]> wrote:
> Hi Jaegeuk,
> On 09/10/2013 08:59 AM, Jaegeuk Kim wrote:
>
> > 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()
>
> Agree. This fs_lock here is used to protect the xattr from parallel
> modification,
> but here is in the initxattrs routine, parallel modification can not
> happen.
> And in the normal setxattr routine the inode->i_mutex (vfs layer) is used
> to
> avoid parallel modification. So I think this fs_lock is needless.
> Am I missing something?
>
> Regards,
> Gu
>
> > 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<[email protected]>
> >>
> >> 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 <[email protected]> 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 <[email protected]>
> >>
> >> 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
> >> [email protected]
> >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >
>
>
>
------------------------------------------------------------------------------
How ServiceNow helps IT people transform IT departments:
1. Consolidate legacy IT systems to a single system of record for IT
2. Standardize and globalize service processes across IT
3. Implement zero-touch automation to replace manual, redundant tasks
http://pubads.g.doubleclick.net/gampad/clk?id=51271111&iu=/4140/ostg.clktrk
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
