I wasn't sure if f2fs_setxattr() needed the lock or not, since the call
path from f2fs_xattr_set_acl() doesn't take one anywhere. My first thought
was to move the lock over to the acl code and remove it from here.
Russ
On Mon, Sep 9, 2013 at 7:59 PM, Jaegeuk Kim jaegeuk@samsung.com wrote:
Hi,
2013-09-07 (토), 08:00 +, 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 Knizeruss.kn...@motorola.com
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...@samsung.com 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...@samsung.com
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;
