Re: [RFC 1/1] f2fs: don't GC or take an fs_lock from f2fs_initxattrs()

[Jaegeuk Kim]

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@xxxxxxxxx> wrote:
> > From: Russ Knize <Russ.Knize@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Jaegeuk Kim
Samsung

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/