Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753529AbdFVVYr (ORCPT ); Thu, 22 Jun 2017 17:24:47 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:49290 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753680AbdFVVYp (ORCPT ); Thu, 22 Jun 2017 17:24:45 -0400 Date: Thu, 22 Jun 2017 14:24:44 -0700 From: Andrew Morton To: Joseph Qi Cc: Eric Ren , ocfs2-devel@oss.oracle.com, mfasheh@versity.com, jlbec@evilplan.org, tv@lio96.de, ghe@suse.com, junxiao.bi@oracle.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] ocfs2: fix deadlock caused by recursive locking in xattr Message-Id: <20170622142444.7bb5873c27c105bbb90fdfec@linux-foundation.org> In-Reply-To: <8348cb21-60e1-f0a2-54c3-e36a2e9ddf7b@gmail.com> References: <20170622014746.5815-1-zren@suse.com> <8348cb21-60e1-f0a2-54c3-e36a2e9ddf7b@gmail.com> X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5641 Lines: 144 On Thu, 22 Jun 2017 14:10:38 +0800 Joseph Qi wrote: > Looks good. > Reviewed-by: Joseph Qi Should this fix be backported into -stable kernels? > On 17/6/22 09:47, Eric Ren wrote: > > Another deadlock path caused by recursive locking is reported. > > This kind of issue was introduced since commit 743b5f1434f5 ("ocfs2: > > take inode lock in ocfs2_iop_set/get_acl()"). Two deadlock paths > > have been fixed by commit b891fa5024a9 ("ocfs2: fix deadlock issue when > > taking inode lock at vfs entry points"). Yes, we intend to fix this > > kind of case in incremental way, because it's hard to find out all > > possible paths at once. > > > > This one can be reproduced like this. On node1, cp a large file from > > home directory to ocfs2 mountpoint. While on node2, run setfacl/getfacl. > > Both nodes will hang up there. The backtraces: > > > > On node1: > > [] __ocfs2_cluster_lock.isra.39+0x357/0x740 [ocfs2] > > [] ocfs2_inode_lock_full_nested+0x17d/0x840 [ocfs2] > > [] ocfs2_write_begin+0x43/0x1a0 [ocfs2] > > [] generic_perform_write+0xa9/0x180 > > [] __generic_file_write_iter+0x1aa/0x1d0 > > [] ocfs2_file_write_iter+0x4f4/0xb40 [ocfs2] > > [] __vfs_write+0xc3/0x130 > > [] vfs_write+0xb1/0x1a0 > > [] SyS_write+0x46/0xa0 > > > > On node2: > > [] __ocfs2_cluster_lock.isra.39+0x357/0x740 [ocfs2] > > [] ocfs2_inode_lock_full_nested+0x17d/0x840 [ocfs2] > > [] ocfs2_xattr_set+0x12e/0xe80 [ocfs2] > > [] ocfs2_set_acl+0x22d/0x260 [ocfs2] > > [] ocfs2_iop_set_acl+0x65/0xb0 [ocfs2] > > [] set_posix_acl+0x75/0xb0 > > [] posix_acl_xattr_set+0x49/0xa0 > > [] __vfs_setxattr+0x69/0x80 > > [] __vfs_setxattr_noperm+0x72/0x1a0 > > [] vfs_setxattr+0xa7/0xb0 > > [] setxattr+0x12d/0x190 > > [] path_setxattr+0x9f/0xb0 > > [] SyS_setxattr+0x14/0x20 > > > > Fixes this one by using ocfs2_inode_{lock|unlock}_tracker, which is > > exported by commit 439a36b8ef38 ("ocfs2/dlmglue: prepare tracking > > logic to avoid recursive cluster lock"). > > > > Changes since v1: > > - Revised git commit description style in commit log. > > > > Reported-by: Thomas Voegtle > > Tested-by: Thomas Voegtle > > Signed-off-by: Eric Ren > > --- > > fs/ocfs2/dlmglue.c | 4 ++++ > > fs/ocfs2/xattr.c | 23 +++++++++++++---------- > > 2 files changed, 17 insertions(+), 10 deletions(-) > > > > diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c > > index 3b7c937..4689940 100644 > > --- a/fs/ocfs2/dlmglue.c > > +++ b/fs/ocfs2/dlmglue.c > > @@ -2591,6 +2591,10 @@ void ocfs2_inode_unlock_tracker(struct inode *inode, > > struct ocfs2_lock_res *lockres; > > > > lockres = &OCFS2_I(inode)->ip_inode_lockres; > > + /* had_lock means that the currect process already takes the cluster > > + * lock previously. If had_lock is 1, we have nothing to do here, and > > + * it will get unlocked where we got the lock. > > + */ > > if (!had_lock) { > > ocfs2_remove_holder(lockres, oh); > > ocfs2_inode_unlock(inode, ex); > > diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c > > index 3c5384d..f70c377 100644 > > --- a/fs/ocfs2/xattr.c > > +++ b/fs/ocfs2/xattr.c > > @@ -1328,20 +1328,21 @@ static int ocfs2_xattr_get(struct inode *inode, > > void *buffer, > > size_t buffer_size) > > { > > - int ret; > > + int ret, had_lock; > > struct buffer_head *di_bh = NULL; > > + struct ocfs2_lock_holder oh; > > > > - ret = ocfs2_inode_lock(inode, &di_bh, 0); > > - if (ret < 0) { > > - mlog_errno(ret); > > - return ret; > > + had_lock = ocfs2_inode_lock_tracker(inode, &di_bh, 0, &oh); > > + if (had_lock < 0) { > > + mlog_errno(had_lock); > > + return had_lock; > > } > > down_read(&OCFS2_I(inode)->ip_xattr_sem); > > ret = ocfs2_xattr_get_nolock(inode, di_bh, name_index, > > name, buffer, buffer_size); > > up_read(&OCFS2_I(inode)->ip_xattr_sem); > > > > - ocfs2_inode_unlock(inode, 0); > > + ocfs2_inode_unlock_tracker(inode, 0, &oh, had_lock); > > > > brelse(di_bh); > > > > @@ -3537,11 +3538,12 @@ int ocfs2_xattr_set(struct inode *inode, > > { > > struct buffer_head *di_bh = NULL; > > struct ocfs2_dinode *di; > > - int ret, credits, ref_meta = 0, ref_credits = 0; > > + int ret, credits, had_lock, ref_meta = 0, ref_credits = 0; > > struct ocfs2_super *osb = OCFS2_SB(inode->i_sb); > > struct inode *tl_inode = osb->osb_tl_inode; > > struct ocfs2_xattr_set_ctxt ctxt = { NULL, NULL, NULL, }; > > struct ocfs2_refcount_tree *ref_tree = NULL; > > + struct ocfs2_lock_holder oh; > > > > struct ocfs2_xattr_info xi = { > > .xi_name_index = name_index, > > @@ -3572,8 +3574,9 @@ int ocfs2_xattr_set(struct inode *inode, > > return -ENOMEM; > > } > > > > - ret = ocfs2_inode_lock(inode, &di_bh, 1); > > - if (ret < 0) { > > + had_lock = ocfs2_inode_lock_tracker(inode, &di_bh, 1, &oh); > > + if (had_lock < 0) { > > + ret = had_lock; > > mlog_errno(ret); > > goto cleanup_nolock; > > } > > @@ -3670,7 +3673,7 @@ int ocfs2_xattr_set(struct inode *inode, > > if (ret) > > mlog_errno(ret); > > } > > - ocfs2_inode_unlock(inode, 1); > > + ocfs2_inode_unlock_tracker(inode, 1, &oh, had_lock); > > cleanup_nolock: > > brelse(di_bh); > > brelse(xbs.xattr_bh); > >