All of lore.kernel.org
 help / color / mirror / Atom feed
* 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
* Re: [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, 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
* [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
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:53 [RFC 1/1] f2fs: don't GC or take an fs_lock from f2fs_initxattrs() Russ Knize
2013-09-24 20:53 ` Russ Knize
2013-09-25  8:52 ` Jaegeuk Kim
2013-09-25  8:52   ` Jaegeuk Kim
  -- strict thread matches above, loose matches on Subject: below --
2013-09-24 20:49 Russ Knize
2013-09-25  2:48 ` Gu Zheng
2013-09-25  2:48   ` Gu Zheng

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.