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

From: Jin Xu
Date: Wed Sep 11 2013 - 19:56:00 EST

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 <Russ.Knize@xxxxxxxxxxxx>:
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 <guz.fnst@xxxxxxxxxxxxxx> 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<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

















------------------------------------------------------------------------------
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
Linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/