[Ocfs2-devel] [PATCH] ocfs2: should wait dio before inode lock in ocfs2_setattr()

[Changwei Ge]

Hi Alex,

Thanks for reporting.
I probably get your point. You mean that for a lock resource(say A), it
is used to protect metadata changing  among nodes in cluster.

Unfortunately, it was marks as BLOCKED since it was granted with a EX 
lock, and the lock can't be unblocked since it has more or equal to one 
::ex_holder(s), furthermore, since process 1 is waiting for all inflight 
dio accomplishment, it won't give up its ownership of lock source A.

Thus, hang, right?

 From code reviewing, I admit that the hang situation does exit.

But as for your patch, how can you guarantee no more bio will be issued 
from other nodes in cluster?

Also, I cc this patch to ocfs2 maintainers.

Thanks,
Changwei

On 2017/10/27 16:01, alex chen wrote:
> we should wait dio requests to finish before inode lock in
> ocfs2_setattr(), otherwise the following deadlock will be happened:
> process 1                  process 2                    process 3
> truncate file 'A'          end_io of writing file 'A'   receiving the bast messages
> ocfs2_setattr
>   ocfs2_inode_lock_tracker
>    ocfs2_inode_lock_full
>   inode_dio_wait
>    __inode_dio_wait
>    -->waiting for all dio
>    requests finish
>                                                          dlm_proxy_ast_handler
>                                                           dlm_do_local_bast
>                                                            ocfs2_blocking_ast
>                                                             ocfs2_generic_handle_bast
>                                                              set OCFS2_LOCK_BLOCKED flag
>                          dio_end_io
>                           dio_bio_end_aio
>                            dio_complete
>                             ocfs2_dio_end_io
>                              ocfs2_dio_end_io_write
>                               ocfs2_inode_lock
>                                __ocfs2_cluster_lock
>                                 ocfs2_wait_for_mask
>                                 -->waiting for OCFS2_LOCK_BLOCKED
>                                 flag to be cleared, that is waiting
>                                 for 'process 1' unlocking the inode lock
>                             inode_dio_end
>                             -->here dec the i_dio_count, but will never
>                             be called, so a deadlock happened.
> 
> Signed-off-by: Alex Chen <alex.chen at huawei.com>
> Reviewed-by: Jun Piao <piaojun at huawei.com>
> 
> ---
>   fs/ocfs2/file.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index 6e41fc8..50e09a6 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -1161,6 +1161,13 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
>   	}
>   	size_change = S_ISREG(inode->i_mode) && attr->ia_valid & ATTR_SIZE;
>   	if (size_change) {
> +
> +		/* here we should wait dio to finish before inode lock
> +		 * to avoid a deadlock between ocfs2_setattr() and
> +		 * ocfs2_dio_end_io_write()
> +		 */
> +		inode_dio_wait(inode);
> +
>   		status = ocfs2_rw_lock(inode, 1);
>   		if (status < 0) {
>   			mlog_errno(status);
> @@ -1200,8 +1207,6 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
>   		if (status)
>   			goto bail_unlock;
> 
> -		inode_dio_wait(inode);
> -
>   		if (i_size_read(inode) >= attr->ia_size) {
>   			if (ocfs2_should_order_data(inode)) {
>   				status = ocfs2_begin_ordered_truncate(inode,
>