From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751464AbbDZW4n (ORCPT ); Sun, 26 Apr 2015 18:56:43 -0400 Received: from ipmail04.adl6.internode.on.net ([150.101.137.141]:35962 "EHLO ipmail04.adl6.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751043AbbDZW4k (ORCPT ); Sun, 26 Apr 2015 18:56:40 -0400 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: A2BlBwCabD1VPPLlLHlcgwyBL4JMg3ytbgEFBpk7BAICgR1NAQEBAQEBBwEBAQFBP4QhAQEEJxMcIxAIAw4KCSUPBSUDBxoTiCrEZgEBAQcCIBiFfoUihDdOB4QtBYsmkGeMIIVGg3yBA4EFgiEsMYECAQMcgSIBAQE Date: Mon, 27 Apr 2015 08:56:25 +1000 From: Dave Chinner To: Brian Foster Cc: Waiman Long , linux-kernel@vger.kernel.org, xfs@oss.sgi.com Subject: Re: [PATCH] xfs: call xfs_idestroy_fork() in xfs_ilock() critical section Message-ID: <20150426225625.GP15810@dastard> References: <1429724021-7675-1-git-send-email-Waiman.Long@hp.com> <20150422231758.GQ21261@dastard> <20150423122149.GA13131@bfoster.bfoster> <20150423220823.GJ15810@dastard> <20150424115733.GA4177@laptop.bfoster> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150424115733.GA4177@laptop.bfoster> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Apr 24, 2015 at 07:57:33AM -0400, Brian Foster wrote: > On Fri, Apr 24, 2015 at 08:08:23AM +1000, Dave Chinner wrote: > > On Thu, Apr 23, 2015 at 08:21:50AM -0400, Brian Foster wrote: > > > On Thu, Apr 23, 2015 at 09:17:58AM +1000, Dave Chinner wrote: > > > > @@ -410,11 +418,12 @@ xfs_attr_inactive(xfs_inode_t *dp) > > > > + lock_mode = XFS_ILOCK_EXCL; > > > > + cancel_flags = XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT; > > > > + xfs_ilock(dp, lock_mode); > > > > > > > > /* > > > > * No need to make quota reservations here. We expect to release some > > > > @@ -423,28 +432,36 @@ xfs_attr_inactive(xfs_inode_t *dp) > > > > xfs_trans_ijoin(trans, dp, 0); > > > > > > > > /* > > > > - * Decide on what work routines to call based on the inode size. > > > > + * It's unlikely we've raced with an attribute fork creation, but check > > > > + * anyway just in case. > > > > */ > > > > - if (!xfs_inode_hasattr(dp) || > > > > - dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) { > > > > - error = 0; > > > > - goto out; > > > > + if (!XFS_IFORK_Q(dp)) > > > > + goto out_cancel; > > > > > > What about attribute fork creation would cause di_forkoff == 0 if that > > > wasn't the case above? Do you mean to say a potential race with > > > attribute fork destruction? > > > > atrtibute fork creation will never leave di_forkoff == 0. See > > xfs_attr_shortform_bytesfit() as a guideline for the min/max fork > > offset at attribute fork creation time. > > > > The race I'm talking about is the fact we check for an attr fork, > > then drop the lock, do the trans reserve and then grab the lock > > again. The inode could have changed in that time, so we need to > > check again. It's extremely unlikely that the inode has changed due > > to the fact it is in the ->evict path and can't be referenced by the > > VFS again until it's in a reclaimable state. Hence it is only > > internal filesystem stuff that could modify it, which I don't think > > can happen. So, leave the check, mark the race as unlikely to occur. > > The check seems fine to me. I'm referring to the comment above: "It's > unlikely we've raced with an attribute fork creation, ..." Oh, ok, I missed that. I'll fix it. Cheers, Dave. -- Dave Chinner david@fromorbit.com