| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-id: <1378774750.2354.110.camel@kjgkr>
Date: Tue, 10 Sep 2013 09:59:10 +0900
From: Jaegeuk Kim <jaegeuk.kim@...sung.com>
To: chao2.yu@...sung.com
Cc: Russ Knize <Russ.Knize@...orola.com>,
"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
谭姝 <shu.tan@...sung.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-f2fs-devel@...ts.sourceforge.net"
<linux-f2fs-devel@...ts.sourceforge.net>
Subject: Re: Re: [f2fs-dev] [PATCH] f2fs: optimize fs_lock for better
performance
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<Russ.Knize@...orola.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.yu@...sung.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.yu@...sung.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;
>
> 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@...ts.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
>
>
>
>
>
>
>
>
>
>
>
--
Jaegeuk Kim
Samsung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists