Hi,
Nice catch.
This is definitely a bug where one thread grabbed two fs_locks across
the same flow.
Any idea?
Thanks,
2013-09-06 (금), 14:25 -0500, Russ Knize:
> 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
>
>
>
--
Jaegeuk Kim
Samsung
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<[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
>
>
>
>
>
>
>
>
>
>
>
>
--
Jaegeuk Kim
Samsung
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
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>