I sent this as RFC do that we can continue the discussion about the
design here. Even with the above patch, we uncovered another deadlock
condition:
- 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
The fundamental problem is the same: we are trying to call
f2fs_settxattr from within the F2FS driver itself. It seems like
f2fs_init_security() should be using an internal API here. The
alternative is to remove the f2fs_balance_fs() from f2fs_setxattr() as
well as the fs_lock.
Russ
On Tue, Sep 24, 2013 at 9:39 AM, Russ W. Knize <[email protected]> wrote:
> The VFS layer already protects xattr_handler.set from concurrent
> access via an inode mutex, so there is no reason to take an fs_lock
> as well. This avoids a potential dealock:
>
> - 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.
>
> Signed-off-by: Russ Knize <[email protected]>
> ---
> fs/f2fs/xattr.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index 1ac8a5f..18b4080 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -478,7 +478,6 @@ int f2fs_setxattr(struct inode *inode, int
> name_index, const char *name,
> void *base_addr;
> int found, newsize;
> size_t name_len;
> - int ilock;
> __u32 new_hsize;
> int error = -ENOMEM;
>
> @@ -495,8 +494,6 @@ int f2fs_setxattr(struct inode *inode, int
> name_index, const char *name,
>
> f2fs_balance_fs(sbi);
>
> - ilock = mutex_lock_op(sbi);
> -
> base_addr = read_all_xattrs(inode, ipage);
> if (!base_addr)
> goto exit;
> @@ -578,7 +575,6 @@ 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;
> }
> --
> 1.7.10.4
>
>
> ------------------------------------------------------------------------------
> 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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
