Hi Russ,
The usage of fs_locks is for the recovery, so it doesn't matter
with stress-testing.
Actually what I've concerned is that we should not grab two or
more fs_locks in the same call path.
Thanks,
2013/9/11 Russ Knize <[email protected]>:
> 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
>
Jaegeuk,
My tests include forced kernel panics while fsstress is running, which
generates a lot of recovery activity. Sorry I wasn't more clear.
I understand your concern, which is why I first tried to keep the
fs_lock in the xattr_handler->set() path from VFS while removing it
from the call path I shared earlier. Doing this requires reworking
the xattr code a bit. Based on what Gu said about the inode mutex, it
seems like we can just remove it.
Russ
On Wed, Sep 11, 2013 at 8:19 AM, Kim Jaegeuk <[email protected]> wrote:
> Hi Russ,
>
> The usage of fs_locks is for the recovery, so it doesn't matter
> with stress-testing.
> Actually what I've concerned is that we should not grab two or
> more fs_locks in the same call path.
> Thanks,
>
> 2013/9/11 Russ Knize <[email protected]>:
>> 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
>>
Hi,
On 11/09/2013 21:19, Kim Jaegeuk wrote:
> Hi Russ,
>
> The usage of fs_locks is for the recovery, so it doesn't matter
> with stress-testing.
> Actually what I've concerned is that we should not grab two or
> more fs_locks in the same call path.
> Thanks,
>
I am wondering why we don't use other kind of methods like r/w semaphore
instead of the fs_locks for access control purpose. Is it due to the
little lower performance of r/w semaphore or other reasons?
I think the r/w semaphore can bring more clearer access control over
the current fs_locks, and will not suffer the problems reported here.
It can be used in a way that i/o tasks just acquire read sem while the
checkpoint task takes the write sem.
Or do we have other better method?
Regards,
Jin
> 2013/9/11 Russ Knize <[email protected]>:
>> 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
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>