From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758178AbbDVTLu (ORCPT ); Wed, 22 Apr 2015 15:11:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38679 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758162AbbDVTLp (ORCPT ); Wed, 22 Apr 2015 15:11:45 -0400 Date: Wed, 22 Apr 2015 15:11:37 -0400 From: Brian Foster To: Waiman Long Cc: Dave Chinner , xfs@oss.sgi.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] xfs: call xfs_idestroy_fork() in xfs_ilock() critical section Message-ID: <20150422191137.GF6688@bfoster.bfoster> References: <1429724021-7675-1-git-send-email-Waiman.Long@hp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1429724021-7675-1-git-send-email-Waiman.Long@hp.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 22, 2015 at 01:33:41PM -0400, Waiman Long wrote: > The commit f7be2d7f594cbc ("xfs: push down inactive transaction > mgmt for truncate") refactored the xfs_inactive() function > in fs/xfs/xfs_inode.c. However, it also moved the call to > xfs_idestroy_fork() from inside the xfs_ilock() critical section to > outside. That was causing memory corruption and strange failures like > deferencing NULL pointers in some circumstances. > > This patch moves the xfs_idestroy_fork() call back into an xfs_ilock() > critical section to avoid memory corruption problem. > > Signed-off-by: Waiman Long > --- Interesting... so from your previous mail we have an inactive/reclaim racing with an xfs_iflush_fork() of the attr fork, or something of that nature? Is there a specific reproducer or is it some kind of stress test? Good catch in any case, it looks like a deviation from the previous code... > fs/xfs/xfs_inode.c | 5 ++++- > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 6163767..31850fb 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -1900,8 +1900,11 @@ xfs_inactive( > return; > } > > - if (ip->i_afp) > + if (ip->i_afp) { > + xfs_ilock(ip, XFS_ILOCK_EXCL); > xfs_idestroy_fork(ip, XFS_ATTR_FORK); > + xfs_iunlock(ip, XFS_ILOCK_EXCL); > + } It probably doesn't matter, but I wonder if it would be better to just place the lock outside of the ip->i_afp check to preserve the original behavior if nothing else... Brian > > ASSERT(ip->i_d.di_anextents == 0); > > -- > 1.7.1 > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs