* [RFC 1/1] f2fs: don't GC or take an fs_lock from f2fs_initxattrs()
@ 2013-09-24 20:49 Russ Knize
2013-09-25 2:48 ` Gu Zheng
0 siblings, 1 reply; 7+ messages in thread
From: Russ Knize @ 2013-09-24 20:49 UTC (permalink / raw)
To: linux-f2fs-devel; +Cc: linux-fsdevel, linux-kernel, Russ Knize
From: Russ Knize <Russ.Knize@motorola.com>
f2fs_initxattrs() is called internally from within F2FS and should
not call functions that are used by VFS handlers. This avoids
certain deadlocks:
- 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 the caller happens to grab the same fs_lock from the pool in both
places, they will deadlock. There are also deadlocks involving
multiple threads and mutexes:
- f2fs_write_begin()
- f2fs_balance_fs() <-- takes gc_mutex
- f2fs_gc()
- write_checkpoint()
- block_operations()
- mutex_lock_all() <-- blocks trying to grab all fs_locks
- f2fs_mkdir() <-- takes an fs_lock
- __f2fs_add_link()
- f2fs_init_security()
- security_inode_init_security()
- f2fs_initxattrs()
- f2fs_setxattr()
- f2fs_balance_fs() <-- blocks trying to take gc_mutex
Signed-off-by: Russ Knize <Russ.Knize@motorola.com>
---
fs/f2fs/xattr.c | 35 +++++++++++++++++++++++++----------
1 file changed, 25 insertions(+), 10 deletions(-)
diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
index 1ac8a5f..3d900ea 100644
--- a/fs/f2fs/xattr.c
+++ b/fs/f2fs/xattr.c
@@ -154,6 +154,9 @@ static int f2fs_xattr_advise_set(struct dentry *dentry, const char *name,
}
#ifdef CONFIG_F2FS_FS_SECURITY
+static int __f2fs_setxattr(struct inode *inode, int name_index,
+ const char *name, const void *value, size_t value_len,
+ struct page *ipage);
static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
void *page)
{
@@ -161,7 +164,7 @@ static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
int err = 0;
for (xattr = xattr_array; xattr->name != NULL; xattr++) {
- err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
+ err = __f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
xattr->name, xattr->value,
xattr->value_len, (struct page *)page);
if (err < 0)
@@ -469,16 +472,15 @@ cleanup:
return error;
}
-int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
- const void *value, size_t value_len, struct page *ipage)
+static int __f2fs_setxattr(struct inode *inode, int name_index,
+ const char *name, const void *value, size_t value_len,
+ struct page *ipage)
{
- struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
struct f2fs_inode_info *fi = F2FS_I(inode);
struct f2fs_xattr_entry *here, *last;
void *base_addr;
int found, newsize;
size_t name_len;
- int ilock;
__u32 new_hsize;
int error = -ENOMEM;
@@ -493,10 +495,6 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
if (name_len > F2FS_NAME_LEN || value_len > MAX_VALUE_LEN(inode))
return -ERANGE;
- f2fs_balance_fs(sbi);
-
- ilock = mutex_lock_op(sbi);
-
base_addr = read_all_xattrs(inode, ipage);
if (!base_addr)
goto exit;
@@ -578,7 +576,24 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
else
update_inode_page(inode);
exit:
- mutex_unlock_op(sbi, ilock);
kzfree(base_addr);
return error;
}
+
+int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
+ const void *value, size_t value_len, struct page *ipage)
+{
+ struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
+ int ilock;
+ int err;
+
+ f2fs_balance_fs(sbi);
+
+ ilock = mutex_lock_op(sbi);
+
+ err = __f2fs_setxattr(inode, name_index, name, value, value_len, ipage);
+
+ mutex_unlock_op(sbi, ilock);
+
+ return err;
+}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [RFC 1/1] f2fs: don't GC or take an fs_lock from f2fs_initxattrs()
2013-09-24 20:49 [RFC 1/1] f2fs: don't GC or take an fs_lock from f2fs_initxattrs() Russ Knize
@ 2013-09-25 2:48 ` Gu Zheng
0 siblings, 0 replies; 7+ messages in thread
From: Gu Zheng @ 2013-09-25 2:48 UTC (permalink / raw)
To: Russ Knize; +Cc: linux-f2fs-devel, linux-fsdevel, linux-kernel, Russ Knize
On 09/25/2013 04:49 AM, Russ Knize wrote:
> From: Russ Knize <Russ.Knize@motorola.com>
>
> f2fs_initxattrs() is called internally from within F2FS and should
> not call functions that are used by VFS handlers. This avoids
> certain deadlocks:
>
> - 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 the caller happens to grab the same fs_lock from the pool in both
> places, they will deadlock. There are also deadlocks involving
> multiple threads and mutexes:
>
> - f2fs_write_begin()
> - f2fs_balance_fs() <-- takes gc_mutex
> - f2fs_gc()
> - write_checkpoint()
> - block_operations()
> - mutex_lock_all() <-- blocks trying to grab all fs_locks
>
> - f2fs_mkdir() <-- takes an fs_lock
> - __f2fs_add_link()
> - f2fs_init_security()
> - security_inode_init_security()
> - f2fs_initxattrs()
> - f2fs_setxattr()
> - f2fs_balance_fs() <-- blocks trying to take gc_mutex
>
> Signed-off-by: Russ Knize <Russ.Knize@motorola.com>
This solution is more thorough.
Reviewed-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> ---
> fs/f2fs/xattr.c | 35 +++++++++++++++++++++++++----------
> 1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index 1ac8a5f..3d900ea 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -154,6 +154,9 @@ static int f2fs_xattr_advise_set(struct dentry *dentry, const char *name,
> }
>
> #ifdef CONFIG_F2FS_FS_SECURITY
> +static int __f2fs_setxattr(struct inode *inode, int name_index,
> + const char *name, const void *value, size_t value_len,
> + struct page *ipage);
> static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
> void *page)
> {
> @@ -161,7 +164,7 @@ static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
> int err = 0;
>
> for (xattr = xattr_array; xattr->name != NULL; xattr++) {
> - err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
> + err = __f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
> xattr->name, xattr->value,
> xattr->value_len, (struct page *)page);
> if (err < 0)
> @@ -469,16 +472,15 @@ cleanup:
> return error;
> }
>
> -int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> - const void *value, size_t value_len, struct page *ipage)
> +static int __f2fs_setxattr(struct inode *inode, int name_index,
> + const char *name, const void *value, size_t value_len,
> + struct page *ipage)
> {
> - struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> struct f2fs_inode_info *fi = F2FS_I(inode);
> struct f2fs_xattr_entry *here, *last;
> void *base_addr;
> int found, newsize;
> size_t name_len;
> - int ilock;
> __u32 new_hsize;
> int error = -ENOMEM;
>
> @@ -493,10 +495,6 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> if (name_len > F2FS_NAME_LEN || value_len > MAX_VALUE_LEN(inode))
> return -ERANGE;
>
> - f2fs_balance_fs(sbi);
> -
> - ilock = mutex_lock_op(sbi);
> -
> base_addr = read_all_xattrs(inode, ipage);
> if (!base_addr)
> goto exit;
> @@ -578,7 +576,24 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> else
> update_inode_page(inode);
> exit:
> - mutex_unlock_op(sbi, ilock);
> kzfree(base_addr);
> return error;
> }
> +
> +int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> + const void *value, size_t value_len, struct page *ipage)
> +{
> + struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> + int ilock;
> + int err;
> +
> + f2fs_balance_fs(sbi);
> +
> + ilock = mutex_lock_op(sbi);
> +
> + err = __f2fs_setxattr(inode, name_index, name, value, value_len, ipage);
> +
> + mutex_unlock_op(sbi, ilock);
> +
> + return err;
> +}
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [RFC 1/1] f2fs: don't GC or take an fs_lock from f2fs_initxattrs()
@ 2013-09-25 2:48 ` Gu Zheng
0 siblings, 0 replies; 7+ messages in thread
From: Gu Zheng @ 2013-09-25 2:48 UTC (permalink / raw)
To: Russ Knize; +Cc: linux-fsdevel, Russ Knize, linux-kernel, linux-f2fs-devel
On 09/25/2013 04:49 AM, Russ Knize wrote:
> From: Russ Knize <Russ.Knize@motorola.com>
>
> f2fs_initxattrs() is called internally from within F2FS and should
> not call functions that are used by VFS handlers. This avoids
> certain deadlocks:
>
> - 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 the caller happens to grab the same fs_lock from the pool in both
> places, they will deadlock. There are also deadlocks involving
> multiple threads and mutexes:
>
> - f2fs_write_begin()
> - f2fs_balance_fs() <-- takes gc_mutex
> - f2fs_gc()
> - write_checkpoint()
> - block_operations()
> - mutex_lock_all() <-- blocks trying to grab all fs_locks
>
> - f2fs_mkdir() <-- takes an fs_lock
> - __f2fs_add_link()
> - f2fs_init_security()
> - security_inode_init_security()
> - f2fs_initxattrs()
> - f2fs_setxattr()
> - f2fs_balance_fs() <-- blocks trying to take gc_mutex
>
> Signed-off-by: Russ Knize <Russ.Knize@motorola.com>
This solution is more thorough.
Reviewed-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> ---
> fs/f2fs/xattr.c | 35 +++++++++++++++++++++++++----------
> 1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index 1ac8a5f..3d900ea 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -154,6 +154,9 @@ static int f2fs_xattr_advise_set(struct dentry *dentry, const char *name,
> }
>
> #ifdef CONFIG_F2FS_FS_SECURITY
> +static int __f2fs_setxattr(struct inode *inode, int name_index,
> + const char *name, const void *value, size_t value_len,
> + struct page *ipage);
> static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
> void *page)
> {
> @@ -161,7 +164,7 @@ static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
> int err = 0;
>
> for (xattr = xattr_array; xattr->name != NULL; xattr++) {
> - err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
> + err = __f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
> xattr->name, xattr->value,
> xattr->value_len, (struct page *)page);
> if (err < 0)
> @@ -469,16 +472,15 @@ cleanup:
> return error;
> }
>
> -int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> - const void *value, size_t value_len, struct page *ipage)
> +static int __f2fs_setxattr(struct inode *inode, int name_index,
> + const char *name, const void *value, size_t value_len,
> + struct page *ipage)
> {
> - struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> struct f2fs_inode_info *fi = F2FS_I(inode);
> struct f2fs_xattr_entry *here, *last;
> void *base_addr;
> int found, newsize;
> size_t name_len;
> - int ilock;
> __u32 new_hsize;
> int error = -ENOMEM;
>
> @@ -493,10 +495,6 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> if (name_len > F2FS_NAME_LEN || value_len > MAX_VALUE_LEN(inode))
> return -ERANGE;
>
> - f2fs_balance_fs(sbi);
> -
> - ilock = mutex_lock_op(sbi);
> -
> base_addr = read_all_xattrs(inode, ipage);
> if (!base_addr)
> goto exit;
> @@ -578,7 +576,24 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> else
> update_inode_page(inode);
> exit:
> - mutex_unlock_op(sbi, ilock);
> kzfree(base_addr);
> return error;
> }
> +
> +int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> + const void *value, size_t value_len, struct page *ipage)
> +{
> + struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> + int ilock;
> + int err;
> +
> + f2fs_balance_fs(sbi);
> +
> + ilock = mutex_lock_op(sbi);
> +
> + err = __f2fs_setxattr(inode, name_index, name, value, value_len, ipage);
> +
> + mutex_unlock_op(sbi, ilock);
> +
> + return err;
> +}
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60133471&iu=/4140/ostg.clktrk
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC 1/1] f2fs: don't GC or take an fs_lock from f2fs_initxattrs()
@ 2013-09-24 20:53 ` Russ Knize
0 siblings, 0 replies; 7+ messages in thread
From: Russ Knize @ 2013-09-24 20:53 UTC (permalink / raw)
To: Russ Knize; +Cc: linux-f2fs-devel, linux-fsdevel, linux-kernel
This is an alternate solution to the deadlock problem I mentioned in
my previous patch.
On Tue, Sep 24, 2013 at 3:49 PM, Russ Knize <rknize@gmail.com> wrote:
> From: Russ Knize <Russ.Knize@motorola.com>
>
> f2fs_initxattrs() is called internally from within F2FS and should
> not call functions that are used by VFS handlers. This avoids
> certain deadlocks:
>
> - 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 the caller happens to grab the same fs_lock from the pool in both
> places, they will deadlock. There are also deadlocks involving
> multiple threads and mutexes:
>
> - f2fs_write_begin()
> - f2fs_balance_fs() <-- takes gc_mutex
> - f2fs_gc()
> - write_checkpoint()
> - block_operations()
> - mutex_lock_all() <-- blocks trying to grab all fs_locks
>
> - f2fs_mkdir() <-- takes an fs_lock
> - __f2fs_add_link()
> - f2fs_init_security()
> - security_inode_init_security()
> - f2fs_initxattrs()
> - f2fs_setxattr()
> - f2fs_balance_fs() <-- blocks trying to take gc_mutex
>
> Signed-off-by: Russ Knize <Russ.Knize@motorola.com>
> ---
> fs/f2fs/xattr.c | 35 +++++++++++++++++++++++++----------
> 1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index 1ac8a5f..3d900ea 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -154,6 +154,9 @@ static int f2fs_xattr_advise_set(struct dentry *dentry, const char *name,
> }
>
> #ifdef CONFIG_F2FS_FS_SECURITY
> +static int __f2fs_setxattr(struct inode *inode, int name_index,
> + const char *name, const void *value, size_t value_len,
> + struct page *ipage);
> static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
> void *page)
> {
> @@ -161,7 +164,7 @@ static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
> int err = 0;
>
> for (xattr = xattr_array; xattr->name != NULL; xattr++) {
> - err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
> + err = __f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
> xattr->name, xattr->value,
> xattr->value_len, (struct page *)page);
> if (err < 0)
> @@ -469,16 +472,15 @@ cleanup:
> return error;
> }
>
> -int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> - const void *value, size_t value_len, struct page *ipage)
> +static int __f2fs_setxattr(struct inode *inode, int name_index,
> + const char *name, const void *value, size_t value_len,
> + struct page *ipage)
> {
> - struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> struct f2fs_inode_info *fi = F2FS_I(inode);
> struct f2fs_xattr_entry *here, *last;
> void *base_addr;
> int found, newsize;
> size_t name_len;
> - int ilock;
> __u32 new_hsize;
> int error = -ENOMEM;
>
> @@ -493,10 +495,6 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> if (name_len > F2FS_NAME_LEN || value_len > MAX_VALUE_LEN(inode))
> return -ERANGE;
>
> - f2fs_balance_fs(sbi);
> -
> - ilock = mutex_lock_op(sbi);
> -
> base_addr = read_all_xattrs(inode, ipage);
> if (!base_addr)
> goto exit;
> @@ -578,7 +576,24 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> else
> update_inode_page(inode);
> exit:
> - mutex_unlock_op(sbi, ilock);
> kzfree(base_addr);
> return error;
> }
> +
> +int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> + const void *value, size_t value_len, struct page *ipage)
> +{
> + struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> + int ilock;
> + int err;
> +
> + f2fs_balance_fs(sbi);
> +
> + ilock = mutex_lock_op(sbi);
> +
> + err = __f2fs_setxattr(inode, name_index, name, value, value_len, ipage);
> +
> + mutex_unlock_op(sbi, ilock);
> +
> + return err;
> +}
> --
> 1.7.10.4
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [RFC 1/1] f2fs: don't GC or take an fs_lock from f2fs_initxattrs()
@ 2013-09-24 20:53 ` Russ Knize
0 siblings, 0 replies; 7+ messages in thread
From: Russ Knize @ 2013-09-24 20:53 UTC (permalink / raw)
To: Russ Knize; +Cc: linux-f2fs-devel, linux-fsdevel, linux-kernel
This is an alternate solution to the deadlock problem I mentioned in
my previous patch.
On Tue, Sep 24, 2013 at 3:49 PM, Russ Knize <rknize@gmail.com> wrote:
> From: Russ Knize <Russ.Knize@motorola.com>
>
> f2fs_initxattrs() is called internally from within F2FS and should
> not call functions that are used by VFS handlers. This avoids
> certain deadlocks:
>
> - 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 the caller happens to grab the same fs_lock from the pool in both
> places, they will deadlock. There are also deadlocks involving
> multiple threads and mutexes:
>
> - f2fs_write_begin()
> - f2fs_balance_fs() <-- takes gc_mutex
> - f2fs_gc()
> - write_checkpoint()
> - block_operations()
> - mutex_lock_all() <-- blocks trying to grab all fs_locks
>
> - f2fs_mkdir() <-- takes an fs_lock
> - __f2fs_add_link()
> - f2fs_init_security()
> - security_inode_init_security()
> - f2fs_initxattrs()
> - f2fs_setxattr()
> - f2fs_balance_fs() <-- blocks trying to take gc_mutex
>
> Signed-off-by: Russ Knize <Russ.Knize@motorola.com>
> ---
> fs/f2fs/xattr.c | 35 +++++++++++++++++++++++++----------
> 1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index 1ac8a5f..3d900ea 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -154,6 +154,9 @@ static int f2fs_xattr_advise_set(struct dentry *dentry, const char *name,
> }
>
> #ifdef CONFIG_F2FS_FS_SECURITY
> +static int __f2fs_setxattr(struct inode *inode, int name_index,
> + const char *name, const void *value, size_t value_len,
> + struct page *ipage);
> static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
> void *page)
> {
> @@ -161,7 +164,7 @@ static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
> int err = 0;
>
> for (xattr = xattr_array; xattr->name != NULL; xattr++) {
> - err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
> + err = __f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
> xattr->name, xattr->value,
> xattr->value_len, (struct page *)page);
> if (err < 0)
> @@ -469,16 +472,15 @@ cleanup:
> return error;
> }
>
> -int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> - const void *value, size_t value_len, struct page *ipage)
> +static int __f2fs_setxattr(struct inode *inode, int name_index,
> + const char *name, const void *value, size_t value_len,
> + struct page *ipage)
> {
> - struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> struct f2fs_inode_info *fi = F2FS_I(inode);
> struct f2fs_xattr_entry *here, *last;
> void *base_addr;
> int found, newsize;
> size_t name_len;
> - int ilock;
> __u32 new_hsize;
> int error = -ENOMEM;
>
> @@ -493,10 +495,6 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> if (name_len > F2FS_NAME_LEN || value_len > MAX_VALUE_LEN(inode))
> return -ERANGE;
>
> - f2fs_balance_fs(sbi);
> -
> - ilock = mutex_lock_op(sbi);
> -
> base_addr = read_all_xattrs(inode, ipage);
> if (!base_addr)
> goto exit;
> @@ -578,7 +576,24 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> else
> update_inode_page(inode);
> exit:
> - mutex_unlock_op(sbi, ilock);
> kzfree(base_addr);
> return error;
> }
> +
> +int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> + const void *value, size_t value_len, struct page *ipage)
> +{
> + struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> + int ilock;
> + int err;
> +
> + f2fs_balance_fs(sbi);
> +
> + ilock = mutex_lock_op(sbi);
> +
> + err = __f2fs_setxattr(inode, name_index, name, value, value_len, ipage);
> +
> + mutex_unlock_op(sbi, ilock);
> +
> + return err;
> +}
> --
> 1.7.10.4
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [RFC 1/1] f2fs: don't GC or take an fs_lock from f2fs_initxattrs()
2013-09-24 20:53 ` Russ Knize
@ 2013-09-25 8:52 ` Jaegeuk Kim
-1 siblings, 0 replies; 7+ messages in thread
From: Jaegeuk Kim @ 2013-09-25 8:52 UTC (permalink / raw)
To: Russ Knize; +Cc: Russ Knize, linux-f2fs-devel, linux-fsdevel, linux-kernel
Hi Russ,
That's what I wanted before.
Merged.
Thanks,
2013-09-24 (화), 15:53 -0500, Russ Knize:
> This is an alternate solution to the deadlock problem I mentioned in
> my previous patch.
>
> On Tue, Sep 24, 2013 at 3:49 PM, Russ Knize <rknize@gmail.com> wrote:
> > From: Russ Knize <Russ.Knize@motorola.com>
> >
> > f2fs_initxattrs() is called internally from within F2FS and should
> > not call functions that are used by VFS handlers. This avoids
> > certain deadlocks:
> >
> > - 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 the caller happens to grab the same fs_lock from the pool in both
> > places, they will deadlock. There are also deadlocks involving
> > multiple threads and mutexes:
> >
> > - f2fs_write_begin()
> > - f2fs_balance_fs() <-- takes gc_mutex
> > - f2fs_gc()
> > - write_checkpoint()
> > - block_operations()
> > - mutex_lock_all() <-- blocks trying to grab all fs_locks
> >
> > - f2fs_mkdir() <-- takes an fs_lock
> > - __f2fs_add_link()
> > - f2fs_init_security()
> > - security_inode_init_security()
> > - f2fs_initxattrs()
> > - f2fs_setxattr()
> > - f2fs_balance_fs() <-- blocks trying to take gc_mutex
> >
> > Signed-off-by: Russ Knize <Russ.Knize@motorola.com>
> > ---
> > fs/f2fs/xattr.c | 35 +++++++++++++++++++++++++----------
> > 1 file changed, 25 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> > index 1ac8a5f..3d900ea 100644
> > --- a/fs/f2fs/xattr.c
> > +++ b/fs/f2fs/xattr.c
> > @@ -154,6 +154,9 @@ static int f2fs_xattr_advise_set(struct dentry *dentry, const char *name,
> > }
> >
> > #ifdef CONFIG_F2FS_FS_SECURITY
> > +static int __f2fs_setxattr(struct inode *inode, int name_index,
> > + const char *name, const void *value, size_t value_len,
> > + struct page *ipage);
> > static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
> > void *page)
> > {
> > @@ -161,7 +164,7 @@ static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
> > int err = 0;
> >
> > for (xattr = xattr_array; xattr->name != NULL; xattr++) {
> > - err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
> > + err = __f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
> > xattr->name, xattr->value,
> > xattr->value_len, (struct page *)page);
> > if (err < 0)
> > @@ -469,16 +472,15 @@ cleanup:
> > return error;
> > }
> >
> > -int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> > - const void *value, size_t value_len, struct page *ipage)
> > +static int __f2fs_setxattr(struct inode *inode, int name_index,
> > + const char *name, const void *value, size_t value_len,
> > + struct page *ipage)
> > {
> > - struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> > struct f2fs_inode_info *fi = F2FS_I(inode);
> > struct f2fs_xattr_entry *here, *last;
> > void *base_addr;
> > int found, newsize;
> > size_t name_len;
> > - int ilock;
> > __u32 new_hsize;
> > int error = -ENOMEM;
> >
> > @@ -493,10 +495,6 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> > if (name_len > F2FS_NAME_LEN || value_len > MAX_VALUE_LEN(inode))
> > return -ERANGE;
> >
> > - f2fs_balance_fs(sbi);
> > -
> > - ilock = mutex_lock_op(sbi);
> > -
> > base_addr = read_all_xattrs(inode, ipage);
> > if (!base_addr)
> > goto exit;
> > @@ -578,7 +576,24 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> > else
> > update_inode_page(inode);
> > exit:
> > - mutex_unlock_op(sbi, ilock);
> > kzfree(base_addr);
> > return error;
> > }
> > +
> > +int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> > + const void *value, size_t value_len, struct page *ipage)
> > +{
> > + struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> > + int ilock;
> > + int err;
> > +
> > + f2fs_balance_fs(sbi);
> > +
> > + ilock = mutex_lock_op(sbi);
> > +
> > + err = __f2fs_setxattr(inode, name_index, name, value, value_len, ipage);
> > +
> > + mutex_unlock_op(sbi, ilock);
> > +
> > + return err;
> > +}
> > --
> > 1.7.10.4
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Jaegeuk Kim
Samsung
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [RFC 1/1] f2fs: don't GC or take an fs_lock from f2fs_initxattrs()
@ 2013-09-25 8:52 ` Jaegeuk Kim
0 siblings, 0 replies; 7+ messages in thread
From: Jaegeuk Kim @ 2013-09-25 8:52 UTC (permalink / raw)
To: Russ Knize; +Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel
Hi Russ,
That's what I wanted before.
Merged.
Thanks,
2013-09-24 (화), 15:53 -0500, Russ Knize:
> This is an alternate solution to the deadlock problem I mentioned in
> my previous patch.
>
> On Tue, Sep 24, 2013 at 3:49 PM, Russ Knize <rknize@gmail.com> wrote:
> > From: Russ Knize <Russ.Knize@motorola.com>
> >
> > f2fs_initxattrs() is called internally from within F2FS and should
> > not call functions that are used by VFS handlers. This avoids
> > certain deadlocks:
> >
> > - 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 the caller happens to grab the same fs_lock from the pool in both
> > places, they will deadlock. There are also deadlocks involving
> > multiple threads and mutexes:
> >
> > - f2fs_write_begin()
> > - f2fs_balance_fs() <-- takes gc_mutex
> > - f2fs_gc()
> > - write_checkpoint()
> > - block_operations()
> > - mutex_lock_all() <-- blocks trying to grab all fs_locks
> >
> > - f2fs_mkdir() <-- takes an fs_lock
> > - __f2fs_add_link()
> > - f2fs_init_security()
> > - security_inode_init_security()
> > - f2fs_initxattrs()
> > - f2fs_setxattr()
> > - f2fs_balance_fs() <-- blocks trying to take gc_mutex
> >
> > Signed-off-by: Russ Knize <Russ.Knize@motorola.com>
> > ---
> > fs/f2fs/xattr.c | 35 +++++++++++++++++++++++++----------
> > 1 file changed, 25 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> > index 1ac8a5f..3d900ea 100644
> > --- a/fs/f2fs/xattr.c
> > +++ b/fs/f2fs/xattr.c
> > @@ -154,6 +154,9 @@ static int f2fs_xattr_advise_set(struct dentry *dentry, const char *name,
> > }
> >
> > #ifdef CONFIG_F2FS_FS_SECURITY
> > +static int __f2fs_setxattr(struct inode *inode, int name_index,
> > + const char *name, const void *value, size_t value_len,
> > + struct page *ipage);
> > static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
> > void *page)
> > {
> > @@ -161,7 +164,7 @@ static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
> > int err = 0;
> >
> > for (xattr = xattr_array; xattr->name != NULL; xattr++) {
> > - err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
> > + err = __f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
> > xattr->name, xattr->value,
> > xattr->value_len, (struct page *)page);
> > if (err < 0)
> > @@ -469,16 +472,15 @@ cleanup:
> > return error;
> > }
> >
> > -int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> > - const void *value, size_t value_len, struct page *ipage)
> > +static int __f2fs_setxattr(struct inode *inode, int name_index,
> > + const char *name, const void *value, size_t value_len,
> > + struct page *ipage)
> > {
> > - struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> > struct f2fs_inode_info *fi = F2FS_I(inode);
> > struct f2fs_xattr_entry *here, *last;
> > void *base_addr;
> > int found, newsize;
> > size_t name_len;
> > - int ilock;
> > __u32 new_hsize;
> > int error = -ENOMEM;
> >
> > @@ -493,10 +495,6 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> > if (name_len > F2FS_NAME_LEN || value_len > MAX_VALUE_LEN(inode))
> > return -ERANGE;
> >
> > - f2fs_balance_fs(sbi);
> > -
> > - ilock = mutex_lock_op(sbi);
> > -
> > base_addr = read_all_xattrs(inode, ipage);
> > if (!base_addr)
> > goto exit;
> > @@ -578,7 +576,24 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> > else
> > update_inode_page(inode);
> > exit:
> > - mutex_unlock_op(sbi, ilock);
> > kzfree(base_addr);
> > return error;
> > }
> > +
> > +int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> > + const void *value, size_t value_len, struct page *ipage)
> > +{
> > + struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> > + int ilock;
> > + int err;
> > +
> > + f2fs_balance_fs(sbi);
> > +
> > + ilock = mutex_lock_op(sbi);
> > +
> > + err = __f2fs_setxattr(inode, name_index, name, value, value_len, ipage);
> > +
> > + mutex_unlock_op(sbi, ilock);
> > +
> > + return err;
> > +}
> > --
> > 1.7.10.4
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Jaegeuk Kim
Samsung
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60133471&iu=/4140/ostg.clktrk
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-09-25 8:53 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-24 20:49 [RFC 1/1] f2fs: don't GC or take an fs_lock from f2fs_initxattrs() Russ Knize
2013-09-25 2:48 ` Gu Zheng
2013-09-25 2:48 ` Gu Zheng
2013-09-24 20:53 Russ Knize
2013-09-24 20:53 ` Russ Knize
2013-09-25 8:52 ` Jaegeuk Kim
2013-09-25 8:52 ` Jaegeuk Kim
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.