* [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; 4+ 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] 4+ 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; 4+ 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] 4+ 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
0 siblings, 1 reply; 4+ messages in thread
From: Russ Knize @ 2013-09-24 20:53 UTC (permalink / raw)
To: Russ Knize
Cc: linux-f2fs-devel@lists.sourceforge.net,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
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] 4+ 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
0 siblings, 0 replies; 4+ messages in thread
From: Jaegeuk Kim @ 2013-09-25 8:52 UTC (permalink / raw)
To: Russ Knize
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net
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] 4+ messages in thread
end of thread, other threads:[~2013-09-25 8:52 UTC | newest]
Thread overview: 4+ 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
-- strict thread matches above, loose matches on Subject: below --
2013-09-24 20:53 Russ Knize
2013-09-25 8:52 ` Jaegeuk Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).